On Wed, 29 Jan 2020 13:59:12 GMT, Ambarish Rapte <ara...@openjdk.org> wrote:
>> The pull request has been updated with 1 additional commit.
>
> 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.
-------------
PR: https://git.openjdk.java.net/jfx/pull/68
