round_out certain cache bounds

[jonahwilliams]

This fixes the worst of the pixel jumping issues, though issues with raster cached/animated display list layers are still present.

Fixes flutter/flutter#110959
Fixes flutter/flutter#110957

Previously I computed the bounds using the logical rect, and rounded up to get a texture size. To create the texture for the raster cache entry, we first appended a translation which attempted to position the origin of the display_list/layer at (0,0). This led to a number of issues I did not anticipate, and had difficulty observing on mobile devices with high device pixel ratios.

For display list raster cache entries, this would lead to the raster cached entry being drawn differently than the non-raster cached entry. This is fixed by this PR, by rounding out the bounds.

This does not appear to fix the issue with layer raster caching.

[flutter-dashboard]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[jonahwilliams]

I think this still might be incorrect. We may have been unconditionally aligning the origin to a physical pixel before, thus the round out would at most add 1 pixel in each direction. Now the the origin can be fractional, rounding out may add up to two pixels in each direction

[jonahwilliams]

A potential fix would be to only pixel snap in display_list_layers. That would not have the instability issues that pixel snapping in all composited layers does, but may have other tradeoffs that I'm not aware of

[jpnurmi]

I'm seeing this with c130819. A transitioning page gets rasterized at fractional X=-138.6796 and then renders wrong when the transition ends at X=0.0. Things jump in place after moving the mouse cursor over the button to make it re-render without the cache.

Kooha-09-07-2022-14-05-06.mp4

RasterCache::Rasterize
	matrix: [  1,0000   0,0000 -138,6796][  0,0000   1,0000   0,0000][  0,0000   0,0000   1,0000]
	logical rect: 0,000000000 0,000000000 960,000000000 633,000000000 (960,000000000x633,000000000)
	dest rect: -139 0 822 633 (961x633)
RasterCacheResult::draw
	matrix: [  1,0000   0,0000 -138,6796][  0,0000   1,0000   0,0000][  0,0000   0,0000   1,0000]
	logical rect: 0,000000000 0,000000000 960,000000000 633,000000000 (960,000000000x633,000000000)
	rounded bounds: -139 0 822 633 (961x633)
RasterCacheResult::draw
	matrix: [  1,0000   0,0000 -25,6627][  0,0000   1,0000   0,0000][  0,0000   0,0000   1,0000]
	logical rect: 0,000000000 0,000000000 960,000000000 633,000000000 (960,000000000x633,000000000)
	rounded bounds: -26 0 935 633 (961x633)
RasterCacheResult::draw
	matrix: [  1,0000   0,0000 -18,1801][  0,0000   1,0000   0,0000][  0,0000   0,0000   1,0000]
	logical rect: 0,000000000 0,000000000 960,000000000 633,000000000 (960,000000000x633,000000000)
	rounded bounds: -19 0 942 633 (961x633)
RasterCacheResult::draw
	matrix: [  1,0000   0,0000  -9,3807][  0,0000   1,0000   0,0000][  0,0000   0,0000   1,0000]
	logical rect: 0,000000000 0,000000000 960,000000000 633,000000000 (960,000000000x633,000000000)
	rounded bounds: -10 0 951 633 (961x633)
RasterCacheResult::draw
	matrix: [  1,0000   0,0000  -4,6098][  0,0000   1,0000   0,0000][  0,0000   0,0000   1,0000]
	logical rect: 0,000000000 0,000000000 960,000000000 633,000000000 (960,000000000x633,000000000)
	rounded bounds: -5 0 956 633 (961x633)
RasterCacheResult::draw
	matrix: [  1,0000   0,0000  -0,8447][  0,0000   1,0000   0,0000][  0,0000   0,0000   1,0000]
	logical rect: 0,000000000 0,000000000 960,000000000 633,000000000 (960,000000000x633,000000000)
	rounded bounds: -1 0 960 633 (961x633)
RasterCacheResult::draw
	matrix: [  1,0000   0,0000   0,0000][  0,0000   1,0000   0,0000][  0,0000   0,0000   1,0000]
	logical rect: 0,000000000 0,000000000 960,000000000 633,000000000 (960,000000000x633,000000000)
	rounded bounds: 0 0 960 633 (960x633)
RasterCacheResult::draw
	matrix: [  1,0000   0,0000   0,0000][  0,0000   1,0000   0,0000][  0,0000   0,0000   1,0000]
	logical rect: 0,000000000 0,000000000 960,000000000 633,000000000 (960,000000000x633,000000000)
	rounded bounds: 0 0 960 633 (960x633)
RasterCacheResult::draw
	matrix: [  1,0000   0,0000   0,0000][  0,0000   1,0000   0,0000][  0,0000   0,0000   1,0000]
	logical rect: 0,000000000 0,000000000 960,000000000 633,000000000 (960,000000000x633,000000000)
	rounded bounds: 0 0 960 633 (960x633)

[jpnurmi]

The cached image has fractional gaps at the edges because it was rasterized at fractional coordinates. The visual result is off by that fraction when rendered in the origin where the transition ends.

[jonahwilliams]

Yes, I can fix the jumping of display list raster cache entries by performing a round out. Perhaps because there is a round out somewhere in the DL or Skia code (@flar would be the expert here). What I can't seem to fix is the difference between the layer raster cache entry and non-raster cache entry.

Fixing the jumping is probably more critical, but I still feel like I'm missing a piece of the puzzle here. Simply removing the roundOut for layers seemingly isn't enough

[jonahwilliams]

possibly, the issue is something like this: We normally draw the layer at a fractional coordinate, and then the child display list at a fractional coordinate (which gets rounded out to a physical pixel, somewhere).

Now, when we raster cache the layer, we align things such that the top left corner is 0,0. Thus the child display list is already aligned and doesn't move. However, when we draw the layer back on the page, the display list is now at a different position than it would otherwise be. In order to fix this, when raster caching the layer, we need to make sure that the display list ends up at the same fractional coordinate it would have been at, so that whatever is pixel snapping it leaves it at the same position.

[jonahwilliams]

but of course, I try fixing that and it doesn't quite work, so maybe I am missing something simple

[jonahwilliams]

This latest change forces an integral ctm when rasterizing a layer. That seems to fix the issue where the layers are sometimes rendered on a fractional pixel, but doesn't fix the issue where the layer raster caching is different from the display list raster caching

[jonahwilliams]

Okay: last update.

The assumption of the raster cache is that we can replace any layer or display list with a rasterized version of itself. At very low DPR, this is only true if that layer or display list is aligned to physical pixels. Even if we ensure that the texture of the layer or display list is rasterized pixel aligned, when we go to the draw that entry we may find the offset is not physical pixel aligned. Thus we can either snap it to a pixel, leaving it in a different location than the un-rasterized version, or we can draw it in the correct location with obvious aliasing issues.

Unfortunately, that leaves us in a bit of a bind. It means we can't get rid of pixel snapping as long as we have the raster cache. Furthermore, since we didn't notice this issue until the stable release, and there have been many refactors on top of the raster cache since then, it means that a revert is not very straight forward.

Seemingly the only idea long term solution is to revert back to pixel snapping any layers that may be raster cached. The short term change is more uncertain. I can begin working on a patch against the release branch that reverts the removal of pixel snapping. Separately I can also work on re-landing pixel snapping on ToT

[dnfield]

@JsouLiang you may find this interesting.

[flar]

Okay: last update.

The assumption of the raster cache is that we can replace any layer or display list with a rasterized version of itself. At very low DPR, this is only true if that layer or display list is aligned to physical pixels. Even if we ensure that the texture of the layer or display list is rasterized pixel aligned, when we go to the draw that entry we may find the offset is not physical pixel aligned. Thus we can either snap it to a pixel, leaving it in a different location than the un-rasterized version, or we can draw it in the correct location with obvious aliasing issues.

Unfortunately, that leaves us in a bit of a bind. It means we can't get rid of pixel snapping as long as we have the raster cache. Furthermore, since we didn't notice this issue until the stable release, and there have been many refactors on top of the raster cache since then, it means that a revert is not very straight forward.

Seemingly the only idea long term solution is to revert back to pixel snapping any layers that may be raster cached. The short term change is more uncertain. I can begin working on a patch against the release branch that reverts the removal of pixel snapping. Separately I can also work on re-landing pixel snapping on ToT

I think this is probably the most accurate assessment. Caching items with fractional translations and displaying them at different fractional translations will have some visual impact. That impact should not be visible much during a transition animation, but for cases where we switch from caching to not caching while the content is relatively unmoving the difference can be detected.

Another option is to always have the endpoints of the rendering be at an integer location which would involve pixel-aware layout. If the before/after locations of a transition were both snapped to a pixel location then the rendering would be consistent between them and for the sub-pixel offsets during an animation from one location to another, the movement would hide the inaccuracies while the animation was happening.

[addendum - and the engine does not know if the location of an item is a "final layout location" or "a random frame during an animation" so we have no way of enforcing snapping just for endpoints unless all renderings of the item are pixel snapped...]

[jonahwilliams]

@jpnurmi PTAL at #35981 . Note that you may see pixel snapping after an animated opacity fade in. We'll need to make a separate revert to the framework to fix that.