Partial revert of fractional translation changes to raster cache

[jonahwilliams]

Revert changes to raster cache entries, which must be SkiRect to accurately represent texture size

flutter/flutter#110957

[jonahwilliams]

FYI @jpnurmi , I think I messed up implementing this and your fix is right. These should all be SkIRect since we're dealing with texture size and not "pixels" anymore. This fixes the jumping on my end, if I turn my display dpr from 1.25 to 1 it becomes noticeable. Not sure exactly why this isn't apparent at higher dpr

[jpnurmi]

Unfortunately, I'm still seeing some jumps with this (as I did with the retracted round-out proposal). A lot less than with the current main, though.

35918.mp4

[jpnurmi]

Also, the FML_DCHECK in RasterCacheResult::draw occasionally fails. For example, a page like this:

Would cache the list of WiFi connections:

RasterCache::Rasterize LTRB (WxH)
	logical rect: 0,000000000 0,000000000 588,400024414 122,000000000 (588,400024414x122,000000000)
	device rect: 101,142059326 241,000000000 689,542114258 363,000000000 (588,400024414x122,000000000)
	cache rect: 101 241 690 363 (589x122)

First, drawing at the exact location where it was cached, would be fine:

RasterCacheResult::draw LTRB (WxH)
	logical rect: 0,000000000 0,000000000 588,400024414 122,000000000 (588,400024414x122,000000000)
	device rect: 101,142059326 241,000000000 689,542114258 363,000000000 (588,400024414x122,000000000)
	bounds: 101 241 690 363 (589x122)
	image dimensions: 589x122

But after the page contents have been fully loaded, the cached image would get drawn at different coordinates, where rounding out results in a different size:

RasterCacheResult::draw LTRB (WxH)
	logical rect: 0,000000000 0,000000000 588,400024414 122,000000000 (588,400024414x122,000000000)
	device rect: 86,829368591 241,000000000 675,229370117 363,000000000 (588,400024414x122,000000000)
	bounds: 86 241 676 363 (590x122)
	image dimensions: 589x122

[FATAL:flutter/flow/raster_cache.cc(46)] Check failed: image_->dimensions().width() - bounds.width() > -epsilon && image_->dimensions().height() - bounds.height() > -epsilon && image_->dimensions().width() - bounds.width() < 1 + epsilon && image_->dimensions().height() - bounds.height() < 1 + epsilon.

[jpnurmi]

Raster cache keys used to include the fractions. Perhaps that should be restored to avoid drawing cached images at different fractions?

diff --git a/flow/raster_cache_key.h b/flow/raster_cache_key.h
index 64292b6e89..4815cb6d7d 100644
--- a/flow/raster_cache_key.h
+++ b/flow/raster_cache_key.h
@@ -83,8 +83,8 @@ class RasterCacheKey {
 
   RasterCacheKey(RasterCacheKeyID id, const SkMatrix& ctm)
       : id_(std::move(id)), matrix_(ctm) {
-    matrix_[SkMatrix::kMTransX] = 0;
-    matrix_[SkMatrix::kMTransY] = 0;
+    matrix_[SkMatrix::kMTransX] = SkScalarFraction(ctm.getTranslateX());
+    matrix_[SkMatrix::kMTransY] = SkScalarFraction(ctm.getTranslateY());
   }
 
   const RasterCacheKeyID& id() const { return id_; }

[jonahwilliams]

We can change the DCHECK, that was update for fractional translation and not ever reverted.

Raster cache keys used to include the fractions. Perhaps that should be restored to avoid drawing cached images at different fractions?

That would be the same as disabling the raster cache for all purposes, since a translating entry would be constantly redrawn as one scrolled.

[jonahwilliams]

Raster cache keys used to include the fractions.

Maybe I'm misunderstanding you here. I could try that out. We could also use whether or not the translation results in an exact pixel boundary. That wouldn't disable caching while scrolling but might help with the 1dpr case.

I think I need to keep looking at this, we should be able to fix this.

[jonahwilliams]

@jpnurmi do you have a small example that repros that hover jump? Think I have an idea for that

[jonahwilliams]

Pushed an update that tries to make roundOut usage smarter about how much larger the texture needs to be, and to use the logical origin for drawing/offseting the RC entry.

[jonahwilliams]

actually, maybe a simpler approach is #35930

[jonahwilliams]

So #35930 is probably the right approach. We need to round out the bounds so we don't end up on some weird pixel boundary. But that apparently has different results depending on exactly how we cache the children. I think we can get away with tracking the amount the top corner gets offset and adjusting later on, I just need to find another example where that still happens so I can debug it.

[tgucio]

@jonahwilliams In case this helps I put together a small example in flutter/flutter#107921.

[jpnurmi]

With @tgucio's example, the popup borders and icons are no longer jumping but the tooltips still are:

3.0.5	main	alt_approach
raster_cache_tooltip-3.0.5.mp4	raster_cache_tooltip-main.mp4	raster_cache_tooltip-alt_approach.mp4

[jpnurmi]

Is there a way to get the total matrix in RasterCache::Rasterize?

Rasterized "Open" tooltip:

RasterCache::Rasterize LTRB (WxH)
        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 72,872169495 24,000000000 (72,872169495x24,000000000)
	raw dest rect: 0,000000000 0,000000000 72,872169495 24,000000000 (72,872169495x24,000000000)
	dest rect: 0 0 73 24 (73x24)
	image: 73x24
	texture edge: 0,000000000 0,000000000

RasterCacheResult::draw LTRB (WxH)
        matrix: [  1,0000   0,0000 677,0000][  0,0000   1,0000 470,5000][  0,0000   0,0000   1,0000]
	logical rect: 0,000000000 0,000000000 72,872169495 24,000000000 (72,872169495x24,000000000)
	bounds: 677,000000000 470,500000000 749,872192383 494,500000000 (72,872192383x24,000000000)
	rounded bounds: 677 470 750 495 (73x25)
	image: 73x24
	texture edge: 0,000000000 0,000000000

[jonahwilliams]

Yeah that makes sense, So the changes fixes the jumping if we're rasterizing a display list, but not a layer. I think the fix is to offset the layer by the texture leading edge

[jonahwilliams]

I'm much less concerned about the tooltips moving than I am about the picture offsets changing. Ultimately that is an old issue that this patch re-introduces, wherein we need to keep animated opacity layers in the tree to stabilize the rendering.

More significant changes to work around this would likely be risky for a cherry pick, but a small change to fix the jumping and a second cherry pick to the animated opacity layer should fix both of these issues

[jonahwilliams]

Looking at this more today, and I think the ultimately we've got some rounding out that is either happening in display_list, or skia, in a way that isn't apparent to our code. We may be able to work around this but it will take more investigation