Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Impeller] Make transparent fills have no coverage #34055

Merged
merged 2 commits into from
Jun 15, 2022

Conversation

bdero
Copy link
Member

@bdero bdero commented Jun 15, 2022

No description provided.

@flar
Copy link
Contributor

flar commented Jun 15, 2022

I'm curious about how this interacts with various use cases.

If I have a drawing and I want to blur it and I use a blur with kClamp, but I surround it with a transparent fill to give it a border of transparency, would this optimize out the transparent fill and cause the blur to use clamp mode on the border of the drawing? Should it? (Does it currently with Flutter on Skia?)

To clarify - it would be OK to not apply the rendering in that case, but should it contribute to the size of a layer in a saveLayer?

@flar
Copy link
Contributor

flar commented Jun 15, 2022

I may be wrong about using the coverage size of a rendering to compute the input for a filter.

https://fiddle.skia.org/c/51a32450b524fed60bff5894f22ed55b

It looks like Skia assumes a boundary of transparency if you don't specify a bounds. And if you do specify a bounds, then you are in control of where the edge pixels to "clamp" will come from.

@bdero
Copy link
Member Author

bdero commented Jun 15, 2022

Good point. We don't support clamp sampling in the blur filter yet (It only behaves like decal), but once we do, I think the correct behavior would be to always force decal sampling when used on a SaveLayer.

Coverage is kind of an implementation detail in Impeller used to save on resources and can't be relied on for clipping (occasionally coverage is too large, like with solid strokes). In general, if every override of Contents::GetCoverage were to suddenly start returning the entire screen rectangle, nothing should visibly change about the frame.

@dnfield
Copy link
Contributor

dnfield commented Jun 15, 2022

My spidey sense says flutter/flutter#72526 (comment) is relevant?

@bdero
Copy link
Member Author

bdero commented Jun 15, 2022

Oh, indeed it is. Note that the render method ducks out and doesn't draw anything if the color is transparent as-is, which is a bug because if the blend mode is kSrc, for example, it would result in contents changing on the screen.

However, when taking into account a layer in isolation, expanding the coverage in this case would have no affect, as the additional pixels would initialize as 0, 0, 0, 0. And any blend with fully transparent black as both the source and destination should wind up remaining at 0, 0, 0, 0.

BUT there's an another potential problem that needs to be investigated -- consider this render list:

  1. paint red (fills the screen)
  2. save layer, blend mode = kSrc
  3. PaintGreenRectangleXYWH(0, 0, 100, 100), blend mode = kSrc
  4. PaintInvisibleRectangleXYWH(50, 50, 100, 100), blend mode = kSrc
  5. restore

We know for sure that the bottom right quadrant of the green rectangle should end up being black on-screen, but should the layer itself expand with the invisible rectangle? This would cause an additional 50 pixel black border to appear around the right and bottom sides of the green rectangle.

@dnfield
Copy link
Contributor

dnfield commented Jun 15, 2022

Without checking what Skia does, I'd expect it to expand the layer size - but it'd be good to just run this through Skia fiddle and see what it does. If it doesn't expand, then we're probably safe to drop the coverage since it won't cause regressions. If it does then we either have to detect the blend modes to see if it's safe, or keep it always if that's too hard.

@bdero
Copy link
Member Author

bdero commented Jun 15, 2022

Right, I'm trying it in Skia fiddle at the moment but I think I missed the forest for the trees here. For kSrc mode, it ends up expanding the layer to cover everything, and the whole background gets erased (https://fiddle.skia.org/c/675b146b99cc1a116bce0528d9d74415). This seems to be consistent with how I've been conceptualizing coverage so far:

In general, if every override of Contents::GetCoverage were to suddenly start returning the entire screen rectangle, nothing should visibly change about the frame.

So the TL;DR is: Skipping over coverage should actually be safe here, but we need to add logic to always expand subpass coverage to cover the entire screen if the subpass blend mode may modify the destination color when given a source color of fully transparent black. Going to make a bug to follow up on this.

@dnfield
Copy link
Contributor

dnfield commented Jun 15, 2022

SGTM. Please link the bug to this discussio nfor context :)

@dnfield
Copy link
Contributor

dnfield commented Jun 15, 2022

Oh and one comment on your fiddle - using nullptr for the bounds of the saveLayer call in Skia tells it to use the current clip. If you specify bounds matching the rect you're going to draw it doesn't fill the whole clip and you get a green square inside the red one.

@bdero bdero added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jun 15, 2022
@bdero bdero force-pushed the bdero/solid-fill-coverage branch from c27fb8f to 3e0d9cc Compare June 15, 2022 18:21
@flar
Copy link
Contributor

flar commented Jun 15, 2022

I was confused about the saveLayer trying to compute bounds. I thought I saw that in a discussion from long ago, but I wouldn't know where to look for it now. As Dan indicates and Skia just confirmed, the bounds of a saveLayer in the absence of a parameter are the entire clip or surface.

So, I'm not exactly sure where computing the coverage of a transparent operation would matter (modulo fixing the case of a destructive blend mode)? Where does this coverage get used? For determining which pixels to target with quads/tris on a given rendering op?

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 16, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 16, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 16, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 16, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 16, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 16, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 16, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 16, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 16, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 16, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 16, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 16, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 17, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 17, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 17, 2022
houhuayong pushed a commit to houhuayong/engine that referenced this pull request Jun 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: engine e: impeller waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants