Fix imagick stale layers#879
Merged
Merged
Conversation
Image::layers() lazily creates a Layers object that captures the current \Imagick resource. Multi-frame resize()/crop() (and flatten(), and the optimize output path) replace the resource via coalesceImages() / deconstructImages() without invalidating that cache, so any subsequent layers() access (get, coalesce, iteration, animate, merge) operated on the orphaned pre-operation frames. Among other effects, this caused the animated.delay and animated.loops save options to be silently lost after resizing an animated image. Replace direct resource reassignments with a setter that invalidates the cached layers when the \Imagick instance changes.
Collaborator
|
Nice catch! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resize an animated GIF on the Imagick driver, then ask for its layers: you get the pre-resize frames back.
Layersgrabs the\Imagickinstance in its constructor, andImage::layers()caches the object forever. That was harmless until the multi-frame paths started swapping the instance:coalesceImages()/deconstructImages()return a new\Imagick, whichresize(),crop(),flatten()and theoptimizesave path assign to$this->imagick, so the cachedLayerskeeps reading frames nothing else uses anymore. One concrete casualty:animated.delay/animated.loopsare applied throughlayers()inprepareOutput(), land on the orphaned frames, and the saved file keeps its original timing.Reassignments now go through a small setter that drops the cached
Layerswhen the instance changes (__clone()already rebuilds it the same way), so the nextlayers()call reads the live resource.