On Tue, 3 Sep 2024 17:07:46 GMT, Sergey Bylokhov <s...@openjdk.org> wrote:
>> Daniel Gredler has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - JDK-8337681: add tests for TYPE_4BYTE_ABGR
>> - JDK-8337681: add tests for TYPE_INT_RGB
>
> src/java.desktop/share/classes/com/sun/imageio/plugins/png/PNGImageWriter.java
> line 924:
>
>> 922: for (int row = minY + yOffset; row < minY + height; row +=
>> ySkip) {
>> 923: Raster ras;
>> 924: if (image instanceof BufferedImage bi) {
>
> Do we really need to run these two blocks in the loop for each iteration?
Hi Sergey! The current logic does make it very clear where and how `ras` is
initialized, which is nice. The top two initialization blocks (for
`BufferedImage`s and tiled images) could in theory be hoisted outside the loop,
but the third one cannot (and we might have to jump through some hoops to
guarantee to the compiler that the variable is initialized if we start
initializing it sometimes in one place and sometimes in another).
AFAICT the `BufferedImage.getRaster()` call is not doing any work (just
providing a reference to an instance variable), but the `getTile(...)` call
might be doing some extra work, depending on the specific instance type (but in
any event will not be copying raster data, which was by far the biggest waste
here).
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20432#discussion_r1743656653
