On Wed, 12 Feb 2020 14:57:33 GMT, Frederic Thevenet <github.com+7450507+ftheve...@openjdk.org> wrote:
>> Issue JDK-8088198, where an exception would be thrown when trying to capture >> a snapshot whose final dimensions would be larger than the running >> platform's maximum supported texture size, was addressed in openjfx14. >> The fix, based around the idea of capturing as many tiles of the maximum >> possible size and re-compositing the final snapshot out of these, is >> currently only attempted after the original, non-tiled, strategy has already >> failed. >> This was decided to avoid any risk of regressions, either in terms of >> performances and correctness, while still offering some relief to the >> original issue. >> >> This follow-on issue aims to propose a fix to the original issue, that is >> able to correctly decide on the best snapshot strategy (tiled or not) to >> adopt before applying it and ensure best performances possible when tiling >> is necessary while still introducing no regressions compared to the original >> solution. > > I've marked this PR as a WIP for the time being because I've only tested it > on the d3d rendering pipeline so far, so I think it is too early to call for > a formal review yet. > Still, I welcome feedback if someone wants to have a look at it already. > > In a nutshell, this is what this PR does: > > - Reverts changes from 8088198 > - Implements tiling within `QuantumToolkit::renderToImage` instead > - Gets the maxTextureSize directly from the `ResourceFactory` instance > instead of relying on `PrismSettings.maxTextureSize` (which may be wrong in > the event the maxTexture size supported by the hardware is less than the > clamped value set in PrismSettings) > - Tries to align the PixelFomat for the target `WritableImage` with that of > the `RTTexture` when possible, since converting IntARGB to ByteBGRA (or the > other way round) is a major cost factor when copying large amounts of pixels. > - Avoids as much as possible having to resize the tile in between subsequent > calls to `RTTexture::readPixels`, as it is another major cost factor, under > the d3d rendering pipeline at least. (Native method > `D3DResourceFactory_nReadPixels` attempts to reuse the same surface in > between calls but is forced to create a new one if dimensions are not exactly > the same, which is quite costly). > > TODO: > > - [x] Make sure chosen PixelFormat is optimum when using es2 pipeline. > - [ ] Consider using subtexture pixel read with es2 (SW and d3d don't support > it) as a better alternative to relying on same size tiles to avoid surface > thrashing. I finally got a chance to do some more extensive testing when running this patch with the es2 pipeline on Linux. It works as expected, and from what I saw, using a IntARGB pixelBuffer when no WritableImage is provided avoids swapping around pixel components, like under d3d. I've also verified than the patch's behaviour is correct when a writable image is provided as a parameter to Node.snapshot, whether the underlying image is actually a PlatformImage or a wrapper for a PixelBuffer, which corrects another limitation of the previous implementation. ------------- PR: https://git.openjdk.java.net/jfx/pull/112
