fix: Initialize GIF transparency correctly in absence of a GCE block #276
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.
Initialize GIF transparency correctly
Resolves #267.
When initializing GIF
GraphicsControlBlockfallback values ingiflib_get_frame_gcb()ingiflib.cpp,gcb->TransparentColorwas missing an initializer. This caused an undefined palette index (usually 0) to be temporarily treated as "transparent" for frames lacking a GCE block, causing colour corruption.Why does it cause colour corruption?
The output GIF emitted by
giflib.cppwill preserve when there is no GCE block for a frame, which is perfectly valid behaviour. Since GCE blocks are required in order to define transparency, the lack of a GCE block means that a frame must have no transparency, and this is how most GIF decoders will read a frame of this type. So far so good: even though the code ingiflib.cppmisinterprets a random palette index as being transparent, it doesn't enshrine that into the output file, so decoders will still treat it correctly as being opaque.However,
giflib.cpp's interpretation of transparency is used internally in the GIF encoding process ingiflib.cpp—namely for palette search.lilliput/giflib.cpp
Lines 1032 to 1039 in 686cbce
When reëncoding the colours of that frame, because it thinks a (rightfully opaque) colour is transparent, it will disallow it from being used for colours the output. In the case of the example image in #267, because colour index 0 (
#4F564D, the entire background of the image) is temporarily misinterpreted as being transparent, the palette search code has to find some colour other than 0 to replace all instances of#4F564D, resulting in the background of the image totally changing in colour.Additionally, since the decoder sets the nominal alpha channel value to 0 for colours like this with misidentified transparency, other encoders (like the WebP encoder) can misinterpret this as being actual full transparency, as would the GIF encoder's output if it were to actually emit a GCE block signaling transparency for the output. This means that transcoding GIF → animated WebP would turn some unexpected palette colour into a transparent one.
Changes
The only change needed to fix this image is one line of code properly initializing
gcb->TransparentColortoNO_TRANSPARENT_COLOR. I thought the breakdown of how it causes corruption was interesting, though, even though the fix was simple.Caveats
The
gcb->TransparentColorinitializer used to be present, but was removed in #255. Removing it doesn't seem to be necessary for the fix to that issue, however, as the sample image given in that PR description still encodes correctly after this change. Since all calls togiflib_get_frame_gcb()occur immediately after declaring a fresh, uninitializedGraphicsControlBlock gcbvariable, it seems highly unlikely that removing the initialization at the start ofgiflib_get_frame_gcb()as was done in that PR would "preserve" anything useful in thegcb. It would just grab a random value for the transparency index from somewhere in stack memory. I'm pretty sure the actual bugfix in #255 was from the other changes there.