On Sun, 4 Oct 2026 09:25:50 GMT, John Hendrikx <[email protected]> wrote:
>> 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 r... > > 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. Could you create a JBS ticket for this follow-up please? ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1969#discussion_r4189611281
