[ios][platform_view] Use CAShapeLayer as the mask to avoid software rendering #53072

hellohuanlin · 2024-05-28T20:32:04Z

This PR uses CAShapeLayer as the mask to avoid software rendering.

I kept UIView as the mask, so that we can measure just the improvement related to avoiding software rendering. This also allows me to land this change sooner. I created a separate issue to track removing UIView as the mask.

Note: the previous behavior seems to be incorrect (or at least not pixel perfect). This PR fixed it. See comments.

See design doc: https://docs.google.com/document/d/1TqG_N4GK_qctuk73Gk3zOdAiILUrwMqxoCMgroK_AeA/edit?resourcekey=0-jUiidfzIS642ngG2w9vSUA&tab=t.0

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

Fixes flutter/flutter#142813
Fixes flutter/flutter#142830

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

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.

hellohuanlin · 2024-05-28T20:47:47Z

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

+  */
+  CGRect innerClipping1 = CGRectMake(3, 2, 4, 6);
+  CGRect innerClipping2 = CGRectMake(2, 3, 6, 4);
+  CGRect outterClipping = CGRectMake(2, 2, 6, 6);


Again the existing behavior is incorrect (or at least not pixel perfect).

In the picture below, the black rounded rect is the mask.

Pixels within the red rect (innerClipping1) or green rect (innerClipping2) should be fully opaque; pixels outside of black bounding rect (outterClipping) should be fully transparent; the 4 corner pixels ((2, 2), (2, 7), (7, 2) and (7, 7)) should be partially transparent.

hellohuanlin · 2024-05-28T20:51:21Z

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

  for (int i = 0; i < 10; i++) {
    for (int j = 0; j < 10; j++) {
      CGPoint point = CGPointMake(i, j);
      int alpha = [self alphaOfPoint:CGPointMake(i, j) onView:mockFlutterView];
-      // Edges of the clipping might have a semi transparent pixel, we only check the pixels that
-      // are fully inside the clipped area.
-      CGRect insideClipping = CGRectMake(3, 3, 1, 1);


The existing behavior is incorrect - there shouldn't be any semi-transparent pixels on the edge.

(Introduced in #20050)

hellohuanlin · 2024-05-28T21:21:52Z

I am able to confirm the procedures related to software rendering (described here) are gone. For example, CGBlt_fillBytes and ripc_render.

jonahwilliams

LGTM!

hellohuanlin · 2024-05-28T21:28:50Z

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

  [self setNeedsDisplay];
 }

+- (void)dealloc {
+  CGPathRelease(pathSoFar_);
+  [super dealloc];


@jmagman fyi potentially code conflict with your ARC migration PRs

jmagman · 2024-05-28T22:53:23Z

Unsurprisingly there's a scenario test failure here.

Expected: checked in screenshot

Actual: from the xctest

hellohuanlin · 2024-05-29T21:37:43Z

This golden failure is a bit strange:

2024-05-29 14:13:09.200565-0700 ScenariosUITests-Runner[95948:735109] GOLDEN DIFF FAILED: image diff greater than threshold. Current diff: 0.7507098090416192, threshold: 0.5
t = 4.09s Added attachment named 'golden_platform_view_cliprect_with_transform_iPhone SE (3rd generation)_17.0_simulator_actual.png'

I can confirm that I have regenerated this image from my simulator iPhone SE (3rd gen) 17.0.

jmagman · 2024-05-29T22:02:42Z

I can confirm that I have regenerated this image from my simulator iPhone SE (3rd gen) 17.0.

Maybe something super subtle with the macOS version? Can you pull the attachment right off the failing test or use the one I attached from the failing test?

hellohuanlin · 2024-05-30T15:49:47Z

@cbracken friendly ping since you requested review, but no rush :)

cbracken

This looks great; I'm assuming this class is never accessed from multiple threads? Otherwise consider allocating the new mutable path to a temp, freeing the old one, and then setting the ivar to the temp.

hellohuanlin · 2024-06-05T02:39:27Z

I'm assuming this class is never accessed from multiple threads?

Yes, only on main thread.

auto-submit · 2024-06-05T14:20:33Z

A reason for requesting a revert of flutter/engine/53072 could
not be found or the reason was not properly formatted. Begin a comment with 'Reason for revert:' to tell the bot why
this issue is being reverted.

jason-simmons · 2024-06-05T14:22:06Z

Reason for revert: compilation errors on iOS targets

FlutterPlatformViews_Internal.mm:277:10: error: ARC forbids explicit message send of 'dealloc'
  277 |   [super dealloc];
      |    ~~~~~ ^

(see https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8746006425774972161/+/u/build_ci_ios_debug_unopt_sim_flutter_testing_scenario_app_flutter_shell_platform_darwin_ios:ios_test_flutter/stdout)

…ftware rendering (#53072)" This reverts commit 1314faf.

…oftware rendering (#53072)" (#53220) Reverts: #53072 Initiated by: jason-simmons Reason for reverting: compilation errors on iOS targets ``` FlutterPlatformViews_Internal.mm:277:10: error: ARC forbids explicit message send of 'dealloc' 277 | [super dealloc]; | ~~~~~ ^ ``` (see https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8746006425774972161/+/u/build_ci_ios_debug_unopt_sim_flutter_testing_scenario_app_flutter_shell_platform_darwin_ios:ios_test_flutter/st Original PR Author: hellohuanlin Reviewed By: {cbracken, jonahwilliams} This change reverts the following previous change: This PR uses `CAShapeLayer` as the mask to avoid software rendering. I kept `UIView` as the mask, so that we can measure just the improvement related to avoiding software rendering. This also allows me to land this change sooner. I created [a separate issue](flutter/flutter#149212) to track removing UIView as the mask. Note: the previous behavior seems to be incorrect (or at least not pixel perfect). This PR fixed it. See comments. See design doc: https://docs.google.com/document/d/1TqG_N4GK_qctuk73Gk3zOdAiILUrwMqxoCMgroK_AeA/edit?resourcekey=0-jUiidfzIS642ngG2w9vSUA&tab=t.0 *List which issues are fixed by this PR. You must list at least one issue.* Fixes flutter/flutter#142813 Fixes flutter/flutter#142830 *If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].* [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

…software rendering (flutter/engine#53072)

…149770) flutter/engine@11a32d4...f377330 2024-06-05 skia-flutter-autoroll@skia.org Roll Skia from 37755d48cca3 to 8448abc95867 (7 revisions) (flutter/engine#53230) 2024-06-05 skia-flutter-autoroll@skia.org Roll Dart SDK from dac7c04c7342 to f838a9a8d45f (3 revisions) (flutter/engine#53225) 2024-06-05 skia-flutter-autoroll@skia.org Roll Skia from 337c3c4d1f1b to 37755d48cca3 (45 revisions) (flutter/engine#53224) 2024-06-05 yjbanov@google.com [web] enable always_specify_types for web_ui (flutter/engine#53226) 2024-06-05 matanlurey@users.noreply.github.com Re-land #52859: Revamp the engine style guide, remove `always_specify_types` (flutter/engine#53223) 2024-06-05 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from pagJoGS4kQ8Efa_if... to 2xWubo2mRP_2_wXKJ... (flutter/engine#53222) 2024-06-05 fmil@google.com [icu] Manual roll of icu (flutter/engine#53199) 2024-06-05 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[ios][platform_view] Use CAShapeLayer as the mask to avoid software rendering (#53072)" (flutter/engine#53220) 2024-06-05 41930132+hellohuanlin@users.noreply.github.com [ios][platform_view] Use CAShapeLayer as the mask to avoid software rendering (flutter/engine#53072) 2024-06-05 skia-flutter-autoroll@skia.org Roll Dart SDK from 343c20614708 to dac7c04c7342 (1 revision) (flutter/engine#53209) 2024-06-04 jonahwilliams@google.com [Impeller] Intel iOS Simulators must block on GPU completion. (flutter/engine#53073) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from pagJoGS4kQ8E to 2xWubo2mRP_2 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 matanl@google.com,rmistry@google.com,zra@google.com 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

…ftware rendering #53072" (#53256) The previous PR was reverted because it was on top of an outdated commit prior to ARC. This PR rebases the main. The only change is removing the `[super dealloc]` call that caused the compiler error. *List which issues are fixed by this PR. You must list at least one issue.* Fixes flutter/flutter#142813 Fixes flutter/flutter#142830 *If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].* [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

hellohuanlin added 2 commits May 24, 2024 09:41

[ios][platform_view]use cashapelayer as the backing layer for mask view

4e5bb14

add some tests, and fix some pixel errors

7b84f68

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

hellohuanlin commented May 28, 2024

View reviewed changes

hellohuanlin mentioned this pull request May 28, 2024

[ios][platform_view]Use CALayer directly (rather than UIView) as the clip mask flutter/flutter#149212

Open

hellohuanlin marked this pull request as ready for review May 28, 2024 21:04

hellohuanlin mentioned this pull request May 28, 2024

[DRAFT][ios][platform_view]Use CAShapeLayer as the mask to avoid software rendering #52957

Closed

8 tasks

hellohuanlin requested review from jmagman, jonahwilliams and flar May 28, 2024 21:15

jonahwilliams approved these changes May 28, 2024

View reviewed changes

hellohuanlin commented May 28, 2024

View reviewed changes

cbracken self-requested a review May 28, 2024 21:50

hellohuanlin force-pushed the pv_mask_view_use_cashapelayer_backing branch 2 times, most recently from df7fb5c to b190a44 Compare May 29, 2024 18:12

more pixel perfect golden files

55dc130

hellohuanlin force-pushed the pv_mask_view_use_cashapelayer_backing branch from b190a44 to 55dc130 Compare May 29, 2024 18:16

fix golden again

5f5ba6d

hellohuanlin force-pushed the pv_mask_view_use_cashapelayer_backing branch from 916f3b2 to 5f5ba6d Compare May 29, 2024 23:42

cbracken approved these changes Jun 4, 2024

View reviewed changes

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

auto-submit bot merged commit 1314faf into flutter:main Jun 5, 2024
28 checks passed

jason-simmons added the revert Label used to revert changes in a closed and merged pull request. label Jun 5, 2024

auto-submit bot removed the revert Label used to revert changes in a closed and merged pull request. label Jun 5, 2024

jason-simmons added the revert Label used to revert changes in a closed and merged pull request. label Jun 5, 2024

auto-submit bot pushed a commit that referenced this pull request Jun 5, 2024

Revert "[ios][platform_view] Use CAShapeLayer as the mask to avoid so…

c0bbc06

…ftware rendering (#53072)" This reverts commit 1314faf.

auto-submit bot mentioned this pull request Jun 5, 2024

Reverts "[ios][platform_view] Use CAShapeLayer as the mask to avoid software rendering (#53072)" #53220

Merged

8 tasks

auto-submit bot removed the revert Label used to revert changes in a closed and merged pull request. label Jun 5, 2024

matanlurey mentioned this pull request Jun 5, 2024

Determine why #53072 broke post-submit (after passing pre-submit) flutter/flutter#149742

Closed

engine-flutter-autoroll mentioned this pull request Jun 5, 2024

Roll Flutter Engine from 11a32d43e3f6 to 75104bea5ab2 (5 revisions) flutter/flutter#149749

Closed

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

1314faf00 [ios][platform_view] Use CAShapeLayer as the mask to avoid …

46d125c

…software rendering (flutter/engine#53072)

engine-flutter-autoroll mentioned this pull request Jun 5, 2024

Roll Flutter Engine from 11a32d43e3f6 to c7667fde2253 (7 revisions) flutter/flutter#149759

Closed

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

1314faf00 [ios][platform_view] Use CAShapeLayer as the mask to avoid …

cc57706

…software rendering (flutter/engine#53072)

engine-flutter-autoroll mentioned this pull request Jun 5, 2024

Roll Flutter Engine from 11a32d43e3f6 to 46ba10b8a2c8 (9 revisions) flutter/flutter#149762

Closed

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

1314faf00 [ios][platform_view] Use CAShapeLayer as the mask to avoid …

34862f4

…software rendering (flutter/engine#53072)

engine-flutter-autoroll mentioned this pull request Jun 5, 2024

Roll Flutter Engine from 11a32d43e3f6 to 01172f78188c (10 revisions) flutter/flutter#149768

Closed

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

1314faf00 [ios][platform_view] Use CAShapeLayer as the mask to avoid …

6d98766

…software rendering (flutter/engine#53072)

engine-flutter-autoroll mentioned this pull request Jun 5, 2024

Roll Flutter Engine from 11a32d43e3f6 to f37733035060 (11 revisions) flutter/flutter#149770

Merged

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

1314faf00 [ios][platform_view] Use CAShapeLayer as the mask to avoid …

125965d

…software rendering (flutter/engine#53072)

hellohuanlin mentioned this pull request Jun 26, 2024

iOS PlatformViews: pre-determine if clipPath is necessary flutter/flutter#118650

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ios][platform_view] Use CAShapeLayer as the mask to avoid software rendering #53072

[ios][platform_view] Use CAShapeLayer as the mask to avoid software rendering #53072

hellohuanlin commented May 28, 2024 •

edited

Loading

hellohuanlin May 28, 2024 •

edited

Loading

hellohuanlin May 28, 2024

jmagman May 28, 2024

hellohuanlin commented May 28, 2024 •

edited

Loading

jonahwilliams left a comment

hellohuanlin May 28, 2024

jmagman commented May 28, 2024

hellohuanlin commented May 29, 2024

jmagman commented May 29, 2024

hellohuanlin commented May 30, 2024

cbracken left a comment

hellohuanlin commented Jun 5, 2024

auto-submit bot commented Jun 5, 2024

jason-simmons commented Jun 5, 2024

[ios][platform_view] Use CAShapeLayer as the mask to avoid software rendering #53072

[ios][platform_view] Use CAShapeLayer as the mask to avoid software rendering #53072

Conversation

hellohuanlin commented May 28, 2024 • edited Loading

Pre-launch Checklist

hellohuanlin May 28, 2024 • edited Loading

Choose a reason for hiding this comment

hellohuanlin May 28, 2024

Choose a reason for hiding this comment

jmagman May 28, 2024

Choose a reason for hiding this comment

hellohuanlin commented May 28, 2024 • edited Loading

jonahwilliams left a comment

Choose a reason for hiding this comment

hellohuanlin May 28, 2024

Choose a reason for hiding this comment

jmagman commented May 28, 2024

hellohuanlin commented May 29, 2024

jmagman commented May 29, 2024

hellohuanlin commented May 30, 2024

cbracken left a comment

Choose a reason for hiding this comment

hellohuanlin commented Jun 5, 2024

auto-submit bot commented Jun 5, 2024

jason-simmons commented Jun 5, 2024

hellohuanlin commented May 28, 2024 •

edited

Loading

hellohuanlin May 28, 2024 •

edited

Loading

hellohuanlin commented May 28, 2024 •

edited

Loading