[Impeller] Re-enable fast blur path for elliptical rrects #53704

flar · 2024-07-02T23:50:35Z

Fast round rects were recently restricted to circular corners, but elliptical round rects can also go through the fast path.

flar · 2024-07-02T23:57:52Z

I'm expecting an error with a diff to list the new golden images, but passing other than that.

gaaclarke

lgtm, looks like you need to update an existing test.

gaaclarke · 2024-07-03T15:12:14Z

impeller/display_list/dl_golden_unittests.cc

+      DlColor::kMaroon(),
+  };
+
+  auto make_rrect_path = [](const SkRect& rect, DlScalar rx, DlScalar ry) {


Can you add an explicit return type here please? It's a bit of a hunt to understand the signature of this closure.

https://google.github.io/styleguide/cppguide.html#Type_deduction

flutter-dashboard · 2024-07-03T22:49:50Z

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 #53704 at sha 0139868

flar · 2024-07-03T22:52:44Z

OpenGLES is not rendering the path variants in the golden - is this a known issue? I've seen it on other goldens as well.

gaaclarke · 2024-07-03T22:56:52Z

OpenGLES is not rendering the path variants in the golden - is this a known issue? I've seen it on other goldens as well.

Yep, that's a known bug.

gaaclarke · 2024-07-03T23:01:43Z

impeller/display_list/skia_conversions_unittests.cc

+    auto save = test.values[i];
+    test.values[i] -= kEhCloseEnough * 0.5f;
+    rrect.setRectRadii(rect, test.radii);
+    EXPECT_TRUE(skia_conversions::IsNearlySimpleRRect(rrect));


Since this is in a loop the line number isn't sufficient to know where the failure happened. Can you also print out the radii?

something like:

Suggested change

EXPECT_TRUE(skia_conversions::IsNearlySimpleRRect(rrect));

EXPECT_TRUE(skia_conversions::IsNearlySimpleRRect(rrect)) << "radii: " << test.radii;

I don't think it will print the array but I can print the index and the substituted value...

Yeah, the array just prints out the address, but I can do this (temporarily sabotaging the test to get error output):

../../flutter/impeller/display_list/skia_conversions_unittests.cc:204: Failure Value of: skia_conversions::IsNearlySimpleRRect(rrect) Actual: true Expected: false values[7] == 8.9995002746582031 ../../flutter/impeller/display_list/skia_conversions_unittests.cc:208: Failure Value of: skia_conversions::IsNearlySimpleRRect(rrect) Actual: false Expected: true values[7] == 8.9975004196166992

gaaclarke

still lgtm modulo test cleanup

flutter-dashboard · 2024-07-03T23:47:53Z

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #53704 at sha 4839f13

…lutter/engine#53704)

…151293) flutter/engine@4190543...8e2d05f 2024-07-04 flar@google.com [Impeller] Re-enable fast blur path for elliptical rrects (flutter/engine#53704) 2024-07-03 skia-flutter-autoroll@skia.org Roll Skia from 9fd1dc779589 to d5f8dde714e4 (2 revisions) (flutter/engine#53721) 2024-07-03 yjbanov@google.com [web] ignore pointer events on plain text spans (flutter/engine#53694) 2024-07-03 mdebbar@google.com Add Semantics Property `linkUrl` (flutter/engine#53507) 2024-07-03 chinmaygarde@google.com [Embedder] Document incorrectly named field in FlutterOpenGLFramebuffer. (flutter/engine#53720) 2024-07-03 chinmaygarde@google.com [Impeller] Make storage sizes typed. (flutter/engine#53700) 2024-07-03 matanlurey@users.noreply.github.com Convert `run_ios_tests.sh` to `run_ios_tests.dart`. (flutter/engine#53645) 2024-07-03 matanlurey@users.noreply.github.com Move `//third_party/android_embedding_dependencies` to `//flutter/third_party`. (flutter/engine#53587) 2024-07-03 chinmaygarde@google.com [Impeller] Document how to debug/profile OpenGL ES on macOS. (flutter/engine#53671) 2024-07-03 kingtous@qq.com [Flutter Web(HTML)] fix: shader mask is painted incorrectly on shared offscreen canvas (flutter/engine#44998) 2024-07-03 kingtous@qq.com fix: mask disappeared when having nested mask filter on Flutter web HTML (flutter/engine#45166) 2024-07-03 skia-flutter-autoroll@skia.org Roll Skia from 86ee8cc61508 to 9fd1dc779589 (3 revisions) (flutter/engine#53715) 2024-07-03 skia-flutter-autoroll@skia.org Roll Skia from c14cce59222b to 86ee8cc61508 (1 revision) (flutter/engine#53713) 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 aaclarke@google.com,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

…lutter#151293) flutter/engine@4190543...8e2d05f 2024-07-04 flar@google.com [Impeller] Re-enable fast blur path for elliptical rrects (flutter/engine#53704) 2024-07-03 skia-flutter-autoroll@skia.org Roll Skia from 9fd1dc779589 to d5f8dde714e4 (2 revisions) (flutter/engine#53721) 2024-07-03 yjbanov@google.com [web] ignore pointer events on plain text spans (flutter/engine#53694) 2024-07-03 mdebbar@google.com Add Semantics Property `linkUrl` (flutter/engine#53507) 2024-07-03 chinmaygarde@google.com [Embedder] Document incorrectly named field in FlutterOpenGLFramebuffer. (flutter/engine#53720) 2024-07-03 chinmaygarde@google.com [Impeller] Make storage sizes typed. (flutter/engine#53700) 2024-07-03 matanlurey@users.noreply.github.com Convert `run_ios_tests.sh` to `run_ios_tests.dart`. (flutter/engine#53645) 2024-07-03 matanlurey@users.noreply.github.com Move `//third_party/android_embedding_dependencies` to `//flutter/third_party`. (flutter/engine#53587) 2024-07-03 chinmaygarde@google.com [Impeller] Document how to debug/profile OpenGL ES on macOS. (flutter/engine#53671) 2024-07-03 kingtous@qq.com [Flutter Web(HTML)] fix: shader mask is painted incorrectly on shared offscreen canvas (flutter/engine#44998) 2024-07-03 kingtous@qq.com fix: mask disappeared when having nested mask filter on Flutter web HTML (flutter/engine#45166) 2024-07-03 skia-flutter-autoroll@skia.org Roll Skia from 86ee8cc61508 to 9fd1dc779589 (3 revisions) (flutter/engine#53715) 2024-07-03 skia-flutter-autoroll@skia.org Roll Skia from c14cce59222b to 86ee8cc61508 (1 revision) (flutter/engine#53713) 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 aaclarke@google.com,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

[Impeller] Re-enable fast blur path for elliptical rrects

af9c825

github-actions bot added the e: impeller label Jul 2, 2024

flar requested review from jonahwilliams and gaaclarke July 2, 2024 23:57

gaaclarke approved these changes Jul 3, 2024

View reviewed changes

test fixes

0139868

flar requested a review from gaaclarke July 3, 2024 22:25

flutter-dashboard bot added the will affect goldens label Jul 3, 2024

gaaclarke reviewed Jul 3, 2024

View reviewed changes

gaaclarke approved these changes Jul 3, 2024

View reviewed changes

print out test values on error

4839f13

flar added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 3, 2024

auto-submit bot merged commit 8e2d05f into flutter:main Jul 4, 2024
29 checks passed

engine-flutter-autoroll mentioned this pull request Jul 4, 2024

Roll Flutter Engine from 4190543cb093 to 8e2d05fa95d7 (13 revisions) flutter/flutter#151290

Closed

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

8e2d05fa9 [Impeller] Re-enable fast blur path for elliptical rrects (f…

8d1069f

…lutter/engine#53704)

engine-flutter-autoroll mentioned this pull request Jul 4, 2024

Roll Flutter Engine from 4190543cb093 to 8e2d05fa95d7 (13 revisions) flutter/flutter#151293

Merged

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

8e2d05fa9 [Impeller] Re-enable fast blur path for elliptical rrects (f…

b73cb54

…lutter/engine#53704)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Impeller] Re-enable fast blur path for elliptical rrects #53704

[Impeller] Re-enable fast blur path for elliptical rrects #53704

flar commented Jul 2, 2024

flar commented Jul 2, 2024

gaaclarke left a comment

gaaclarke Jul 3, 2024

flutter-dashboard bot commented Jul 3, 2024

flar commented Jul 3, 2024

gaaclarke commented Jul 3, 2024

gaaclarke Jul 3, 2024

flar Jul 3, 2024

flar Jul 3, 2024

gaaclarke left a comment

flutter-dashboard bot commented Jul 3, 2024

	EXPECT_TRUE(skia_conversions::IsNearlySimpleRRect(rrect));
	EXPECT_TRUE(skia_conversions::IsNearlySimpleRRect(rrect)) << "radii: " << test.radii;

[Impeller] Re-enable fast blur path for elliptical rrects #53704

[Impeller] Re-enable fast blur path for elliptical rrects #53704

Conversation

flar commented Jul 2, 2024

flar commented Jul 2, 2024

gaaclarke left a comment

Choose a reason for hiding this comment

gaaclarke Jul 3, 2024

Choose a reason for hiding this comment

flutter-dashboard bot commented Jul 3, 2024

flar commented Jul 3, 2024

gaaclarke commented Jul 3, 2024

gaaclarke Jul 3, 2024

Choose a reason for hiding this comment

flar Jul 3, 2024

Choose a reason for hiding this comment

flar Jul 3, 2024

Choose a reason for hiding this comment

gaaclarke left a comment

Choose a reason for hiding this comment

flutter-dashboard bot commented Jul 3, 2024