iOS PlatformView only sets a maskView when necessary #37434

cyanglaz · 2022-11-08T17:47:34Z

TL;DR

The maskView reduces the performance, and we should avoid the usage as much as possible, details in: flutter/flutter#107486 (comment)

This PR checks whether the the bounding rect of a clip mutator contains the entire PlatformView. If that is the case, we skip that clip mutator because the visual result will be the same.

This is particular helpful when there are multiple PlatformViews in a list view because there is usually a clipRect layer around the whole list view but the PlatformViews aren't really need to be clipped.

Fixes: flutter/flutter#107486

Performance comparison

Before fix:

After fix:

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.

jmagman · 2022-11-09T21:13:03Z

There are scenario test golden failures GOLDEN DIFF FAILED

	-[PlatformViewWithOtherBackdropFilterTests testPlatformView]
	-[TwoPlatformViewsWithOtherBackDropFilterTests testPlatformView]
	-[UnobstructedPlatformViewTests testMultiplePlatformViewsWithOverlays]
	-[UnobstructedPlatformViewTests testMultiplePlatformViewsWithoutOverlays]
	-[PlatformViewMutationTransformTests testPlatformView]
	-[MultiplePlatformViewsBackgroundForegroundTest testPlatformView]
	-[MultiplePlatformViewsTest testPlatformView]

stuartmorgan

Are the failing tests false positives from fragile tests, or is this actually causing unexpected side-effects?

stuartmorgan · 2022-11-10T15:02:03Z

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm

@@ -32,6 +32,16 @@ - (BOOL)flt_hasFirstResponderInViewHierarchySubtree {
 }
 @end

+static bool ClipBoundsContainsPlatformViewBoundingRect(const SkRect& clipBounds,


This needs a declaration comment.

It would be especially helpful to explain what the bounds and rect are supposed to be relative to.

(And what the matrix is applied to.)

stuartmorgan · 2022-11-10T15:05:02Z

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm

+                                                       const SkRect& platformViewBoundingRect,
+                                                       const SkMatrix& transformMatrix,
+                                                       CGFloat screenScale) {
+  SkRect transformedClipBounds = transformMatrix.mapRect(clipBounds);


Maybe it's just because I'm not familiar with this code, but the logic here doesn't seem very obvious. E.g., I don't know why there's a reverse screen scale applied to one of the parameters (but not the other). Comments explaining why the code here is doing what it is doing would be good.

I rearranged the code in ApplyMutators so that the reverseScreenScale matrix is added later, which helps eliminate the screen scale factor in this method. Hopefully it is now easier to understand.

stuartmorgan · 2022-11-10T15:07:01Z

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm

@@ -32,6 +32,16 @@ - (BOOL)flt_hasFirstResponderInViewHierarchySubtree {
 }
 @end

+static bool ClipBoundsContainsPlatformViewBoundingRect(const SkRect& clipBounds,
+                                                       const SkRect& platformViewBoundingRect,


Optional nit for here and the local variables: while either snake_case or camelCase are allowed by style for C(++) functions in ObjC++ code, it looks like most of the surrounding code here is C++ using snake_case, so it seems like snake_case for these would be more consistent.

stuartmorgan · 2022-11-10T15:08:27Z

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews_Internal.h

@@ -280,7 +280,9 @@ class FlutterPlatformViewsController {
  // T_1 is applied to C_2, T_3 and T_4 are applied to C_5, and T_6 is applied to PLATFORM_VIEW.
  //
  // After each clip operation, we update the head to the super view of the current head.


Could you update the comment here to explain bounding_rect? It's not clear from immediate context what exactly it is the bounds of.

This is still missing.

stuartmorgan · 2022-11-10T15:17:35Z

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm

-  clipView.maskView = maskView;
+  embedded_view.layer.transform =
+      CATransform3DConcat(GetCATransform3DFromSkMatrix(transformMatrix), reverseTranslate);
+  if (needMask) {


Optional: You could eliminate needMask and just call clipView.maskView = maskView each place you currently have needMask = YES, so there's less local state to track.

cyanglaz · 2022-11-10T18:18:27Z

The failing tests are due a bug, I had the matrix concatenation backwards. Will fix it along with other nits in the next few commits.

cyanglaz · 2022-11-11T00:07:07Z

I'm not sure why mac_host_engine keeps failing on a seemingly unrelated impeller unit test.

[ RUN      ] Play/RuntimeStageTest.CanCreatePipelineFromRuntimeStage/Metal
[ERROR:flutter/fml/backtrace.cc(108)] Caught signal SIGSEGV during program execution.
Frame 0: 0x7ff80d19a177 newRenderPipeline()
Frame 1: 0x7ff80d226b7c __233-[MTLCompiler createVertexStageAndLinkPipelineWithFragment:fragmentVariant:vertexFunction:serializedVertexDescriptor:descriptor:destinationArchive:options:reflection:compileStatistics:fragmentCompileTimeData:error:completionHandler:]_block_invoke.1436
Frame 2: 0x7ff80461c0cc _dispatch_call_block_and_release
Frame 3: 0x7ff80461d317 _dispatch_client_callout
Frame 4: 0x7ff804623317 _dispatch_lane_serial_drain
Frame 5: 0x7ff804623dfd _dispatch_lane_invoke
Frame 6: 0x7ff80462deee _dispatch_workloop_worker_thread
Frame 7: 0x7ff8047d0fd0 _pthread_wqthread
Frame 8: 0x7ff8047cff57 start_wqthread



!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!


Failed Command:

/Volumes/Work/s/w/ir/cache/builder/src/out/host_profile/impeller_unittests --gtest_repeat=2 --gtest_shuffle

Exit Code: -11

The same test passed locally.

cyanglaz · 2022-11-11T00:42:59Z

The impeller unit test failure is related to flutter/flutter#114872

cyanglaz · 2022-11-11T17:31:04Z

@stuartmorgan Updated per review comments, PTAL.
I also added a scenario app test which combines transform and clips.

stuartmorgan

LGTM other than a couple of missing comments (with the caveat that I'm not familiar with the details of this clipping system, so I'm just trusting you and the tests that all the matrix/transform logic is correct 🙂 ).

stuartmorgan · 2022-11-11T21:14:45Z

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews_Internal.h

@@ -280,7 +280,9 @@ class FlutterPlatformViewsController {
  // T_1 is applied to C_2, T_3 and T_4 are applied to C_5, and T_6 is applied to PLATFORM_VIEW.
  //
  // After each clip operation, we update the head to the super view of the current head.


This is still missing.

stuartmorgan · 2022-11-11T21:16:09Z

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews_Internal.mm

@@ -241,6 +241,8 @@ - (NSMutableArray*)backdropFilterSubviews {

 @interface FlutterClippingMaskView ()

+@property(nonatomic) CATransform3D reverseScreenScale;


This needs a declaration comment, per style guide.

cyanglaz · 2022-11-14T18:16:01Z

Added comments to explain the bounding_rect parameter and reverseScreenScale property.

chinmaygarde · 2022-11-17T21:08:47Z

The presub failure looks related to this patch but the fix ought to be simple. Should be good go soon!

cyanglaz · 2022-11-17T23:32:43Z

The presub failure looks related to this patch but the fix ought to be simple. Should be good go soon!

Yes, I'm currently working on a different local branch and plan to fix it later today.

tests fix tests format review fix add scenario tset more scenario tests add docs and comments use offset.zero

…er/engine#37434)

…er/engine#37434) (#115621)

…ry (flutter/engine#37434) (#115621) Commit: 9c9f7818ac16b80e030cd101fe050e9e96f5c1eb

flutter/engine#37434) (flutter/flutter#115621)

* 70b7445 Reland Added Badge.count constructor (flutter/flutter#115566) * 31f8631 fa7e1965e [Impeller] Fix glyph atlas uploads and renders (flutter/engine#37691) (flutter/flutter#115556) * a1ea383 Label should always be aligned with text in filled input decoration (flutter/flutter#115540) * c2b2950 Add selection feedback for both selection area and text field (flutter/flutter#115373) * 0344407 Rev package:pub_semver to the latest version (flutter/flutter#115570) * ac06523 Add Material 3 support for `Slider` - Part 2 (flutter/flutter#114624) * b181d07 a2fa4e9 cirrus to luci (flutter/plugins#6711) (flutter/flutter#115573) * e1efd0d b241e69fd [ui] reland add docs to FragmentShader (flutter/engine#37699) (flutter/flutter#115578) * efb0694 Remove unused flutter_attach_test_fuchsia (flutter/flutter#115515) * a5a368c 487ee66f6 [macOS] Merge FlutterRenderer and implementation (flutter/engine#37696) (flutter/flutter#115581) * 4ff7fc6 Fixes a bug where dragging a collapsed handle in TextField does not vibrate (flutter/flutter#115586) * 20be280 da9534ea6 [macOS] Consolidate external texture classes (flutter/engine#37703) (flutter/flutter#115585) * 8a7102e Roll Flutter Engine from da9534ea6534 to d955a72c5604 (3 revisions) (flutter/flutter#115589) * e1903a2 Roll Flutter Engine from d955a72c5604 to 1e1a4ab3c993 (4 revisions) (flutter/flutter#115592) * 78390a0 Roll Flutter Engine from 1e1a4ab3c993 to b65c24ce621a (2 revisions) (flutter/flutter#115598) * 75a0a72 [devicelab] measure entire release folder size, zipped (flutter/flutter#115597) * 59a01b6 Roll Flutter Engine from b65c24ce621a to 49b52db603cc (3 revisions) (flutter/flutter#115606) * ec03f1c Revert "[devicelab] measure entire release folder size, zipped (#115597)" (flutter/flutter#115609) * 710e708 Improve showSnackBar documentation (flutter/flutter#114612) * 915c3de Roll Flutter Engine from 49b52db603cc to 80b25a302b4c (2 revisions) (flutter/flutter#115608) * 450f162 Roll Flutter Engine from 80b25a302b4c to e812122e4060 (2 revisions) (flutter/flutter#115614) * 0b33b85 [devicelab] measure entire release folder size, zipped (flutter/flutter#115612) * 9379c32 Revert "[devicelab] measure entire release folder size, zipped (#115612)" (flutter/flutter#115617) * b746557 f27666d2f [macOS] Merge FlutterBackingStore implementations (flutter/engine#37730) (flutter/flutter#115616) * 5487a7d Roll Flutter Engine from f27666d2f4da to 39f546585b0b (2 revisions) (flutter/flutter#115618) * f261c2f update comments (flutter/flutter#115603) * 9c9f781 04aea3c47 iOS PlatformView only sets a maskView when necessary (flutter/engine#37434) (flutter/flutter#115621) * 6926960 4ca2c1d78 Roll Skia from 55f654bf5cff to 9d56e506b4df (13 revisions) (flutter/engine#37739) (flutter/flutter#115625) * de4c0b1 Use `double.isNaN` instead of `... == double.nan` (which is always false) (flutter/flutter#115424) * a655f85 a62736769 Roll Skia from 9d56e506b4df to d693b4b9fe5e (5 revisions) (flutter/engine#37741) (flutter/flutter#115640) * 18c8727 f092cd826 Roll Fuchsia Mac SDK from SVtX810D2U_ZgBcpx... to tklUfTsSOVKk49tYq... (flutter/engine#37742) (flutter/flutter#115643)

…er/engine#37434) (flutter#115621)

flutter-dashboard bot added the platform-ios label Nov 8, 2022

cyanglaz requested a review from stuartmorgan November 8, 2022 17:47

cyanglaz marked this pull request as draft November 8, 2022 19:42

cyanglaz removed the request for review from stuartmorgan November 8, 2022 19:43

cyanglaz mentioned this pull request Nov 9, 2022

Platform-View List / Grid scrolling is janky on IOS / Android flutter/flutter#107486

Closed

cyanglaz marked this pull request as ready for review November 9, 2022 00:32

cyanglaz requested a review from stuartmorgan November 9, 2022 00:32

cyanglaz force-pushed the platform_view_perf branch from ef2033e to b865ee5 Compare November 9, 2022 18:31

cyanglaz force-pushed the platform_view_perf branch from b865ee5 to 704d519 Compare November 9, 2022 23:40

stuartmorgan requested changes Nov 10, 2022

View reviewed changes

cyanglaz force-pushed the platform_view_perf branch from 686fdb5 to f2c3d78 Compare November 10, 2022 23:35

cyanglaz mentioned this pull request Nov 11, 2022

[Impeller] Some try jobs are failing on segfaults internal to the Metal shader compiler. flutter/flutter#114872

Closed

cyanglaz requested a review from stuartmorgan November 11, 2022 17:30

stuartmorgan approved these changes Nov 11, 2022

View reviewed changes

cyanglaz force-pushed the platform_view_perf branch from e8a2e84 to e96abe0 Compare November 14, 2022 18:15

cyanglaz force-pushed the platform_view_perf branch from e96abe0 to f327175 Compare November 16, 2022 21:25

only clip when necessary

bdfe5f3

tests fix tests format review fix add scenario tset more scenario tests add docs and comments use offset.zero

cyanglaz force-pushed the platform_view_perf branch from 8ce1db4 to bdfe5f3 Compare November 18, 2022 01:55

remove mistakenly checked-in code

e93023a

cyanglaz added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 18, 2022

auto-submit bot merged commit 04aea3c into flutter:main Nov 18, 2022

engine-flutter-autoroll mentioned this pull request Nov 18, 2022

Roll Flutter Engine from 39f546585b0b to 04aea3c47b2c (1 revision) flutter/flutter#115621

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 18, 2022

04aea3c47 iOS PlatformView only sets a maskView when necessary (flutt…

4133dfb

…er/engine#37434)

auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Nov 18, 2022

04aea3c47 iOS PlatformView only sets a maskView when necessary (flutt…

9c9f781

…er/engine#37434) (#115621)

sourcegraph-bot pushed a commit to sgtest/megarepo that referenced this pull request Nov 18, 2022

flutter: 04aea3c47 iOS PlatformView only sets a maskView when necessa…

89cf7f7

…ry (flutter/engine#37434) (#115621) Commit: 9c9f7818ac16b80e030cd101fe050e9e96f5c1eb

This was referenced Nov 18, 2022

Roll Flutter from 537f89ca0ef1 to 18c87274cdc9 (31 revisions) flutter/packages#2822

Merged

Roll Flutter from 61e927d22fe6 to 18c87274cdc9 (133 revisions) flutter/plugins#6716

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 18, 2022

9c9f781 04aea3c47 iOS PlatformView only sets a maskView when necessary (

ea38bc7

flutter/engine#37434) (flutter/flutter#115621)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Nov 18, 2022

9c9f781 04aea3c47 iOS PlatformView only sets a maskView when necessary (

7fa9f5e

flutter/engine#37434) (flutter/flutter#115621)

engine-flutter-autoroll mentioned this pull request Nov 18, 2022

Roll Flutter from 61e927d22fe6 to 87cb150c2062 (134 revisions) flutter/plugins#6717

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Nov 18, 2022

9c9f781 04aea3c47 iOS PlatformView only sets a maskView when necessary (

744b4e0

flutter/engine#37434) (flutter/flutter#115621)

engine-flutter-autoroll mentioned this pull request Nov 18, 2022

Roll Flutter from 61e927d22fe6 to 10c039b2f871 (135 revisions) flutter/plugins#6718

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Nov 18, 2022

9c9f781 04aea3c47 iOS PlatformView only sets a maskView when necessary (

a07ed18

flutter/engine#37434) (flutter/flutter#115621)

engine-flutter-autoroll mentioned this pull request Nov 18, 2022

Roll Flutter from 61e927d22fe6 to 7fd3bb2aedf7 (136 revisions) flutter/plugins#6719

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Nov 18, 2022

9c9f781 04aea3c47 iOS PlatformView only sets a maskView when necessary (

2a3cde3

flutter/engine#37434) (flutter/flutter#115621)

cyanglaz deleted the platform_view_perf branch November 18, 2022 16:59

engine-flutter-autoroll mentioned this pull request Nov 18, 2022

Roll Flutter from 61e927d22fe6 to 3a3a0db13e48 (137 revisions) flutter/plugins#6720

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Nov 18, 2022

9c9f781 04aea3c47 iOS PlatformView only sets a maskView when necessary (

b2a8a35

flutter/engine#37434) (flutter/flutter#115621)

engine-flutter-autoroll mentioned this pull request Nov 18, 2022

Roll Flutter from 61e927d22fe6 to ca78327402bf (138 revisions) flutter/plugins#6721

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Nov 18, 2022

9c9f781 04aea3c47 iOS PlatformView only sets a maskView when necessary (

77d11dd

flutter/engine#37434) (flutter/flutter#115621)

engine-flutter-autoroll mentioned this pull request Nov 18, 2022

Roll Flutter from 61e927d22fe6 to d2e6dfefa5ca (143 revisions) flutter/plugins#6722

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Nov 18, 2022

9c9f781 04aea3c47 iOS PlatformView only sets a maskView when necessary (

9ee073c

flutter/engine#37434) (flutter/flutter#115621)

shogohida pushed a commit to shogohida/flutter that referenced this pull request Dec 7, 2022

04aea3c47 iOS PlatformView only sets a maskView when necessary (flutt…

ee9fda9

…er/engine#37434) (flutter#115621)

0xZOne mentioned this pull request Dec 13, 2022

[ios] Swiping a listview containing platform views has noticeable jitter on Promotion devices flutter/flutter#116640

Closed

gspencergoog pushed a commit to gspencergoog/flutter that referenced this pull request Jan 19, 2023

04aea3c47 iOS PlatformView only sets a maskView when necessary (flutt…

16d916c

…er/engine#37434) (flutter#115621)

jmagman mentioned this pull request Feb 6, 2024

Round SkRects in ClipRectContainsPlatformViewBoundingRect #50387

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

iOS PlatformView only sets a maskView when necessary #37434

iOS PlatformView only sets a maskView when necessary #37434

cyanglaz commented Nov 8, 2022 •

edited

Loading

jmagman commented Nov 9, 2022

stuartmorgan left a comment

stuartmorgan Nov 10, 2022

stuartmorgan Nov 10, 2022

stuartmorgan Nov 10, 2022

cyanglaz Nov 10, 2022

stuartmorgan Nov 10, 2022

stuartmorgan Nov 10, 2022

stuartmorgan Nov 11, 2022

cyanglaz Nov 18, 2022

stuartmorgan Nov 10, 2022

cyanglaz commented Nov 10, 2022

cyanglaz commented Nov 11, 2022 •

edited

Loading

cyanglaz commented Nov 11, 2022 •

edited

Loading

cyanglaz commented Nov 11, 2022 •

edited

Loading

stuartmorgan left a comment

stuartmorgan Nov 11, 2022

stuartmorgan Nov 11, 2022

cyanglaz Nov 18, 2022

cyanglaz commented Nov 14, 2022

chinmaygarde commented Nov 17, 2022

cyanglaz commented Nov 17, 2022

		@@ -241,6 +241,8 @@ - (NSMutableArray*)backdropFilterSubviews {

		@interface FlutterClippingMaskView ()

		@property(nonatomic) CATransform3D reverseScreenScale;

iOS PlatformView only sets a maskView when necessary #37434

iOS PlatformView only sets a maskView when necessary #37434

Conversation

cyanglaz commented Nov 8, 2022 • edited Loading

TL;DR

Performance comparison

Pre-launch Checklist

jmagman commented Nov 9, 2022

stuartmorgan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyanglaz commented Nov 10, 2022

cyanglaz commented Nov 11, 2022 • edited Loading

cyanglaz commented Nov 11, 2022 • edited Loading

cyanglaz commented Nov 11, 2022 • edited Loading

stuartmorgan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyanglaz commented Nov 14, 2022

chinmaygarde commented Nov 17, 2022

cyanglaz commented Nov 17, 2022

cyanglaz commented Nov 8, 2022 •

edited

Loading

cyanglaz commented Nov 11, 2022 •

edited

Loading

cyanglaz commented Nov 11, 2022 •

edited

Loading

cyanglaz commented Nov 11, 2022 •

edited

Loading