Migrate Mutators to DisplayList/Impeller geometry #164258

flar · 2025-02-27T01:51:29Z

Mutator types and MutatorsStack will now use DisplayList/Impeller geometry objects.

jonahwilliams

Approach LGTM. I'll look more closely tomorrow.

jonahwilliams · 2025-02-28T16:21:31Z

engine/src/flutter/shell/platform/android/platform_view_android_jni_impl.cc

-        const SkMatrix& matrix = (*iter)->GetMatrix();
-        SkScalar matrix_array[9];
-        matrix.get9(matrix_array);
+      case MutatorType::kTransform: {


Note to self, investigate if we need to drop to a 3x3 here or if we could use a full 4x4

The Android class being populated was a 3x3-based class. If they have a more accurate class we can consider adopting that, in a separate issue.

Filed as #164596

jonahwilliams · 2025-02-28T16:22:45Z

engine/src/flutter/shell/platform/android/platform_view_android_jni_impl.cc


  std::vector<std::shared_ptr<Mutator>>::const_iterator iter =
      mutators_stack.Begin();
  while (iter != mutators_stack.End()) {


A note for @gmackall , we should refactor this logic to share the mutator encoding between the two platform view JNI methods.

Should this and the 4x4 vs 3x3 issue be combined or separate?

I'll leave this to @gmackall to file if necessary.

jonahwilliams · 2025-02-28T16:26:29Z

engine/src/flutter/shell/platform/fuchsia/flutter/external_view_embedder.cc

  ViewMutators mutators;
-  SkMatrix total_transform = SkMatrix::I();
-  SkMatrix transform_accumulator = SkMatrix::I();
+  flutter::DlMatrix total_transform;


I wonder if the fuchsia code is going to have a problem with this because we've ifdef'd out impeller? But if it seems to be compiling I dunno.

FYI @zanderso

The external view embedder unit tests are failing on Fuchsia...

The only thing I found wrong with them is that I was using slightly different math to convert the int alphas to float alphas. I think I fixed that in the last 2 commits.

jonahwilliams

LGTM

flar · 2025-03-04T23:35:04Z

I have found the source of the iOS golden failures. The code in the platform view tries to turn an rrect clip into a path, but it botches the job - you can look at it in the "old code", it doesn't have any of the typical "magic bezier scale factors" that should be there, it just repeats the corner and end points as "control points".

My new code was passing the work off to an existing RR->Path converter which was doing an accurate job and that caused problems.

I think I'm just going to do a straight translation of the old code into the new data structures and leave "fixing the rrect conversion" to another issue to be fixed independently (see #164595).

flar · 2025-03-05T02:13:47Z

@jonahwilliams this should be good to go now, with no golden file changes needed.

jonahwilliams · 2025-03-05T02:27:55Z

Will take another look tomorrow. lets let #164525 land first, it should have only a small impact on this PR but will cause conflits one way or the other.

jonahwilliams

LGTM!

flutter-dashboard · 2025-03-05T20:53:53Z

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.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #164258 at sha 1156eb5

flar · 2025-03-05T21:10:11Z

The latest golden diffs are not intentional, I believe the issue is that the path being iterated is a converted path from the original SkPath rather than the raw path. The converted path has an approximation of the curves that doesn't match. I have an idea to enable simpler path iteration via DlPath that will isolate the caller from knowing which is the authoritative version of the path without having to embed knowledge of SkPath.

flar · 2025-03-05T23:52:33Z

The golden issue is clipping to an Oval which is handled with high precision code in the Impeller rendering, but passed along to the android platform as an approximated path. For now, we will pull out the simplified/reduced path types in the Android embedder code, but we really should handle this at a higher level (see #164666)

jonahwilliams

LGTM

flutter-dashboard · 2025-03-06T00:58:53Z

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

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #164258 at sha 0e5fab0

flar · 2025-03-06T09:04:40Z

A more general solution to the issue found with the oval clips is being pursued in #164693

Since that other PR has a risk of causing tiny golden diffs throughout the many layers of testing, it is being pursued as its own PR to have its own life cycle of potential golden image disruption.

…utter#164258)

flutter/flutter@321fbc0...6b93cf9 2025-03-07 [email protected] Roll Skia from 32c1931117b8 to cbc7e99d6c2f (1 revision) (flutter/flutter#164788) 2025-03-07 [email protected] Roll Packages from fc9d5ca to 4c5a7ed (4 revisions) (flutter/flutter#164785) 2025-03-07 [email protected] Roll Skia from 79f8af105a61 to 32c1931117b8 (1 revision) (flutter/flutter#164782) 2025-03-07 [email protected] Roll Fuchsia Linux SDK from fhm5z889sA5T1AQao... to ixl5bKWCqsRiYGvps... (flutter/flutter#164780) 2025-03-07 [email protected] Roll Skia from 181d81920670 to 79f8af105a61 (1 revision) (flutter/flutter#164770) 2025-03-07 [email protected] Roll Skia from cc74d34e7e68 to 181d81920670 (1 revision) (flutter/flutter#164766) 2025-03-07 [email protected] Clip layers reduce rrects and paths to simpler shapes when possible (flutter/flutter#164693) 2025-03-07 [email protected] [Impeller] test empty snapshot and allocation failure. (flutter/flutter#164668) 2025-03-07 [email protected] Roll Skia from 263308ea4386 to cc74d34e7e68 (2 revisions) (flutter/flutter#164746) 2025-03-06 [email protected] Adds aria-controls support (flutter/flutter#163894) 2025-03-06 [email protected] Migrate Mutators to DisplayList/Impeller geometry (flutter/flutter#164258) 2025-03-06 [email protected] Add lldb init file (flutter/flutter#164344) 2025-03-06 [email protected] Use separate artifacts for arm64 and x64 versions of gen_snapshot on Apple platforms (flutter/flutter#164419) 2025-03-06 [email protected] Roll Skia from ccd8cc23aa94 to 263308ea4386 (1 revision) (flutter/flutter#164728) 2025-03-06 [email protected] [hcpp] Add tests for transform mutator (flutter/flutter#164664) 2025-03-06 [email protected] Roll pub packages (flutter/flutter#164721) 2025-03-06 [email protected] Roll Packages from abba683 to fc9d5ca (3 revisions) (flutter/flutter#164714) 2025-03-06 [email protected] Roll pub packages (flutter/flutter#164713) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: 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

…utter#164258)

flutter/flutter@321fbc0...6b93cf9 2025-03-07 [email protected] Roll Skia from 32c1931117b8 to cbc7e99d6c2f (1 revision) (flutter/flutter#164788) 2025-03-07 [email protected] Roll Packages from fc9d5ca to 4c5a7ed (4 revisions) (flutter/flutter#164785) 2025-03-07 [email protected] Roll Skia from 79f8af105a61 to 32c1931117b8 (1 revision) (flutter/flutter#164782) 2025-03-07 [email protected] Roll Fuchsia Linux SDK from fhm5z889sA5T1AQao... to ixl5bKWCqsRiYGvps... (flutter/flutter#164780) 2025-03-07 [email protected] Roll Skia from 181d81920670 to 79f8af105a61 (1 revision) (flutter/flutter#164770) 2025-03-07 [email protected] Roll Skia from cc74d34e7e68 to 181d81920670 (1 revision) (flutter/flutter#164766) 2025-03-07 [email protected] Clip layers reduce rrects and paths to simpler shapes when possible (flutter/flutter#164693) 2025-03-07 [email protected] [Impeller] test empty snapshot and allocation failure. (flutter/flutter#164668) 2025-03-07 [email protected] Roll Skia from 263308ea4386 to cc74d34e7e68 (2 revisions) (flutter/flutter#164746) 2025-03-06 [email protected] Adds aria-controls support (flutter/flutter#163894) 2025-03-06 [email protected] Migrate Mutators to DisplayList/Impeller geometry (flutter/flutter#164258) 2025-03-06 [email protected] Add lldb init file (flutter/flutter#164344) 2025-03-06 [email protected] Use separate artifacts for arm64 and x64 versions of gen_snapshot on Apple platforms (flutter/flutter#164419) 2025-03-06 [email protected] Roll Skia from ccd8cc23aa94 to 263308ea4386 (1 revision) (flutter/flutter#164728) 2025-03-06 [email protected] [hcpp] Add tests for transform mutator (flutter/flutter#164664) 2025-03-06 [email protected] Roll pub packages (flutter/flutter#164721) 2025-03-06 [email protected] Roll Packages from abba683 to fc9d5ca (3 revisions) (flutter/flutter#164714) 2025-03-06 [email protected] Roll pub packages (flutter/flutter#164713) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: 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 platform-android Android applications specifically platform-ios iOS applications specifically engine flutter/engine related. See also e: labels. platform-fuchsia Fuchsia code specifically team-ios Owned by iOS platform team labels Feb 27, 2025

flar force-pushed the mutators-migrate-to-dl-geometry branch from 7514084 to e4c4add Compare February 27, 2025 23:16

flar requested review from gmackall and jonahwilliams February 27, 2025 23:33

jonahwilliams reviewed Feb 28, 2025

View reviewed changes

jonahwilliams approved these changes Feb 28, 2025

View reviewed changes

flar marked this pull request as ready for review March 5, 2025 02:06

This was referenced Mar 5, 2025

Android PlatformView creates inaccurate path for RoundRect clips #164595

Open

Android PlatformView conveys transforms using a 3x3 representation. #164596

Closed

flar requested a review from jonahwilliams March 5, 2025 02:13

flar added 2 commits March 5, 2025 10:44

Migrate MutatorsStack to DL/Impeller geometry

85d7e25

migrate recently added Mutators code

1156eb5

flar force-pushed the mutators-migrate-to-dl-geometry branch from 8c43b81 to 1156eb5 Compare March 5, 2025 19:42

flar mentioned this pull request Mar 5, 2025

[Engine] Allow MutatorsStack to use non-skia classes #163716

Closed

jonahwilliams approved these changes Mar 5, 2025

View reviewed changes

flutter-dashboard bot added the will affect goldens Changes to golden files label Mar 5, 2025

better matching on oval clipping in hcpp mutators

73a7c6b

flar mentioned this pull request Mar 5, 2025

Reduce Path clip Mutators to simpler geometry #164666

Open

document workaround in hcpp clip Mutator

0e5fab0

jonahwilliams approved these changes Mar 5, 2025

View reviewed changes

flar added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 6, 2025

flar mentioned this pull request Mar 6, 2025

Clip layers reduce rrects and paths to simpler shapes when possible #164693

Merged

Merge branch 'master' into mutators-migrate-to-dl-geometry

832c193

auto-submit bot added this pull request to the merge queue Mar 6, 2025

Merged via the queue into flutter:master with commit 66e910d Mar 6, 2025
177 checks passed

flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Mar 6, 2025

flar mentioned this pull request Mar 7, 2025

DL/Geometry engine migration tracking issue #161456

Open

37 tasks

flar added a commit to flar/flutter that referenced this pull request Mar 7, 2025

remove workaround for flutter#164258

ec5836b

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 7, 2025

66e910d Migrate Mutators to DisplayList/Impeller geometry (flutter/fl…

3c08516

…utter#164258)

engine-flutter-autoroll mentioned this pull request Mar 7, 2025

Roll Flutter from 321fbc0e7e81 to e190868b71de (15 revisions) flutter/packages#8812

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 7, 2025

66e910d Migrate Mutators to DisplayList/Impeller geometry (flutter/fl…

70d90a3

…utter#164258)

engine-flutter-autoroll mentioned this pull request Mar 7, 2025

Roll Flutter from 321fbc0e7e81 to d9f48174fca6 (16 revisions) flutter/packages#8815

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 7, 2025

66e910d Migrate Mutators to DisplayList/Impeller geometry (flutter/fl…

2e66047

…utter#164258)

engine-flutter-autoroll mentioned this pull request Mar 7, 2025

Roll Flutter from 321fbc0e7e81 to 1b14b0c11a5b (17 revisions) flutter/packages#8816

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 7, 2025

66e910d Migrate Mutators to DisplayList/Impeller geometry (flutter/fl…

07738e7

…utter#164258)

engine-flutter-autoroll mentioned this pull request Mar 7, 2025

Roll Flutter from 321fbc0e7e81 to 6b93cf93c100 (18 revisions) flutter/packages#8817

Merged

dkwingsmt mentioned this pull request Mar 19, 2025

[Engine] Make platform mutators support kClipRSE #165508

Open

6 tasks

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 20, 2025

66e910d Migrate Mutators to DisplayList/Impeller geometry (flutter/fl…

69c6064

…utter#164258)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 20, 2025

66e910d Migrate Mutators to DisplayList/Impeller geometry (flutter/fl…

316bdf4

…utter#164258)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 21, 2025

66e910d Migrate Mutators to DisplayList/Impeller geometry (flutter/fl…

5577166

…utter#164258)

Migrate Mutators to DisplayList/Impeller geometry #164258

Migrate Mutators to DisplayList/Impeller geometry #164258

Uh oh!

Conversation

flar commented Feb 27, 2025

Uh oh!

jonahwilliams left a comment

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!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

flar commented Mar 5, 2025

Uh oh!

jonahwilliams commented Mar 5, 2025

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

flutter-dashboard bot commented Mar 5, 2025

Uh oh!

flar commented Mar 5, 2025

Uh oh!

flar commented Mar 5, 2025

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

flutter-dashboard bot commented Mar 6, 2025

Uh oh!

flar commented Mar 6, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

flar commented Mar 4, 2025 •

edited

Loading