8361286: Allow enabling of background loading for images loaded from an InputStream #1875

hjohn · 2025-08-18T09:35:23Z

Support background loading of raw input streams

Fixed generics (mix up of two ImageLoader types)
Removed unused code for handling headers, methods, request parameters
Use long for progress as streams may exceed 2 GB
Improved documentation of Image regarding background loading

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change requires a CSR request matching fixVersion jfx26 to be approved (needs to be created)
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

JDK-8361286: Allow enabling of background loading for images loaded from an InputStream (Enhancement - P4)

Reviewers

Andy Goryachev (@andy-goryachev-oracle - Reviewer)
Jayathirth D V (@jayathirthrao - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1875

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-08-18T09:35:41Z

👋 Welcome back jhendrikx! 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.

openjdk · 2025-08-18T09:38:33Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

- Fixed generics (mix up of two ImageLoader types) - Removed unused code for handling headers, methods, request parameters - Use long for progress as streams max exceed 2 GB - Improved documentation of Image regarding background loading

mlbridge · 2025-08-18T14:04:40Z

Webrevs

kevinrushforth · 2025-08-18T19:52:28Z

Reviewers: @kevinrushforth @jayathirthrao

/reviewers 2

openjdk · 2025-08-18T19:52:56Z

@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 1 Reviewer, 1 Author).

kevinrushforth · 2025-08-18T19:55:04Z

This will need a CSR. The newly proposed constructors should have an @since tag.

/csr needed

openjdk · 2025-08-18T19:55:23Z

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

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

modules/javafx.graphics/src/main/java/javafx/scene/image/Image.java

nlisker · 2025-08-18T20:14:48Z

Do we still use @NamedArg? I thought it's a remnant of pre-Java 8 where parameter names weren't saved.

hjohn · 2025-08-18T20:16:19Z

Do we still use @NamedArg? I thought it's a remnant of pre-Java 8 where parameter names weren't saved.

I don't know about the NamedArg specifics, but I do know that saving parameter names is optional and I think still off by default (ie. you need to pass a compiler option to save the names). Other than that, I just copied what was there and kept the same format.

mstr2 · 2025-08-18T20:33:08Z

Do we still use @NamedArg? I thought it's a remnant of pre-Java 8 where parameter names weren't saved.

FXMLoader uses the @NamedArg annotation to associate XML attribute values with constructor arguments.

andy-goryachev-oracle · 2025-08-18T20:38:54Z

FXMLoader uses the @NamedArg annotation to associate XML attribute values with constructor arguments.

I wonder if this should be explained in the @NamedArg javadoc.

hjohn · 2025-08-18T20:42:20Z

FXMLoader uses the @NamedArg annotation to associate XML attribute values with constructor arguments.

I wonder if this should be explained in the @NamedArg javadoc.

It probably should, I never use FXML, but I should have guessed it was for that purpose :)

andy-goryachev-oracle

Looks very good!

Noticed it does not close the input stream with a synchronous constructor (it does with the async one).

some minor comments and suggestions.

modules/javafx.graphics/src/main/java/com/sun/javafx/runtime/async/AbstractRemoteResource.java

modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java

modules/javafx.graphics/src/main/java/javafx/scene/image/Image.java

andy-goryachev-oracle · 2025-08-19T21:24:53Z

modules/javafx.graphics/src/test/java/test/javafx/scene/image/ImageTest.java

@@ -154,6 +155,35 @@ public void loadImageAsyncProgressTest() {
        assertTrue(p3 == p4);
    }

+    @Test


Do you think it would make sense to add a test that actually loads a valid image?

I don't think that can be done with the stub toolkit This test is specifically stubbing/mocking everything out to verify that Image works correctly. However, there are many areas where (real) images are loaded already, and all the code involved (excluding the new parts in Image) was already there in one way or another as the URL variants that allowed background loading is also just an InputStream with some extra steps to determine their size (for progress).

modules/javafx.graphics/src/test/java/test/javafx/scene/image/ImageTest.java

andy-goryachev-oracle · 2025-08-20T16:29:21Z

@NamedArg

FYI Created https://bugs.openjdk.org/browse/JDK-8365864 but if someone wants to do it, I can send it their way.

jayathirthrao

Change looks to me apart from some questions.

Will also run tests on our CI system and check.

jayathirthrao · 2025-08-21T06:51:18Z

modules/javafx.graphics/src/main/java/com/sun/javafx/runtime/async/AbstractRemoteResource.java

            return processStream(stream);
        }
-        finally {
-            stream.close();


Do we have to continue closing the stream when background loading is happening with URL?

I see that the resource is getting closed.

jayathirthrao · 2025-08-21T06:54:19Z

modules/javafx.graphics/src/main/java/com/sun/javafx/runtime/async/AbstractRemoteResource.java


-    protected AbstractRemoteResource(String url, String method, String outboundContent, AsyncOperationListener<T> listener) {


I see that outboundContent calls are no where used. Is this the reason we are cleaning up this code?

Yeah, I generally remove code that I see is unused, if it otherwise would have needed to be adjusted.

andy-goryachev-oracle · 2025-08-21T14:29:47Z

@hjohn in case you missed my earlier comment:

Noticed it does not close the input stream with a synchronous constructor (it does with the async one).

you can try it with the monkey tester from this branch https://github.com/andy-goryachev-oracle/MonkeyTest/tree/image.from.stream

or simply

            byte[] b = ImageTools.writePNG(src);
            ByteArrayInputStream in = new ByteArrayInputStream(b) {
                @Override
                public void close() throws IOException {
                    System.out.println("Closed " + Thread.currentThread());
                }
            };

            // requires JDK-8361286: Allow enabling of background loading for images loaded from an InputStream
            Image im = new Image(in, loadInBackground);
            imageView.setImage(im);
        } catch (IOException e) {
            e.printStackTrace();
        }

hjohn · 2025-08-21T16:45:09Z

@hjohn in case you missed my earlier comment:

Noticed it does not close the input stream with a synchronous constructor (it does with the async one).

you can try it with the monkey tester from this branch https://github.com/andy-goryachev-oracle/MonkeyTest/tree/image.from.stream

or simply
            byte[] b = ImageTools.writePNG(src);
            ByteArrayInputStream in = new ByteArrayInputStream(b) {
                @Override
                public void close() throws IOException {
                    System.out.println("Closed " + Thread.currentThread());
                }
            };

            // requires JDK-8361286: Allow enabling of background loading for images loaded from an InputStream
            Image im = new Image(in, loadInBackground);
            imageView.setImage(im);
        } catch (IOException e) {
            e.printStackTrace();
        }

A good question would be what the behavior should be. For the URL variants, since the Image class is creating the stream in both cases, it makes sense that it closes that stream.

For InputStream parameters, the standard practice is to leave closing up to the caller, unless it is a wrapper (like BufferedInputStream). So, I think how the synchronous variant does it is probably the correct approach.

However, when consuming the stream asynchronously, it is very hard for users to know when to safely close the stream as you basically handed ownership of the stream to another thread (you can't use the stream for anything while background loading is in progress...) The only way to know when the other thread is done with the stream is to look at the progress property.

So, I think there are a few options:

Document that we'll be closing the stream in the background loading case; effectively, since you're giving this InputStream to be used by a different thread, the ownership of the stream transfers from the user to Image.
Document that you need to hook into the progress property to know when to close the stream (yikes!)
Don't provide the background loading option for InputStream (ie. don't integrate this PR, or only the documentation and clean-up bits)
Provide alternative API that takes Supplier<InputStream>, very powerful, and it would be clear who owns the stream and who closes it, and this would work for both sync/async.

A prime motivation for allowing input streams for background loading was that the URL API is limited (you can't do authentication, use POST, or provide a body). In those cases you're forced to InputStream but then forego the option to load in the background, meaning that you can't have a progress indicator for large images, or show progressive images like with PJPEG.

andy-goryachev-oracle · 2025-08-21T16:54:56Z

re: closing stream

I wonder if we should close the stream in both sync and async variants (and document that). I would say there should be no reason not to close - suggesting that if the application wants not to close it's surely a bad choice for the application.

For example, one can think of a situation where multiple images are stored within the same stream, but there should be a reliable way for the application to specify the size of each image (and then the app can provide a derived InputStream which EOFs when the end of an image is reached).

Another alternative is to add another boolean closeStream.

What do you think?

hjohn · 2025-08-21T17:20:07Z

re: closing stream

I wonder if we should close the stream in both sync and async variants (and document that). I would say there should be no reason not to close - suggesting that if the application wants not to close it's surely a bad choice for the application.

For example, one can think of a situation where multiple images are stored within the same stream, but there should be a reliable way for the application to specify the size of each image (and then the app can provide a derived InputStream which EOFs when the end of an image is reached).

Another alternative is to add another boolean closeStream.

What do you think?

It's unusual to close the stream when you're not the owner (although some API's do that, they are often obscure and not used that much). Changing this behavior for the sync variant would be a breaking change, so I'm not sure we want to even go there.

Adding another boolean is of course an option, but I think that if we're going for that route, going for a Supplier would be more clear API and more powerful (in the internal code, we basically already have that abstraction as it can abstract over both the URL and InputStream variants with just one API).

If we go the Supplier route, we can probably also unify the URL and InputStream constructors to go through the same route, moving the URL opening logic into Image class and removing it from the internal code.

andy-goryachev-oracle · 2025-08-21T17:33:07Z

I like the Supplier idea, though I am not sure how the async path would work (unless you add the closeStream parameter).

kevinrushforth · 2025-08-21T17:46:56Z

We need to leave the existing synchronous constructors alone, other than to document that they do not close the input stream (which is what we do in the Font::loadFont methods that take an InputStream).

As for the new constructors, I think the options are:

Keep it as you propose, where the async variant always closes the stream (the rationale being that the caller has transferred ownership of the stream)
Change the new constructors to never close the stream; this would be OK as long as we provide an easy way for the caller to know when they should do it
Add a Supplier to the constructors (I'm a little fuzzy on the details of this one)

andy-goryachev-oracle · 2025-08-21T17:51:34Z

I think at least the convenience constructor Image(InputStream) should close the stream.

kevinrushforth · 2025-08-21T18:01:49Z

No, that would be an incompatible change.

andy-goryachev-oracle · 2025-08-21T18:10:25Z

You are right, please disregard the comment.

I think we should clarify the behavior in each constructor that takes InputStream, similar to Font::loadFont().

A boolean closeStream parameter might be the lightest solution from the API perspective then.

mstr2 · 2025-08-21T18:32:59Z

A new parameter to toggle whether the stream will be closed should have at least a moderately strong use case to justify its existence. I can't think of any. If you turn over a stream to an asynchronous process, you effectively relinquish ownership of that stream, as you don't know when (if ever) it will be processed.

But even then, you can very easily achieve the non-closing behavior without new JavaFX API, just by using existing stream APIs:

var streamThatWillNotCloseUnderlyingStream = new FilterInputStream(myStream) {
    @Override
    public void close() {
        // don't call super.close(), so underlying stream won't be closed
    }
}

jayathirthrao · 2025-08-22T02:13:08Z

If user is passing stream its better to keep the decision of closing the stream with user for synchronous loading.

Asynchronous URL loading : Current changes takes care of closing the stream and follows already present behaviour. Its better to update the documentation about it.
Asynchronous InputStream loading : This change now explicitly closes the Input Stream which is fine but this needs to be captured in the documentation.

Giving an option to user about how to close the stream in case of asynchronous loading can be taken up as future task, if needed. CI testing is green with current code update.

hjohn · 2025-08-22T10:31:36Z

I like the Supplier idea, though I am not sure how the async path would work (unless you add the closeStream parameter).

When you have a Supplier<InputStream> instead of URL or plain InputStream it will look like this:

 new Image(() -> new FileInputStream("xyz"));

Since the supplier must create a new stream each time (as it can't know how often it will be called), it makes sense that the caller of the supplier is also the one to close it, whether or not that is sync or async.

hjohn · 2025-08-22T10:41:49Z

Thanks for all the input. I think there are now two ways to proceed:

Leave it as is, including the new backgroundLoading variants for InputStream, but document clearly for both the old and new constructors when the stream will be closed (ie. only when backgroundLoading is true we will close the stream).
Don't add these new constructors, but add a more obvious variant with Supplier<InputStream>, where the ownership is more clear, with the caller of the supplier owning the stream created.

I get the impression the consensus leans towards option 1. I don't see a direct compelling need for the Supplier variant, although I'm happy to provide an implementation if option 2 is favored.

kevinrushforth · 2025-08-22T14:14:17Z

I prefer option 1.

andy-goryachev-oracle · 2025-08-22T14:32:54Z

I agree with Kevin: option 1 is clearer.

jayathirthrao

Latest change looks good to me.

This update causes a failure in one of our closed white-box tests. 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.

andy-goryachev-oracle

thank you for making the requested changes @hjohn !
looks good.

openjdk bot changed the title ~~8361286 Allow enabling of background loading for images loaded from an InputStream~~ 8361286: Allow enabling of background loading for images loaded from an InputStream Aug 18, 2025

Support background loading of raw input streams

1452b5c

- Fixed generics (mix up of two ImageLoader types) - Removed unused code for handling headers, methods, request parameters - Use long for progress as streams max exceed 2 GB - Improved documentation of Image regarding background loading

hjohn force-pushed the feature/image-constructor branch from 7efebfd to 1452b5c Compare August 18, 2025 10:30

hjohn marked this pull request as ready for review August 18, 2025 14:00

openjdk bot added the rfr Ready for review label Aug 18, 2025

kevinrushforth self-requested a review August 18, 2025 19:51

openjdk bot added the csr Need approved CSR to integrate pull request label Aug 18, 2025

andy-goryachev-oracle reviewed Aug 18, 2025

View reviewed changes

modules/javafx.graphics/src/main/java/javafx/scene/image/Image.java Show resolved Hide resolved

Add since tags

63e652e

andy-goryachev-oracle reviewed Aug 19, 2025

View reviewed changes

hjohn added 2 commits August 20, 2025 15:42

Remove wrapping

cef710c

Add links to progress property in javadoc

2e23626

danielpeintner reviewed Aug 20, 2025

View reviewed changes

modules/javafx.graphics/src/test/java/test/javafx/scene/image/ImageTest.java Outdated Show resolved Hide resolved

Fix typo

65bc250

jayathirthrao reviewed Aug 21, 2025

View reviewed changes

Clarify documentation with regards to stream closing

6494c52

jayathirthrao approved these changes Aug 25, 2025

View reviewed changes

andy-goryachev-oracle approved these changes Aug 26, 2025

View reviewed changes


		protected AbstractRemoteResource(String url, String method, String outboundContent, AsyncOperationListener<T> listener) {

8361286: Allow enabling of background loading for images loaded from an InputStream #1875

Are you sure you want to change the base?

8361286: Allow enabling of background loading for images loaded from an InputStream #1875

Uh oh!

Conversation

hjohn commented Aug 18, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Aug 18, 2025

Uh oh!

openjdk bot commented Aug 18, 2025

Uh oh!

mlbridge bot commented Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

kevinrushforth commented Aug 18, 2025

Uh oh!

openjdk bot commented Aug 18, 2025

Uh oh!

kevinrushforth commented Aug 18, 2025

Uh oh!

openjdk bot commented Aug 18, 2025

Uh oh!

Uh oh!

nlisker commented Aug 18, 2025

Uh oh!

hjohn commented Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mstr2 commented Aug 18, 2025

Uh oh!

andy-goryachev-oracle commented Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hjohn commented Aug 18, 2025

Uh oh!

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andy-goryachev-oracle Aug 19, 2025

Choose a reason for hiding this comment

Uh oh!

hjohn Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andy-goryachev-oracle commented Aug 20, 2025

Uh oh!

jayathirthrao left a comment

Choose a reason for hiding this comment

Uh oh!

jayathirthrao Aug 21, 2025

Choose a reason for hiding this comment

Uh oh!

jayathirthrao Aug 22, 2025

Choose a reason for hiding this comment

Uh oh!

jayathirthrao Aug 21, 2025

Choose a reason for hiding this comment

Uh oh!

hjohn Aug 21, 2025

Choose a reason for hiding this comment

Uh oh!

andy-goryachev-oracle commented Aug 21, 2025

Uh oh!

hjohn commented Aug 21, 2025

Uh oh!

andy-goryachev-oracle commented Aug 21, 2025

Uh oh!

hjohn commented Aug 21, 2025

Uh oh!

hjohn commented Aug 18, 2025 •

edited by openjdk bot

Loading

mlbridge bot commented Aug 18, 2025 •

edited

Loading

hjohn commented Aug 18, 2025 •

edited

Loading

andy-goryachev-oracle commented Aug 18, 2025 •

edited

Loading

hjohn commented Aug 22, 2025 •

edited

Loading