Skip to content

Conversation

@hjohn
Copy link
Collaborator

@hjohn hjohn commented Nov 10, 2025

This PR adds a getDrawingContext method to WritableImage, which works similar to Canvas::getGraphicsContext and shares the same signatures. Key features include:

  • Shape rendering: strokeRect, fillRect, strokeOval, fillOval, strokeArc, fillArc, strokePolyline, fillPolygon, etc.
  • Stroke and fill attributes: lineWidth, lineCap, lineJoin, miterLimit, fillRule, stroke and fill paints.
  • Global graphics settings: globalAlpha and globalBlendMode.
  • Image drawing: draw other Image instances with scaling and source/destination rectangles.

This feature enables direct software rendering to WritableImage without requiring a Canvas + snapshot.

Additional notes:

  • The implementation leverages the software Pisces renderer.
  • This lays the groundwork for future support of text rendering and path operations.

Example usage:

WritableImage img = new WritableImage(400, 400);
DrawingContext ctx = img.getDrawingContext();
ctx.setFill(Color.RED);
ctx.fillRect(50, 50, 100, 100);

See the sample program RandomShapesDemo to see a WritableImage and Canvas side by side performing the same operations:

image

Progress

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

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1969

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 10, 2025

👋 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
Copy link

openjdk bot commented Nov 10, 2025

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

@hjohn hjohn force-pushed the feature/writable-image-drawing branch from 5660c3b to 9a50c68 Compare November 10, 2025 14:14
@openjdk
Copy link

openjdk bot commented Nov 10, 2025

@hjohn Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

Comment on lines +1509 to +1510
IntBuffer buf = IntBuffer.allocate(w * h);
return com.sun.prism.Image.fromIntArgbPreData(buf, w, h);
Copy link
Collaborator Author

@hjohn hjohn Nov 10, 2025

Choose a reason for hiding this comment

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

This seems to be no problem to change (an image heavy application still runs absolutely fine), but it is kind of a global change. If there are issues with this, we could make a specific method for writable images to use so we always get an int[] buffer that the software renderer expects.

As it is now, the renderer is writing directly into the underlying image storage, without any copies being made (which is nice and efficient).

Copy link
Contributor

Choose a reason for hiding this comment

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

I am curious why the byte buffer was chosen initially.
Will IntBuffer be better on every platform?

Also, w * h might be negative if the product is greater than ~2B, though it will result in an IllegalArgumentException with a cryptic "capacity expected to be negative" message.

@andy-goryachev-oracle
Copy link
Contributor

/csr
/reviewers 3

@openjdk openjdk bot added the csr Need approved CSR to integrate pull request label Nov 10, 2025
@openjdk
Copy link

openjdk bot commented Nov 10, 2025

@andy-goryachev-oracle has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@hjohn this pull request must refer to an issue in JBS to be able to link it to a CSR request. To refer this pull request to an issue in JBS, please update the title of this pull request to just the issue ID.

@openjdk
Copy link

openjdk bot commented Nov 10, 2025

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

@andy-goryachev-oracle
Copy link
Contributor

This is interesting, similar to BufferedImage.createGraphics() in AWT.

Questions:

  • so this will aways be slower than Canvas?
  • are the results going to be exactly the same, or there will be platform-specific differences in anti-aliasing etc?

@hjohn
Copy link
Collaborator Author

hjohn commented Nov 10, 2025

This is interesting, similar to BufferedImage.createGraphics() in AWT.

Questions:

  • so this will aways be slower than Canvas?

See my mailinglist answer, but in short, there are cases where WritableImage (which is not a Node) can be faster, especially when you want to access the results with say a PixelReader.

  • are the results going to be exactly the same, or there will be platform-specific differences in anti-aliasing etc?

I think I missed a nuance here in my mailinglist reply. I think the software renderer will always give the same results, regardless of platform (interesting for certain use cases I suppose, and perhaps for testing as well). It will almost certainly differ however from GPU renderings of the same operations, but for supported operations it may be hard to distinguish what was GPU and what was CPU rendered.

@andy-goryachev-oracle
Copy link
Contributor

See my mailinglist answer

https://mail.openjdk.org/pipermail/openjfx-dev/2025-November/057463.html

(for reference, it's a good answer)

@hjohn
Copy link
Collaborator Author

hjohn commented Nov 16, 2025

Adding a note here for myself:

    private final AbstractNotifyListener platformImageChangeListener =
            new AbstractNotifyListener() {
        @Override
        public void invalidated(Observable valueModel) {
            invalidateWidthHeight();
            NodeHelper.markDirty(ImageView.this, DirtyBits.NODE_CONTENTS);
            NodeHelper.geomChanged(ImageView.this);
        }
    };

The above code in ImageView responds to pixel changes in the platform image, however, such a change can never influence the width/height (it is triggered by pixelsDirty in WritableImage). Because it also invalidates width/height, and triggers a geomChanged a new layout is triggered. If you happen to be drawing something in your writable image as part of layoutChildren via getDrawingContext or the pixel writer, this may then trigger infinite layouts.

I get the impression that the code is unaware that even though it is called "platform image CHANGE listener" there is no actual image reference change; only some pixels were dirty in the same image...

I think the invalidateWidthHeight and NodeHelper.geomChanged lines need to be removed as part of this PR.

@andy-goryachev-oracle
Copy link
Contributor

I think the invalidateWidthHeight and NodeHelper.geomChanged lines need to be removed as part of this PR.

This explains why my Canvas-based table cell implementation resulted in a continuous layout.

This looks like a bug to me, and I think we should fix it separately.

@andy-goryachev-oracle
Copy link
Contributor

Also, perhaps we need to add a condition to suppress invalidateWidthHeight and NodeHelper.geomChanged only when it's the same image - we do need to handle the actual image change.

@andy-goryachev-oracle
Copy link
Contributor

Created https://bugs.openjdk.org/browse/JDK-8372007 to investigate the re-layout case.

@hjohn
Copy link
Collaborator Author

hjohn commented Nov 17, 2025

I think the invalidateWidthHeight and NodeHelper.geomChanged lines need to be removed as part of this PR.

This explains why my Canvas-based table cell implementation resulted in a continuous layout.

Maybe, but I doubt it, I don't think Canvas interacts with Image / WritableImage. You may have hit on a similar bug.

This looks like a bug to me, and I think we should fix it separately.

Yeah, this is a bug no doubt, and can be fixed separately also.

Also, perhaps we need to add a condition to suppress invalidateWidthHeight and NodeHelper.geomChanged only when it's the same image - we do need to handle the actual image change.

There should not be an image change, this is some internal platform image, and it changes, but not in the way we normally think of it for Change listeners (the reference does not change, but the contents do). It seems that in Image this was added:

void pixelsDirty() {  // called by WritableImage / PixelWriter to signal pixels were modified
    platformImagePropertyImpl().fireValueChangedEvent();
}

Basically it just fires a change event (directly) but without there actually being a change in the value of that property (it's a bit dirty, but luckily internal code). So I'm pretty sure there is never a geometry change via this route (I mean, WritableImage is basically constructed with a fixed size, so how could there be?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

csr Need approved CSR to integrate pull request

Development

Successfully merging this pull request may close these issues.

2 participants