Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8306707: Support pluggable image loading via javax.imageio #1593

Closed
wants to merge 34 commits into from

Conversation

mstr2
Copy link
Collaborator

@mstr2 mstr2 commented Oct 7, 2024

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


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)
  • Change requires CSR request JDK-8343315 to be approved

Warning

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

Issues

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

Reviewers

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

Using Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 7, 2024

👋 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.

@mstr2
Copy link
Collaborator Author

mstr2 commented Oct 7, 2024

/reviewers 2
/csr

@openjdk
Copy link

openjdk bot commented Oct 7, 2024

@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:

  • 688f7fa: 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.

@openjdk openjdk bot added the rfr Ready for review label Oct 7, 2024
@mlbridge
Copy link

mlbridge bot commented Oct 7, 2024

@openjdk
Copy link

openjdk bot commented Oct 7, 2024

@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).

@kevinrushforth
Copy link
Member

/reviewers 2 reviewers
/csr

@kevinrushforth kevinrushforth self-requested a review October 7, 2024 16:36
@openjdk openjdk bot added the csr Need approved CSR to integrate pull request label Oct 7, 2024
@openjdk
Copy link

openjdk bot commented Oct 7, 2024

@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.

@openjdk
Copy link

openjdk bot commented Oct 7, 2024

@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).

@openjdk
Copy link

openjdk bot commented Oct 7, 2024

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

@kevinrushforth
Copy link
Member

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.

@nlisker
Copy link
Collaborator

nlisker commented Oct 15, 2024

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.

@hjohn
Copy link
Collaborator

hjohn commented Oct 15, 2024

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?

@hjohn
Copy link
Collaborator

hjohn commented Oct 15, 2024

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.

@hjohn
Copy link
Collaborator

hjohn commented Oct 15, 2024

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...

Copy link
Collaborator

@hjohn hjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some comments.

@jayathirthrao
Copy link
Member

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.

@mstr2
Copy link
Collaborator Author

mstr2 commented Nov 13, 2024

/integrate delegate

@openjdk openjdk bot added the delegated label Nov 13, 2024
@openjdk
Copy link

openjdk bot commented Nov 13, 2024

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

@openjdk openjdk bot removed the csr Need approved CSR to integrate pull request label Nov 13, 2024
@mstr2
Copy link
Collaborator Author

mstr2 commented Nov 13, 2024

@jayathirthrao @hjohn Please review the latest changes.

Copy link
Collaborator

@hjohn hjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, I will re-approve if you decide to apply my suggestion for the exception messages.

@@ -211,6 +211,21 @@ public static int[] computeDimensions(int sourceWidth, int sourceHeight,
return new int[]{finalWidth, finalHeight};
}

public static void validateMaxDimensions(double width, double height, double scaleFactor) {
if (width * scaleFactor > Integer.MAX_VALUE) {
throw new IllegalArgumentException("Image width exceeds maximum value");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may want to include the values used to reach this conclusion here and with the other 2 exceptions; if these exceptions ever come up, that's the first thing a developer will likely want to know (which may be you if this is posted in an issue).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed as suggested.

@kevinrushforth
Copy link
Member

I think @mstr2 can use /integrate delegate Skara command

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

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All looks good. I'll reapprove if you decide to make the changes suggested by John.

I'll integrate it later this morning (Pacific time) if all is settled.

@openjdk openjdk bot added the ready Ready to be integrated label Nov 13, 2024
@openjdk openjdk bot removed the ready Ready to be integrated label Nov 13, 2024
@openjdk openjdk bot added the ready Ready to be integrated label Nov 13, 2024
@kevinrushforth
Copy link
Member

/integrate

@openjdk
Copy link

openjdk bot commented Nov 13, 2024

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

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

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Nov 13, 2024
@openjdk openjdk bot closed this Nov 13, 2024
@openjdk openjdk bot removed ready Ready to be integrated rfr Ready for review delegated labels Nov 13, 2024
@openjdk
Copy link

openjdk bot commented Nov 13, 2024

@kevinrushforth Pushed as commit 72af9e2.

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

@mstr2 mstr2 deleted the feature/ximageloader branch November 13, 2024 21:15
@andy-goryachev-oracle
Copy link
Contributor

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

@mstr2
Copy link
Collaborator Author

mstr2 commented Nov 13, 2024

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?

@andy-goryachev-oracle
Copy link
Contributor

nope, only this one (we've cleaned up the rest)
can you enable this warning in your IDE?
and yes, could you please create a ticket? if not, I'll create one tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated
Development

Successfully merging this pull request may close these issues.

8 participants