[Rev 04] RFR: 8238954: Improve performance of tiled snapshot rendering

[Frederic Thevenet]

On Mon, 15 Jun 2020 15:55:25 GMT, Frederic Thevenet <github.com+7450507+fthevenet at openjdk.org> wrote:

>> Overall, this looks quite good. In particular the tiled rendering, as implemented by the `renderTile` method, should be
>> reasonably efficient.
>> My only high-level comment is that I'm somewhat skeptical of `computeOptimumTileSize` to determine the size and
>> direction of tiling. I note that in the case of an image that is tiled in both X and Y, there are at most 4 distinct
>> tile sizes if it doesn't fit evenly. In the case where only one of X or Y is tiled, there are at most 2 distinct tile
>> sizes. Here is an example:  +-----------+-----------+  .  +-------+ |           |           |  .  |       |
>> |     M     |     M     |  .  |   R   |
>> |           |           |  .  |       |
>> +-----------+-----------+  .  +-------+
>> |           |           |  .  |       |
>> |     M     |     M     |  .  |   R   |
>> |           |           |  .  |       |
>> +-----------+-----------+  .  +-------+
>>       .           .        .      .
>> +-----------+-----------+  .  +-------+
>> |           |           |  .  |       |
>> |     M     |     M     |  .  |   R   |
>> |           |           |  .  |       |
>> +-----------+-----------+  .  +-------+
>> |     B     |     B     |  .  |   C   |
>> +-----------+-----------+  .  +-------+
>> 
>> Where `M` represents the middle set of tiles each with a size of `tileW x tileH`. `R` is the right hand column of
>> tiles, `B` is bottom row, and `C` is corner.
>> Recognizing this, I wonder if it might be better to always use the maximum tile size, but fill all of the middle tiles
>> of that size first, and then pick up the right and/or bottom edges as needed. This will minimize thrashing (no more
>> than 3 changes of tile size), while avoiding the more complicated logic that tries to keep the tiles all the same size
>> at the cost of smaller tiles, and which has to fall back to using uneven tiles anyway. If you do it this way, there is
>> also no need to have code that switches the order of the inner loop. It will naturally handle that.  Either way, I'd
>> like to see some additional system tests that cover all of the cases of X and Y fitting/not-fitting exactly (and if you
>> stick with your current approach, X or Y as the inner loop).  I left a couple inline comments as well.
>
>> [...] I'd like to see some additional system tests that cover all of the cases of X and Y fitting/not-fitting exactly
>> (and if you stick with your current approach, X or Y as the inner loop).
> 
> What kind of tests do you have in mind? More specifically do you mean simply adding tests that expand on the existing
> `doTestSnapshotScaleNodeDefer`and `doTestSnapshotScaleNodeImm` (which basically just prove that taking a snapshot
> returns a non-null image of the expected size)? Or do you think we need to include a test that proves the snapshot
> produced by tiling is entirely faithful to the original, pixel-wise?

I went ahead and wrote a bunch of tests that:
1. Setup a scene to display an `ImageView` of a selected dimensions chosen to trigger tiling in different ways when
taking snapshots. 2. Fill up the image with noise.
3. Take a snapshot and do a pixel-wise comparison with the original image.

I've added the new tests to the existing `Snapshot2Test.java`.

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

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