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

[Mikhail Yankelevich]

On Mon, 5 May 2025 17:07:11 GMT, Jeremy Wood <duke at 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.

Just a few questions.

Also, a copyright in GifImageDecoder.java

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

> 1: /*
> 2:  * Copyright (c) 2002, 2025, Oracle and/or its affiliates. All rights reserved.

Nitpick: do you need 2002 here? Isn't it a new file?

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

> 26:  * @bug 8356137
> 27:  * @summary This test verifies a non-zero transparent pixel in gifs works when
> 28:  * the disposal method changes from 2 to 1, and when the

Nitpick: Do you need the `, and when the` ? 😄

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

> 49:         boolean pass = true;
> 50:         if (new Color(frames[3].getRGB(20, 20), true).getAlpha() != 0) {
> 51:             System.err.println("Sampling at (20,20) failed");

Do you think it would be cleaner if you jsut throw a `RuntimeException("Sampling at (20,20) failed");` instead of the whole

 System.err.println("Sampling at (20,20) failed");
  pass = false;
        }

        if (!pass)
            throw new Error("See System.err for details");

? 
It should result in the same level of details but with better readability imo

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

> 103:                     // we don't expect this to happen, but if something goes
> 104:                     // wrong nobody else will print our stacktrace for us:
> 105:                     e.printStackTrace();

Do you need this print here? Runtime exception should print it out anyway to the system.error afaik

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

> 131:                     }
> 132:                 } catch(Exception e) {
> 133:                     e.printStackTrace();

Do you need to print out the stack trace here when you are throwing it below?

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

> 141:         return returnValue.toArray(new BufferedImage[0]);
> 142:     }
> 143: }

nitpick: could you please add a new line here for github? 😃

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

PR Review: https://git.openjdk.org/jdk/pull/25044#pullrequestreview-2817764726
PR Review Comment: https://git.openjdk.org/jdk/pull/25044#discussion_r2075186796
PR Review Comment: https://git.openjdk.org/jdk/pull/25044#discussion_r2075200131
PR Review Comment: https://git.openjdk.org/jdk/pull/25044#discussion_r2075190122
PR Review Comment: https://git.openjdk.org/jdk/pull/25044#discussion_r2075197940
PR Review Comment: https://git.openjdk.org/jdk/pull/25044#discussion_r2075193365
PR Review Comment: https://git.openjdk.org/jdk/pull/25044#discussion_r2075192144