[ios][platform_view] fix platform view clipping path intersection #54820

hellohuanlin · 2024-08-28T00:49:43Z

It turns out that when there are multiple paths to clip, they are unioned together, rather than intersected. But clipping paths need to be intersected.

There isn't any good way to intersect arbitrary paths. However, it is easy to intersect rect paths, which is the most common use case. Then we simply fallback to software rendering if we have to intersect non-rect paths. That is:

Case 1 Only 1 clipping path (either rect path or arbitrary path):
Hardware rendering. This should be the most common use case

Case 2 Multiple rect clipping path:
Hardware rendering. This is also common, and it's the linked issue that we are fixing.

Case 3 Other complex case (multiple non-rect clipping path)
Fallback to software rendering. This should be rare.

After #53826, we don't have a working benchmark that measures the main thread anymore. However, this PR shouldn't impact our ad benchmark, since it only has 1 clipping path. I will verify manually by checking Instruments and make sure no software rendering is happening. But we really should make the benchmark working again, not just for performance improvement, but also for monitoring regression.

List which issues are fixed by this PR. You must list at least one issue.

Fixes flutter/flutter#153904

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

working copy also only 1 shape is fine

jonahwilliams

You could also create multiple CAShape layers instead

hellohuanlin · 2024-08-30T21:05:43Z

It's pretty hard to review the scenario tests, so I made a video.

The existing scenarios only clip a single path. So I added a "multi paths" version for each scenario, and made sure the resulting clip is the intersection of the paths (rather than the union).

golden.files.walk.through.mp4

hellohuanlin · 2024-08-30T21:09:25Z

You could also create multiple CAShape layers instead

After some research, it seems the only possible way is to have nested parents, each taking one shape layer as the mask. I don't think it's possible to just have one single mask that is the intersection of multiple shape layers. So this will require much larger change, and make the already complex view hierarchy more complex.

hellohuanlin · 2024-08-30T21:11:11Z

Verified the scrolling ad banner example still runs smoothly on device, and avoids software rendering related instructions (by looking at the Instruments).

hellohuanlin · 2024-08-30T21:15:59Z

Verified the linked issue worked after this fix

nateshmbhat · 2024-09-02T20:03:48Z

Eagerly waiting for this PR to be merged. Our video player ui broke after we upgraded to 3.24. 🙏🏻

jmagman · 2024-09-03T17:58:21Z

testing/scenario_app/bin/run_ios_tests.dart

@@ -358,14 +358,23 @@ final _skipTestsForImpeller = [
  'ScenariosUITests/MultiplePlatformViewsTest/testPlatformView',
  'ScenariosUITests/NonFullScreenFlutterViewPlatformViewUITests/testPlatformView',
  'ScenariosUITests/PlatformViewMutationClipPathTests/testPlatformView',
+  'ScenariosUITests/PlatformViewMutationClipPathMultipleClipsTests/testPlatformView',


Oh I didn't realize these were skipped for impeller... flutter/flutter#131888 (comment)

jonahwilliams

Approach makes sense. Its unfortunate to lose the hardware acceleration, but in most cases I suspect we'll only ever have rect clips.

Skia paths do have some ability to compute intersections/overlaps via skia path ops, which we definitely have access to: https://api.skia.org/SkPathOps_8h.html

Have you looked at using these methods to compute the path intersections?

jonahwilliams

LGTM

…ection (flutter/engine#54820)

…154583) flutter/engine@e042202...1e74e9a 2024-09-03 [email protected] [ios][platform_view] fix platform view clipping path intersection (flutter/engine#54820) 2024-09-03 [email protected] [Impeller] Add all requested glyphs if TypographerContextSkia needs to create a new atlas (flutter/engine#54912) 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

kishangohel · 2024-09-06T07:55:19Z

@hellohuanlin can we cherry-picked this to the current stable channel? As it's most required fixed ASAP

hellohuanlin · 2024-09-06T16:05:58Z

Yes we should cherry pick this. Thanks for creating the request.

flutteractionsbot · 2024-09-06T18:00:26Z

Failed to create CP due to merge conflicts.
You will need to create the PR manually. See the cherrypick wiki for more info.

…utter#54820) It turns out that when there are multiple paths to clip, they are unioned together, rather than intersected. But clipping paths need to be intersected. There isn't any good way to intersect arbitrary paths. However, it is easy to intersect rect paths, which is the most common use case. Then we simply fallback to software rendering if we have to intersect non-rect paths. That is: **Case 1** Only 1 clipping path (either rect path or arbitrary path): Hardware rendering. This should be the most common use case **Case 2** Multiple rect clipping path: Hardware rendering. This is also common, and it's the linked issue that we are fixing. **Case 3** Other complex case (multiple non-rect clipping path) Fallback to software rendering. This should be rare. After flutter#53826, we don't have a working benchmark that measures the main thread anymore. However, this PR shouldn't impact our ad benchmark, since it only has 1 clipping path. I will verify manually by checking Instruments and make sure no software rendering is happening. But we really should make the benchmark working again, not just for performance improvement, but also for monitoring regression. *List which issues are fixed by this PR. You must list at least one issue.* Fixes flutter/flutter#153904 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

#54820 on stable branch. That PR was using some of the symbol changes in #54335, so I had so switch some back: e0a630d Cherry-pick request: flutter/flutter#154712 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

wip

393e7b2

working copy also only 1 shape is fine

github-actions bot added the platform-ios label Aug 28, 2024

jonahwilliams reviewed Aug 28, 2024

View reviewed changes

hellohuanlin added 4 commits August 29, 2024 11:09

add unit tests

37b00d5

add clip rect multiple clips scenario

d101c84

add remaining golden testes

6a8dd34

add golden files

2ca4d9b

hellohuanlin mentioned this pull request Aug 30, 2024

[iOS] Clip not working for video on iOS in Flutter 3.24.1 flutter/flutter#154249

Closed

format

07c7017

nit comments

f075b4c

hellohuanlin force-pushed the pv_clipping_path_intersect_rather_than_union branch from c75d430 to f075b4c Compare August 30, 2024 21:24

hellohuanlin requested a review from cbracken August 31, 2024 17:13

hellohuanlin marked this pull request as ready for review August 31, 2024 17:13

hellohuanlin added 4 commits August 31, 2024 10:20

fix warning

5c40311

test compile error

b9300b1

fix typo in tests

a39a166

update wrong golden

906e732

jmagman reviewed Sep 3, 2024

View reviewed changes

jonahwilliams reviewed Sep 3, 2024

View reviewed changes

jonahwilliams approved these changes Sep 3, 2024

View reviewed changes

jonahwilliams mentioned this pull request Sep 3, 2024

[iOS] Consider using Skia PathOps to compute path intersections for iOS platform views. flutter/flutter#154576

Open

hellohuanlin added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 3, 2024

auto-submit bot merged commit 1e74e9a into flutter:main Sep 3, 2024
32 checks passed

engine-flutter-autoroll mentioned this pull request Sep 3, 2024

Roll Flutter Engine from e04220209943 to 1e74e9ab10ce (2 revisions) flutter/flutter#154583

Merged

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

1e74e9ab1 [ios][platform_view] fix platform view clipping path inters…

cc8825f

…ection (flutter/engine#54820)

kishangohel mentioned this pull request Sep 6, 2024

[CP-Stable][ios][platform_view] fix platform view clipping path intersection flutter/flutter#154712

Closed

jmagman added the cp: stable cherry pick to the stable release candidate branch label Sep 6, 2024

This was referenced Sep 6, 2024

[CP-stable] fix iOS platform view clipping path intersection #55007

Merged

Combine clipping rect iOS scenario test goldens to reduce maintenance burden keeping them updated flutter/flutter#154754

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[ios][platform_view] fix platform view clipping path intersection #54820

[ios][platform_view] fix platform view clipping path intersection #54820

Uh oh!

hellohuanlin commented Aug 28, 2024 •

edited

Loading

Uh oh!

jonahwilliams left a comment

Uh oh!

hellohuanlin commented Aug 30, 2024 •

edited

Loading

Uh oh!

hellohuanlin commented Aug 30, 2024

Uh oh!

hellohuanlin commented Aug 30, 2024

Uh oh!

hellohuanlin commented Aug 30, 2024

Uh oh!

nateshmbhat commented Sep 2, 2024

Uh oh!

jmagman Sep 3, 2024

Uh oh!

jonahwilliams left a comment

Uh oh!

jonahwilliams left a comment

Uh oh!

Uh oh!

kishangohel commented Sep 6, 2024

Uh oh!

hellohuanlin commented Sep 6, 2024

Uh oh!

flutteractionsbot commented Sep 6, 2024

Uh oh!

Uh oh!

[ios][platform_view] fix platform view clipping path intersection #54820

[ios][platform_view] fix platform view clipping path intersection #54820

Uh oh!

Conversation

hellohuanlin commented Aug 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

hellohuanlin commented Aug 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hellohuanlin commented Aug 30, 2024

Uh oh!

hellohuanlin commented Aug 30, 2024

Uh oh!

hellohuanlin commented Aug 30, 2024

Uh oh!

nateshmbhat commented Sep 2, 2024

Uh oh!

jmagman Sep 3, 2024

Choose a reason for hiding this comment

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kishangohel commented Sep 6, 2024

Uh oh!

hellohuanlin commented Sep 6, 2024

Uh oh!

flutteractionsbot commented Sep 6, 2024

Uh oh!

Uh oh!

hellohuanlin commented Aug 28, 2024 •

edited

Loading

hellohuanlin commented Aug 30, 2024 •

edited

Loading