8356137: GifImageDecode can produce opaque image when disposal method changes #25044
Conversation
Merge openjdk/jdk into mickleness/jdk
Merge openjdk/jdk
Merge from openjdk/jdk
Updating mickleness/jdk from openjdk/jdk
updating to openjdk/jdk
Also adding accompanying unit test. I think there's been a mix-up regarding the sample image attached to the original ticket. This ticket should refer to `clyde.gif` (originally based on a data structure identified here: https://free-gifs.org/gif/CC0-3D ). There is another similar looking (but different) ticket I filed last week for which `leo.gif` is the appropriate test case file. (See incident report 9218362. As far as I can see that hasn't made its way to the bug database yet.) I had to email both gifs to someone on the triage team, and I think they got mixed up.
|
👋 Welcome back mickleness! A progress list of the required criteria for merging this PR into |
|
@mickleness This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 22 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@jayathirthrao, @prrace) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
|
@mickleness The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
|
For reference, the commented out lines here are my first approach at resolving this problem. 
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. |
| @@ -0,0 +1,143 @@ | |||
| /* | |||
| * Copyright (c) 2002, 2025, Oracle and/or its affiliates. All rights reserved. | |||
There was a problem hiding this comment.
Nitpick: do you need 2002 here? Isn't it a new file?
There was a problem hiding this comment.
OK, this is removed
|
|
||
| boolean pass = true; | ||
| if (new Color(frames[3].getRGB(20, 20), true).getAlpha() != 0) { | ||
| System.err.println("Sampling at (20,20) failed"); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I don't have a strong opinion; this is removed. (I often follow that pattern in case I try to add multiple criteria to pass/fail decisions.)
|
|
||
| return returnValue.toArray(new BufferedImage[0]); | ||
| } | ||
| } |
There was a problem hiding this comment.
nitpick: could you please add a new line here for github? 😃
There was a problem hiding this comment.
OK, this is updated
| image.flush(); | ||
| } | ||
| } catch(Exception e) { | ||
| e.printStackTrace(); |
There was a problem hiding this comment.
Do you need to print out the stack trace here when you are throwing it below?
There was a problem hiding this comment.
Yes, this is caught in the same place the other one is ( #25044 (comment) )
| } catch (RuntimeException e) { | ||
| // we don't expect this to happen, but if something goes | ||
| // wrong nobody else will print our stacktrace for us: | ||
| e.printStackTrace(); |
There was a problem hiding this comment.
Do you need this print here? Runtime exception should print it out anyway to the system.error afaik
There was a problem hiding this comment.
Do you need this print here?
Yes, because this eventually is caught here in GifImageDecoder.java:
try {
if (!readImage(totalframes == 0,
disposal_method,
delay)) {
return;
}
} catch (Exception e) {
if (verbose) {
e.printStackTrace();
}
return;
}
In this specific test file: I never expect an exception to be thrown, but one did come up when I was first drafting this test (because of my own error). It was hard to debug because it was unreported. I would prefer to leave these printStackTrace calls in, in case a developer someday makes a change and needs to see potential errors.
(Technically I could try to make verbose true, but that's declared as a final variable and I don't want to modify GifImageDecoder.java just for this.)
| * @test | ||
| * @bug 8356137 | ||
| * @summary This test verifies a non-zero transparent pixel in gifs works when | ||
| * the disposal method changes from 2 to 1, and when the |
There was a problem hiding this comment.
Nitpick: Do you need the , and when the ? 😄
There was a problem hiding this comment.
Thanks, this is updated
This is in response to: openjdk#25044 (comment)
This is in response to: openjdk#25044 (comment)
This is in response to: openjdk#25044 (comment)
This is in response to: openjdk#25044 (comment)
This comment was marked as outdated.
This comment was marked as outdated.
This can be used by multiple gif tests in this directory. This is in response to: openjdk#25044 (review)
This is in response to: openjdk#25044 (review)
I wrote a new class GifBuilder that programmatically creates the test gif file; and it can generate test files for some of the other related pending bugs.
I think we need 3 frames. The test currently resembles: If I comment out any one of these three frames then this test passes when I run it against the master branch. For additional context: |
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: openjdk#25044 (comment)
This can be used by multiple gif tests in this directory. This is in response to: openjdk#25044 (review)
This is an extension of work for this PR: openjdk#25044 (review)
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: openjdk#25044 (comment)
This can be used by multiple gif tests in this directory. This is in response to: openjdk#25044 (review)
This is an extension of work for this PR: openjdk#25044 (review)
|
@mickleness This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a |
Yes and also this will happen only when transparentPixelIndex > 0. I have again verified updated test with updated code and change looks good to me. Only thing is change should be updated to follow 80 characters per line, i see at many places it is >100 characters and difficult to read. I have given full CI test run and will update here after the results. |
This is in response to: openjdk#25044 (comment)
This is in response to: openjdk#25044 (comment)
OK, I re-wrapped text in the test java files. |
|
|
/reviewers 2 reviewer |
|
@jayathirthrao |
Sure. |
|
/integrate |
|
@mickleness |
This resolves a gif parsing bug where an unwanted opaque rectangle could appear under these conditions:
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:
This bug can be observed reading these gif animations:
https://pixabay.com/gifs/cat-kitten-black-cats-pet-animal-20315/
https://free-gifs.org/gif/CC0-3D
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25044/head:pull/25044$ git checkout pull/25044Update a local copy of the PR:
$ git checkout pull/25044$ git pull https://git.openjdk.org/jdk.git pull/25044/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 25044View PR using the GUI difftool:
$ git pr show -t 25044Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25044.diff
Using Webrev
Link to Webrev Comment