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

[Jayathirth D V]

On Mon, 26 May 2025 05:29:45 GMT, Jeremy Wood <d...@openjdk.org> wrote:

>> This resolves a gif parsing bug where an unwanted opaque rectangle could 
>> appear under these conditions:
>> 
>> 1. The disposal method for frames is 1 (meaning "do not dispose", aka 
>> "DISPOSAL_SAVE")
>> 2. The transparent pixel is non-zero
>> 3. There's more than one such consecutive frame
>> 
>> Previously: the GifImageDecoder would leave the saved_image pixels as zero 
>> when they were supposed to be transparent. This works great if the 
>> transparent pixel index is zero, but it flood fills the background of your 
>> frame with the zeroeth color otherwise.
>> 
>> 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)
>> 2. [8356137](https://github.com/openjdk/jdk/pull/25044) (this one)
>> 3. [8356320](https://github.com/openjdk/jdk/pull/25076)
>> 4. [8351913](https://github.com/openjdk/jdk/pull/24271)
>
> Jeremy Wood has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   8356137: Adding copyright to GifComparison

I think we a need a gif with only 2 frames having transpixel>0 and disposal 
method save to reproduce this scenario.

Also its better if we can actually create test gif file using ImageIO(There are 
examples in test/jdk/javax/imageio/plugins/gif/) programatically and use it 
instead of external GIF files for testing.

> For reference, the commented out lines here are my first approach at 
> resolving this problem.
> <img alt="image" width="687" 
> src="https://private-user-images.githubusercontent.com/7669569/440467227-8caa3372-ee42-412d-959c-6d55d258e4ba.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3NDgzNTU3OTUsIm5iZiI6MTc0ODM1NTQ5NSwicGF0aCI6Ii83NjY5NTY5LzQ0MDQ2NzIyNy04Y2FhMzM3Mi1lZTQyLTQxMmQtOTU5Yy02ZDU1ZDI1OGU0YmEucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI1MDUyNyUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNTA1MjdUMTQxODE1WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9NGUzNmFkYjg1MjEyOTk4OWMwZWNiMzViOTg3NzY5M2QyMmFjMjNkMGY4YTdhOTJiNzEzOTQzM2UwN2Y0OGU1YSZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QifQ.p-JV9ycUKEB7RuDArvsHhWTNGWwnCcYOpwZH9UvpyiE";>
> 
> This works, and it may be more readable/intuitive than this current PR. When 
> we initialize saved_image we should flood fill it with transparent pixels.
> 
> I chose the approach in this PR though because it's lazier. (That is: it 
> should be a little bit less work, CPU-wise.)
> 
> If anyone wants we can switch to this approach.

I think this is better then current approach of having check multiple times.

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.

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.

test/jdk/sun/awt/image/gif/GifComparison.java line 117:

> 115:      *         conditions are met.
> 116:      */
> 117:     public static BufferedImage run(URL srcURL) throws Throwable {

If we can actually update this helper to check a specific frame is GIF and not 
run throw all the frame its better.
In this PR we just need the info of 4th frame.

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

test/jdk/sun/awt/image/gif/GifEmptyBackgroundTest.java line 40:

> 38:         BufferedImage bi = GifComparison.run(srcURL);
> 39: 
> 40:         if (new Color(bi.getRGB(20, 20), true).getAlpha() != 0) {

Without the product fix this test fails while checking opacity of (0,0) itself 
and we don't hit this check.

`Exception in thread "main" java.lang.Error: pixels at (0, 0) have different 
opacities: 0 vs ff000000`

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

PR Review: https://git.openjdk.org/jdk/pull/25044#pullrequestreview-2871107750
PR Comment: https://git.openjdk.org/jdk/pull/25044#issuecomment-2912699099
PR Review Comment: https://git.openjdk.org/jdk/pull/25044#discussion_r2109208157
PR Review Comment: https://git.openjdk.org/jdk/pull/25044#discussion_r2109288435
PR Review Comment: https://git.openjdk.org/jdk/pull/25044#discussion_r2109298270
PR Review Comment: https://git.openjdk.org/jdk/pull/25044#discussion_r2109318427