On Mon, 15 Jun 2020 15:55:25 GMT, Frederic Thevenet <github.com+7450507+ftheve...@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
