On Sun, 1 Jun 2025 06:43:44 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. >> >> 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](https://github.com/openjdk/jdk/pull/25264) >> 2. [8356137](https://github.com/openjdk/jdk/pull/25044) (this one) >> 3. [8356320](https://github.com/openjdk/jdk/pull/25076) >> 4. [8351913](https://github.com/openjdk/jdk/pull/24271) > > Jeremy Wood has updated the pull request incrementally with four additional > commits since the last revision: > > - 8356137: adding copyright > - 8356137: remove test file now that we use GifBuilder > - 8356137: Use new GifBuilder for 8356137 test case > > This is in response to: > https://github.com/openjdk/jdk/pull/25044#pullrequestreview-2871107750 > - 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 > > 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. ------------- PR Comment: https://git.openjdk.org/jdk/pull/25044#issuecomment-3051978321
