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

dnfield · 2023-08-25T22:01:20Z

This is a reland of #44248

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 Trap NaN in geometry. 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 [Impeller] Investigate "DlCanvas implementation wrapping Aiks canvas" 90th percentile increase 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 [Impeller] Improve AiksLayer::IsReplacing 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.

jonahwilliams

LGTM

jonahwilliams · 2023-08-25T22:40:22Z

Seems like there are some GN config issues with flatbuffers tho

jonahwilliams · 2023-08-25T22:48:55Z

impeller/aiks/canvas.cc

  picture.pass = std::move(base_pass_);

+  std::vector<SkRect> rects;
+  picture.pass->IterateAllEntities([&rects](const Entity& entity) {


This is showing up as being pretty hot. This might need to be fixed to see a larger difference.

Perhaps we could append to the Rtree instead of temp storage? I think that is how it was designed to work.

The current RTree impl expects to get an array of SkRect objects. We could probably refactor it to make it append.

But this should be happening on the UI thread now.

How would that help anything? The rects have to be accumulated at some point, you are just pushing it from your own code into code that you don't own - it won't actually save anything.

You still have to do the iteration no matter how you create the RTree. The bounds still have to be accumulated into a tree. If Rtree is going to accumulate the bounds for you before it makes them into a tree then accumulation is still happening, it's just not in "your" code. If you want RTree to rebuild the tree on every append then not only will it have to still accumulate the bounds, but you will also have to add rebuilding the tree overhead on each rect.

Meanwhile, the dl_aiks_canvas was already creating an RTree accumulator that does all of this for you plus it will give you the bounds at the end and then the class wasn't even using it...

We could just ask the rtree for the bounds.

Correct, but I though this discussion was about creating the RTree, not using it to get the bounds...?

Sorry, I wrote my comment on the wrong thread.

jonahwilliams · 2023-08-25T22:53:30Z

flow/layers/aiks_layer.cc

+    : offset_(offset), picture_(picture) {
+#if IMPELLER_SUPPORTS_RENDERING
+  if (picture_) {
+    auto bounds = picture_->pass->GetElementsCoverage(std::nullopt)


Thie one is also really slow.

Looks like the TextFrame bounds are slow, at least in ComplexLayout (This is UI thread)

We end up needing to get these multiple times per frame AFAICT, we should probably cache them once we've calculated them.

See also: flutter/flutter#133388 . We should just cache the value we get from Skia. They're computing the bounds anyway.

Not just the text bounds - why isn't the picture itself remembering its own bounds? These will get used frame after frame and we have to iterate the elements each time to save ... 16 bytes in the object?

This is where we could just ask the rtree for the bounds :)

Does that code always produce an rtree? Then, yes, it's a very cheap way to get the bounds. We had been making that mistake in DL for a long time until I fixed it recently in the "split DL builder" PR.

jonahwilliams · 2023-08-25T22:59:21Z

I suspect you'll see fewer improvements on this than you'd like due to the naïve coverage implementation. Lets talk next week about a strategy to make this faster.

jonahwilliams · 2023-08-26T00:53:26Z

We also still seem to be spending time populating raster cache entries?

…LE_3D

dnfield · 2023-08-26T02:11:46Z

We also still seem to be spending time populating raster cache entries?

Let's fix that in a different patch. Looks like flow is missing a raster cache enabled check for this.

auto-submit · 2023-08-26T02:38:19Z

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

…anvas (#45131)" This reverts commit 14242a0.

…anvas" (#45149) Reverts #45131 This is failing the Impeller variants of the unobstructed platform views tests: https://ci.chromium.org/ui/p/flutter/builders/prod/Mac%20Production%20Engine%20Drone/132249/overview ``` Failing tests: -[UnobstructedPlatformViewTests testPlatformViewsMaxOverlays] -[UnobstructedPlatformViewTests testOneOverlay] -[UnobstructedPlatformViewTests testOneOverlayPartialIntersection] -[UnobstructedPlatformViewTests testTwoIntersectingOverlays] -[UnobstructedPlatformViewTests testOneOverlayAndTwoIntersectingOverlays] ** TEST FAILED ** ``` We've retried it a few times so I suspect this isn't a flake.

…:Canvas (flutter/engine#45131)

…133389) flutter/engine@c9b584d...4aaf77e 2023-08-26 [email protected] Revert "[Impeller] DlAiksCanvas as a DlCanvas wrapper for impeller::Canvas" (flutter/engine#45149) 2023-08-26 [email protected] [Impeller] DlAiksCanvas as a DlCanvas wrapper for impeller::Canvas (flutter/engine#45131) 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

@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.

…anvas" (flutter#45149) Reverts flutter#45131 This is failing the Impeller variants of the unobstructed platform views tests: https://ci.chromium.org/ui/p/flutter/builders/prod/Mac%20Production%20Engine%20Drone/132249/overview ``` Failing tests: -[UnobstructedPlatformViewTests testPlatformViewsMaxOverlays] -[UnobstructedPlatformViewTests testOneOverlay] -[UnobstructedPlatformViewTests testOneOverlayPartialIntersection] -[UnobstructedPlatformViewTests testTwoIntersectingOverlays] -[UnobstructedPlatformViewTests testOneOverlayAndTwoIntersectingOverlays] ** TEST FAILED ** ``` We've retried it a few times so I suspect this isn't a flake.

@zanderso

Relands #45131 Fixes flutter/flutter#132416 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.

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

f1f68b4

dnfield requested review from chinmaygarde, jonahwilliams and flar August 25, 2023 22:01

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

missing include

6cd9055

jonahwilliams approved these changes Aug 25, 2023

View reviewed changes

jonahwilliams reviewed Aug 25, 2023

View reviewed changes

Missing file from exclusions, missing include guard for IMPELLER_ENAB…

2d4ca36

…LE_3D

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

dnfield mentioned this pull request Aug 26, 2023

Flow unconditionally does raster-cache related work for CacheableContainerLayer, which is wasted if the raster cache is disabled. flutter/flutter#133377

Closed

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

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

auto-submit bot merged commit 14242a0 into flutter:main Aug 26, 2023

dnfield deleted the impdl branch August 26, 2023 05:48

jonahwilliams mentioned this pull request Aug 26, 2023

Use SkTextBlob::bounds instead of TextFrame::getBounds(). #45148

Merged

jonahwilliams pushed a commit that referenced this pull request Aug 26, 2023

Revert "[Impeller] DlAiksCanvas as a DlCanvas wrapper for impeller::C…

b83dd90

…anvas (#45131)" This reverts commit 14242a0.

jonahwilliams mentioned this pull request Aug 26, 2023

Revert "[Impeller] DlAiksCanvas as a DlCanvas wrapper for impeller::Canvas" #45149

Merged

matanlurey mentioned this pull request Aug 26, 2023

[Impeller] TextFrame::getBounds() is wrong and/or slow flutter/flutter#133388

Closed

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

Roll Flutter Engine from c9b584d59219 to 4aaf77e33de6 (2 revisions) flutter/flutter#133389

Merged

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

14242a0a8 [Impeller] DlAiksCanvas as a DlCanvas wrapper for impeller:…

230d356

…:Canvas (flutter/engine#45131)

dnfield mentioned this pull request Aug 29, 2023

[Impeller] Reland DlAiksCanvas #45232

Merged

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

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

Uh oh!

Conversation

dnfield commented Aug 25, 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!

jonahwilliams commented Aug 25, 2023

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Aug 25, 2023

Uh oh!

jonahwilliams commented Aug 26, 2023

Uh oh!

dnfield commented Aug 26, 2023

Uh oh!

auto-submit bot commented Aug 26, 2023

Uh oh!

Uh oh!

dnfield commented Aug 25, 2023 •

edited

Loading