RFR: 8356137: GifImageDecode can produce opaque image when disposal method changes [v6]

[Jeremy Wood]

On Tue, 27 May 2025 13:32:33 GMT, Jayathirth D V <jdv at openjdk.org> wrote:

>> Jeremy Wood has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   8356137: Adding copyright to GifComparison
>
> src/java.desktop/share/classes/sun/awt/image/GifImageDecoder.java line 419:
> 
>> 417:                 // are NOT the transparent pixel: we need to replace it
>> 418:                 // with the appropriate transparent pixel.
>> 419:                 boolean replaceTransPixelsInSavedImage = save &&
> 
> We can move this check to the place where we actually create and initialize saved image.

I think this thread is moot having addressed https://github.com/openjdk/jdk/pull/25044#issuecomment-2912699099 .

> src/java.desktop/share/classes/sun/awt/image/GifImageDecoder.java line 436:
> 
>> 434:                         }
>> 435:                         runstart = -1;
>> 436:                         if (replaceTransPixelsInSavedImage) {
> 
> We are hitting this check for each transparent pixel for all frames with transparent pixel index > 0 and not initial frame until saved_image is not null. In the .gif present in this PR we are checking this condition for each transparent pixel of 2nd and 3rd frame. If GIF contains many frames and disposal method save comes in later part of animation we will hit this check many times.
> 
> I think its better to just initialize saved_image with proper transparent pixel index when it is getting created. Then we can completely remove state management of `replaceTransPixelsInSavedImage` and this check.

OK; I think this thread is moot having addressed https://github.com/openjdk/jdk/pull/25044#issuecomment-2912699099 .

> Is this way of checking all frames useful in any other PR's which you have raised?

No, I don't think so. (I liked the assurance that every frame was identical, but technically that is beyond the scope of each individual unit test.)

I changed this method to only inspect the last frame.

This made the GifComparison's main() class much less useful, so I removed it. So now the GifComparison class is more limited in scope to unit tests -- and it's no longer a generic tool to identify new problems.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25044#discussion_r2109860678
PR Review Comment: https://git.openjdk.org/jdk/pull/25044#discussion_r2109860766
PR Review Comment: https://git.openjdk.org/jdk/pull/25044#discussion_r2109856464