Fix position of ImageFilter layer when raster-cached #38567
Conversation
|
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. |
|
We need tests here. Also, we need to verify that this works for both the "layer-cache" case and the "layer-children-cache" case. In all cases, an IFLayer with an offset of x,y should perform identically to the same IFLayer with an offset of 0,0, but with a parent TransformLayer that has a translation transform of x,y. The integralTransform call should also be relative to the offset, so moving the translation after the call to integralTransform can cause the round-off of the translation components to differ. I think what we really want to do is to cache the layer (and/or children?) without the offset and then apply them with it. That's conjecture, but the above conditions are the ones that we need to satisfy regardless of how we solve it. One suggestion might be to add a "PaintForCache(PaintContext context)" method to CacheableContainerLayer and that would simply apply the filter and draw the children without the offset for the case of IFLayer...? |
|
With this change I saw it drawing in the right position for layer-children-cache. Will look at moving things around so that the integralTransform is always applied after the offset. Regardless of the solution, do you have some guidance on how to test that the painted pixels after the Draw call are the same in all 3 paths? I couldn't find an example of a similar thing being done in other tests, only comparing of display-lists. |
The children may work fine because they are cached without calling this layer's paint method that does the translate.
Unfortunately you can't use the comparison of DLs when the cache is in use because I think it still uses the canvas for rendering and I don't think capturing the drawImage calls works well enough in that situation to do DL compares. Pixel comparisons also tend to be finicky to rasterization changes even when we deliver the same values to the renderer. It might be good to see if the MockRasterCache can record where it decided to render the cache image and then we can double check the positioning. That way it is only our own logic that will change the answer. |
|
Before this change, the draw calls for CacheImageFilterLayerSelf when cached were
After this change, the draw calls are
There isn't any change to the draw calls for CacheImageFilterLayerSelf uncached part or CacheChildren. Rounding the offset in the Preroll avoids caching an extra 1 pixel row/column due to roundOut if the offset would later round up in the IntegralTransform within Paint. |
|
@flar Anything else you want to see a test case of? |
There was a problem hiding this comment.
I'm not happy with this solution. It works around the issues rather than addressing them head-on.
We're going to have more layers that will involve an offset, it would be better to address this in layer_raster_cache_item.cc by passing in the offset and having it correct the caching and drawing accordingly there. We already do this with display_list_raster_cache_item.
LayerRasterCacheItem::GetBoundsForLayer should adjust the bounds that it returns (conditionally depending on whether children or layer are being cached).
flow/layers/image_filter_layer.cc
Outdated
| SkScalarRoundToScalar(offset_.y())); | ||
| } else { | ||
| child_bounds.offset(offset_); | ||
| } |
There was a problem hiding this comment.
I would just offset by offset_. None of the Preroll methods take integralTransform issues into account and that is an ongoing discussion about how to deal with that.
There was a problem hiding this comment.
I did this because the raster cache bounds get rounded-out. So if the child is at 99.9, IntegralTransform will snap it to 100, but the raster cache will capture from 99. It doesn't cause a position mismatch, but it means you get 1 blank row and/or column raster cached. But if there is another effort to fix it in a more general sense, I will take it out here.
There was a problem hiding this comment.
The child's bounds are also floating point so rounding the offset is only half of the problem. We round the sum, not the translation.
There was a problem hiding this comment.
Isn't this just as likely to cause a rounding problem as if you didn't round the offset - depending on the bounds of the children?
There was a problem hiding this comment.
This round here doesn't affect the draw position at all, just the bounds of the raster cache. I added it to make my tests more clear, as well as save on raster-caching 1 blank row and/or column in the case child_bounds would round up. But if I'm misunderstanding or it's too confusing just to have it handled here instead of generally, it can be removed without affecting the actual graphical output.
| mutator.translate(offset_); | ||
| if (context.raster_cache) { | ||
| mutator.integralTransform(); | ||
| } |
There was a problem hiding this comment.
I don't like this reorganization of the integral Transform. It's similar, but not exactly the math that should be happening.
There was a problem hiding this comment.
Hm where should the rounding go? I put it here to preserve the fractional part and only round at the end. But I guess that isn't what happens in Preroll, it's rounded before going through the image filter.
|
Not sure I get it still. I am not really familiar with this area, just jumped in to try and fix is it is quite an annoying problem in a screen of my app (an animation often flips between cached and not). Do you mean when drawing for raster cache (within |
|
This is starting to look even more problematic as I dig into it. With rendering the layer into the cache with an offset bounds, the cache code does an integralTransform on the un-translated matrix, then maps the bounds, and then rounds it out. This integralTransform and roundOut is baked into the cache code. With rendering the layer without a cache with an offset bounds, the Paint method translates the matrix by the offset and then does the integralTransform. Mathematically those won't match unless we instead provide more info to the cache code so that it can include the offset into the matrix before the integralTransform operation. This will require modifying a bunch of the LayerRasterCacheItem code and providing it with the offset and the un-translated bounds. |
|
@JsouLiang thoughts? |
|
Is this a good enough stopgap to land now till we reason about a more complete fix? |
|
I would prefer if we can land this for now and maybe make a more general fix in #38055. From what I can see the rounding order concerns existed even before moving offset into the ImageFilterLayer. As it is, the bug has a big impact, with blur effects drawn in seemingly-random positions. |
The only thing I'd change is to just always translate by offset_ since the rounding error will be a combination of the fractional values of both the child bounds and the matrix, of which the offset is only a part, so no rounding of any particular value will solve the problem. Note that the offset is 99.9999% of the time multiplied by a pixel DPI factor so rounding it in the absolute has no particular value to its scaled sub-pixel position. In fact, rounding 0.5 is unnecessary if the DPI is 2. The existing incoming matrix translation values also come into play and the size and shape of the content which may or may not have integer bounds also come into play. The round in Preroll might help some case, but it might just make a different case have a problem that it wouldn't have otherwise... |
|
Just need to clarify for my understanding: What's the purpose of pixel-snapping when raster-cache is present? Is it just to bake-in any subpixel rendering into the rasterized image at a "safe" alignment so it can be freely translated? Or does it also matter when drawing the rasterized image again? In that case we would need to snap after the offset. Of course the child might snap itself as well, right? With the ImageFilter matrix it could apply some additional transform which would require that. |
|
When we enter preroll we do the translate by offset then we create an AutoCache object which calls PrerollSetup on the LayerRasterCacheItem and records the matrix. Thus, the matrix under which we will render the cache item will include the unrounded offset. If/when we decide to cache, the rasterization will transform the layer's bounds by that matrix, round it out, and then use those bounds as the basis for how large to cache a surface. So, it is even more complicated than I originally pointed out because the remembered matrix has the offset and we've adjusted the layer's paint_bounds by the offset and so we end up with double the adjustment when computing the surface to render into. As you point out, this doesn't necessarily place the cache draw into the wrong location because we recompute the final position from the bounds so all it does is further complicate the rounding of the surface bounds. By rounding the offset when adjusting the bounds you limit this double-accounting to an integer adjustment, but it is relative to the existing transform, so if the transform has integer scale then you are only moving the bounds by an integer amount. But if the incoming scale isn't an integer scale then rounding the offset by which you translate the bounds will change the sub-pixel position of the destination bounds. This is a big mess right now and the round you perform on the offset is only benign under certain common input conditions. The double-transform by offset is a really big glitch in the system and we only evade its most common problems by luck. |
|
@flar: Removed the round in Preroll, was that the one remaining concern you had? |
|
Yes, that's OK. I'm hoping to rework how we manage the rounding and multiple bounds in a future PR. |
moffatman
force-pushed
the
image_filter_raster_cache
branch
from
cdc4422 to
2270716
Compare
moffatman
added
the
autosubmit
…119185) * c0329ece2 Roll ICU from 1b7d391f0528 to 2cce76fd67af (5 revisions) (flutter/engine#39136) * 44362c90f Fix position of ImageFilter layer when raster-cached (flutter/engine#38567)
* Fix position of ImageFilter layer when raster-cached * Move integral transform and add tests * Don't round in Preroll
* Fix position of ImageFilter layer when raster-cached * Move integral transform and add tests * Don't round in Preroll Co-authored-by: Callum Moffat <[email protected]>
When ImageFilter is drawn from raster-cache, the offset is in effect getting applied twice, so the resulting content is in the wrong position. Since the offset is baked into the raster-cache entry, it shouldn't be applied if drawing from there.
Fixes #111863
Pre-launch Checklist
///).