[Impeller] Collapse DrawRects into clear colors optimization #43168

gaaclarke · 2023-06-23T23:58:21Z

design doc: https://docs.google.com/document/d/1Lqf1BRn4uCcUfv9dZlDyAgdSMeQ3FTnPgPjKF9yQ3MI/edit

fixes: flutter/flutter#129292

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

gaaclarke · 2023-06-27T00:10:29Z

impeller/aiks/canvas.cc

 }

 void Canvas::DrawPaint(const Paint& paint) {
-  if (xformation_stack_.size() == 1 &&  // If we're recording the root pass,


@bdero note that I got rid of this criteria. I don't think think it's important that it's the root pass, just that the render target for the render pass is covered. Let me know if there is something I'm missing.

Render() is not recursively called; it only runs for the root pass. So it looks like this PR is only overriding the root pass clear color.

gaaclarke · 2023-06-27T00:54:42Z

This is broken, i'm going to drop it back into draft and look into the failures tomorrow.

bdero

Okay, so I had a big thonk about this... It occurs to me that Aiks won't be able to pack the conditional_clears_ list properly in the future when we start reversing the draw order.

What if instead of relying on Aiks to append items to a list, we push this entire concern into the Entities layer by adding the necessary APIs to perform this conversion internally? I.e.:

Add virtual bool Geometry::CoversArea(const Entity& entity, Rect area) const for determining if the given rect is completely covered by the geometry.
- Default implementation just returns false.
- Add an override for CoverGeometry which returns true to handle the DrawPaint case that we're already handling.
- Add an override for RectGeometry to handle the new case we're trying to tackle.
Add virtual std::optional<Color> Contents::AsBackgroundColor(const Entity& entity, ISize target_size) const, which returns a Color if the Contents can safely be replaced with a background color.
- Default implementation just returns std::nullopt.
- Add an override for SolidColorContents that returns the color if geometry_.CoversArea(entity, Rect::MakeSize(Size(target_size))) returns true.
Add std::optional<Color> Entity::AsBackgroundColor(ISize target_size) const as a helper to call Contents::AsBackgroundColor (common pattern).
Then, before the render target for a pass is formed in EntityPass, preprocess the elements up until either a subpass is found or Entity::AsBackgroundColor() returns std::nullopt. Set the resulting clear color on the render target.
Pass a start_element index to OnRender() to offset the start of the element iteration.

A design like this would allow us to:

Make the optimization independent from the Element draw order.
Avoid copying the entire Element list.
Expand this optimization in the future by just adding more Geometry::CoversArea() and Contents::AsBackgroundColor() overrides.

bdero · 2023-06-27T00:38:28Z

impeller/entity/entity_pass.cc

+  for (const EntityPass::ConditionalClear& clear : conditional_clears) {
+    std::optional<Rect> coverage = clear.entity.GetCoverage();
+    bool should_collapse =
+        coverage.has_value() && coverage.value().Contains(render_target_rect);


Checking if the coverage rect covers the whole pass isn't enough to determine that the transformed geometry covers all fragments in the subpass. Additional restrictions are needed. For example, this check would work if the Entity transform only translates and/or scales the space.

bdero · 2023-06-27T02:45:34Z

impeller/aiks/canvas.cc

 }

 void Canvas::DrawPaint(const Paint& paint) {
-  if (xformation_stack_.size() == 1 &&  // If we're recording the root pass,


Render() is not recursively called; it only runs for the root pass. So it looks like this PR is only overriding the root pass clear color.

gaaclarke · 2023-06-27T17:48:58Z

@bdero I'm thinking about it. It may be cleaner in terms of bookkeeping but complicates the API.

Avoid copying the entire Element list.

This isn't copying the Element list, it's copying references to the elements. It isn't ideal but it isn't as bad as copying the elements.

gaaclarke · 2023-06-27T20:32:18Z

I think we're going to have to do like brandon suggested and avoid holding the conditional clears separately. There is order information getting lost by having that parallel data structure.

gaaclarke · 2023-06-27T21:38:18Z

Transitioning this back from a draft to allow golden image testing since the tests are passing.

gaaclarke · 2023-06-27T23:53:37Z

@bdero yay, goldens are passing. I'm just going to push one more cleanup then re-request a review.

I made the changes you suggested about taking this to the Contents level.

impeller/entity/contents/solid_color_contents.cc

impeller/entity/entity_pass.cc

bdero · 2023-06-28T16:37:40Z

impeller/entity/entity_pass.cc

+      } else {
+        is_collapsing_clear_colors = false;
+      }
+    }


This skip behavior LGTM and will be applied to every EntityPass, but right now the clear color is only being applied to the root EntityPass in this PR (which has it's render target created in Render()).

We also need to set the clear color when constructing subpasses, which happens in GetEntityForElement(). There, you could just use subpass->GetClearColor() to grab it and feed it to the RT creation routine.

I'll do this is a follow up PR since it isn't strictly necessary to get the optimization I want in Gallery. In the gallery the first operation is adjusting the clip actually, although I think it is a noop. It would be nice to get that to collapse too if it's the case.

If you defer this fix to a future PR, I think we'll need to add a mechanism to ensure this skipping behavior only occurs on the root pass, lest some apps will render incorrectly due to the unhandled dropping of entities on subpasses.

I thought this was just an optimization. This is surprising that it isn't covered in existing tests. How could i make sure to hit this in a test? Would savelayer with a drawrect be enough?

We just have to be careful to avoid the opacity peephole subpass collapse optimization. Drawing a fullscreen rect in a SaveLayer, but giving that SaveLayer an interesting blend mode like Modulate should work.

Okay done. I used multiply since it was easier to verify, let me know if you think that may be a problem in the future. I think that should be good.

Multiply is even more "interesting" than Modulate, so we should be good. 😄

bdero

LGTM!

flutter-dashboard · 2023-06-29T00:30:53Z

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

Changes reported for pull request #43168 at sha 5e0c7f8

flutter/engine#43168)

…129750) flutter/engine@4e49b9d...d5c25ea 2023-06-29 [email protected] [Impeller] Add a missing method to the Context mock (flutter/engine#43326) 2023-06-29 [email protected] [Impeller] Collapse DrawRects into clear colors optimization (flutter/engine#43168) 2023-06-29 [email protected] [Impeller] Add trace events to Vulkan texture and buffer lifecycle events. (flutter/engine#43321) 2023-06-29 [email protected] Roll Dart SDK from 5a59cd06e49d to a7151d73b88d (2 revisions) (flutter/engine#43318) 2023-06-28 [email protected] [Impeller] Fix advanced CPU blend modes (flutter/engine#43294) 2023-06-28 [email protected] Roll Fuchsia Linux SDK from 10hNrVMjnCypybnz2... to fxCNy4QivAngZXkvM... (flutter/engine#43314) 2023-06-28 [email protected] Roll Skia from 0b4f472a8c44 to 26fa4b343fd3 (2 revisions) (flutter/engine#43306) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from 10hNrVMjnCyp to fxCNy4QivAng 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

…#43168) design doc: https://docs.google.com/document/d/1Lqf1BRn4uCcUfv9dZlDyAgdSMeQ3FTnPgPjKF9yQ3MI/edit fixes: flutter/flutter#129292 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

github-actions bot added the e: impeller label Jun 23, 2023

gaaclarke force-pushed the elide-draw-rects branch 4 times, most recently from 8391154 to 1aaec8f Compare June 24, 2023 00:02

[Impeller] started collapsing draw rects into clear colors

512be0e

gaaclarke force-pushed the elide-draw-rects branch from 1aaec8f to 512be0e Compare June 24, 2023 00:21

chinmaygarde assigned gaaclarke Jun 24, 2023

gaaclarke force-pushed the elide-draw-rects branch from 1214c4b to 737ca28 Compare June 26, 2023 21:27

got the 2 tests passing

640423e

gaaclarke force-pushed the elide-draw-rects branch from 737ca28 to 640423e Compare June 26, 2023 21:31

gaaclarke added 5 commits June 26, 2023 14:51

updated licenses

0848cb6

started squashing on any render pass

9e3b0fb

took off root pass requirement for the DrawPaint optimization too

be63ff6

reverted accidental changes to gradle

f8bc35b

lint

3cb2a2a

gaaclarke marked this pull request as ready for review June 26, 2023 23:48

gaaclarke requested a review from bdero June 26, 2023 23:48

gaaclarke commented Jun 27, 2023

View reviewed changes

This comment was marked as outdated.

Sign in to view

flutter-dashboard bot added the will affect goldens label Jun 27, 2023

gaaclarke marked this pull request as draft June 27, 2023 00:54

gaaclarke removed the request for review from bdero June 27, 2023 00:55

This comment was marked as outdated.

Sign in to view

bdero reviewed Jun 27, 2023

View reviewed changes

gaaclarke force-pushed the elide-draw-rects branch from a9f28ef to eb0b4c4 Compare June 27, 2023 21:29

removed the seperate storage of conditional clears

54ea87b

gaaclarke force-pushed the elide-draw-rects branch from eb0b4c4 to 54ea87b Compare June 27, 2023 21:37

gaaclarke marked this pull request as ready for review June 27, 2023 21:37

This comment was marked as outdated.

Sign in to view

gaaclarke added 2 commits June 27, 2023 15:52

fixed check for opaque source blends

22ea132

a little bit of cleanup

bd5087f

gaaclarke requested a review from bdero June 27, 2023 23:54

gaaclarke removed the will affect goldens label Jun 28, 2023

This comment was marked as outdated.

Sign in to view

flutter-dashboard bot added the will affect goldens label Jun 28, 2023

IsTransparent() -> IsOpaque

ee8839a

bdero reviewed Jun 28, 2023

View reviewed changes

gaaclarke added 3 commits June 28, 2023 10:44

added geometry::coversarea

41d40db

removed clear_color_ from entitypass

a29dea6

stopped collapsing rotated rects

92a1169

gaaclarke requested a review from bdero June 28, 2023 18:06

made sure subpasses set their clear colors

5e0c7f8

gaaclarke force-pushed the elide-draw-rects branch from 389e7cd to 5e0c7f8 Compare June 28, 2023 20:05

bdero approved these changes Jun 28, 2023

View reviewed changes

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

auto-submit bot merged commit 584d72f into flutter:main Jun 29, 2023

engine-flutter-autoroll mentioned this pull request Jun 29, 2023

Roll Flutter Engine from 4e49b9deb72d to d5c25ead07e9 (7 revisions) flutter/flutter#129750

Merged

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

584d72f79 [Impeller] Collapse DrawRects into clear colors optimization (

37ebf83

flutter/engine#43168)

[Impeller] Collapse DrawRects into clear colors optimization #43168

[Impeller] Collapse DrawRects into clear colors optimization #43168

Uh oh!

Conversation

gaaclarke commented Jun 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

gaaclarke commented Jun 27, 2023

Uh oh!

This comment was marked as outdated.

bdero 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!

gaaclarke commented Jun 27, 2023

Uh oh!

gaaclarke commented Jun 27, 2023

Uh oh!

gaaclarke commented Jun 27, 2023

Uh oh!

This comment was marked as outdated.

gaaclarke commented Jun 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bdero Jun 28, 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!

bdero left a comment

Choose a reason for hiding this comment

Uh oh!

flutter-dashboard bot commented Jun 29, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gaaclarke commented Jun 23, 2023 •

edited

Loading

gaaclarke commented Jun 27, 2023 •

edited

Loading

bdero Jun 28, 2023 •

edited

Loading