[Impeller] Reland DlAiksCanvas #45232

dnfield · 2023-08-29T17:28:58Z

Differences from last time:

Some minor merge conflict fixes
Use the RTree to get the bounds instead of recalculating the bounds
Make the iOS platform view controller implementation use the impeller-aware slices instead of the display list ones. This has been fixed for Android and the desktop embedding, but I missed iOS. The unit tests weren't actually running before I branched for my PR, @zanderso fixed them up separately and this resulted in catching the failures on post submit last time.

jonahwilliams

Lets get this moving!

auto-submit · 2023-08-29T17:40:23Z

auto label is removed for flutter/engine/45232, due to - The status or check suite Linux linux_android_debug_engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

The status or check suite Linux linux_android_aot_engine has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Linux linux_host_desktop_engine has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Linux linux_arm_host_engine has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Linux linux_clang_tidy_presubmit has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Linux Framework Smoke Tests has failed. Please fix the issues identified (or deflake) before re-applying this label.

dnfield · 2023-08-29T17:45:56Z

Failures are due to a missing compensating change after merging, will fix...

auto-submit · 2023-08-29T18:31:05Z

auto label is removed for flutter/engine/45232, due to - The status or check suite Linux linux_unopt has failed. Please fix the issues identified (or deflake) before re-applying this label.

auto-submit · 2023-08-30T19:00:09Z

auto label is removed for flutter/engine/45232, due to - The status or check suite Linux mac_unopt has failed. Please fix the issues identified (or deflake) before re-applying this label.

flar · 2023-08-30T23:32:02Z

flow/layers/aiks_layer.cc

+  // There is no opacity peepholing to do here because Impeller handles that
+  // in the Entities, and this layer will never participate in raster caching.
+  FML_DCHECK(!frame->raster_cache);
+  set_paint_bounds(bounds_);


Missing the offset here. Notice that DLLayer translates the bounds_ field.

What's a test case I could use for this?

We might not have one.

aiks layer under an offset layer

A clip or partial repaint would have to be involved as well. Bad bounds in a layer don't immediately cause a problem, but they can trigger mistakes in culling during partial repaint and layer culling

flar · 2023-08-30T23:34:18Z

flow/layers/aiks_layer.cc

+
+  FML_DCHECK(!context.raster_cache);
+
+  SkScalar opacity = context.state_stack.outstanding_opacity();


You never set the flags to indicate that you could render your contents with opacity, so the state stack will do a protective saveLayer around you instead and this value will always be 1.0.

If AiksCanvas.DrawImpellerPicture can distribute opacity, then you will need to set the appropriate flag in Preroll to be able to do this.

(but we could certainly update that later, or see about using the new split you created to handle that right?)

flar · 2023-08-30T23:38:05Z

flow/surface_frame.cc

+        std::make_shared<impeller::DlAiksCanvas>(SkRect::Make(frame_size));
+    canvas_ = aiks_canvas_.get();
+#else
+    FML_DCHECK(false);


The flag is kind of misnamed now. It really means "we're rendering in Impeller mode, so do the Impeller thing". It was named after the (former) solution rather than the trigger that caused it...

flar · 2023-08-30T23:42:57Z

impeller/display_list/dl_aiks_canvas.cc

+
+DlAiksCanvas::~DlAiksCanvas() = default;
+
+static Paint::Style ToStyle(flutter::DlDrawStyle style) {


For many of these, if they are enum classes with the same ordering then you can get away with a cast like dl_sk_conversions.h - though that class has unit tests to verify that the conversions produce the right values so that we'll catch if there is ever a mismatch in the enum values.

flar · 2023-08-30T23:50:06Z

impeller/display_list/dl_aiks_canvas.cc

+
+// |flutter::DlCanvas|
+void DlAiksCanvas::ClipRRect(const SkRRect& rrect, ClipOp clip_op, bool is_aa) {
+  if (rrect.isSimple()) {


Simple allows different radx and rady. When it says "equal radii" it means all 4 corners are equal, not that the horizontal and vertical radii are the same.

Also, it has an isRect() that could be trapped and converted into a ClipRect()

This was copy pasted from dl_dispatcher

Filed: flutter/flutter#133793

flar · 2023-08-30T23:52:36Z

impeller/display_list/dl_aiks_canvas.cc

+// |flutter::DlCanvas|
+void DlAiksCanvas::DrawRRect(const SkRRect& rrect,
+                             const flutter::DlPaint& paint) {
+  if (rrect.isSimple()) {


isSimple != horizontal and vertical radii are the same.

flar · 2023-08-30T23:54:01Z

impeller/display_list/dl_aiks_canvas.cc

+  if (!picture) {
+    return;
+  }
+  FML_DCHECK(opacity == 1);


Might want to DCHECK this in aiks_layer as well.

…133724) flutter/engine@73e8636...3b75e3d 2023-08-31 [email protected] [Impeller] Reland DlAiksCanvas (flutter/engine#45232) 2023-08-31 [email protected] Add a build_bucket_golden_scraper tool. (flutter/engine#45243) 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],[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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

This reverts commit 3b75e3d.

dnfield added 3 commits August 28, 2023 12:18

revert

9b5a707

Use rtree for bounds, fix FlutterPlatformViews on iOS

b569c13

merge

4374076

dnfield requested review from jonahwilliams and flar August 29, 2023 17:28

github-actions bot added platform-ios platform-android e: impeller labels Aug 29, 2023

jonahwilliams approved these changes Aug 29, 2023

View reviewed changes

dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 29, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 29, 2023

bad merge

6370bec

dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 29, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 29, 2023

chinmaygarde approved these changes Aug 29, 2023

View reviewed changes

chinmaygarde assigned dnfield Aug 29, 2023

more bad merge

220828d

dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 30, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 30, 2023

flar reviewed Aug 30, 2023

View reviewed changes

Missing files

ccf56b4

dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 30, 2023

auto-submit bot merged commit 3b75e3d into flutter:main Aug 31, 2023

engine-flutter-autoroll mentioned this pull request Aug 31, 2023

Roll Flutter Engine from 73e86369614d to 3b75e3de7ef3 (2 revisions) flutter/flutter#133724

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 31, 2023

3b75e3de7 [Impeller] Reland DlAiksCanvas (flutter/engine#45232)

4fa8579

zanderso added a commit to zanderso/engine that referenced this pull request Aug 31, 2023

Revert "[Impeller] Reland DlAiksCanvas (flutter#45232)"

338a691

This reverts commit 3b75e3d.


		FML_DCHECK(!context.raster_cache);

		SkScalar opacity = context.state_stack.outstanding_opacity();


		DlAiksCanvas::~DlAiksCanvas() = default;

		static Paint::Style ToStyle(flutter::DlDrawStyle style) {

[Impeller] Reland DlAiksCanvas #45232

[Impeller] Reland DlAiksCanvas #45232

Uh oh!

Conversation

dnfield commented Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

auto-submit bot commented Aug 29, 2023

Uh oh!

dnfield commented Aug 29, 2023

Uh oh!

auto-submit bot commented Aug 29, 2023

Uh oh!

auto-submit bot commented Aug 30, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flar Aug 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dnfield commented Aug 29, 2023 •

edited

Loading

flar Aug 31, 2023 •

edited

Loading