RFR: 8306707: Support pluggable image loading via javax.imageio
John Hendrikx
jhendrikx at openjdk.org
Tue Oct 15 23:07:17 UTC 2024
On Mon, 7 Oct 2024 15:13:17 GMT, Michael Strauß <mstrauss at openjdk.org> wrote:
> This PR is an improved version of #1093.
>
> JavaFX can load BMP, GIF, PNG, and JPEG images with its built-in image loaders. It has been a long-standing request to support more image formats, most notably (but not limited to) SVG. However, adding more built-in image loaders is a significant effort not only in creating the functionality, but also in maintaining the additional dependencies.
>
> This will probably not happen any time soon, so we are left with three alternatives:
> 1. Accept the fact that JavaFX will never be able to load additional image formats.
> 2. Create a public image loader API, and hope that developers in the JavaFX ecosystem will create image loader plugins.
> 3. Leverage the existing Java Image I/O API.
>
> From these options, I think we should simply support existing Java APIs; both because it is the shortest and most realistic path forward, but also because I don't think it is sensible to bifurcate pluggable image loading in the Java ecosystem.
>
> Of course, Java Image I/O is a part of the `java.desktop` module, which as of now, all JavaFX applications require. However, it has been noted in the previous PR that we shouldn't lock JavaFX into the `java.desktop` dependency even further.
>
> I've improved this PR to not permanently require the `java.desktop` dependency: if the module is present, then JavaFX will use Image I/O for image formats that it can't load with the built-in loaders; if the module is not present, only the built-in loaders are available.
>
> I have prepared a small sample application that showcases how the feature can be used to load SVG images in a JavaFX application: https://github.com/mstr2/jfx-imageio-sample
I left some comments.
modules/javafx.graphics/src/main/java/com/sun/javafx/iio/ImageStorage.java line 201:
> 199: return null;
> 200: }
> 201: }
Can we document why this is done? If I had to guess: loading `XImageLoaderFactory`, even though it is present in this module, may fail loading dependent classes (`ImageIO`).
modules/javafx.graphics/src/main/java/com/sun/javafx/iio/javax/XImageLoader.java line 203:
> 201: case TYPE_BYTE_INDEXED -> "TYPE_BYTE_INDEXED";
> 202: default -> Integer.toString(image.getType());
> 203: });
Hm, since we can't select which format an image reader will output, lacking conversions for these can mean that a lot of AWT supported image formats still can't be loaded in FX. For some formats that can have multiple representations that can mean some images load while others won't. For example, if GIF wasn't natively supported, then it wouldn't work via ImageIO either (since it is TYPE_BYTE_INDEXED afaik).
I also wonder what SVG will generally output (`TYPE_BYTE_GRAY`?)
modules/javafx.graphics/src/main/java/com/sun/javafx/iio/javax/XImageLoader.java line 217:
> 215: }
> 216:
> 217: return ByteBuffer.wrap(Arrays.copyOf(data, size - offset));
This doesn't look correct. If `offset` isn't zero, then dropping the last few bytes seems incorrect. It should start the copy from `offset`?
I would think this should be: `ByteBuffer.wrap(data, offset, size - offset)` (avoids copy but may waste some space).
Or if you still want to make a copy, then `Arrays.copyOfRange(data, offset, size)`.
Note: I have the impression that calling `getData` means the bank won't be modified anymore, so there may not be a need to copy it.
modules/javafx.graphics/src/main/java/com/sun/javafx/iio/javax/XImageLoader.java line 230:
> 228: }
> 229:
> 230: return IntBuffer.wrap(Arrays.copyOf(data, size - offset));
See other comment.
modules/javafx.graphics/src/main/java/com/sun/javafx/image/PixelUtils.java line 215:
> 213: getB2BConverter(PixelGetter<ByteBuffer> src, PixelSetter<ByteBuffer> dst)
> 214: {
> 215: if (src == ByteRgba.getter) {
minor: I'd be in favor of fixing this formatting. I know it means well, but it really isn't improving readability. Perhaps it is more of a job for a `switch` or `Map` lookup.
modules/javafx.graphics/src/main/java/com/sun/javafx/image/impl/BaseByteToByteConverter.java line 296:
> 294: {
> 295: srcscanbytes -= w * 3;
> 296: srcscanbytes -= w * 4;
This really doesn't look right. Should it not be `dstscanbytes` on the 2nd line, and also multiplied by `3` instead of `4`?
It looks like this was incorrect in the original?
modules/javafx.graphics/src/main/java/com/sun/javafx/image/impl/BaseByteToByteConverter.java line 318:
> 316: {
> 317: srcscanbytes -= w * 3;
> 318: srcscanbytes -= w * 4;
Same comment.
modules/javafx.graphics/src/main/java/com/sun/javafx/image/impl/IntBgr.java line 43:
> 41: public static final IntPixelAccessor accessor = Accessor.instance;
> 42:
> 43: public static IntToIntPixelConverter ToIntArgbPreConverter() {
This method is not following Java naming conventions:
Suggestion:
public static IntToIntPixelConverter toIntArgbPreConverter() {
modules/javafx.graphics/src/main/java/com/sun/javafx/image/impl/IntBgr.java line 68:
> 66: int b = (pixel >> 16) & 0xff;
> 67: return (255 << 24) | (r << 16) | (g << 8) | b;
> 68: }
Not sure if we optimizing here, but could eliminate two shifts like this:
Suggestion:
public int getArgb(int[] arr, int offset) {
int pixel = arr[offset];
int r = pixel & 0xff;
int g = pixel & 0xff00;
int b = pixel & 0xff0000;
return (255 << 24) | (r << 16) | g | (b >> 16);
}
modules/javafx.graphics/src/main/java/com/sun/javafx/image/impl/IntBgr.java line 72:
> 70: @Override
> 71: public int getArgbPre(int[] arr, int offset) {
> 72: return PixelUtils.NonPretoPre(getArgb(arr, offset));
It seems `PixelUtils` contains methods not following Java naming conventions. This should be `nonPreToPre` IMHO (and others as well).
modules/javafx.graphics/src/main/java/com/sun/javafx/image/impl/IntRgb.java line 43:
> 41: public static final IntPixelAccessor accessor = Accessor.instance;
> 42:
> 43: public static IntToIntPixelConverter ToIntArgbPreConverter() {
This method is not following Java naming conventions:
Suggestion:
public static IntToIntPixelConverter toIntArgbPreConverter() {
modules/javafx.graphics/src/main/java/com/sun/javafx/image/impl/IntRgb.java line 63:
> 61: @Override
> 62: public int getArgb(int[] arr, int offset) {
> 63: return arr[offset] | (0xff << 24);
Second variant I seen of `255 << 24` now; perhaps this may be better to make into a constant for clarity? Or if we're going for short `-1 << 24`? :)
Suggestion:
private static final int OPAQUE = 0xff000000;
modules/javafx.graphics/src/main/java/com/sun/prism/Image.java line 225:
> 223: // GRAY, RGB, BGRA_PRE, and INT_ARGB_PRE are directly supported by Prism.
> 224: // We'll need to convert all other formats that we might encounter to one of the supported formats.
> 225: // TODO: 3D - need a way to handle pre versus non-Pre
Is this TODO still relevant?
modules/javafx.graphics/src/main/java/javafx/scene/image/Image.java line 75:
> 73: * <li><a href="http://www.libpng.org/pub/png/spec/">PNG</a></li>
> 74: * </ul>
> 75: * For all other formats, JavaFX uses the {@link javax.imageio Java Image I/O API} on supported platforms.
This doesn't look like a valid javadoc link; in Eclipse at least it doesn't do anything. Perhaps:
Suggestion:
* For all other formats, JavaFX uses the {@link javax.imageio.ImageIO Java Image I/O API} on supported platforms.
-------------
PR Review: https://git.openjdk.org/jfx/pull/1593#pullrequestreview-2370443197
PR Review Comment: https://git.openjdk.org/jfx/pull/1593#discussion_r1802046200
PR Review Comment: https://git.openjdk.org/jfx/pull/1593#discussion_r1802069617
PR Review Comment: https://git.openjdk.org/jfx/pull/1593#discussion_r1802078956
PR Review Comment: https://git.openjdk.org/jfx/pull/1593#discussion_r1802079276
PR Review Comment: https://git.openjdk.org/jfx/pull/1593#discussion_r1802083595
PR Review Comment: https://git.openjdk.org/jfx/pull/1593#discussion_r1802087453
PR Review Comment: https://git.openjdk.org/jfx/pull/1593#discussion_r1802087587
PR Review Comment: https://git.openjdk.org/jfx/pull/1593#discussion_r1802104560
PR Review Comment: https://git.openjdk.org/jfx/pull/1593#discussion_r1802100014
PR Review Comment: https://git.openjdk.org/jfx/pull/1593#discussion_r1802103877
PR Review Comment: https://git.openjdk.org/jfx/pull/1593#discussion_r1802103126
PR Review Comment: https://git.openjdk.org/jfx/pull/1593#discussion_r1802102566
PR Review Comment: https://git.openjdk.org/jfx/pull/1593#discussion_r1802110851
PR Review Comment: https://git.openjdk.org/jfx/pull/1593#discussion_r1801838924
More information about the openjfx-dev
mailing list