[Impeller] add clip coverage stack to exp canvas. #52215
Conversation
flar
left a comment
flar
left a comment
There was a problem hiding this comment.
One thing that needs fixing and a question...
| // save layers may transform the subpass texture after it's rendered, | ||
| // causing parent clip coverage to get misaligned with the actual area that | ||
| // the subpass will affect in the parent pass. | ||
| clip_coverage_stack_.PushSubpass(subpass_coverage, GetClipHeight()); |
There was a problem hiding this comment.
This is problematic because there may be an op just out of view that contributes pixels to the subpass due to "splatter". If the filter is a blur, it can blur something just out of view and get a blur fringe that is in bounds. Existing DLBuilder will not use a cull rect for accumulating the content of a saveLayer when it has a filter because of this. The new version of DlBuilder that I'm just about to push does an inverse transform of the bounds by the filter (not sure if the Impeller classes have this method, but it exists in DlImageFilter).
There was a problem hiding this comment.
This pass cannot render anything outside of the subpass coverage, because that is the size of the render target. If the bounds need to be bigger, then DL needs to give this bigger bounds.
There was a problem hiding this comment.
Also, we need to distinguish between the input/output of a filter. If this subpass coverage is limiting the output of a filter, that doesn't necessarily have any impact on the rendering of the filter inputs. That would use a different render target and have a different size.
There was a problem hiding this comment.
Ah, right. This applies when I am deducing a cull rect from the existing clip area, not when the bounds are supplied. DlBuilder has always used the supplied bounds for culling. In your case the bounds should always be supplied. It might apply if the code at the top of the function that uses the render target size as the layer bounds is ever invoked, but that should be vestigial at this point.
There was a problem hiding this comment.
So, to be more specific, if there is an existing cull rect, it should not be used to cull the contents of a saveLayer with a filter applied - even if that existing cull rect is "the bounds of the parent drawable/layer". You can either reverse transform the drawable (or cull rect) bounds through the filter (not through the transform) or use an infinite cull rect.
The bounds from the saveLayer, though, can be applied.
Consider that this isn't just the case of expanding or contracting the bounds for a blur radius - another ImageFilter transforms the pixels by a coordinate transform and so the pixels that will end up on the parent drawable could be rendered with any coordinates, even ones that bear no resemblance to the destination drawable. So, do not inherit any "culling rect" without consulting the filter.
In this case, the DlBuilder is already promising bounds, so the reuse-the-parent-bounds case is legacy/not-a-priority. I would just replace the "if we didn't get a bounds" code at the top of the function with "assert(we got bounds)" and this whole issue goes away.
There was a problem hiding this comment.
There is still a saveLayer without bounds here:
There was a problem hiding this comment.
It has a ToRect in there...? And it claims "kContainsContents"...? Doesn't that mean "I have bounds and you can trust them?
| size_t num_clips = transform_stack_.back().num_clips; | ||
| transform_stack_.pop_back(); | ||
|
|
||
| if (num_clips > 0) { |
There was a problem hiding this comment.
Aren't we using the new StC depth buffer for clips? I thought it didn't need a restore...?
There was a problem hiding this comment.
We need to encode the restore so that we can correctly remove certain clips (since we see pairs of clip - restore). But the entity itself doesn't do anything.
There was a problem hiding this comment.
I'm still not clear on why all of the computations here need to be done if this entity doesn't actually do anything and is only used to balance a restore...?
Also, what is the "certain" in "certain clips"? Are there some clips that do not use the StC model of depth buffer encoding to enforce the clips?
There was a problem hiding this comment.
oh, because we also need to update the clip scissor
There was a problem hiding this comment.
Hopefully Brandon can simplify this later...
| size_t num_clips = transform_stack_.back().num_clips; | ||
| transform_stack_.pop_back(); | ||
|
|
||
| if (num_clips > 0) { |
There was a problem hiding this comment.
Hopefully Brandon can simplify this later...
jonahwilliams
added
the
autosubmit
…147905) flutter/engine@b64e230...b7bfd94 2024-05-07 [email protected] [Impeller] add clip coverage stack to exp canvas. (flutter/engine#52215) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
2 participants
Uses clip scissor and clip stack to update the clip state. This is working now!
flutter/flutter#142054