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
