Use a single mask view to clip platform view #20050

cyanglaz · 2020-07-27T05:15:10Z

Description

We used to use a logic that stacking ChildClippingView to achieve clippings for the iOS platform view. (Introduced in aa9b3a1)
This approach has a drawback that the ChildClippingView needs to be detached and re-attached to the flutter view whenever there is a change to the mutator stack.
This can cause a serious bug. (explained in the issue below)

This PR uses a single mask view to do the clipping. So we always only have one ChildClippingView.
In drawRect: of the mask view, it fills the final clipping area with an opaque color. And the ChildClippingView replaces its alpha channel with the mask view's alpha channel by setting the maskView property.
https://developer.apple.com/documentation/uikit/uiview/1622557-maskview?language=objc

Related Issues

Fixes flutter/flutter#46167

Tests

There is no behavior change, but we need to make sure the scenario app passes the tests so we know the mutations still works.
I'm not sure how do we test the behavior of flutter/flutter#46167. I have manually tested to make sure it works now.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the contributor guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the C++, Objective-C, Java style guides for the engine.
I read the tree hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation.
All existing and new tests are passing.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read handling breaking changes.

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: https://flutter.dev/go/breaking-changes-template Replace this with a link to a pull request that adds the migration guide to https://flutter.dev/docs/release/breaking-changes

flutter-dashboard · 2020-07-27T18:09:47Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

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

blasten · 2020-07-28T03:07:13Z

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

@@ -16,6 +16,32 @@
 #include "flutter/shell/platform/darwin/ios/ios_context.h"
 #include "third_party/skia/include/core/SkPictureRecorder.h"

+// A UIView acts as a clipping mask for the |ChildClippingView|.
+//
+// On |DrawRect:|, this view performs a series of clipping operations and sets the alpha channel


should this interface contain the virtual drawRect method?

This View is an OBJC class, drawRect is inherited from the UIView. So I don't think any virtual keyword is necessary here.

sg. do you where I can read about the doc format? I'm not sure what |DrawRect:| means, but I can see it means what you described.

https://developer.apple.com/documentation/uikit/uiview/1622529-drawrect

I mean the notation. It reads like a class to me, but I’m not sure what the colon symbol is used for.

ah you are right, let me fix that

…s_Internal.h Co-authored-by: Emmanuel Garcia <egarciad@google.com>

…laz/engine into platform_view_mutation_single_view

blasten · 2020-07-28T19:25:57Z

@gaaclarke Chris mentioned that this PR is causing the scenario app UI test to fail even though there's not difference in pixels. (as reported by external image comparison tools). Would you mind taking a look?

cyanglaz · 2020-07-28T19:37:51Z

@gaaclarke I update committed the new screenshot into this PR. You should see that their pixels are identical. Maybe we should make the thread holder bigger?

cwlp8024 · 2020-07-29T10:03:48Z

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

-  return NO;
+- (CGRect)getCGRectFromSkRect:(const SkRect&)clipSkRect {
+  return CGRectMake(clipSkRect.fLeft, clipSkRect.fTop, clipSkRect.fRight - clipSkRect.fLeft,
+                    clipSkRect.fBottom - clipSkRect.fTop);
 }

 @end


gpus_ReturnNotPermittedKillClient crash on iOS 62403 Can you help us have a look，please？

The issue doesn't seem to related to this PR?

cyanglaz · 2020-07-29T18:17:29Z

@blasten @gaaclarke The new screenshots are identical with the old golden images. I think our image diff logic is just more strict than regular image diff tools. Let's update the golden images here?

gaaclarke · 2020-07-30T00:34:21Z

I'm fine updating the golden images if they are acceptable. I talked to @cyanglaz offline and I suspect this has to do with different handling of transparent / semi-transparent pixels.

blasten · 2020-07-30T19:29:15Z

As discussed offline, @cyanglaz will add unit tests

blasten · 2020-07-30T22:54:00Z

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

+  // Create embedded view params
+  flutter::MutatorsStack stack;
+  // Layer tree always pushes a screen scale factor to the stack
+  SkMatrix screenScaleMatrix = SkMatrix::MakeScale([UIScreen mainScreen].scale, [UIScreen mainScreen].scale);


I'm not sure what [UIScreen mainScreen].scale would be when executed on LUCI. Maybe pick a constant value?

It has to be screenScale because the CompositeEmbeddedView uses screenScale to add a reverse scale matrix to the final transform matrix.

blasten · 2020-07-30T22:55:05Z

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

+  // Create embedded view params
+  flutter::MutatorsStack stack;
+  // Layer tree always pushes a screen scale factor to the stack
+  SkMatrix screenScaleMatrix = SkMatrix::MakeScale([UIScreen mainScreen].scale, [UIScreen mainScreen].scale);


blasten · 2020-07-30T22:59:55Z

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

+  // Push a rotate matrix
+  SkMatrix rotateMatrix;
+  rotateMatrix.setRotate(10);
+  stack.PushTransform(rotateMatrix);


This is fine, but what about PushClipPath?

engine/flow/embedded_views.h

Line 133 in c57aff1

void PushClipPath(const SkPath& path);

Should that be in a separate unit test?

To test the clipping, we will probably have to render the view into an image, then check the pixels (sort like a screenshot). I think the scenario tests already covered all the clippings, it is kind of redundant.

blasten

LGTM % nits

cyanglaz · 2020-08-01T18:36:16Z

@blasten Added unit tests for clipping. PTAL.

blasten · 2020-08-03T16:35:13Z

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

+  SkMatrix screenScaleMatrix =
+      SkMatrix::MakeScale([UIScreen mainScreen].scale, [UIScreen mainScreen].scale);
+  stack.PushTransform(screenScaleMatrix);


nit: does it need this to test clip specifically?

yes. as the clip paths will need to get scaled as well.

blasten · 2020-08-03T16:37:35Z

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

+      if (CGRectContainsPoint(insideClipping, point)) {
+        XCTAssertEqual(alpha, 255);
+      } else {
+        XCTAssertLessThan(alpha, 255);


is there an exact value for the alpha?

blasten

Thanks for adding the tests! LGTM

…ine#20050)

* 8a0e30b Roll Dart SDK from 2d98d25e71c3 to c59cdee365b9 (1 revision) (flutter/engine#20200) * 0519052 Roll Dart SDK from c59cdee365b9 to ad9c73cb3270 (1 revision) (flutter/engine#20201) * fc5b777 Roll Skia from 7225788b9070 to 8e77631a80f0 (8 revisions) (flutter/engine#20184) * cb8dfdb Roll Skia from 8e77631a80f0 to a6df9da28963 (17 revisions) (flutter/engine#20202) * ea811fc Roll Dart SDK from ad9c73cb3270 to 0f0e04ec3afa (3 revisions) (flutter/engine#20204) * 02d71d6 Use a single mask view to clip iOS platform view (flutter/engine#20050)

* Update 1.20.2 engine to use Dart 2.9.1 * Use a single mask view to clip iOS platform view (#20050) * Moved to RMSE for image comparison to account for slight variations in golden image tests (#19658) Moved to RMSE for image comparison to account for slight variations in golden image production. (also fixed a flakey test) Co-authored-by: Chris Yang <ychris@google.com> Co-authored-by: gaaclarke <30870216+gaaclarke@users.noreply.github.com>

* 8a0e30b Roll Dart SDK from 2d98d25e71c3 to c59cdee365b9 (1 revision) (flutter/engine#20200) * 0519052 Roll Dart SDK from c59cdee365b9 to ad9c73cb3270 (1 revision) (flutter/engine#20201) * fc5b777 Roll Skia from 7225788b9070 to 8e77631a80f0 (8 revisions) (flutter/engine#20184) * cb8dfdb Roll Skia from 8e77631a80f0 to a6df9da28963 (17 revisions) (flutter/engine#20202) * ea811fc Roll Dart SDK from ad9c73cb3270 to 0f0e04ec3afa (3 revisions) (flutter/engine#20204) * 02d71d6 Use a single mask view to clip iOS platform view (flutter/engine#20050)

flutter-dashboard bot added the platform-ios label Jul 27, 2020

googlebot added the cla: yes label Jul 27, 2020

use mask view to clip

db33eae

cyanglaz force-pushed the platform_view_mutation_single_view branch from 5e49efb to db33eae Compare July 27, 2020 05:17

cyanglaz requested a review from blasten July 27, 2020 05:17

Chris Yang added 3 commits July 27, 2020 09:46

only set alpha channel

31db69b

use fml::CFRef to wrap CGPathRef

0158df3

cleanup

9785f97

cyanglaz marked this pull request as ready for review July 27, 2020 18:09

auto-assign bot requested a review from liyuqian July 27, 2020 18:12

cyanglaz requested a review from chinmaygarde July 27, 2020 18:17

blasten reviewed Jul 28, 2020

View reviewed changes

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews_Internal.h Outdated Show resolved Hide resolved

blasten reviewed Jul 28, 2020

View reviewed changes

Chris Yang and others added 8 commits July 28, 2020 10:02

Update shell/platform/darwin/ios/framework/Source/FlutterPlatformView…

eab8257

…s_Internal.h Co-authored-by: Emmanuel Garcia <egarciad@google.com>

Merge branch 'master' into platform_view_mutation_single_view

c0627b6

update golden for iphone 8

31d44e8

revert

087b769

update doc

7c2ae3a

Merge branch 'platform_view_mutation_single_view' of github.com:cyang…

3a703fd

…laz/engine into platform_view_mutation_single_view

revert goldens

1ad2449

make mask view the same frame as flutter view

cab73d9

Chris Yang added 2 commits July 28, 2020 12:30

fix mask view orignal and clipping operations's matrix

9b59753

update iphone 8 golden

ae69e2b

cwlp8024 reviewed Jul 29, 2020

View reviewed changes

blasten reviewed Jul 30, 2020

View reviewed changes

blasten approved these changes Jul 30, 2020

View reviewed changes

Chris Yang added 2 commits July 30, 2020 16:50

formatting

a1cd71a

add tests for clipping

4c102ce

blasten reviewed Aug 3, 2020

View reviewed changes

blasten approved these changes Aug 3, 2020

View reviewed changes

cyanglaz merged commit 02d71d6 into flutter:master Aug 3, 2020

cyanglaz deleted the platform_view_mutation_single_view branch August 3, 2020 17:07

cyanglaz mentioned this pull request Aug 3, 2020

iOS PlatformView interrupts pan gesture flutter/flutter#57931

Closed

engine-flutter-autoroll mentioned this pull request Aug 3, 2020

Roll Engine from f86b83010d6c to 02d71d60effa (6 revisions) flutter/flutter#62804

Merged

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

02d71d6 Use a single mask view to clip iOS platform view (flutter/eng…

1401b6a

…ine#20050)

pcsosinski pushed a commit to pcsosinski/engine that referenced this pull request Aug 12, 2020

Use a single mask view to clip iOS platform view (flutter#20050)

ea61df9

blasten mentioned this pull request Aug 21, 2020

[webView_flutter][iOS] WebView is frozen in release mode with flutter 1.20.2 stable version flutter/flutter#64228

Closed

dannyvalentesonos mentioned this pull request Sep 17, 2020

Platform views in Add2App (iOS only) are broken as a result of https://github.com/flutter/engine/pull/20050 flutter/flutter#66026

Closed

csells added customer: amplify and removed customer: amplify labels Sep 17, 2020

njovy mentioned this pull request Sep 17, 2020

[iOS][WebView] Gesture recognizers not blocked in iOS 13.4 and 13.5 flutter/flutter#53490

Closed

joniaiuser mentioned this pull request Sep 19, 2020

image_picker Camera activation leads to unresponsive webview_flutter flutter/flutter#66165

Closed

cyanglaz mentioned this pull request Aug 25, 2021

iOS platform view clipping chain reconstruction can be faster flutter/flutter#35023

Closed

jmagman mentioned this pull request Feb 7, 2024

Round SkRects in ClipRectContainsPlatformViewBoundingRect #50387

Closed

8 tasks

jmagman mentioned this pull request May 28, 2024

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

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use a single mask view to clip platform view #20050

Use a single mask view to clip platform view #20050

cyanglaz commented Jul 27, 2020 •

edited

Loading

flutter-dashboard bot commented Jul 27, 2020

blasten Jul 28, 2020

cyanglaz Jul 28, 2020

blasten Jul 28, 2020

cyanglaz Jul 28, 2020

blasten Jul 28, 2020

cyanglaz Jul 28, 2020

blasten commented Jul 28, 2020

cyanglaz commented Jul 28, 2020

cwlp8024 Jul 29, 2020

cyanglaz Jul 29, 2020

cyanglaz commented Jul 29, 2020 •

edited

Loading

gaaclarke commented Jul 30, 2020

blasten commented Jul 30, 2020

blasten Jul 30, 2020

cyanglaz Jul 30, 2020

blasten Jul 30, 2020

blasten Jul 30, 2020

cyanglaz Jul 31, 2020

blasten left a comment

cyanglaz commented Aug 1, 2020

blasten Aug 3, 2020

cyanglaz Aug 3, 2020

blasten Aug 3, 2020

blasten left a comment

Use a single mask view to clip platform view #20050

Use a single mask view to clip platform view #20050

Conversation

cyanglaz commented Jul 27, 2020 • edited Loading

Description

Related Issues

Tests

Checklist

Breaking Change

flutter-dashboard bot commented Jul 27, 2020

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

blasten commented Jul 28, 2020

cyanglaz commented Jul 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyanglaz commented Jul 29, 2020 • edited Loading

gaaclarke commented Jul 30, 2020

blasten commented Jul 30, 2020

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

blasten left a comment

Choose a reason for hiding this comment

cyanglaz commented Aug 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blasten left a comment

Choose a reason for hiding this comment

cyanglaz commented Jul 27, 2020 •

edited

Loading

cyanglaz commented Jul 29, 2020 •

edited

Loading