On Mon, 5 May 2025 17:07:11 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.
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
