On Tue, 22 Sep 2026 20:58:01 GMT, Andy Goryachev <[email protected]> wrote:
>> John Hendrikx has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Optimized dirty rect handling (benefits more than just SWDraingContext)
>
> modules/javafx.graphics/src/main/java/javafx/scene/image/WritableImage.java
> line 175:
>
>> 173: public final DrawingContext getDrawingContext() {
>> 174: if (drawingContext == null) {
>> 175: drawingContext =
>> Toolkit.getToolkit().createDrawingContext(getWritablePlatformImage(),
>> this::notifyDrawingContextDirty);
>
> `setPlatformImage()` L1086 can replace the platform image (as a result of
> `snapshot`, for instance). this will disassociate the cached
> `drawingContext` from the platform image.
It seems that the same happens when the user supplies their own `PixelBuffer`
-- snapshot just replaces the platform image, effectively orphaning the storage
provided by the user -- that's bad, as the user can easily hold on to this and
expect it to be updated.
So IMHO, `snapshot` is broken, and `setPlatformImage` should be removed for two
reasons:
- A user supplied buffer must be honored, and it currently is just silently
replaced; and this already shows up as a bug (`PixelBuffer.updateBuffer` still
targetting the user storage, while `image.getPixelReader().getArgb(...)` reads
the new snapshot provided storage
- The GPU and CPU path currently both allocate a new massive heap object for
the snapshot, which basically already exists. The GPU path copies the data from
a direct memory allocation to this new heap buffer (even though it could have
just copied it to the existing one), and the CPU path renders into a new heap
buffer, even though it could have rendered into the existing one
Also the tiled path (when the snapshot would be larger than the maximum texture
size) already does in-place updates, so having the normal path do so as well
would make it consistent.
So, I'd like to make a real fix for this in a different PR, rather than
stacking this into this one. The fix would entail:
- Add `Tookit.renderToImage` method that accepts a target platform image, and
use that for `snapshot`
- For the CPU/SW path, if the format matches, use the target as-is; if not
allocate a temp buffer, and copy -- either way, the user supplied buffer is
updated as expected, not silently orphaned
- For the GPU path, it always allocates a new off-heap buffer regardless for
the GPU to access, but it doesn't need to also allocate a new heap buffer to
copy that buffer into; it can just copy it into the given target instead --
same amount of work, but saves a (potentially huge) buffer allocation
- The `loadTkImage`/`getTkImageLoader`/`Image.setPlatformImage` can all be
removed (all only used by snapshots)
- After snapshotting into the existing buffer, mark it dirty
- Stop holding on to a render target for the snapshot (currently, the GPU path
holds on to a buffer the size of the snapshot even after it is copied to the
heap, potentially holding on to several MB's of memory to avoid a relatively
cheap render target creation 'just in case' another snapshot is made... -- if
the snapshot is 20 MB, then the current GPU path allocates another 20 MB as
render target (and never discards it) and during a snapshot holds on to another
20 MB; so memory use goes like this:
- Pre-snapshot there is just the Image: 20 MB
- During snapshot a render target is created: +20 MB (40 MB retained)
- After the GPU render completes a heap buffer is allocated: +20 MB (60 MB
retained)
- The render target is copied to the new buffer
- The new buffer replaces the user buffer (or initial buffer if not user
supplied)
- When GC decides to run the old buffer is collected: -20 MB (but still 40 MB
retained forever as the render target is kept)
**The end result of the above changes:**
- User suppled buffers are correctly honored even for snapshots (this is a
pre-existing bug given that the documentation states that it renders the
snapshot into the provided image, which can have a user supplied buffer)
- Less GC churn when snapshotting on both the CPU and GPU paths, unless the
user buffer type mismatches (but for correctness this must then be honored
anyway)
- The platform image is stable, so no need to query it each time and wrapping
it with `DrawingContext` is safe -- `PixelWriter` can also then just wrap it
instead of querying it each time
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1969#discussion_r4171290341
