On Wed, 29 Jan 2020 16:33:16 GMT, Nir Lisker <nlis...@openjdk.org> wrote:
>> 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`. Yes, I'm fine with working further on this and the related issues for openjfx15, and I have fairly good idea on how to approach it. ------------- PR: https://git.openjdk.java.net/jfx/pull/68
