[Rev 03] RFR: 8088198: Exception thrown from snapshot if dimensions are larger than max texture size

[Kevin Rushforth]

On Mon, 20 Jan 2020 11:24:05 GMT, Frederic Thevenet <github.com+7450507+fthevenet at openjdk.org> wrote:

>> Looks good to me.
>> Below is just an observation about time taken by the API,
>> Platform: Windows10,  `maxTextureSize`: 4096
>> For a snapshot of (4096 * n, 4096 * n): each call to `doSnapshotTile()` takes ~100 ms, and each call to `setPixels()` takes ~30 ms.
>> 
>> Please wait for one more approval before integrating.
> 
>> 
>> 
>> Looks good to me.
>> Below is just an observation about time taken by the API,
>> Platform: Windows10, `maxTextureSize`: 4096
>> For a snapshot of (4096 * n, 4096 * n): each call to `doSnapshotTile()` takes ~100 ms, and each call to `setPixels()` takes ~30 ms.
>> 
>> Please wait for one more approval before integrating.
> 
> Do you have the same kind of measurements for similar uses cases without this patch? I'm expecting performances to remain the same for snapshots less than `maxTextureSize*maxTextureSize` in size, since there is no extra pixel copy in that case, and the rest of the code if globally unchanged.
> 
> Still there exists a window for performance regressions, which for instance on Windows 10 w/ the D3D rendering pipeline would be when one of the dimension of a snapshot is >4096  and <8192: in that case a snapshot would require up to 4 extra copy pixels steps with this patch.
> This is due to the fact that the old code would accept to render in a texture of a size up to the non-clamped max texture size (8192 in D3D, 16384 in ES2), but because I couldn't find a clean way to access the non clamped maxTextureSize exposed by the render factory from the Scene class, I settled for Prsim.maxTextureSize, which is the clamped value (4096 by default).
> I haven't dug deep enough in the code to understand precisely why the max texture size is clamped to 4096 by default, but assuming that it is for a good reason wouldn't that also apply in that case? Or is it always safe to use the non-clamped value in that particular case?

I haven't done any testing yet, but I have two comments on the patch:

1. Using the clamped texture size as the upper limit is the right thing to do, but `Prism.maxTexture` isn't guaranteed to be that size. The `Prism.maxTexture` constant represents the value to clamp the texture size to. In the event that D3D or OpenGL were to report a maximum less than the value of `Prism.maxTexture` (unlikely, but maybe on some low-end embedded systems?), then that value is what should be used. The way to get the clamped texture size is to call [`ResourceFactory::getMaximumTextureSize`](https://github.com/openjdk/jfx/blob/jfx14/modules/javafx.graphics/src/main/java/com/sun/prism/ResourceFactory.java#L222), but you can't do that directly from the scene graph code.

2. Allocating multiple `WritableImage` objects and using `PixelWriter::setPixels` to stitch them together will take more temporary memory, and is likely to be slower, than having the snapshot code write into a subimage of the larger allocated image directly.

Having said that, the current proposed solution is still better than the current behavior is almost all cases, although there could be a performance hit in the case of an 8K x 8K image, which will now be tiled. I want to do a more careful review and some testing. If any other users of snapshot have any comments or concerns, they would be welcome.

I think the best long-term solution might be to modify the [`QuantumToolkit::renderToImage`](https://github.com/openjdk/jfx/blob/jfx14/modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/QuantumToolkit.java#L1490) method, although that would certainly be out of scope for JavaFX 14.

-------------

PR: https://git.openjdk.java.net/jfx/pull/68