share platform view slicing logic across iOS and Android. #54010

jonahwilliams · 2024-07-20T00:05:57Z

This removes support for "unobstructed platform views" on iOS - instead prefering to use the Android strategy of minimizing overlay layers, as this is generally more performant.

…ne into combine_slicing_logic

…_logic

hellohuanlin · 2024-07-22T21:58:03Z

instead prefering to use the Android strategy of minimizing overlay layers, as this is generally more performant.

By Android strategy, do you mean having a global limit of overlay count?

jonahwilliams · 2024-07-22T22:08:00Z

AFAIK there is no global limit on the number of overlays, but we create at most one overlay per embedder view slice.

hellohuanlin · 2024-07-22T22:23:07Z

flow/view_slicer.cc

+    const std::unordered_map<int64_t, SkRect>& view_rects) {
+  std::unordered_map<int64_t, SkRect> overlay_layers;
+
+  auto current_frame_view_count = composition_order.size();


just so that i understand - this count is the count of platform views right?

hellohuanlin · 2024-07-22T22:26:53Z

flow/view_slicer.cc

+      // Limit the number of native views, so it doesn't grow forever.
+      //
+      // In this case, the rects are merged into a single one that is the union
+      // of all the rects.


iOS used to have 2 overlays per platform view as the limit. Do you have any context why we picked 2?

as far as I can tell it was aribtrarily chosen

Wondering if there are any benefit of having multiple overlays despite of the performance overhead. Otherwise it's odd that we picked 2 before. @cbracken do you recall?

its possible that at one point the overlay surfaces weren't screen sized, so shrinking the overlays as small as possible improved performance. overlay sizes aren;t stable however, which is what lead to using the frame size in all cases - which defeats the purpose of minimizing overlay size.

Talked to @jonahwilliams about this this morning but no, I don't recall this splitting logic at all and it's surprising to me that we're doing it -- adding layers should harm performance if anything. Could be something like what Jonah is suggesting.

the subpixel overlap issue

Do you mean this? flutter/flutter#143420

Gotcha. This PR should move the metrics on both iOS (by reducing the limit from 2 to 1) and Android (by fixing the 1 px overlay). Do you think I should make our ads benchmark work on Android (should be a quick change? CC @jmagman correct me if it's not), so that we can measure both?

Almost all Android PVs will use TLHC by default, which does not use platform view slicing. There is work this year to make android PVs work more like iOS by default, but without any of the terrible downsides of the current approach.

This means we don't need the Android benchmark to run for this PR (since it's not the implementation used in Android yet), but we may need it later down the road, right? (flutter/flutter#152200)

chinmaygarde · 2024-07-23T01:14:13Z

flow/view_slicer.h

+
+namespace flutter {
+
+/// @brief Compute the required overlay layers and clip the view slices


Can this documentation be elaborated? What are the integer handles? How are they created. What is the rect that is returned? The embedder view slice is also not documented so there are no breadcrumbs there either.

Can these go in a struct with a custom hasher and equality checker?

If this is just moving code around and you want to limit scope, then we could probably do it in a followup. Just that from the perspective of someone not super familiar with slicing, this reads a bit dense.

I agree for the most part, the platform view code is not very abstracted - lots of vecs/maps with the integer id keys that should be refactored. I'd like to do that sort of gradually though, since its very related to the work in

#53882 and #53826

jonahwilliams · 2024-07-24T16:51:23Z

ping @chinmaygarde @cbracken @hellohuanlin can you either approve or make suggestions? This patch is blocking very high priority work.

hellohuanlin · 2024-07-24T16:58:54Z

Sorry for the delay. Will take another look today!

chinmaygarde

Ah, didn't realize you were blocked without pending following.

I'm not super comfortable with this code but gradually refactoring and documenting this code works for me. Looks fine to me and a lot of it is moving stuff to be shared. For this reason, happy to LGTM but please me know if I should focus on something specific.

hellohuanlin · 2024-07-24T17:24:13Z

@jonahwilliams let me quickly pull the code and check if the overlay merging logic still works on iOS.

hellohuanlin

Confirmed the overlay merging logic worked! Hopefully this can move our benchmark a bit

…lutter/engine#54010)

…152260) flutter/engine@490576d...c2f489d 2024-07-24 [email protected] Fix embedder comments about struct_size (flutter/engine#54077) 2024-07-24 [email protected] share platform view slicing logic across iOS and Android. (flutter/engine#54010) 2024-07-24 [email protected] [skwasm] Fix platform view occlusion logic. (flutter/engine#54061) 2024-07-24 [email protected] Roll Skia from 25f26f673502 to c11932925658 (4 revisions) (flutter/engine#54081) 2024-07-24 [email protected] Upgrade Engine Android SDK to 35 (flutter/engine#53574) 2024-07-24 [email protected] Roll Skia from a9b1043eb23e to 25f26f673502 (1 revision) (flutter/engine#54079) 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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…lutter#152260) flutter/engine@490576d...c2f489d 2024-07-24 [email protected] Fix embedder comments about struct_size (flutter/engine#54077) 2024-07-24 [email protected] share platform view slicing logic across iOS and Android. (flutter/engine#54010) 2024-07-24 [email protected] [skwasm] Fix platform view occlusion logic. (flutter/engine#54061) 2024-07-24 [email protected] Roll Skia from 25f26f673502 to c11932925658 (4 revisions) (flutter/engine#54081) 2024-07-24 [email protected] Upgrade Engine Android SDK to 35 (flutter/engine#53574) 2024-07-24 [email protected] Roll Skia from a9b1043eb23e to 25f26f673502 (1 revision) (flutter/engine#54079) 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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

share slicing logic across iOS and Android.

d2eed60

github-actions bot added platform-ios platform-android labels Jul 20, 2024

Jonah Williams added 10 commits July 19, 2024 17:39

Update FlutterPlatformViews.mm

df98325

Update FlutterPlatformViews.mm

87e37ea

Update view_slicer.cc

0a681cc

++

8c73382

Merge branch 'combine_slicing_logic' of github.com:jonahwilliams/engi…

08f691b

…ne into combine_slicing_logic

create single overlay.

4a47fae

Update UnobstructedPlatformViewTests.m

e211be2

++

b0bfd8b

Merge branch 'main' of github.com:flutter/engine into combine_slicing…

c16de49

…_logic

license update.

3dcefa2

jonahwilliams marked this pull request as ready for review July 22, 2024 14:55

jonahwilliams changed the title ~~share slicing logic across iOS and Android.~~ share platform view slicing logic across iOS and Android. Jul 22, 2024

jonahwilliams requested review from hellohuanlin and chinmaygarde July 22, 2024 14:55

hellohuanlin reviewed Jul 22, 2024

View reviewed changes

chinmaygarde reviewed Jul 23, 2024

View reviewed changes

jonahwilliams requested review from chinmaygarde, cbracken and hellohuanlin July 24, 2024 16:50

chinmaygarde approved these changes Jul 24, 2024

View reviewed changes

hellohuanlin approved these changes Jul 24, 2024

View reviewed changes

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

auto-submit bot merged commit 3bba8a4 into flutter:main Jul 24, 2024

engine-flutter-autoroll mentioned this pull request Jul 24, 2024

Roll Flutter Engine from 490576daf810 to c2f489d783d6 (6 revisions) flutter/flutter#152260

Merged

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

3bba8a48c share platform view slicing logic across iOS and Android. (f…

e2f7996

…lutter/engine#54010)

hellohuanlin mentioned this pull request Jul 25, 2024

Nesting PointerInterceptor platform views with Dropdown will cause crash on iOS flutter/flutter#151535

Closed


		namespace flutter {

		/// @brief Compute the required overlay layers and clip the view slices

share platform view slicing logic across iOS and Android. #54010

share platform view slicing logic across iOS and Android. #54010

Uh oh!

Conversation

jonahwilliams commented Jul 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hellohuanlin commented Jul 22, 2024

Uh oh!

jonahwilliams commented Jul 22, 2024

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!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Jul 24, 2024

Uh oh!

hellohuanlin commented Jul 24, 2024

Uh oh!

chinmaygarde left a comment

Choose a reason for hiding this comment

Uh oh!

hellohuanlin commented Jul 24, 2024

Uh oh!

hellohuanlin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonahwilliams commented Jul 20, 2024 •

edited

Loading