8388450: ImageIO.write(SwingFXUtils.fromFXImage()) creates 0 length JPEG for some images - #2254
8388450: ImageIO.write(SwingFXUtils.fromFXImage()) creates 0 length JPEG for some images#2254prsadhuk wants to merge 2 commits into
Conversation
…PEG for some images
|
👋 Welcome back psadhukhan! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
The total number of required reviews for this PR has been set to 2 based on the presence of this label: |
Webrevs
|
| for (int y = 0; y < ih; y++) { | ||
| pr.getPixels(0, y, iw, 1, format, pixels, 0, iw); | ||
| for (int pixel : pixels) { | ||
| if ((pixel >>> 24) != 0xff) { |
There was a problem hiding this comment.
I would have done
((pixel & 0xff000000) != 0xff000000)
but I think there is no difference in performance whatsoever
There was a problem hiding this comment.
then let it remain same :-)
| if (color.getOpacity() != 1.0) { | ||
| int[] pixels = new int[iw]; | ||
| WritablePixelFormat<IntBuffer> format = | ||
| PixelFormat.getIntArgbPreInstance(); |
There was a problem hiding this comment.
just curious: why is this line broken? it fits in 120 columns just fine. time to update the formatting rules?
There was a problem hiding this comment.
updated..it was just to keep line consistent with the javadoc beneath
| @@ -250,16 +256,15 @@ public static BufferedImage fromFXImage(Image img, BufferedImage bimg) { | |||
| int ih = (int) img.getHeight(); | |||
| PixelFormat<?> fxFormat = pr.getPixelFormat(); | |||
| boolean srcPixelsAreOpaque = false; | |||
| boolean opacityMatters = bimg == null || | |||
There was a problem hiding this comment.
minor: this is calculated even when it's not needed. could it be moved to L267?
| if (bimg != null && | ||
| (bimg.getType() == BufferedImage.TYPE_INT_BGR || | ||
| bimg.getType() == BufferedImage.TYPE_INT_RGB)) { | ||
| case BYTE_INDEXED: |
There was a problem hiding this comment.
question: this switch statement is missing BYTE_BGRA. is this a problem?
There was a problem hiding this comment.
yes, missed...added
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I guess PixelBuffer supports only INT_ARGB_PRE and BYTE_BGRA_PRE
Pixel data should be stored either in an IntBuffer using a PixelFormat of type INT_ARGB_PRE or in a ByteBuffer using a PixelFormat 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
|
Something is wrong: the reproducer fails to render the PNG image attached to the ticket, |
I guess it will fail as ImgUtil class which is copied implementation of SwingFXUtils is not having this PR fix.. ./jdk/bin/java @C:/Users/Prasantas/dev/javafx/jfx/rt/build/run.args ImageWriteTest_8388450.java |
you are right, I am sorry. this also means the |
Hmm. Maybe as a stop-gap, but it would not be acceptable for any |
What can we do? We could provide a utility in Or we can keep duplicating the code and potentially risk tripping over the same situation in the future. |
|
or we could port ImageIO parts to work with JPG/PNG in javafx. |
|
Applied these changes to |
|
@Ziad-Mid could you be the second reviewer please? |
|
I've filed https://bugs.openjdk.org/browse/JDK-8390345 to avoid duplicating image i/o code. |
If an image has RGBA encoding and all pixels are opaque, then
SwingFXUtils.fromFXImage(image, null)selects TYPE_INT_ARGB_PRE pixel format but the JPEG ImageIO writer cannot encode this alpha-bearing image pixel format since it has no support for it and returns false; so an empty byte array is created.Issue is when
bimg(used to store the returned pixel data fromfromFXImage) is null, fromFXImageuses the JavaFX PixelReader’s storage format, not the alpha values of individual pixels.For an RGBA PNG, JavaFX commonly decodes the pixels into premultiplied ARGB/BGRA, so
fxFormat.getType()is one of INT_ARGB_PRE/BYTE_BGRA_PRE sogetBestBufferedImageTypereturnsBufferedImage.TYPE_INT_ARGB_PREbecause the pixel format has an alpha component. It does not inspect whether every alpha value happens to be 255.The existing
fromFXImagecode only callscheckFXImageOpaque()when the caller supplies a non-nullbImgBufferedImage.If
bImgis null, all-opaque RGBA PNG produces an alpha-capable BufferedImage i.e., BufferedImage.TYPE_INT_ARGB_PREand since JPEG has no standard alpha channel so it cannot store transparency, so when
ImageIO.write(image, "jpg", out)runs, ImageIO looks for a registered JPEG writer which can encode that pre-multiplied-alpha image type,but the JPEG writer rejects an alpha-bearing BufferedImage, so
ImageIO.writefinds no suitable writer and returns falseso OutputStream is not written into and have 0 bytes
[Basically the JPEG writer does not check whether the alpha values are all 255; it only sees that the input image has an alpha channel and declines to write it]
The proposed JavaFX change avoids the rejection for an RGBA-formatted but fully opaque image by returning TYPE_INT_RGB, which JPEG can encode.
ie., for a JavaFX image with an alpha-capable format but only opaque pixels,
fromFXImage(image, null)is made to choose RGB format rather than ARGB.Additionally, checkFXImageOpaque is improved to scan one row at a time instead of costly full-image scan so that unnecessary Color object for each pixels is not created just to inspect alpha.
A regression subtest is added to existing testcase
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/2254/head:pull/2254$ git checkout pull/2254Update a local copy of the PR:
$ git checkout pull/2254$ git pull https://git.openjdk.org/jfx.git pull/2254/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 2254View PR using the GUI difftool:
$ git pr show -t 2254Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/2254.diff
Using Webrev
Link to Webrev Comment