-
-
Notifications
You must be signed in to change notification settings - Fork 887
Only set GIF canvas background when required. #2982
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
Conversation
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.
Pull Request Overview
This PR fixes issue #2980 by improving how GIF decoder and encoder handle canvas background colors, particularly ensuring that background colors are only set when required and that transparency handling is correctly applied.
- Optimizes background color handling to only apply when disposal methods require it
- Removes unnecessary transparency alpha manipulation during decoding
- Adds proper background index calculation that respects metadata values
Reviewed Changes
Copilot reviewed 74 out of 74 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Images/Input/Gif/issues/issue_2980.gif | Adds test GIF file for issue reproduction |
| tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2980_* | New reference output frames for issue 2980 test |
| tests/Images/External/ReferenceOutput/GifDecoderTests/* | Updated reference outputs reflecting improved transparency handling |
| tests/ImageSharp.Tests/TestImages.cs | Adds constant for new test GIF |
| tests/ImageSharp.Tests/Quantization/QuantizedImageTests.cs | Removes transparency assertions that are no longer valid |
| tests/ImageSharp.Tests/Formats/Gif/GifDecoderTests.cs | Adds test case for issue 2980 |
| src/ImageSharp/Formats/Gif/GifEncoderCore.cs | Improves background index calculation and transparency handling |
| src/ImageSharp/Formats/Gif/GifDecoderCore.cs | Optimizes canvas background setting logic |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| index = 0; | ||
| return false; | ||
| return ClampIndex(match); |
Copilot
AI
Sep 10, 2025
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.
The method returns ClampIndex(match) but match could be -1, which would be clamped to 0. This could incorrectly return index 0 as the background when no match is found and no valid background index exists from metadata.
| return ClampIndex(match); | |
| // If no match was found, return byte.MaxValue to indicate "no valid background index". | |
| return match == -1 ? byte.MaxValue : ClampIndex(match); |
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.
0 is the default in GIF
Prerequisites
Description
Fix #2980