[Impeller] implement mask blur for textures #51183

gaaclarke · 2024-03-04T23:00:36Z

fixes flutter/flutter#144266

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

impeller/aiks/canvas.cc

impeller/aiks/paint.cc

jonahwilliams · 2024-03-07T17:27:54Z

impeller/aiks/canvas.cc

+  bool needs_color_filter = paint.HasColorFilter();
+  if (needs_color_filter) {
+    auto color_filter = paint.GetColorFilter();
+    if (texture_contents->ApplyColorFilter(


I don't think this will ever pass? This is for automagically filtering solid colors or gradients, things that exist on host memory.

Done, removed it.

jonahwilliams · 2024-03-07T17:31:07Z

impeller/aiks/paint.cc

+}
+
 std::shared_ptr<FilterContents> Paint::MaskBlurDescriptor::CreateMaskBlur(
    std::shared_ptr<ColorSourceContents> color_source_contents,


We already have a path for creating a mask blur from a color source contents. If we created a TiledTextureContents (which is a color source contents) instead of a TextureContents for drawImageRect calls with a mask blur descriptor, then could we pass it into this function and have it just work?

Nope, we have to essentially create a transparent halo around the texture to make the blending work correctly. That requires modifying texcoords. The path below doesn't know anything about that, just expanding geometry which would stretch the image.

Sorry, not a transparent halo, but sampling outside of the image based on whatever the sampler settings are.

Oh, that's a bug in the existing path, then. And that same issue actually applies to all the ColorSource types (except for solid colors, of course).

One easy solution for that would be to amend the effect transform of the ColorSourceContents.

Oh yea, that would explain some of the stretching I saw of gradients. I'll file an issue.

flutter/flutter#144836

jonahwilliams · 2024-03-07T19:22:12Z

impeller/aiks/paint.cc

                                              absorb_opacity);
 }

+std::shared_ptr<FilterContents> Paint::MaskBlurDescriptor::CreateMaskBlur(


I would defer to @bdero to review the mask blur implementation. Overall code looks reasonable to me.

jonahwilliams

LGTM

gaaclarke · 2024-03-08T17:49:31Z

This should be generating goldens but hasn't yet. Let's see if it does this time around.

bdero · 2024-03-08T20:54:45Z

Mask blurring for all color sources (including textures) is already implemented if the user sets the color source and calls a path drawing command on Aiks (DrawPath, DrawRect, DrawCircle, etc.)

Not blocking this, but we'll need to fix the bugs in the generic color source path anyway. So later on I think we'll end up switching DrawImageRect to use TiledTextureContents (as @jonahwilliams suggested) in favor of copying and tweaking the existing flawed path (which I suspect has more behavioral flaws than just the UV mapping -- flaws that are being duplicated here).

gaaclarke · 2024-03-08T21:50:44Z

we'll end up switching DrawImageRect to use TiledTextureContents (as @jonahwilliams suggested) in favor of copying and tweaking the existing flawed path

I tried working with TiledTextureContents and I wasn't seeing the mechanism to effectively edit the texcoords like I needed. It doesn't have any documentation, I think I'm missing some information. Can someone fill me in and/or fill out the TiledTextureContents documentation?

edit: I think you mention effect transform should affect the texcoords?

which I suspect has more behavioral flaws than just the UV mapping

Probably worth filing bugs if we find those so we can make sure we address them.

jonahwilliams · 2024-03-08T22:13:29Z

This isn't the color source responsibility as much as the geometry responsibility. not that the geometry class has docs, but I'd start by looking at ComputeUVGeometryForRect to see how the UVS are computed

gaaclarke · 2024-03-08T22:19:44Z

Ahh okay, so you're thinking something like contents->SetGeometry(contents->GetGeometry()->Scale(foo)->ScaleTexCoords(1/foo)). Makes sense.

jonahwilliams · 2024-03-08T22:21:13Z

Yeah, by adjusting the geometry we should be able to adjust the size/texture coordinates for all color sources and solve this generically.

bdero · 2024-03-08T22:29:03Z

There's no need to modify any Geometry or Contents; everything that's needed to support the DrawImageRect use case with TiledTextureContents is already in place.

Just use the ColorSourceContents effect transform to:

Apply the source rectangle mapping done by DrawImageRect.
Fix the bug in the generic mask blur path by amending a source->destination rect transform to the existing ColorSourceContents effect transform when we perform the geometry border expansion.

bdero · 2024-03-08T23:03:45Z

Ahh okay, so you're thinking something like contents->SetGeometry(contents->GetGeometry()->Scale(foo)->ScaleTexCoords(1/foo)). Makes sense.

Note that the geometry change that happens for the mask blur is a border expansion, which is not a uniform scale-up centered around the UV space origin. So to fix the bug in the existing path, we need to apply the inverse of the border expansion to the UVs with a rect->rect mapping transform.

Yeah, by adjusting the geometry we should be able to adjust the size/texture coordinates for all color sources and solve this generically.

This can't work unless we also add support for applying this redundant geometry effect transform to all the color sources that don't use geometry->GetPositionUVBuffer. Strongly recommend just using the existing effect transform.

gaaclarke · 2024-03-08T23:19:54Z

Ahh I was wondering if that would be a problem. Essentially that addresses the problem of what is the pivot used for the scale with the geometry approach if I'm understanding correctly.

flutter-dashboard · 2024-03-08T23:42:38Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #51183 at sha 5c33bde

…#51183)

…144862) flutter/engine@953927e...7541e90 2024-03-08 [email protected] [Impeller] implement mask blur for textures (flutter/engine#51183) 2024-03-08 [email protected] Don't rely on dart binary on PATH in run_test.py (flutter/engine#51302) 2024-03-08 [email protected] Mark the Flutter Views as focusable by setting a tabindex value. (flutter/engine#50876) 2024-03-08 [email protected] Update the instructions for updating licenses. (flutter/engine#51297) 2024-03-08 [email protected] Optimize overlays in CanvasKit (flutter/engine#47317) 2024-03-08 [email protected] Roll Fuchsia Linux SDK from 5Ra_AjCji-uR1GaX7... to lAV5jgp4796siOZgI... (flutter/engine#51296) 2024-03-08 [email protected] [Impeller] Add the KHR prefix to existing swapchain utilities. (flutter/engine#51295) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from 5Ra_AjCji-uR to lAV5jgp4796s 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] 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

github-actions bot added the e: impeller label Mar 4, 2024

jonahwilliams reviewed Mar 5, 2024

View reviewed changes

impeller/aiks/canvas.cc Show resolved Hide resolved

gaaclarke force-pushed the mask-blur-texture branch from ca123d4 to 4c75338 Compare March 5, 2024 23:53

gaaclarke commented Mar 6, 2024

View reviewed changes

impeller/aiks/paint.cc Show resolved Hide resolved

gaaclarke force-pushed the mask-blur-texture branch 2 times, most recently from 09bbeba to bd3a0c3 Compare March 6, 2024 00:47

gaaclarke marked this pull request as ready for review March 6, 2024 00:48

gaaclarke requested review from bdero and jonahwilliams March 6, 2024 00:48

jonahwilliams reviewed Mar 7, 2024

View reviewed changes

gaaclarke force-pushed the mask-blur-texture branch from e620d09 to 0fee14e Compare March 7, 2024 17:31

gaaclarke requested a review from jonahwilliams March 7, 2024 17:52

jonahwilliams reviewed Mar 7, 2024

View reviewed changes

jonahwilliams approved these changes Mar 7, 2024

View reviewed changes

gaaclarke force-pushed the mask-blur-texture branch from 3c0163d to af8ce92 Compare March 8, 2024 01:47

gaaclarke added 8 commits March 8, 2024 09:42

[Impeller] implement mask blur for textures

35843e8

okay a bit closer, this seems to mimic skia's behavior

0ff9413

removed fudge

81b2a32

lint

88c766b

renamed test

11da599

added golden update

abd12a4

removed color filter

66c4d38

removed changes to impeller golden tests output

3233ee2

gaaclarke force-pushed the mask-blur-texture branch from af8ce92 to 3233ee2 Compare March 8, 2024 17:45

added removed goldens back

6868bd4

gaaclarke mentioned this pull request Mar 8, 2024

[Impeller] mask blurs on ColorSourceContents stretches the contents flutter/flutter#144836

Closed

Merge branch 'main' into mask-blur-texture

2277ce1

gaaclarke and others added 2 commits March 8, 2024 14:57

Merge branch 'main' into mask-blur-texture

22246b5

fixed merge conflict

5c33bde

gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 8, 2024

flutter-dashboard bot added the will affect goldens label Mar 8, 2024

gaaclarke merged commit 7541e90 into flutter:main Mar 8, 2024

engine-flutter-autoroll mentioned this pull request Mar 9, 2024

Roll Flutter Engine from 953927eae78b to 7541e902f1b4 (7 revisions) flutter/flutter#144862

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 9, 2024

7541e902f [Impeller] implement mask blur for textures (flutter/engine…

6fd288d

…#51183)

[Impeller] implement mask blur for textures #51183

[Impeller] implement mask blur for textures #51183

Uh oh!

Conversation

gaaclarke commented Mar 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

gaaclarke commented Mar 8, 2024

Uh oh!

bdero commented Mar 8, 2024

Uh oh!

gaaclarke commented Mar 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonahwilliams commented Mar 8, 2024

Uh oh!

gaaclarke commented Mar 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonahwilliams commented Mar 8, 2024

Uh oh!

bdero commented Mar 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bdero commented Mar 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaaclarke commented Mar 8, 2024

Uh oh!

flutter-dashboard bot commented Mar 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gaaclarke commented Mar 4, 2024 •

edited

Loading

gaaclarke commented Mar 8, 2024 •

edited

Loading

gaaclarke commented Mar 8, 2024 •

edited

Loading

bdero commented Mar 8, 2024 •

edited

Loading

bdero commented Mar 8, 2024 •

edited

Loading