On Mon, 7 Oct 2024 15:13:17 GMT, Michael Strauß <mstra...@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
