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] Add mask blur style support to the RRect blur fast path. #51250

Merged
merged 8 commits into from
Mar 8, 2024

Conversation

bdero
Copy link
Member

@bdero bdero commented Mar 7, 2024

This improves performance by avoiding the 2-pass Gaussian blur in more shadow drawing situations.

The new golden also serves as a good reference for how mask blurs are supposed to work alongside various other paint properties such as color filters, color opacity, image filters, and blending.

Screenshot 2024-03-06 at 11 47 04 PM

The top 5 shapes are various RRect cases and are rendering correctly via the new blur style implementation in this patch.
The two bottom rows (the triangles and arcs) are non-rrect paths, so they're falling back to rendering using the 2-pass Gaussian blur. Rendering errors are circled in red below:

Screenshot 2024-03-06 at 11 47 04 PM

  • Cases 1, 2, 7, and 9 all appear to rendering fine.
  • Cases 3, 4, 5, and 6 all have mask blur styles set to BlurStyle::kSolid. After the first clipped overlay has been drawn, subsequent clipped overlays aren't drawing.
  • Case 6 is also has the blend mode set to BlendMode::kExclusion.
  • Cases 8 and 10 are rendering with BlurStyle::kInner and BlurStyle::kOuter respectfully, but with a blur ImageFilter also set on the paint state. The ImageFilter needs to be applied to the rasterized mask blurred content.

@flar
Copy link
Contributor

flar commented Mar 7, 2024

The SDF implementation suggested here might lend itself to an even more direct implementation if we could teach it to implement unequal corner radii. Since it computes the distance to the interior and knows if it is inside or outside, a lot of variants could be developed from it just by doing some clamping based on the sign of the SDF computation.

Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! The only thing we should do is make the test results easier to talk about and evaluate. There is like 30 images all on one image. These should be separate tests or labeled in the output. People should be able to easily talk about the results and correlate the output with the code, this makes that too difficult.

rrect_paint.color =
rrect_paint.GetColorFilter()->GetCPUColorFilterProc()(paint.color);
rrect_paint.color_filter = nullptr;
rrect_paint.invert_colors = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: This seems tricky, having to clear out invert_colors. I could see someone missing this easily, it might be worth making a function AbsorbColorFilter

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be turned around. Rather than clone and eliminate properties, create a default paint and only copy in the attributes you want.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed this up to try and make the paint construction less confusing. Please TAL.

Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

BlendMode blend_mode = BlendMode::kSourceOver;
};

static Picture MaskBlurVariantTest(const AiksTest& test_context,
Copy link
Member

@gaaclarke gaaclarke Mar 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wait, can you put these in aiks_blur_unittests, please? I wish I had a better way to force the sorting of this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@bdero bdero requested review from gaaclarke and flar March 8, 2024 00:49
@flutter-dashboard
Copy link

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 #51250 at sha 67b9e61

Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

@bdero bdero added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 8, 2024
@auto-submit auto-submit bot merged commit bc4abcd into flutter:main Mar 8, 2024
29 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 8, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Mar 8, 2024
…144817)

flutter/engine@bbb1ed0...bc4abcd

2024-03-08 bdero@google.com [Impeller] Add mask blur style support to the RRect blur fast path. (flutter/engine#51250)
2024-03-08 30870216+gaaclarke@users.noreply.github.com [Impeller] moved tests to aiks_blur_unittests, added warning (flutter/engine#51274)

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 rmistry@google.com,zra@google.com 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App e: impeller will affect goldens
Projects
None yet
3 participants