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

[Frederic Thevenet]

On Tue, 21 Jan 2020 21:53:29 GMT, Kevin Rushforth <kcr 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.
>> 
>> 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.

I've put together a small benchmark to measure the time it takes to snapshots into images of sizes varying from 1024*1024 to 8192*8192, by increment of 1024 in each dimension, which you can find here: https://github.com/fthevenet/snapshot-perf-meter/blob/master/src/main/java/eu/fthevenet/SnapshotPerfMeter.java

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

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