jfx 8306707: Support pluggable image loading via javax.imageio

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:

Accept the fact that JavaFX will never be able to load additional image formats.
Create a public image loader API, and hope that developers in the JavaFX ecosystem will create image loader plugins.
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

Progress

[x] Change must not contain extraneous whitespace
[ ] Change requires a CSR request matching fixVersion jfx24 to be approved (needs to be created)
[x] Commit message must refer to an issue
[ ] Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)

Warning

⚠️ Patch contains a binary file (modules/javafx.graphics/src/test/resources/test/com/sun/javafx/iio/checker.bmp)

Issue

JDK-8306707: Support pluggable image loading via javax.imageio (Enhancement - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1593/head:pull/1593
$ git checkout pull/1593

Update a local copy of the PR:
$ git checkout pull/1593
$ git pull https://git.openjdk.org/jfx.git pull/1593/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1593

View PR using the GUI difftool:
$ git pr show -t 1593

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1593.diff

Webrev

Link to Webrev Comment

Oct 07 '24 15:10 mstr2

:wave: Welcome back mstrauss! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

Oct 07 '24 15:10 bridgekeeper[bot]

/reviewers 2 /csr

Oct 07 '24 15:10 mstr2

@mstr2 This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8306707: Support pluggable image loading via javax.imageio

Reviewed-by: jhendrikx, kcr, jdv

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 1 new commit pushed to the master branch:

688f7fa0168143c9c73dd8f17cbe668f9d19f79c: 8091673: Public focus traversal API for use in custom controls

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch. As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

Oct 07 '24 16:10 openjdk[bot]

Webrevs

Oct 07 '24 16:10 mlbridge[bot]

@mstr2 The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

Oct 07 '24 16:10 openjdk[bot]

/reviewers 2 reviewers /csr

Oct 07 '24 16:10 kevinrushforth

@mstr2 has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@mstr2 please create a CSR request for issue JDK-8306707 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

Oct 07 '24 16:10 openjdk[bot]

@kevinrushforth The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).

Oct 07 '24 16:10 openjdk[bot]

@kevinrushforth an approved CSR request is already required for this pull request.

Oct 07 '24 16:10 openjdk[bot]

With the restructuring to make the dependency optional at runtime, this seems like a reasonable approach to consider. I also want to make sure that we don't initialize the AWT toolkit unless an application tries to load an image file of a format that JavaFX doesn't support. It will take some time to review and test it.

Oct 15 '24 17:10 kevinrushforth

It is at all possible to split the image loaders from the desktop module into its own? I would think it will be useful for more than just JavaFX.

Oct 15 '24 17:10 nlisker

Any idea why FX has custom image loaders at all, and doesn't simply always delegate to ImageIO for this? If the dependency burden is high, it could be further reduced by removing built-in decoders? In other words, what's the advantage of the built-in decoders vs ImageIO ones?

Oct 15 '24 20:10 hjohn

With the restructuring to make the dependency optional at runtime, this seems like a reasonable approach to consider. I also want to make sure that we don't initialize the AWT toolkit unless an application tries to load an image file of a format that JavaFX doesn't support. It will take some time to review and test it.

I've used ImageIO with FX before, and it doesn't initialize AWT. You can check with this code I think:

import java.lang.reflect.InvocationTargetException;
import java.net.URI;

import javax.imageio.ImageIO;

public class ImageIOAWTTest {
    public static void main(String[] args) throws Exception {
        // Check if AWT classes are loaded before using ImageIO
        boolean awtLoadedBefore = isClassLoaded("java.awt.Toolkit");

        // Load an image using ImageIO (this should not trigger AWT initialization)
        ImageIO.read(URI.create("https://picsum.photos/200/300").toURL());

        // Check if AWT classes are loaded after using ImageIO
        boolean awtLoadedAfter = isClassLoaded("java.awt.Toolkit");

        System.out.println("AWT loaded before ImageIO: " + awtLoadedBefore);
        System.out.println("AWT loaded after ImageIO: " + awtLoadedAfter);
    }

    // Method to check if a class is loaded by the ClassLoader
    private static boolean isClassLoaded(String className) throws NoSuchMethodException, SecurityException, IllegalAccessException, InvocationTargetException {
      java.lang.reflect.Method m = ClassLoader.class.getDeclaredMethod("findLoadedClass", new Class[] { String.class });
      m.setAccessible(true);
      ClassLoader cl = ClassLoader.getSystemClassLoader();
      Object test1 = m.invoke(cl, className);
      return test1 != null;
    }
}

It will require --add-opens=java.base/java.lang=ALL-UNNAMED to run.

Oct 15 '24 20:10 hjohn

Just a general comment on other code in ImageStorage:

        byte[] header = new byte[getMaxSignatureLength()];
        try {
            ImageTools.readFully(stream, header);
        } catch (EOFException ignored) {
            return null;
        }

This code will take the largest possible signature length for known registered types (ie. JPG, GIF, BMP, PNG) and load those bytes. But if one of these formats could store a tiny image that would be smaller than the largest signature, then that image can't be loaded as this code would throw EOF and not return a suitable loader...

Oct 15 '24 21:10 hjohn

Just a general comment on other code in ImageStorage:
        byte[] header = new byte[getMaxSignatureLength()];
        try {
            ImageTools.readFully(stream, header);
        } catch (EOFException ignored) {
            return null;
        }
This code will take the largest possible signature length for known registered types (ie. JPG, GIF, BMP, PNG) and load those bytes. But if one of these formats could store a tiny image that would be smaller than the largest signature, then that image can't be loaded as this code would throw EOF and not return a suitable loader...

I've created a ticket to track this issue.

Oct 16 '24 14:10 mstr2

With the restructuring to make the dependency optional at runtime, this seems like a reasonable approach to consider. I also want to make sure that we don't initialize the AWT toolkit unless an application tries to load an image file of a format that JavaFX doesn't support. It will take some time to review and test it.

Independent of whether loading ImageIO will initialize AWT, I've modified the code such that ImageIO will only ever be loaded if we encounter an image format that JavaFX can't load.

Oct 16 '24 14:10 mstr2

As it was pointed out, we can't skip support for palette-based image formats. I've added support for indexed images with 1, 2, 4, and 8-bit indices, as well as opaque, non-premultiplied, and premultiplied colors.

Oct 17 '24 19:10 mstr2

It is at all possible to split the image loaders from the desktop module into its own? I would think it will be useful for more than just JavaFX.

That would still be dependent on the desktop module, so I don't see how that would help anything. It would make the desktop module a tiny bit smaller, but not enough to really matter.

When the JPMS module graph was devised in JDK 9, there was a lot of thought put into whether it made sense to break up the desktop module into pieces. I wasn't 'involved' so I don't know exactly what was looked at but anything broken out would have to be independent of the rest of the module or it wasn't worth it. java.datatransfer was the only case that was done.

Oct 17 '24 19:10 prrace

Any idea why FX has custom image loaders at all, and doesn't simply always delegate to ImageIO for this? If the dependency burden is high, it could be further reduced by removing built-in decoders? In other words, what's the advantage of the built-in decoders vs ImageIO ones?

It is true that there is duplication of functionality but the advantage is independence.

I've seen people complaining about needing java.awt.Desktop and would prefer not to have to do that. I agree with them. FX should have its own support. And that inevitably means duplication of functionality, but maybe duplication is the wrong way to think about it because these toolkits are supposed to be independent - even if they are interoperable.

In general dependencies on the the desktop module are an obstacle to making FX an independent toolkit. We already have that situation of course, but making it more common is the wrong direction, unless you think it an unrealistic goal But even then, mobile & embedded uses of FX wouldn't want to always have to drag in the desktop module.

Oct 17 '24 19:10 prrace

Mailing list message from Johan Vos on openjfx-dev:

On Thu, Oct 17, 2024 at 9:49?PM Phil Race <prr at openjdk.org> wrote:

In general dependencies on the the desktop module are an obstacle to making FX an independent toolkit. We already have that situation of course, but making it more common is the wrong direction, unless you think it an unrealistic goal But even then, mobile & embedded uses of FX wouldn't want to always have to drag in the desktop module.

I totally agree. It is a (maintenance) nightmare to make sure the whole java.desktop module is available and works on mobile and embedded, while only a tiny fraction of it is used. And even for purely desktop apps, the overhead of including the java.desktop module in an application is significant. I often see people comparing the binary size of a JavaFX app versus a similar Electron app, and it's a pity that the size of JavaFX apps is negatively influenced by something that is barely used (that is, most of the bytes in the java.desktop module are never used in JavaFX).

In the ideal world, I think a more granular javax.imageio module could be a solution. That could then be leveraged by the javafx modules. Theoretically, the java.desktop maintainers could leverage that module as well, and remove the internals from java.desktop, but that is not something we should discuss on this list.

- Johan -------------- next part -------------- An HTML attachment was scrubbed... URL: <https://mail.openjdk.org/pipermail/openjfx-dev/attachments/20241018/e27deb90/attachment.htm>

Oct 18 '24 08:10 mlbridge[bot]

2. This patch causes a failure in one of our closed white-box tests, which depends on internal API that has changed. It will be easy for us to fix it, but it will mean that down the road, when this is ready to go in, we will need to coordinate the timing of the integration with you.

I think @mstr2 can use /integrate delegate Skara command as mentioned at https://wiki.openjdk.org/display/SKARA/Pull+Request+Commands#PullRequestCommands-/integrate. So that @kevinrushforth can time this integration in future.

Nov 13 '24 04:11 jayathirthrao

/integrate delegate

Nov 13 '24 04:11 mstr2

@mstr2 Integration of this pull request has been delegated and may be completed by any project committer using the /integrate pull request command.

Nov 13 '24 04:11 openjdk[bot]

@jayathirthrao @hjohn Please review the latest changes.

Nov 13 '24 06:11 mstr2

I think @mstr2 can use /integrate delegate Skara command

Thanks, Jay, I was going to suggest the same as an option for this.

Nov 13 '24 12:11 kevinrushforth

/integrate

Nov 13 '24 20:11 kevinrushforth

Going to push as commit 72af9e2366727a83ca1eace73e1c337cc4f8a255. Since your change was applied there has been 1 commit pushed to the master branch:

688f7fa0168143c9c73dd8f17cbe668f9d19f79c: 8091673: Public focus traversal API for use in custom controls

Your commit was automatically rebased without conflicts.

Nov 13 '24 20:11 openjdk[bot]

@kevinrushforth Pushed as commit 72af9e2366727a83ca1eace73e1c337cc4f8a255.

:bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Nov 13 '24 20:11 openjdk[bot]

too late to the party, but

Description	Resource	Type	Path	Location
The method load(int, double, double, boolean, boolean, float, float) of type GIFImageLoader2 should be tagged with @Override since it actually overrides a superinterface method	GIFImageLoader2.java	Java Problem	/graphics/src/main/java/com/sun/javafx/iio/gif	line 201

Nov 13 '24 22:11 andy-goryachev-oracle

too late to the party, but

Description	Resource	Type	Path	Location
The method load(int, double, double, boolean, boolean, float, float) of type GIFImageLoader2 should be tagged with @Override since it actually overrides a superinterface method	GIFImageLoader2.java	Java Problem	/graphics/src/main/java/com/sun/javafx/iio/gif	line 201

I'm sure there are other cases like this. Time for a clean-up ticket?

Nov 13 '24 23:11 mstr2

jfx jfx copied to clipboard

8306707: Support pluggable image loading via javax.imageio

Progress

Warning

Issue

Reviewing

Webrev

Webrevs

jfx
jfx copied to clipboard