[Impeller] Skip rect clips that do nothing. #43948

bdero · 2023-07-24T07:55:50Z

Skip rect clips that we know won't further restrict the clip shape.
Don't append clips to enforce subpass bounds. We currently don't collapse passes when subpass clips are added anyhow, so just a restriction to subpass collapsing in order to keep the current behavior intact.
Small refactor to reduce confusion: Just place the subpass bounds limit into EntityPass itself instead of the delegate.

Built on #43946 (because iOS currently fails validation without it).

Gallery home screen before/after:

jonahwilliams · 2023-07-24T16:17:16Z

impeller/aiks/canvas.cc

+    auto transformed_rect =
+        rect.TransformBounds(xformation_stack_.back().xformation);
+    if (transformed_rect.Contains(*cull_rect)) {
+      return;  // This clip will do nothing, so skip it.
+    }


This is basically the same as Geometry::CoversArea right? If we use that method instead, then I imagine we could also handle ClipRRects. To do that we could give FilledPathGeometry an optional inner_rect parameter to do the computation (and false if not present)

That would handle the case where a rrect material is scrolled to entirely cover the screen.

I had a similar idea here: jonahwilliams@e3deb28

(Though some of the cullrect math is wrong, its right in your PR though 😆 )

Nice, I like this approach for RRects. Added this in and switched to leaning on Geometry::CoversArea.

gaaclarke · 2023-07-24T16:30:06Z

impeller/entity/entity_pass.h


  void SetDelegate(std::unique_ptr<EntityPassDelegate> delgate);

+  void SetBoundsLimit(std::optional<Rect> bounds_limit);


This should have a docstring. In other comments you've added in this PR mention "absorbed clips". We should mention that this what we are talking about.

gaaclarke · 2023-07-24T20:27:40Z

impeller/entity/entity_pass.h

+  ///         it.
+  void SetBoundsLimit(std::optional<Rect> bounds_limit);
+
+  /// @brief  Set the bounds limit, which is provided by the user when creating


Suggested change

/// @brief Set the bounds limit, which is provided by the user when creating

/// @brief Get the bounds limit, which is provided by the user when creating

jonahwilliams · 2023-07-24T20:34:26Z

impeller/entity/geometry/geometry.h

@@ -1,4 +1,5 @@
 // Copyright 2013 The Flutter Authors. All rights reserved.
+// Copyright 2013 The Flutter Authors. All rights reserved.


Suggested change

// Copyright 2013 The Flutter Authors. All rights reserved.

jonahwilliams · 2023-07-24T20:35:26Z

impeller/entity/geometry/fill_path_geometry.cc

  return path_.GetTransformedBoundingBox(transform);
 }

+bool FillPathGeometry::CoversArea(const Matrix& transform,


I would add a few sanity unit tests that cover the rrect cases, i.e. large rect, empty rect, et cetera.

Good idea, done.

jonahwilliams

LGTM with nits

auto-submit · 2023-07-24T23:29:17Z

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

…#43948)

…131230) flutter/engine@ceb2674...9a0192d 2023-07-25 [email protected] Roll Fuchsia Mac SDK from hUgYN9tEps515M1lg... to DO73K2Twew-a51xHm... (flutter/engine#43981) 2023-07-25 [email protected] Roll Skia from 4554d1b35b6e to e99aea2cffef (2 revisions) (flutter/engine#43980) 2023-07-25 [email protected] [Impeller] Skip rect clips that do nothing. (flutter/engine#43948) Also rolling transitive DEPS: fuchsia/sdk/core/mac-amd64 from hUgYN9tEps51 to DO73K2Twew-a 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

…lutter#131230) flutter/engine@ceb2674...9a0192d 2023-07-25 [email protected] Roll Fuchsia Mac SDK from hUgYN9tEps515M1lg... to DO73K2Twew-a51xHm... (flutter/engine#43981) 2023-07-25 [email protected] Roll Skia from 4554d1b35b6e to e99aea2cffef (2 revisions) (flutter/engine#43980) 2023-07-25 [email protected] [Impeller] Skip rect clips that do nothing. (flutter/engine#43948) Also rolling transitive DEPS: fuchsia/sdk/core/mac-amd64 from hUgYN9tEps51 to DO73K2Twew-a 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 added the e: impeller label Jul 24, 2023

bdero requested review from chinmaygarde, jonahwilliams and zanderso July 24, 2023 07:55

bdero self-assigned this Jul 24, 2023

This comment was marked as outdated.

Sign in to view

bdero force-pushed the bdero/elide-noop-clips branch 2 times, most recently from b79516d to 766d36b Compare July 24, 2023 08:28

dnfield approved these changes Jul 24, 2023

View reviewed changes

jonahwilliams reviewed Jul 24, 2023

View reviewed changes

gaaclarke reviewed Jul 24, 2023

View reviewed changes

bdero added 3 commits July 24, 2023 12:08

[Impeller] Skip rect clips that do nothing.

d62d663

Add test

88f50a3

Fix test, add docstrings

ef2de50

bdero force-pushed the bdero/elide-noop-clips branch from 046defd to ef2de50 Compare July 24, 2023 19:08

Lean on Geometry::CoversArea

25ad25b

gaaclarke reviewed Jul 24, 2023

View reviewed changes

jonahwilliams reviewed Jul 24, 2023

View reviewed changes

jonahwilliams approved these changes Jul 24, 2023

View reviewed changes

bdero added 3 commits July 24, 2023 14:21

Fixes

c9d5f1c

Add more tests

29023cb

Licenses

d07b1eb

bdero added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 24, 2023

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

bdero added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 24, 2023

auto-submit bot merged commit 410838b into flutter:main Jul 25, 2023

engine-flutter-autoroll mentioned this pull request Jul 25, 2023

Roll Flutter Engine from ceb2674e82b4 to 9a0192d965e0 (3 revisions) flutter/flutter#131230

Merged

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

410838b2e [Impeller] Skip rect clips that do nothing. (flutter/engine…

7476ebd

…#43948)

gaaclarke mentioned this pull request Aug 3, 2023

[Impeller] investigate cause of vulkan 90th percentile rasterizer time drop around july 25th flutter/flutter#131884

Closed


		void SetDelegate(std::unique_ptr<EntityPassDelegate> delgate);

		void SetBoundsLimit(std::optional<Rect> bounds_limit);

	/// @brief Set the bounds limit, which is provided by the user when creating
	/// @brief Get the bounds limit, which is provided by the user when creating

		@@ -1,4 +1,5 @@
		// Copyright 2013 The Flutter Authors. All rights reserved.
		// Copyright 2013 The Flutter Authors. All rights reserved.

[Impeller] Skip rect clips that do nothing. #43948

[Impeller] Skip rect clips that do nothing. #43948

Uh oh!

Conversation

bdero commented Jul 24, 2023

Uh oh!

This comment was marked as outdated.

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!

auto-submit bot commented Jul 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants