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