PlatformViewsController: clear composition_order_ in the beginning of each frame. #22574

cyanglaz · 2020-11-18T00:13:35Z

Description

We used to clear composition_order_ in the end of the frame. This caused an issue with a frame drawn by drawLastlayerTree, because the endFrame is not called in such a frame. This causes a crash where a texture layer is constantly updating on top of a platform view, due to the same view_id is pushed to composition_order_ multiple times. (Again, because composition_order_ is not cleared)

This PR moves the composition_order_.clear() to the beginning of each frame.

This PR also removed FlutterViewController::EndFrame as it is not used anymore.

Related Issues

Fixes flutter/flutter#70577

Tests

I added the following tests:

testPlatformViewWithContinuousTexture

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.

Reviewer Checklist

I have submitted any presubmit flakes in this PR using the engine presubmit flakes form before re-triggering the failure.

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

blasten · 2020-11-18T01:01:24Z

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


-void FlutterPlatformViewsController::SetFrameSize(SkISize frame_size) {
+void FlutterPlatformViewsController::BeginFrame(SkISize frame_size) {
+  composition_order_.clear();


fyi: Android does this: https://github.com/flutter/engine/blob/master/shell/platform/android/external_view_embedder/external_view_embedder.cc#L265.

could we also have a unit test for this?

could we make aReset() method, similar to Android? Why not also clearing picture_recorders_?

blasten · 2020-11-18T01:10:59Z

testing/scenario_app/ios/Scenarios/Scenarios/ContinuousTexture.m

+- (CVPixelBufferRef)pixelBuffer
+{
+    NSDictionary *options = @{
+                              (NSString*)kCVPixelBufferCGImageCompatibilityKey : @YES,


do you mind adding some comments explaining why this is all needed?

blasten · 2020-11-18T01:11:43Z

testing/scenario_app/lib/src/platform_view.dart

+
+  @override
+  void onBeginFrame(Duration duration) {
+    print('begin frame');


is this intentional?

blasten · 2020-11-18T01:11:54Z

testing/scenario_app/lib/src/platform_view.dart

+    print('begin frame');
+    final SceneBuilder builder = SceneBuilder();
+
+    builder.pushOffset(0, 0);


nit: this is a noop

blasten · 2020-11-18T01:13:13Z

testing/scenario_app/lib/src/platform_view.dart

  }
 }

+/// A simple platform view for testing platform view with a continuous texture layer.


what does continuous texture mean? Does it mean a texture that simulates a video being played?

cyanglaz · 2020-11-18T01:50:34Z

@blasten updated per comments. PTAL

mehmetf · 2020-11-18T17:59:51Z

Folks, since this is a regression, let's try to get it in today to see whether we can land it internally by end of the week. It will be tight if at all possible.

blasten · 2020-11-18T19:34:40Z

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

+      std::make_unique<flutter::EmbeddedViewParams>(finalMatrix, SkSize::Make(300, 300), stack);
+  flutterPlatformViewsController->PrerollCompositeEmbeddedView(0, std::move(embeddedViewParams1));
+  flutterPlatformViewsController->CompositeEmbeddedView(0);
+  XCTAssertEqual(flutterPlatformViewsController->GetCurrentCanvases().size(), (unsigned long)1);


does 1UL work here?

yes, updated

blasten · 2020-11-18T19:36:01Z

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

+  flutterPlatformViewsController->CompositeEmbeddedView(0);
+  XCTAssertEqual(flutterPlatformViewsController->GetCurrentCanvases().size(), (unsigned long)1);
+
+  flutterPlatformViewsController->Reset();


Is this needed?

no, removed

blasten · 2020-11-18T19:37:46Z

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

+
+  // Second frame, |GetCurrentCanvases| should be empty at the start
+  flutterPlatformViewsController->BeginFrame(SkISize::Make(300, 300));
+  XCTAssertEqual(flutterPlatformViewsController->GetCurrentCanvases().size(), (unsigned long)0);


nit: XCTAssertTrue(flutterPlatformViewsController->GetCurrentCanvases().empty())

blasten · 2020-11-18T19:40:32Z

testing/scenario_app/ios/Scenarios/Scenarios/ContinuousTexture.m

+
+  CGContextDrawImage(context, CGRectMake(0, 0, width, height), self.image.CGImage);
+  CGColorSpaceRelease(rgbColorSpace);
+  CGContextRelease(context);


What parts of this method are essential to test and why?

This is not required. removed.

blasten

LGTM

iskakaushik

LGTM

…nning of each frame. (flutter/engine#22574)

… each frame. (#22574) (#22762) Co-authored-by: Chris Yang <[email protected]>

… each frame. (flutter#22574)

* PlatformViewsController: clear composition_order_ in the beginning of each frame. (#22574)

Chris Yang added 2 commits November 17, 2020 13:37

BeginFrame to reset composition order

f620cb8

add test

0c02873

flutter-dashboard bot added the platform-ios label Nov 18, 2020

cyanglaz requested a review from blasten November 18, 2020 00:13

google-cla bot added the cla: yes label Nov 18, 2020

cyanglaz requested a review from iskakaushik November 18, 2020 00:14

blasten reviewed Nov 18, 2020

View reviewed changes

review

77df228

Merge branch 'master' into thread_crash

83bf1b1

unittest

d83d986

blasten reviewed Nov 18, 2020

View reviewed changes

Chris Yang added 3 commits November 18, 2020 12:23

cleanup test

782e34e

more cleanup

2d22e2d

more test cleanups

319e87c

blasten approved these changes Nov 18, 2020

View reviewed changes

format

d01fc48

cyanglaz added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Nov 18, 2020

iskakaushik approved these changes Nov 18, 2020

View reviewed changes

fluttergithubbot merged commit 74d40c1 into flutter:master Nov 19, 2020

engine-flutter-autoroll mentioned this pull request Nov 19, 2020

Roll Engine from 35a0b9fe6827 to f7383c435058 (18 revisions) flutter/flutter#70864

Closed

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

74d40c1 PlatformViewsController: clear composition_order_ in the begi…

013e3ca

…nning of each frame. (flutter/engine#22574)

engine-flutter-autoroll mentioned this pull request Nov 19, 2020

Roll Engine from 35a0b9fe6827 to a0da844845f5 (20 revisions) flutter/flutter#70871

Merged

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

74d40c1 PlatformViewsController: clear composition_order_ in the begi…

47b7d42

…nning of each frame. (flutter/engine#22574)

mehmetf added a commit that referenced this pull request Nov 26, 2020

PlatformViewsController: clear composition_order_ in the beginning of…

5dcf1d2

… each frame. (#22574) (#22762) Co-authored-by: Chris Yang <[email protected]>

chaselatta pushed a commit to chaselatta/engine that referenced this pull request Nov 30, 2020

PlatformViewsController: clear composition_order_ in the beginning of…

0ccb26e

… each frame. (flutter#22574)

cyanglaz pushed a commit to cyanglaz/engine that referenced this pull request Dec 8, 2020

PlatformViewsController: clear composition_order_ in the beginning of…

9c6f445

… each frame. (flutter#22574)

christopherfujino pushed a commit that referenced this pull request Dec 9, 2020

[flutter_releases] Fix build error (#22950)

27d74a3

* PlatformViewsController: clear composition_order_ in the beginning of each frame. (#22574)

PlatformViewsController: clear composition_order_ in the beginning of each frame. #22574

PlatformViewsController: clear composition_order_ in the beginning of each frame. #22574

Uh oh!

Conversation

cyanglaz commented Nov 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Reviewer Checklist

Breaking Change

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!

cyanglaz commented Nov 18, 2020

Uh oh!

mehmetf commented Nov 18, 2020

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!

blasten left a comment

Choose a reason for hiding this comment

Uh oh!

iskakaushik left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cyanglaz commented Nov 18, 2020 •

edited

Loading