On Fri, 22 Aug 2025 22:53:28 GMT, Jeremy Wood <jw...@openjdk.org> wrote:
>> If there are two consecutive frames that use DISPOSAL_SAVE, but the >> transparent pixel index changed: we might accidentally send the wrong data >> to the ImageConsumer. >> >> We already had logic that submits info "the hard way" (see comment in code); >> this PR just makes sure we trigger that block. >> >> There are a cluster of four related PRs that share the GifComparison class >> in this PR. >> >> 1. [8357034](https://github.com/openjdk/jdk/pull/25264) (this one) >> 2. ~~[8356137](https://github.com/openjdk/jdk/pull/25044)~~ (integrated) >> 3. [8356320](https://github.com/openjdk/jdk/pull/25076) >> 4. [8351913](https://github.com/openjdk/jdk/pull/24271) >> >> This bug can be observed reading these gif animations: >> >> https://giphy.com/gifs/fomoduck-duck-fomo-ZUlzR40oGACqNmy0q8 >> https://media4.giphy.com/media/Zbf4JQzcDhzeraQvk9/giphy.gif >> https://giphy.com/gifs/computer-working-cat-LHZyixOnHwDDy >> https://giphy.com/gifs/90s-fgfgif-r4BmI9xUaDuKJQdd3l >> >> ### Describing This Bug >> >> The steps/explanation that lead to this failure are a little complicated. >> I'll try to summarize the original bug here. >> >> All three frames in this GIF use disposal code doNotDispose, meaning the >> frames are layered one on top of the previous. >> >> The first frame produces 3 circles from left to right using these colors: >> 0xff0000 (red) 0xffff00 (yellow) 0x00ff00 (green) >> >> The background is 0x00ffff (cyan). It is the transparent pixel, so the first >> frame is: >> >> <img width="200" height="100" alt="frame_0" >> src="https://github.com/user-attachments/assets/8bb53530-fc2b-4a8b-b3a6-5c58782ea5ab"; >> /> >> >> Now we start processing the second frame. The pixels are the same, but the >> color model has changed so the zeroeth color (red) is the transparent index. >> >> When the GifImageDecoder layers the 2nd frame on top of the 1st frame, it >> notices that `model.equals(saved_model)` is `false`. This means it enter a >> block of code that refers to "the hard way" of passing new pixel information >> to the ImageConsumers. >> >> The 2nd frame, after being painted on the 1st frame, looks like: >> <img width="200" height="100" alt="frame_1" >> src="https://github.com/user-attachments/assets/0c8ca55b-e25d-418e-bb95-2e6f9e427c08"; >> /> >> >> The 3rd frame is exactly the same as the 2nd. But here's where the bug is: >> now `model.equals(saved_model)` is `true`, so GifImageDecoder tries to pass >> the pixel information the easy way. The result is: >> >> <img width="200" height="100" alt="frame_2_awt" >> src="https://github.com/user-a... > > Jeremy Wood has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 34 commits: > > - 8357034: switch to `isSavedModelReliable` > > This reduces how often we resort to "the hard way". The previous > implementation would resort to the hard way *any* time the disposal code was > SAVE and there was a transparent pixel in use. > > This is in response to: > https://github.com/openjdk/jdk/pull/25264/files#r2292318270 > - Merge branch 'master' into JDK-8357034 > - 8357034: re-wrapping line breaks > - 8356137, 8357034: add debug option to dump PNGs of frames > > This is in response to: > https://github.com/openjdk/jdk/pull/25044#pullrequestreview-3007487527 > - Merge branch 'master' into JDK-8357034 > > # Conflicts: > # test/jdk/sun/awt/image/gif/GifBuilder.java > # test/jdk/sun/awt/image/gif/GifComparison.java > - 8357034: avoid recalculating isSimpleSavedImageComparison for every line > > This is in response to: > https://github.com/openjdk/jdk/pull/25264#discussion_r2197097522 > - Merge branch 'master' into JDK-8357034 > - 8356320: Use new GifBuilder and remove ukraine-flag.gif > > This is an extension of work for this PR: > https://github.com/openjdk/jdk/pull/25044#pullrequestreview-2871107750 > - 8356137: adding copyright > - 8356137: Adding GifBuilder to dynamically create test file > > This can be used by multiple gif tests in this directory. > > This is in response to: > https://github.com/openjdk/jdk/pull/25044#pullrequestreview-2871107750 > - ... and 24 more: https://git.openjdk.org/jdk/compare/57553ca1...e7796df0 src/java.desktop/share/classes/sun/awt/image/GifImageDecoder.java line 400: > 398: boolean save = (curframe.disposal_method == > GifFrame.DISPOSAL_SAVE); > 399: if (trans_pixel >= 0 && !curframe.initialframe) { > 400: if (saved_image != null && model.equals(saved_model) && > isSavedModelReliable) { Without the fix, if the model changes, we go the hard way, on a frame by frame basis. With the fix, if the model changes, we go the hard way, for this and all subsequent frames. But what was wrong with frame-by-frame, that "all subsequent frames" fixes ? I suppose it must be that any frame that had a transparent pixel needs to be preserved for all subsequent frames, but I'm quite hazy on how that works. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/25264#discussion_r2294948284
