RFR: 8357034: GifImageDecoder can produce wrong transparent pixels [v6]

[Jeremy Wood]

On Thu, 21 Aug 2025 23:18:07 GMT, Phil Race <prr at openjdk.org> wrote:

>> Jeremy Wood has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 32 commits:
>> 
>>  - 8357034: re-wrapping line breaks
>>  - 8356137, 8357034: add debug option to dump PNGs of frames
>>    
>>    This is in response to:
>>    https://github.com/openjdk/jdk/pull/25044#pullrequestreview-3007487527
>>  - Merge branch 'master' into JDK-8357034
>>    
>>    # Conflicts:
>>    #	test/jdk/sun/awt/image/gif/GifBuilder.java
>>    #	test/jdk/sun/awt/image/gif/GifComparison.java
>>  - 8357034: avoid recalculating isSimpleSavedImageComparison for every line
>>    
>>    This is in response to:
>>    https://github.com/openjdk/jdk/pull/25264#discussion_r2197097522
>>  - Merge branch 'master' into JDK-8357034
>>  - 8356320: Use new GifBuilder and remove ukraine-flag.gif
>>    
>>    This is an extension of work for this PR:
>>    https://github.com/openjdk/jdk/pull/25044#pullrequestreview-2871107750
>>  - 8356137: adding copyright
>>  - 8356137: Adding GifBuilder to dynamically create test file
>>    
>>    This can be used by multiple gif tests in this directory.
>>    
>>    This is in response to:
>>    https://github.com/openjdk/jdk/pull/25044#pullrequestreview-2871107750
>>  - 8356137: trivial javadoc update
>>  - 8356137: only inspect last frame of gif
>>    
>>    This makes the main() method much less useful, so I removed it too. (I originally used this class to explore a folder of hundreds of gifs to look for discrepancies. But the discrepancies were rarely only on the last frame.)
>>    
>>    This is in response to:
>>    https://github.com/openjdk/jdk/pull/25044#discussion_r2109298270
>>  - ... and 22 more: https://git.openjdk.org/jdk/compare/a86dd56d...7c96ce19
>
> src/java.desktop/share/classes/sun/awt/image/GifImageDecoder.java line 605:
> 
>> 603:         if (isSimpleSavedImageComparison) {
>> 604:             for (int a = 0; a < saved_image.length; a++) {
>> 605:                 if ((saved_image[a] & 0xff) == trans_pixel) {
> 
> How do you know here that the transparent pixel changed ?

Ah, you're right: I don't.

I ran this test:

new GifBuilder.FrameDescription[] {
        new GifBuilder.FrameDescription(
                GifBuilder.Disposal.doNotDispose, true),
        new GifBuilder.FrameDescription(
                GifBuilder.Disposal.doNotDispose, true),
        new GifBuilder.FrameDescription(
                GifBuilder.Disposal.doNotDispose, true)
};


... and it technically passed (the pixels were all correct), but it followed the wrong logic. It kept entering "the hard way" logic (which is slightly slower) when it didn't need to because the color model never changed.

So as of yesterday: this PR would err on the side of sometimes taking on more work than necessary. (The final pixels were always correct, at least.)

Since that logic is tucked away in such a private method: I don't see how to train the unit test to notice which approach (fast or slow) we're using.

I updated this PR tonight to a new approach: now we track an internal boolean `isSavedModelReliable`. We set this to false once we enter "the hard way" logic once, and we're careful from then on.

Also I updated this PR's original description to better document the failure I'm trying resolve. (Because I'd forgotten the details over the last few months.)

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/25264#discussion_r2294927594