fix(core): give a cloned image its own pipeline and meta data - #125
Open
nlemoine wants to merge 4 commits into
Open
fix(core): give a cloned image its own pipeline and meta data#125nlemoine wants to merge 4 commits into
nlemoine wants to merge 4 commits into
Conversation
Image::__clone() clones the core, but the vips Core declared no __clone() of its own. The shallow copy left both cores holding the same meta collection and the same vips image. The image itself is immutable, but the decoders open the source for a single sequential pass, so only one of the two images could be encoded, the other failed with an out of order read. The clone now gets its own meta collection, as the GD and Imagick cores do, and while the stash is in place it reopens the source instead of sharing the pipeline: a fresh lazy load at no raster cost, with the same option string and band normalisation the decoder applied. Without a stash the vips image stays shared.
setLoops() wrote the field on the vips image in place. The image is shared with any clone, so the change leaked to the other image, and it left the stash in place, so a clone reopening the source came back with the loop count of the file. Work on a copy and hand it to setNative(), which also clears the stash.
empty() replaced the vips image directly, so the stashed source stayed in place and described an image that was gone. A later resize reloaded the source, and a clone reopening the stash brought it back whole. Route the replacement through setNative(), which clears the stash.
…rsion A grayscale source (B_W, GREY16) was not stashed: the decoder converts it to sRGB, and a resize reopening the stash through thumbnail() would have come back grayscale. Without a stash a clone shares the single sequential pipeline, so cloning a decoded grayscale image hit the very failure Core::__clone() fixes for every other source. Move the conversion the decoder applies, grayscale to sRGB and the alpha band on 3-band sRGB, into CanNormalizeSource, and replay it wherever a stashed source is reopened: Core::__clone() and the thumbnail fast path of the resize and cover modifiers. Grayscale sources are stashed like any other, which also gives them the shrink on load path. Core::__clone() gets a helper for the reopen and starts the chained operation count from zero, the pipeline is fresh. Its docblock now says the reopen does I/O and can throw.
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
1 participant
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.
Intervention\Image\Image::__clone()clones the driver, the core and the exif. The vipsCoredeclares no__clone()of its own, so the shallow copy leaves both cores holding the sameJcupitt\Vips\Imageand the same meta collection.The vips image is immutable, so sharing it is fine on its own. The problem is the pipeline behind a decoded image: the decoders load with
Access::SEQUENTIAL, so it can be walked once. Encode the clone, then the original, and the second one fails.Same with
decodePath(), PNG and grayscale sources. A resize on either image hides it, the resize modifiers reload from the stash instead of walking the shared pipeline.Core::__clone()clones the meta collection, like the GD and Imagick cores do, and while the stash is in place it reopens the source for the clone, a fresh lazy load at no raster cost. For that to hold, everything that replaces the core's image now goes throughsetNative()so the stash is cleared, grayscale sources are stashed too, and the conversion the decoder applies (CanNormalizeSource, replacingCanNormalizeBands) is replayed wherever a stash is reopened, the clone and the thumbnail fast path of the resize modifiers. Without a stash the vips image stays shared, same as before.Not covered: encoding the same decoded image twice fails the same way with no clone involved, that is the single pass of a sequential load, see #56. Same for a clone made after a modifier that cleared the stash.
Core::ensureInMemory()before cloning renders it once for both. Maybe worth a README note, I left it alone.