On Sat, 3 Oct 2026 02:13:15 GMT, John Hendrikx <[email protected]> wrote:
>> 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 i...
I've added an `@implNote` that this is a known issue, let me know if you're
okay with that until we can resolve the bigger problem.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1969#discussion_r4176927445
