[Impeller] DlCanvas implementation wrapping Aiks canvas #44248

dnfield · 2023-08-02T04:14:26Z

Fixes flutter/flutter#130141

The primary goal of this patch is to move dispatching of dart:ui Canvas commands to the UI thread.

Before this patch, the architecture is something like:

UI Thread

dart:ui talks to DisplayListBuilder, a DlCanvas implementation.
DisplayListBuilder does some clip/bounds tracking and creates a DisplayList object that is held by dart:ui's Picture objects.
DisplayLists are added to DisplayListLayers in flow.

Raster Thread

flow flattens the various operations into a single DisplayList via another DisplayListBuilder.
A DlOpReceiverimplementation converts that DisplayList into an Aiks Canvas/Picture.

After this patch, the architecture instead looks like:

UI Thread

No change for Skia.
If Impeller, use a new DlCanvasImplementation that talks to Aiks's Canvas.
If Impeller, dart:ui Picture's now hold an Aiks Picture, which get shared into AiksLayers in flow.

Raster thread

No change for Skia, but some light refactoring for places that assumed a DisplayListBuilder where they really just needed a DlCanvas.
The Aiks Pictures are combined using new API on DlCanvas and still backed by Aiks.

These changes show significant improvement on raster times on Android and only very small regressions on UI times in local testing, see https://gist.github.com/dnfield/26528090194c9f5abdbac13cdcbf4f79 for old gallery transition perf numbers.

Many of the other changes in this patch are related to the following:

Making DlRTree usable for Impeller.
- It would be nice to have a version of DlRTree that speaks impeller::Rect.
Creating the requisite classes to support EmbeddedViews so that Desktop works.

This patch does not remove the impeller::DlDispatcher, which now would only be used in tests.

…raster

dnfield · 2023-08-03T20:42:37Z

Blocked on flutter/flutter#131887

dnfield · 2023-08-03T22:19:53Z

I have patched in the fix from #44346, as this patch was causing those iOS tests to start crashing since it adds some DCHECKs around Impeller-related codepaths that were now firing due to the usage of software rendering in those tests.

dnfield · 2023-08-03T22:24:34Z

I should also add that my performance numbers were taken from an earlier iteration of this patch and may look slightly different now, but should still be indicative.

jonahwilliams

are there any features from the dlbuilder that we're losing from this switch?

jonahwilliams · 2023-08-04T16:08:49Z

flow/layers/aiks_layer.cc

+
+void AiksLayer::Preroll(PrerollContext* frame) {
+  // TODO(dnfield): Handle group opacity checks.
+  set_paint_bounds(bounds_);


Link to an issue?

I'm going to delete this comment.

In DisplayListLayer::Preroll, there's opacity related checking that happens to make decisions about raster caching. Raster caching isn't a thing for Impeller, and Impeller internally handles its own opacity peepholing.

(adding a comment about that and a dcheck that there's no raster cache)

jonahwilliams · 2023-08-04T16:09:34Z

flow/layers/aiks_layer.cc

+    // We can't do this the same way as on the Skia backend, because Impeller
+    // does not expose primitives for flushing things down to the GPU without
+    // also allocating a texture.
+    FML_LOG(ERROR) << "Leaf layer tracing unsupported for Impeller.";


Lets file an issue to discuss, but my inclination is that we should not support this feature for Impeller and should instead direct folks to either native profiling tools or something based on archivist.

flutter/flutter#131941

Why is this done on display_list_layer then?

jonahwilliams · 2023-08-04T16:10:33Z

flow/layers/layer_tree.cc

+  const FixedRefreshRateStopwatch unused_stopwatch;
+
+  LayerStateStack preroll_state_stack;
+  // No root surface transformation. So assume identity.


I think Vulkan prerotation may eventually give us a root surface transformation.

This comment is duplicated from Flatten below. But I guess we can handle that then? The idea is that this method would only ever be called on a Scene object that is not really attached to a particular surfacde.

Yeah, no action needed, just a comment

jonahwilliams · 2023-08-04T16:14:54Z

impeller/display_list/dl_aiks_canvas.cc

+
+namespace impeller {
+
+#define UNIMPLEMENTED \


This is more or less the same implementation as dl_dispatcher right? if this sticks the landing we should clean that up quickly so they don't diverge.

It's similar but a little different. The two major differences are:

It can't really draw a display list (and vice versa, a display list can't draw an impeller picture)

It's not using RTrees to determine culling when concatenating Impeller pictures (but it probably could if needed).

If this patch lands and sticks we can go ahead and delete dl_dispatcher, which is only used in tests. I didn't want to make this already large patch bigger by doing that now though.

Sounds good to me

I would rather...

Delete this new class which is a poor approximation of nearly identical code in DisplayListBuilder (and a near duplicate of impeller::DlDispatcher)

Break DisplayListBuilder out into a preprocessor that already does the optimizations, and a pluggable backend which already exists and is called impeller::DlDispatcher

Basically the preprocessor output can either go to a recorder which is "OpReceiver in, OpReceiver out" or directly to an impeller DlDispatcher.

jonahwilliams · 2023-08-04T16:16:10Z

impeller/tools/impeller.gni

+  impeller_enable_metal = (is_mac || is_ios) && target_os != "fuchsia"

  # Whether the OpenGLES backend is enabled.
-  impeller_enable_opengles = is_linux || is_win || is_android
+  impeller_enable_opengles =
+      (is_linux || is_win || is_android) && target_os != "fuchsia"

  # Whether the Vulkan backend is enabled.
-  impeller_enable_vulkan = is_linux || is_win || is_android
+  impeller_enable_vulkan =
+      (is_linux || is_win || is_android) && target_os != "fuchsia"



Was this necessary? I don't remember where we landed on the embedder.h issue with fuchsia

If I don't do this, there are host artifacts built for linux or mac when building Fuchsia where the target_os is Fuchsia, and enable_impeller ends up being true in places where it really shouldn't be.

I'd be happy to separate this into another patch if it helps.

Nah thats fine.

jonahwilliams · 2023-08-04T16:16:36Z

lib/ui/compositing/scene_builder.cc

+  if (impeller_picture) {
+    auto layer = std::make_unique<flutter::AiksLayer>(
+        SkPoint::Make(SafeNarrow(dx), SafeNarrow(dy)),
+        std::move(impeller_picture), !!(hints & 1), !!(hints & 2));


Lets just drop the hints since we don't use them for anything.

jonahwilliams · 2023-08-04T16:18:39Z

lib/ui/painting/picture.cc

+  if (display_list()) {
+    size += display_list()->bytes();
  }
+  // TODO(dnfield): Add support to EntityPass to get its allocation size.


What are we using the allocation size for? Is this necesary to support?

It's exposed in https://api.flutter.dev/flutter/dart-ui/Picture/approximateBytesUsed.html

:(

entity count * 100?

jonahwilliams

overall LGTM, the devicelab will be the ultiamte arbiter of success thougj

jonahwilliams · 2023-08-04T17:04:06Z

"ultimate" darn I can't spell

dnfield · 2023-08-04T17:10:49Z

I'll keep an eye on this today to see what it looks like in a full devicelab run. If there are regressions or problems I'll revert.

…utter/engine#44248)

…131948) flutter/engine@e9f80bf...a7bea86 2023-08-04 skia-flutter-autoroll@skia.org Roll Skia from 5eef2e2b94b4 to 85938bb68e75 (1 revision) (flutter/engine#44392) 2023-08-04 dnfield@google.com [Impeller] DlCanvas implementation wrapping Aiks canvas (flutter/engine#44248) 2023-08-04 30870216+gaaclarke@users.noreply.github.com [Impeller] Added a doc page to point out important impeller benchmarks (flutter/engine#44333) 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 jacksongardner@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://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

flar · 2023-08-04T20:43:48Z

flow/layers/aiks_layer.cc

+                               bounds.size.height);
+  }
+#endif
+}


There is no override for IsReplacing. I forget the details, but sometimes a new Layer is constructed with the same ui.Picture as was used in the previous frame and so we need to check if the pictures are the same, not just the layer, otherwise we lose a lot of partial repaint reduction.

Indeed. The way it is now partial repaint will not work correctly with AiksLayer. With DisplayListLayer (and similarly AiksLayer) the diffing is moved one level up - to ContainerLayer. The DisplayListLayer::Diff basically just reports the layer paint region.

The reason for this is that we want to be able to detect added / removed display list layers similar to how the framework diffs element children. (See ContainerLayer::DiffChildren).

The reason why picture layers are different than other layers in this regard is that picture layers don't exit in framework in a similar way other layers exist - so we don't get the old layer to link with the new layer when adding picture like we would get with a backdrop filter or a clip rect layer for example. So we relay on comparing the contents.

flar · 2023-08-04T20:50:14Z

flow/surface_frame.cc

-    dl_builder_ =
-        sk_make_sp<DisplayListBuilder>(SkRect::Make(frame_size), true);
-    canvas_ = dl_builder_.get();
+#if IMPELLER_SUPPORTS_RENDERING


We should perhaps rename this flag if it really means "rendering_to_impeller" otherwise you are creating an impeller picture for a flag that is named "please create a display list"...

This is also an odd combination of tests that implies that a given call can contain either or both of a surface and/or the flag. If it's "one or the other", then delete the flag and just assume it if there is no surface. If it is both, we aren't implementing it very well. If there are cases when neither is provided - why is that?

flar · 2023-08-04T20:58:28Z

impeller/display_list/dl_aiks_canvas.cc

+                             const flutter::DlPaint* paint,
+                             const flutter::DlImageFilter* backdrop) {
+  if (!paint) {
+    return;


This is a bug. Paint is optional, you can't just return. Perhaps we don't end up here with paint being null, but it doesn't invalidate the operation, it just means that the collected pixels are applied with default paint values.

flar · 2023-08-04T21:00:30Z

impeller/display_list/dl_aiks_canvas.cc

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


This is missing the "isRect" optimization.

flar · 2023-08-04T21:02:45Z

impeller/display_list/dl_aiks_canvas.cc

+    : canvas_(skia_conversions::ToRect(cull_rect)) {
+  if (prepare_rtree) {
+    accumulator_ = std::make_unique<flutter::RTreeBoundsAccumulator>();
+  }


else - a rect accumulator. We usually set the flag on most internal uses, but if anyone ever constructs the object with the flag set to false, it will just crash on the first Save call.

flar · 2023-08-04T21:03:18Z

impeller/display_list/dl_aiks_canvas.cc

+}
+
+// |flutter::DlCanvas|
+void DlAiksCanvas::ClipPath(const SkPath& path, ClipOp clip_op, bool is_aa) {


This is also missing isRect and isRRect optimizations.

flar · 2023-08-04T21:12:11Z

impeller/display_list/dl_aiks_canvas.cc

+#define UNIMPLEMENTED \
+  FML_DLOG(ERROR) << "Unimplemented detail in " << __FUNCTION__;
+
+DlAiksCanvas::DlAiksCanvas(const SkRect& cull_rect, bool prepare_rtree)


This is all quite a bit different from what I thought was being done.

As originally described to me, I had thought that Impeller would continue to just have the dispatcher, but the conversion of DlCanvas to DlOpReceiver calls was going to happen independently and then that conversion would just talk to DlDispatcher directly.

As it happens, there are a few mistakes in this and duplication of code (or failure to duplicate important code) from DisplayListBuilder in addition to basically duplicating all of impeller's DlDispatcher.

What I though was going to happen was:

currently DisplayListBuilder talks DlCanvas and optimizes the calls and then records them into a DL buffer it takes "DlCanvas" in, but it records "DlOpReceiver" calls on the back end

Take the part of the Builder that does the optimizations and pull it out into a DisplayListCanvasOptimizer class that forwards the calls to a backend which implements DlOpReceiver

Take the part of the Builder that records the Op calls into a buffer and pull that out into a DisplayListRecorder class that just records the calls into a buffer.

DisplayListBuilder would simply link up a DLOptimizer with a DlRecorder and maintain the same functionality.

For Impeller, DLOptimizer would talk to the existing impeller::DlDispatcher and it would get all of the optimizations for free without having to reinvent the wheel or create essentially a line-for-line replacement of DlDispatcher with, as we see here, missing optimizations and a couple of bugs.

That's what I thought the effort was going to be, but I see now that I didn't hear that right.

flar · 2023-08-15T09:15:27Z

See also: #44718 which can be attached to an impeller::DlDispatcher (which implements DlOpReceiver) to take in DlCanvas methods and more directly feed them to the Impeller renderer.

@bdero

…45131) This is a reland of #44248 Fixes flutter/flutter#132416 Changes from last time: - The `drawPoints` benchmark was failing to render anything meaningful because of an error in `AiksLayer` that resulted in an infinitely sized bounding rectangle poisoning the bounds with `NaN` (this happens, for example, with a `drawPaint` call, which that benchmark happens to use). Added a test covering this and filed flutter/flutter#132770 to explore ways to avoid this in the future. - There was a bug in `DlAiksCanvas::SaveLayer` where a `nullptr` `paint` but non-`nullptr` `backdrop` was failing to actually save the layer. This resulted in incorrect rendering. - There was a bug in `impeller::Canvas::DrawPicture` that resulted in incorrect stencil depth counting. That was fixed separately by @bdero, but was the cause of incorrect rendering in some Wonderous screens. - I've added a simple implementation for `AiksLayer::IsReplacing`. It does not currently compare as deeply as the `DisplayListLayer` version potentially does, but it is good enough to avoid the regression noted in flutter/flutter#132071. That regression was on a benchmark that greatly benefits from partial repaint. With the new implementation, it still gains partial repaint where it previously did not. There is more work that can be done here, filed flutter/flutter#133361 to track that work. I merged but did not fully integrate the `DisplayListBuilder`/`CanvasToReceiver` work that @flar has done. I have a local experiment with that, but would prefer to see this land and run through the device lab so we get some better comparison numbers for which one performs better.

Fixes flutter/flutter#130141 The primary goal of this patch is to move dispatching of `dart:ui` `Canvas` commands to the UI thread. Before this patch, the architecture is something like: ## UI Thread - `dart:ui` talks to `DisplayListBuilder`, a `DlCanvas` implementation. - `DisplayListBuilder` does some clip/bounds tracking and creates a `DisplayList` object that is held by `dart:ui`'s `Picture` objects. - `DisplayList`s are added to `DisplayListLayer`s in `flow`. ## Raster Thread - `flow` flattens the various operations into a single `DisplayList` via another `DisplayListBuilder`. - A `DlOpReceiver`implementation converts that `DisplayList` into an `Aiks` `Canvas`/`Picture`. After this patch, the architecture instead looks like: ## UI Thread - No change for Skia. - If Impeller, use a new `DlCanvasImplementation` that talks to `Aiks`'s `Canvas`. - If Impeller, `dart:ui` Picture's now hold an `Aiks` `Picture`, which get shared into `AiksLayer`s in `flow`. ## Raster thread - No change for Skia, but some light refactoring for places that assumed a `DisplayListBuilder` where they really just needed a `DlCanvas`. - The `Aiks` `Picture`s are combined using new API on `DlCanvas` and still backed by `Aiks`. These changes show significant improvement on raster times on Android and only very small regressions on UI times in local testing, see https://gist.github.com/dnfield/26528090194c9f5abdbac13cdcbf4f79 for old gallery transition perf numbers. Many of the other changes in this patch are related to the following: - Making `DlRTree` usable for Impeller. - It would be nice to have a version of DlRTree that speaks `impeller::Rect`. - Creating the requisite classes to support `EmbeddedViews` so that Desktop works. This patch does not remove the `impeller::DlDispatcher`, which now would only be used in tests.

@bdero

…lutter#45131) This is a reland of flutter#44248 Fixes flutter/flutter#132416 Changes from last time: - The `drawPoints` benchmark was failing to render anything meaningful because of an error in `AiksLayer` that resulted in an infinitely sized bounding rectangle poisoning the bounds with `NaN` (this happens, for example, with a `drawPaint` call, which that benchmark happens to use). Added a test covering this and filed flutter/flutter#132770 to explore ways to avoid this in the future. - There was a bug in `DlAiksCanvas::SaveLayer` where a `nullptr` `paint` but non-`nullptr` `backdrop` was failing to actually save the layer. This resulted in incorrect rendering. - There was a bug in `impeller::Canvas::DrawPicture` that resulted in incorrect stencil depth counting. That was fixed separately by @bdero, but was the cause of incorrect rendering in some Wonderous screens. - I've added a simple implementation for `AiksLayer::IsReplacing`. It does not currently compare as deeply as the `DisplayListLayer` version potentially does, but it is good enough to avoid the regression noted in flutter/flutter#132071. That regression was on a benchmark that greatly benefits from partial repaint. With the new implementation, it still gains partial repaint where it previously did not. There is more work that can be done here, filed flutter/flutter#133361 to track that work. I merged but did not fully integrate the `DisplayListBuilder`/`CanvasToReceiver` work that @flar has done. I have a local experiment with that, but would prefer to see this land and run through the device lab so we get some better comparison numbers for which one performs better.

Create Impeller strucutres on the UI thread, avoid re-dispatching on …

f20fee7

…raster

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

missing gn dep

d107a5d

chinmaygarde assigned dnfield Aug 2, 2023

dnfield added 8 commits August 1, 2023 22:47

Fuchsia rules, lints

150e5c0

Re-enable transparent layer culling

468bbb3

licenses, unnecessary headers

d6de746

guard include

f64c2ef

fix condition

887f134

tidy

23ebd1c

license issue, make Fuchsia build, commented out code

4f8f2fa

more linting and Fuchsia

a6f26e9

dnfield added 6 commits August 3, 2023 14:04

[Impeller] Fail loudly if --enable-software-rendering is used.

371933c

Merge remote-tracking branch 'upstream/main' into software

4b42deb

Fix test

8a9fa32

Merge branch 'software' into impdl

9b51312

Skip fuchsia

9a3dc6c

Merge branch 'software' into impdl

1135df9

dnfield marked this pull request as ready for review August 3, 2023 22:17

dnfield requested review from chinmaygarde, flar and jonahwilliams August 3, 2023 22:17

Merge branch 'main' into impdl

d34957a

jonahwilliams reviewed Aug 4, 2023

View reviewed changes

jonahwilliams mentioned this pull request Aug 4, 2023

[Impeller] Cache Polyline and tessellation for Path #44390

Closed

8 tasks

Jonah review

3b52376

jonahwilliams approved these changes Aug 4, 2023

View reviewed changes

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

auto-submit bot merged commit 6e90446 into flutter:main Aug 4, 2023

dnfield deleted the impdl branch August 4, 2023 17:44

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

Roll Flutter Engine from e9f80bff0703 to a7bea8621f4b (3 revisions) flutter/flutter#131948

Merged

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

6e9044653 [Impeller] DlCanvas implementation wrapping Aiks canvas (fl…

d28f5ec

…utter/engine#44248)

flar reviewed Aug 4, 2023

View reviewed changes

gaaclarke mentioned this pull request Aug 7, 2023

[Impeller] Investigate "DlCanvas implementation wrapping Aiks canvas" 90th percentile increase flutter/flutter#132071

Closed

chinmaygarde mentioned this pull request Aug 11, 2023

[Impeller] Re-land "DlCanvas implementation wrapping Aiks canvas" flutter/flutter#132416

Closed

bdero mentioned this pull request Aug 14, 2023

[Impeller] Cache render target texture allocations across frames. #44527

Merged

dnfield mentioned this pull request Aug 25, 2023

[Impeller] DlAiksCanvas as a DlCanvas wrapper for impeller::Canvas #45131

Merged

chinmaygarde mentioned this pull request Aug 28, 2023

[Impeller] Flutter Gallery DlDispatch is mostly text conversions flutter/flutter#133204

Closed

AbeiOS mentioned this pull request Jan 3, 2024

Raster DisplayList::Dispatch take time too long flutter/flutter#140727

Closed


		namespace impeller {

		#define UNIMPLEMENTED \

[Impeller] DlCanvas implementation wrapping Aiks canvas #44248

[Impeller] DlCanvas implementation wrapping Aiks canvas #44248

Uh oh!

Conversation

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

Uh oh!

UI Thread

Raster Thread

UI Thread

Raster thread

Uh oh!

dnfield commented Aug 3, 2023

Uh oh!

dnfield commented Aug 3, 2023

Uh oh!

dnfield commented Aug 3, 2023

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!

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!

flar Aug 4, 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!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Aug 4, 2023

Uh oh!

dnfield commented Aug 4, 2023

Uh oh!

flar Aug 4, 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

dnfield commented Aug 2, 2023 •

edited

Loading

flar Aug 4, 2023 •

edited

Loading

flar Aug 4, 2023 •

edited

Loading