On Wed, 29 Jan 2020 15:42:51 GMT, Kevin Rushforth <k...@openjdk.org> wrote:

>> I need to re-review the updated PR.
> 
> The code changes look good to me. As long as you are willing to address the 
> follow-on issues for openjfx15, I'll approve this PR for openjfx14, once I 
> run my last test.
> 
> I did a fair bit of testing of the functionality, which reinforces the 
> decision to only try tiling as a fallback when an exception occurs, since 
> there could otherwise be functional regressions in addition to performance 
> regressions.
> 
> With the latest update to the PR, any case that works today will continue to 
> behave exactly as it does today. The worst that will happen is that the 
> tiling might produce very slightly different results for interpolated colors, 
> or possibly fail with a different exception (e.g., if a 4K x 4K texture is 
> still too big for some hypothetical device, or if the user passes in a 
> `WritableImage` created with a `PixelBuffer` (see below)).
> 
> I added a verbose println to snapshot to validate my testing:
> 
> 1. Snapshot with image size = 5k x 5k (should still fit on Windows D3D with 
> no tiling)
> 2. Snapshot with image size = 20k x 3k (should tile in X)
> 3. Snapshot with image size = 3K x 20k (should tile in Y)
> 4. Snapshot with image size = 20K x 20k (should tile in both X and Y)
> 
> RESULT: All of the above work as expected
> 
> 5. Modify the jfx runtime to force tiling, set the tile size to 128 bytes, 
> and run various tests (e.g., a stress test of tiling)
> RESULT: Everything looks visually correct, but four of the tests in 
> `test.robot.test3d.Snapshot3DTest` fail with a color mismatch [can be 
> addressed in a follow-up bug]
> 
> 6. Pass in a `WritableImage` created with a `PixelBuffer` object (NIO buffer)
> RESULT: It works without tiling and throws an exception with tiling [can be 
> addressed in a follow-up bug]
> 
> 7. Pass in a larger than needed `WritableImage` (e.g., using a viewport that 
> is large enough to cause tiling, but smaller than the passed in image) into 
> snapshot when using tiling. Is the entire image cleared?
> RESULT: I still need to run this test, but even if it doesn't work correctly 
> when tiling, it wouldn't be a regression, and could be addressed in a 
> follow-up bug.
> 
> Issues to file for follow-on bug(s):
> 1. Improve performance of tiled snapshot rendering
>     * As part of this, we need to get max texture size from the toolkit
>     * Consider moving the tiled rendering into `QuantumRenderer` thread
>     * Reuse a single temporary buffer rather than creating a new one for each 
> tile
> 2. Passing in a `WritableImage` created with a `PixelBuffer` object (NIO 
> buffer) fails if tiling is needed. I'll file this one.
> 3. Pixel accuracy issues with tiled image; this can be seen in 
> `Snapshot3DTest` by instrumenting the code to force tiling with a tile size 
> of 128. It looks OK visually, but fails the color comparison test. I'll file 
> this one.
> 4. Presuming that the above test 7 produces incorrect results, file a 
> follow-up bug to fix it.

> Improve performance of tiled snapshot rendering

I'll have a closer look at improving `IntTo4ByteSameConverter::doConvert`.

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

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

Reply via email to