-
-
Notifications
You must be signed in to change notification settings - Fork 858
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix various Gif Decoder/Encoder behaviors. #2289
Merged
Merged
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
46f26ba
Correctly read/write graphics control extension
JimBobSquarePants 44906d5
Ensure transparency is written
JimBobSquarePants abd3a5a
Fix transparency handling and optimize encoding
JimBobSquarePants 0dcc73a
Fix leak
JimBobSquarePants 55ae240
Fix gif decoder tests
JimBobSquarePants ab6590c
Fix report
JimBobSquarePants dc58f2b
Better fix for report
JimBobSquarePants 0ad766f
Skip bad test
JimBobSquarePants e3872ae
Read additional gifs and make transparency handling safer
JimBobSquarePants 0e26da8
Better trans index fix.
JimBobSquarePants 04253b4
Merge branch 'main' into defect/2288
JimBobSquarePants File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some images use a transparent index that is the same as the length of the palette.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I've seen, most viewers treat OOB transparent index as 0. That's also the behavior written into the PNG spec. Here's an example where 0 works and last index doesn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's interesting. I found that the second gif in the referenced issue had a frame with a local table that contained 64 colors with a transparent index of 64. Browsers/Windows were rendering it just fine, but we were not.
Here's your input gif processed with this PR code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... yeah, it seems like the behavior I saw was a side-effect of the way I handle OOB values in general. I had just noticed that ImageSharp 2.1.3 was mangling that image, and I knew it had an invalid transparent index. The viewer I've been using (Microsoft GIF Animator) crashes on images like that. What's the viewer you screenshotted there?
I just looked at some web browser code, and it looks like they're just treating any OOB colors as transparent if the GCE has the transparency flag. They take an OOB transparent index as a hint that OOB color values are expected in the frame.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I use this. Stumbled upon it a few years back and it's proven incredibly useful
https://github.com/ata4/gifiddle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wow, that's great, thanks!
I see what's happening here now. Before this fix, you're not setting a transparent color because it's out of range, but you're clamping the image values into palette range, so pixels meant to be transparent are taking the last color. This was the result.
Now you're treating the last palette color as transparent and clamping OOB image values to that color, making the transparency work, but also making any pixels that were supposed to be that last palette color transparent as well. It's not creating artifacts as visible as skipping transparency, but it's also not completely correct.
In my implementation, I made the same mistake, but in the other direction. I use shared code for all animated image formats, so I followed the PNG rules of treating OOB image values as the first color in the palette. That meant I also had to treat invalid transparency index as meaning the first color was supposed to be transparent to get things to look right. Hence my initial comment 😅
So yeah, the browser behavior is better. Don't clamp the values but rather assume the palette is actually bigger than the image says.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed a better fix now. I treat anything greater than the max palette index or equal to the flagged transparent index as transparent.
I only clamp following that check to ensure we don't hit OOB when reading the palette.
Thanks for your help here!