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

[Jayathirth D V]

On Thu, 10 Jul 2025 21:20:07 GMT, Jeremy Wood <duke at openjdk.org> wrote:

>> If there are two consecutive frames that use DISPOSAL_SAVE, but the transparent pixel index changed: we might accidentally send the wrong data to the ImageConsumer.
>> 
>> We already had logic that submits info "the hard way" (see comment in code); this PR just makes sure we trigger that block.
>> 
>> I wrote four PRs that share the GifComparison class in this PR. Once any of them clear code review the other PRs will be much simpler:
>> 
>> 1. [8357034](https://github.com/openjdk/jdk/pull/25264) (this one)
>> 2. [8356137](https://github.com/openjdk/jdk/pull/25044)
>> 3. [8356320](https://github.com/openjdk/jdk/pull/25076)
>> 4. [8351913](https://github.com/openjdk/jdk/pull/24271)
>> 
>> This bug can be observed reading these gif animations:
>> 
>> https://giphy.com/gifs/fomoduck-duck-fomo-ZUlzR40oGACqNmy0q8
>> https://media4.giphy.com/media/Zbf4JQzcDhzeraQvk9/giphy.gif
>> https://giphy.com/gifs/computer-working-cat-LHZyixOnHwDDy
>> https://giphy.com/gifs/90s-fgfgif-r4BmI9xUaDuKJQdd3l
>
> Jeremy Wood has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 29 additional commits since the last revision:
> 
>  - 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
>  - 8356137: Adding copyright to GifComparison
>  - 8357034: fixing classname
>  - 8356320: trivial whitespace and comment changes
>  - ... and 19 more: https://git.openjdk.org/jdk/compare/9de36470...b8a8bf7d

Latest change looks good to me. I will run CI tests and update.
Also if we can add debugging option to dump image as discussed here : https://github.com/openjdk/jdk/pull/25044#pullrequestreview-3007487527 it would be nice.

Also we need to merge latest master into this PR since test utility changes are already integrated.

src/java.desktop/share/classes/sun/awt/image/GifImageDecoder.java line 598:

> 596:         }
> 597: 
> 598:         isSimpleSavedImageComparison = saved_image != null && model.equals(saved_model);

Looks like we can ignore `trans_pixel >= 0 && !curframe.initialframe` check here.

I see that saved model will be null for initial frame at this point in code & if trans_pixel changes `model.equals(saved_model)` fails. So current set of checks suffice.

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

PR Comment: https://git.openjdk.org/jdk/pull/25264#issuecomment-3060816867
PR Comment: https://git.openjdk.org/jdk/pull/25264#issuecomment-3060820975
PR Review Comment: https://git.openjdk.org/jdk/pull/25264#discussion_r2199763038