Skip to content

8356137: GifImageDecode can produce opaque image when disposal method changes #25044

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 41 commits into
base: master
Choose a base branch
from

Conversation

mickleness
Copy link
Contributor

@mickleness mickleness commented May 5, 2025

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
  2. 8356137 (this one)
  3. 8356320
  4. 8351913

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

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)

Issue

  • JDK-8356137: GifImageDecode can produce opaque image when disposal method changes (Bug - P4)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25044/head:pull/25044
$ git checkout pull/25044

Update a local copy of the PR:
$ git checkout pull/25044
$ git pull https://git.openjdk.org/jdk.git pull/25044/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25044

View PR using the GUI difftool:
$ git pr show -t 25044

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25044.diff

Using Webrev

Link to Webrev Comment

mickleness and others added 12 commits May 22, 2022 04:50
Merge openjdk/jdk into mickleness/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.
@bridgekeeper
Copy link

bridgekeeper bot commented May 5, 2025

👋 Welcome back mickleness! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented May 5, 2025

@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:

8356137: GifImageDecode can produce opaque image when disposal method changes

Reviewed-by: jdv, prr

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 master branch:

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 /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the rfr Pull request is ready for review label May 5, 2025
@openjdk
Copy link

openjdk bot commented May 5, 2025

@mickleness The following label will be automatically applied to this pull request:

  • client

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.

@mlbridge
Copy link

mlbridge bot commented May 5, 2025

@mickleness
Copy link
Contributor Author

For reference, the commented out lines here are my first approach at resolving this problem.

image

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.

Copy link
Member

@myankelev myankelev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few questions.

Also, a copyright in GifImageDecoder.java

@@ -0,0 +1,143 @@
/*
* Copyright (c) 2002, 2025, Oracle and/or its affiliates. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, this is updated

image.flush();
}
} catch(Exception e) {
e.printStackTrace();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@mickleness mickleness May 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is updated

@mickleness

This comment was marked as outdated.

@mickleness mickleness closed this May 14, 2025
@mickleness
Copy link
Contributor Author

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.

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 a need a gif with only 2 frames having transpixel>0 and disposal method save to reproduce this scenario.

I think we need 3 frames. The test currently resembles:

    public static void main(String[] args) throws Throwable {
        GifBuilder.test(
                new GifBuilder.FrameDescription(GifBuilder.Disposal.restoreToBackgroundColor, false),
                new GifBuilder.FrameDescription(GifBuilder.Disposal.doNotDispose, false),
                new GifBuilder.FrameDescription(GifBuilder.Disposal.doNotDispose, false) );
    }

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:
A. This file architecture was reverse engineered from https://free-gifs.org/gif/CC0-3D
B. None of this weekend's changes alter the proposed change in GifImageDecoder ; this is all just refactoring the test

mickleness added a commit to mickleness/jdk that referenced this pull request Jun 1, 2025
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)
mickleness added a commit to mickleness/jdk that referenced this pull request Jun 1, 2025
This can be used by multiple gif tests in this directory.

This is in response to:
openjdk#25044 (review)
mickleness added a commit to mickleness/jdk that referenced this pull request Jun 1, 2025
mickleness added a commit to mickleness/jdk that referenced this pull request Jun 1, 2025
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)
mickleness added a commit to mickleness/jdk that referenced this pull request Jun 1, 2025
This can be used by multiple gif tests in this directory.

This is in response to:
openjdk#25044 (review)
mickleness added a commit to mickleness/jdk that referenced this pull request Jun 1, 2025
@bridgekeeper
Copy link

bridgekeeper bot commented Jun 29, 2025

@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 /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@jayathirthrao
Copy link
Member

jayathirthrao commented Jul 9, 2025

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

OK. I switched back to this approach.

(The downside being: now we'll invoke Arrays.fill(..) for all gifs with that disposal method. I assume (?) gifs with this architecture/problem are a small subset of that group; but it's impossible to quantify that hunch.)

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.

@mickleness
Copy link
Contributor Author

Only thing is change should be updated to follow 80 characters per line

OK, I re-wrapped text in the test java files.

Copy link
Member

@jayathirthrao jayathirthrao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI test run is green.
LGTM.

@openjdk
Copy link

openjdk bot commented Jul 10, 2025

⚠️ @mickleness the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:

$ git checkout JDK-8356137
$ git commit --author='Preferred Full Name <[email protected]>' --allow-empty -m 'Update full name'
$ git push

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Jul 10, 2025
@jayathirthrao
Copy link
Member

jayathirthrao commented Jul 10, 2025

/reviewers 2 reviewer

@openjdk
Copy link

openjdk bot commented Jul 10, 2025

@jayathirthrao
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).

@openjdk openjdk bot removed the ready Pull request is ready to be integrated label Jul 10, 2025
Copy link
Contributor

@prrace prrace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
Might get the award for "best original test".
I did modify the test to dump the frames being generated and the accumulated image, just so I could see.
Maybe you could add such a feature as a debugging option in one of the follow-on PRs.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Jul 10, 2025
@mickleness
Copy link
Contributor Author

Maybe you could add such a feature as a debugging option in one of the follow-on PRs.

Sure.

@mickleness
Copy link
Contributor Author

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Jul 10, 2025
@openjdk
Copy link

openjdk bot commented Jul 10, 2025

@mickleness
Your change (at version 1dbbe45) is now ready to be sponsored by a Committer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client [email protected] ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored
Development

Successfully merging this pull request may close these issues.

4 participants