On Tue, 27 May 2025 13:32:33 GMT, Jayathirth D V <j...@openjdk.org> wrote:
>> Jeremy Wood has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8356137: Adding copyright to GifComparison > > src/java.desktop/share/classes/sun/awt/image/GifImageDecoder.java line 419: > >> 417: // are NOT the transparent pixel: we need to replace it >> 418: // with the appropriate transparent pixel. >> 419: boolean replaceTransPixelsInSavedImage = save && > > We can move this check to the place where we actually create and initialize > saved image. I think this thread is moot having addressed https://github.com/openjdk/jdk/pull/25044#issuecomment-2912699099 . > src/java.desktop/share/classes/sun/awt/image/GifImageDecoder.java line 436: > >> 434: } >> 435: runstart = -1; >> 436: if (replaceTransPixelsInSavedImage) { > > We are hitting this check for each transparent pixel for all frames with > transparent pixel index > 0 and not initial frame until saved_image is not > null. In the .gif present in this PR we are checking this condition for each > transparent pixel of 2nd and 3rd frame. If GIF contains many frames and > disposal method save comes in later part of animation we will hit this check > many times. > > I think its better to just initialize saved_image with proper transparent > pixel index when it is getting created. Then we can completely remove state > management of `replaceTransPixelsInSavedImage` and this check. OK; I think this thread is moot having addressed https://github.com/openjdk/jdk/pull/25044#issuecomment-2912699099 . > Is this way of checking all frames useful in any other PR's which you have > raised? No, I don't think so. (I liked the assurance that every frame was identical, but technically that is beyond the scope of each individual unit test.) I changed this method to only inspect the last frame. This made the GifComparison's main() class much less useful, so I removed it. So now the GifComparison class is more limited in scope to unit tests -- and it's no longer a generic tool to identify new problems. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/25044#discussion_r2109860678 PR Review Comment: https://git.openjdk.org/jdk/pull/25044#discussion_r2109860766 PR Review Comment: https://git.openjdk.org/jdk/pull/25044#discussion_r2109856464
