On Mon, 20 Jan 2020 11:24:05 GMT, Frederic Thevenet <github.com+7450507+ftheve...@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
