On Mon, 10 Aug 2026 20:49:24 GMT, Andy Goryachev <[email protected]> wrote:
>> Prasanta Sadhukhan has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Review comment
>
> modules/javafx.swing/src/main/java/javafx/embed/swing/SwingFXUtils.java line
> 209:
>
>> 207: int[] pixels = new int[iw];
>> 208: WritablePixelFormat<IntBuffer> format =
>> 209: PixelFormat.getIntArgbPreInstance();
>
> just curious: why is this line broken? it fits in 120 columns just fine.
> time to update the formatting rules?
updated..it was just to keep line consistent with the javadoc beneath
> modules/javafx.swing/src/main/java/javafx/embed/swing/SwingFXUtils.java line
> 213:
>
>> 211: pr.getPixels(0, y, iw, 1, format, pixels, 0, iw);
>> 212: for (int pixel : pixels) {
>> 213: if ((pixel >>> 24) != 0xff) {
>
> I would have done
> `((pixel & 0xff000000) != 0xff000000)`
>
> but I think there is no difference in performance whatsoever
then let it remain same :-)
> modules/javafx.swing/src/main/java/javafx/embed/swing/SwingFXUtils.java line
> 259:
>
>> 257: PixelFormat<?> fxFormat = pr.getPixelFormat();
>> 258: boolean srcPixelsAreOpaque = false;
>> 259: boolean opacityMatters = bimg == null ||
>
> minor: this is calculated even when it's not needed. could it be moved to
> L267?
ok..modified
> modules/javafx.swing/src/main/java/javafx/embed/swing/SwingFXUtils.java line
> 266:
>
>> 264: case INT_ARGB:
>> 265: case BYTE_BGRA_PRE:
>> 266: case BYTE_INDEXED:
>
> question: this switch statement is missing `BYTE_BGRA`. is this a problem?
yes, missed...added
> tests/system/src/test/java/test/javafx/embed/swing/SwingFXUtilsTest.java line
> 89:
>
>> 87: @Test
>> 88: public void testOpaqueArgbImageCanBeWrittenAsJpeg() throws Exception
>> {
>> 89: WritableImage image = new WritableImage(2, 1);
>
> would it make sense to iterate over every `PixelFormat.Type` using
> `WritableImage(PixelBuffer)` constructor to make sure we are getting a
> meaningful result in each case?
I guess PixelBuffer supports only INT_ARGB_PRE and BYTE_BGRA_PRE
> Pixel data should be stored either in an
> [IntBuffer](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/nio/IntBuffer.html)
> using a
> [PixelFormat](https://openjfx.io/javadoc/21/javafx.graphics/javafx/scene/image/PixelFormat.html)
> of type INT_ARGB_PRE or in a
> [ByteBuffer](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/nio/ByteBuffer.html)
> using a
> [PixelFormat](https://openjfx.io/javadoc/21/javafx.graphics/javafx/scene/image/PixelFormat.html)
> of type BYTE_BGRA_PRE.
INT_ARGB_PRE is already being tested..there's not much test coverage to iterate
so I guess let it
stay small and target the reported behavior
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/2254#discussion_r3754949836
PR Review Comment: https://git.openjdk.org/jfx/pull/2254#discussion_r3754929985
PR Review Comment: https://git.openjdk.org/jfx/pull/2254#discussion_r3754930520
PR Review Comment: https://git.openjdk.org/jfx/pull/2254#discussion_r3754931281
PR Review Comment: https://git.openjdk.org/jfx/pull/2254#discussion_r3754944790
