Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

PlatformView to set the correct overlay view frame, merge thread at the endFrame #33262

Merged
cyanglaz merged 2 commits into from
May 31, 2022
Merged

PlatformView to set the correct overlay view frame, merge thread at the endFrame #33262

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5720b8a
thraed merge to end frame
May 9, 2022
eb94a6b
typo
May 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 14 additions & 6 deletions shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,6 @@
// Eventually, the frame is submitted once this method returns `kSuccess`.
// At that point, the raster tasks are handled on the platform thread.
CancelFrame();
raster_thread_merger->MergeWithLease(kDefaultMergedLeaseDuration);
Copy link
Contributor

@iskakaushik iskakaushik May 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this removed?

Copy link
Contributor Author

@cyanglaz cyanglaz May 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is moved to EndFrame to ensure the threads are merged at the very end of the frame.

return PostPrerollResult::kSkipAndRetryFrame;
}
// If the post preroll action is successful, we will display platform views in the current frame.
Expand All @@ -289,6 +288,14 @@
return PostPrerollResult::kSuccess;
}

void FlutterPlatformViewsController::EndFrame(
bool should_resubmit_frame,
fml::RefPtr<fml::RasterThreadMerger> raster_thread_merger) {
if (should_resubmit_frame) {
raster_thread_merger->MergeWithLease(kDefaultMergedLeaseDuration);
}
}

void FlutterPlatformViewsController::PrerollCompositeEmbeddedView(
int view_id,
std::unique_ptr<EmbeddedViewParams> params) {
Expand Down Expand Up @@ -609,14 +616,18 @@
// This wrapper view masks the overlay view.
overlay_view_wrapper.frame = CGRectMake(rect.x() / screenScale, rect.y() / screenScale,
rect.width() / screenScale, rect.height() / screenScale);
// Set a unique view identifier, so the overlay wrapper can be identified in unit tests.
// Set a unique view identifier, so the overlay_view_wrapper can be identified in XCUITests.
overlay_view_wrapper.accessibilityIdentifier =
[NSString stringWithFormat:@"platform_view[%lld].overlay[%lld]", view_id, overlay_id];

UIView* overlay_view = layer->overlay_view.get();
// Set the size of the overlay view.
// This size is equal to the device screen size.
overlay_view.frame = flutter_view_.get().bounds;
overlay_view.frame = [flutter_view_.get() convertRect:flutter_view_.get().bounds
Copy link

@blasten blasten May 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have you seen any side effects when the frame size is changed in every frame? Try running https://github.com/flutter/flutter/tree/195a1cc413bde5f7c1d194204608c4bc20659124/dev/benchmarks/platform_views_layout with this change. That test has an animated container that should cause this value to change after each frame https://github.com/flutter/flutter/blob/195a1cc413bde5f7c1d194204608c4bc20659124/dev/benchmarks/platform_views_layout/lib/main.dart#L121

Copy link
Contributor Author

@cyanglaz cyanglaz May 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I ran the benchmarks, there doesn't seem to be significant difference:

With my change:

  "average_frame_build_time_millis": 0.2156956521739131,
  "90th_percentile_frame_build_time_millis": 0.27,
  "99th_percentile_frame_build_time_millis": 0.316,
  "worst_frame_build_time_millis": 0.326,
  "missed_frame_build_budget_count": 0,
  "average_frame_rasterizer_time_millis": 4.833215053763439,
  "90th_percentile_frame_rasterizer_time_millis": 6.668,
  "99th_percentile_frame_rasterizer_time_millis": 7.84,
  "worst_frame_rasterizer_time_millis": 9.612,
  "missed_frame_rasterizer_budget_count": 0,

master:

  "average_frame_build_time_millis": 0.21664516129032269,
  "90th_percentile_frame_build_time_millis": 0.286,
  "99th_percentile_frame_build_time_millis": 0.305,
  "worst_frame_build_time_millis": 0.357,
  "missed_frame_build_budget_count": 0,
  "average_frame_rasterizer_time_millis": 4.918989130434783,
  "90th_percentile_frame_rasterizer_time_millis": 6.905,
  "99th_percentile_frame_rasterizer_time_millis": 8.33,
  "worst_frame_rasterizer_time_millis": 9.434,
  "missed_frame_rasterizer_budget_count": 0,

Copy link

@blasten blasten May 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did you see any issue in general? if you run the app, and play with it? e.g. Could this change introduce jank or glitches?

Copy link
Contributor Author

@cyanglaz cyanglaz May 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, I didn't see this change introduce new janks or glitches.

Copy link
Contributor Author

@cyanglaz cyanglaz May 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

friendly ping @blasten
Did you have other suggestions/comments?

Copy link

@blasten blasten May 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no more suggestions. Thanks for looking!

toView:overlay_view_wrapper];
// Set a unique view identifier, so the overlay_view can be identified in XCUITests.
overlay_view.accessibilityIdentifier =
[NSString stringWithFormat:@"platform_view[%lld].overlay_view[%lld]", view_id, overlay_id];

std::unique_ptr<SurfaceFrame> frame = layer->surface->AcquireFrame(frame_size_);
// If frame is null, AcquireFrame already printed out an error message.
Expand All @@ -625,9 +636,6 @@
}
SkCanvas* overlay_canvas = frame->SkiaCanvas();
overlay_canvas->clear(SK_ColorTRANSPARENT);
// Offset the picture since its absolute position on the scene is determined
// by the position of the overlay view.
overlay_canvas->translate(-rect.x(), -rect.y());
overlay_canvas->drawPicture(picture);

layer->did_submit_last_frame = frame->Submit();
Expand Down
65 changes: 63 additions & 2 deletions shell/platform/darwin/ios/framework/Source/FlutterPlatformViewsTest.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -979,11 +979,11 @@ - (void)testFlutterPlatformViewControllerSubmitFrameWithoutFlutterViewNotCrashin
std::make_unique<flutter::EmbeddedViewParams>(finalMatrix, SkSize::Make(300, 300), stack);
flutterPlatformViewsController->PrerollCompositeEmbeddedView(2, std::move(embeddedViewParams_2));
flutterPlatformViewsController->CompositeEmbeddedView(2);
auto mock_surface_submit_false = std::make_unique<flutter::SurfaceFrame>(
auto mock_surface_submit_true = std::make_unique<flutter::SurfaceFrame>(
nullptr, framebuffer_info,
[](const flutter::SurfaceFrame& surface_frame, SkCanvas* canvas) { return true; });
XCTAssertTrue(flutterPlatformViewsController->SubmitFrame(nullptr, nullptr,
std::move(mock_surface_submit_false)));
std::move(mock_surface_submit_true)));
}

- (void)
Expand Down Expand Up @@ -1088,6 +1088,67 @@ - (void)testFlutterPlatformViewControllerBeginFrameShouldResetCompisitionOrder {
XCTAssertEqual(flutterPlatformViewsController->GetCurrentCanvases().size(), 1UL);
}

- (void)testThreadMergeAtEndFrame {
flutter::FlutterPlatformViewsTestMockPlatformViewDelegate mock_delegate;
auto thread_task_runner_platform = CreateNewThread("FlutterPlatformViewsTest1");
auto thread_task_runner_other = CreateNewThread("FlutterPlatformViewsTest2");
flutter::TaskRunners runners(/*label=*/self.name.UTF8String,
/*platform=*/thread_task_runner_platform,
/*raster=*/thread_task_runner_other,
/*ui=*/thread_task_runner_other,
/*io=*/thread_task_runner_other);
auto flutterPlatformViewsController = std::make_shared<flutter::FlutterPlatformViewsController>();
auto platform_view = std::make_unique<flutter::PlatformViewIOS>(
/*delegate=*/mock_delegate,
/*rendering_api=*/flutter::IOSRenderingAPI::kSoftware,
/*platform_views_controller=*/flutterPlatformViewsController,
/*task_runners=*/runners);

UIView* mockFlutterView = [[[UIView alloc] initWithFrame:CGRectMake(0, 0, 500, 500)] autorelease];
flutterPlatformViewsController->SetFlutterView(mockFlutterView);

FlutterPlatformViewsTestMockFlutterPlatformFactory* factory =
[[FlutterPlatformViewsTestMockFlutterPlatformFactory new] autorelease];
flutterPlatformViewsController->RegisterViewFactory(
factory, @"MockFlutterPlatformView",
FlutterPlatformViewGestureRecognizersBlockingPolicyEager);
XCTestExpectation* waitForPlatformView =
[self expectationWithDescription:@"wait for platform view to be created"];
FlutterResult result = ^(id result) {
[waitForPlatformView fulfill];
};

flutterPlatformViewsController->OnMethodCall(
[FlutterMethodCall
methodCallWithMethodName:@"create"
arguments:@{@"id" : @2, @"viewType" : @"MockFlutterPlatformView"}],
result);
[self waitForExpectations:@[ waitForPlatformView ] timeout:30];
XCTAssertNotNil(gMockPlatformView);

flutterPlatformViewsController->BeginFrame(SkISize::Make(300, 300));
SkMatrix finalMatrix;
flutter::MutatorsStack stack;
auto embeddedViewParams =
std::make_unique<flutter::EmbeddedViewParams>(finalMatrix, SkSize::Make(300, 300), stack);
flutterPlatformViewsController->PrerollCompositeEmbeddedView(2, std::move(embeddedViewParams));

fml::RefPtr<fml::RasterThreadMerger> raster_thread_merger =
fml::MakeRefCounted<fml::RasterThreadMerger>(thread_task_runner_platform->GetTaskQueueId(),
thread_task_runner_other->GetTaskQueueId());
XCTAssertEqual(flutterPlatformViewsController->PostPrerollAction(raster_thread_merger),
flutter::PostPrerollResult::kSkipAndRetryFrame);
XCTAssertFalse(raster_thread_merger->IsMerged());

flutterPlatformViewsController->EndFrame(true, raster_thread_merger);
XCTAssertTrue(raster_thread_merger->IsMerged());

// Unmerge threads before the end of the test
// TaskRunners are required to be unmerged before destruction.
while (raster_thread_merger->DecrementLease() != fml::RasterThreadStatus::kUnmergedNow)
;
}

- (int)alphaOfPoint:(CGPoint)point onView:(UIView*)view {
unsigned char pixel[4] = {0};

Expand Down
3 changes: 3 additions & 0 deletions shell/platform/darwin/ios/framework/Source/FlutterPlatformViews_Internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,9 @@ class FlutterPlatformViewsController {

PostPrerollResult PostPrerollAction(fml::RefPtr<fml::RasterThreadMerger> raster_thread_merger);

void EndFrame(bool should_resubmit_frame,
fml::RefPtr<fml::RasterThreadMerger> raster_thread_merger);

std::vector<SkCanvas*> GetCurrentCanvases();

SkCanvas* CompositeEmbeddedView(int view_id);
Expand Down
2 changes: 1 addition & 1 deletion shell/platform/darwin/ios/ios_external_view_embedder.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@
void IOSExternalViewEmbedder::EndFrame(bool should_resubmit_frame,
fml::RefPtr<fml::RasterThreadMerger> raster_thread_merger) {
TRACE_EVENT0("flutter", "IOSExternalViewEmbedder::EndFrame");
FML_CHECK(platform_views_controller_);
Copy link
Contributor

@iskakaushik iskakaushik May 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this deleted?

Copy link
Contributor Author

@cyanglaz cyanglaz May 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a CHECK, so it is essentially crashing the app if platform_views_controller_ is null, which is exactly what the next will do.

I think this check shouldn't be here at all.

Copy link
Contributor

@iskakaushik iskakaushik May 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's better to fail with a reasonable error message than null pointer dereferencing, right?

Is there a situation in which platform_views_controller_ can become null? Or do we check during initialization?

Copy link
Contributor Author

@cyanglaz cyanglaz May 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there's any reason it would happen in the current code, but it might happen if rasterizer's logic changes.

Good point to have a failing message, I will add the check back and add a failing message.

Copy link
Contributor Author

@cyanglaz cyanglaz May 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about it but not sure what additional message to add that would be useful. We assume platform_views_controller_ is not null here and it crashes if it is null.

I could add something like "platform_views_controller_ must not be null" but it is redundant as crashing at latform_views_controller_->EndFrame(should_resubmit_frame, raster_thread_merger); would self-explained it.

Copy link
Contributor Author

@cyanglaz cyanglaz May 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

friendly ping @iskakaushik

platform_views_controller_->EndFrame(should_resubmit_frame, raster_thread_merger);
}

// |ExternalViewEmbedder|
Expand Down
70 changes: 70 additions & 0 deletions testing/scenario_app/ios/Scenarios/ScenariosUITests/UnobstructedPlatformViewTests.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

#import <XCTest/XCTest.h>

static const CGFloat kCompareAccuracy = 0.001;

@interface UnobstructedPlatformViewTests : XCTestCase

@end
Expand Down Expand Up @@ -60,6 +62,15 @@ - (void)testOneOverlay {
XCTAssertEqual(overlay.frame.origin.y, 150);
XCTAssertEqual(overlay.frame.size.width, 50);
XCTAssertEqual(overlay.frame.size.height, 50);

XCUIElement* overlayView = app.otherElements[@"platform_view[0].overlay_view[0]"];
XCTAssertTrue(overlayView.exists);
// Overlay should always be the same frame as the app.
XCTAssertEqualWithAccuracy(overlayView.frame.origin.x, app.frame.origin.x, kCompareAccuracy);
XCTAssertEqualWithAccuracy(overlayView.frame.origin.y, app.frame.origin.x, kCompareAccuracy);
XCTAssertEqualWithAccuracy(overlayView.frame.size.width, app.frame.size.width, kCompareAccuracy);
XCTAssertEqualWithAccuracy(overlayView.frame.size.height, app.frame.size.height,
kCompareAccuracy);
}

// A is the layer above the platform view.
Expand All @@ -86,6 +97,15 @@ - (void)testOneOverlayPartialIntersection {
XCTAssertEqual(overlay.frame.size.width, 50);
// Half the height of the overlay.
XCTAssertEqual(overlay.frame.size.height, 25);

XCUIElement* overlayView = app.otherElements[@"platform_view[0].overlay_view[0]"];
XCTAssertTrue(overlayView.exists);
// Overlay should always be the same frame as the app.
XCTAssertEqualWithAccuracy(overlayView.frame.origin.x, app.frame.origin.x, kCompareAccuracy);
XCTAssertEqualWithAccuracy(overlayView.frame.origin.y, app.frame.origin.x, kCompareAccuracy);
XCTAssertEqualWithAccuracy(overlayView.frame.size.width, app.frame.size.width, kCompareAccuracy);
XCTAssertEqualWithAccuracy(overlayView.frame.size.height, app.frame.size.height,
kCompareAccuracy);
}

// A and B are the layers above the platform view.
Expand Down Expand Up @@ -149,6 +169,24 @@ - (void)testOneOverlayAndTwoIntersectingOverlays {
XCTAssertEqual(overlay2.frame.origin.y, 225);
XCTAssertEqual(overlay2.frame.size.width, 50);
XCTAssertEqual(overlay2.frame.size.height, 50);

XCUIElement* overlayView0 = app.otherElements[@"platform_view[0].overlay_view[0]"];
XCTAssertTrue(overlayView0.exists);
// Overlay should always be the same frame as the app.
XCTAssertEqualWithAccuracy(overlayView0.frame.origin.x, app.frame.origin.x, kCompareAccuracy);
XCTAssertEqualWithAccuracy(overlayView0.frame.origin.y, app.frame.origin.x, kCompareAccuracy);
XCTAssertEqualWithAccuracy(overlayView0.frame.size.width, app.frame.size.width, kCompareAccuracy);
XCTAssertEqualWithAccuracy(overlayView0.frame.size.height, app.frame.size.height,
kCompareAccuracy);

XCUIElement* overlayView1 = app.otherElements[@"platform_view[0].overlay_view[0]"];
XCTAssertTrue(overlayView1.exists);
// Overlay should always be the same frame as the app.
XCTAssertEqualWithAccuracy(overlayView1.frame.origin.x, app.frame.origin.x, kCompareAccuracy);
XCTAssertEqualWithAccuracy(overlayView1.frame.origin.y, app.frame.origin.x, kCompareAccuracy);
XCTAssertEqualWithAccuracy(overlayView1.frame.size.width, app.frame.size.width, kCompareAccuracy);
XCTAssertEqualWithAccuracy(overlayView1.frame.size.height, app.frame.size.height,
kCompareAccuracy);
}

// A is the layer, which z index is higher than the platform view.
Expand Down Expand Up @@ -179,6 +217,8 @@ - (void)testMultiplePlatformViewsWithoutOverlays {

XCTAssertFalse(app.otherElements[@"platform_view[0].overlay[0]"].exists);
XCTAssertFalse(app.otherElements[@"platform_view[1].overlay[0]"].exists);
XCTAssertFalse(app.otherElements[@"platform_view[0].overlay_view[0]"].exists);
XCTAssertFalse(app.otherElements[@"platform_view[1].overlay_view[0]"].exists);
}

// A is the layer above both platform view.
Expand Down Expand Up @@ -220,6 +260,24 @@ - (void)testMultiplePlatformViewsWithOverlays {
XCTAssertEqual(overlay2.frame.origin.y, 25);
XCTAssertEqual(overlay2.frame.size.width, 200);
XCTAssertEqual(overlay2.frame.size.height, 250);

XCUIElement* overlayView0 = app.otherElements[@"platform_view[0].overlay_view[0]"];
XCTAssertTrue(overlayView0.exists);
// Overlay should always be the same frame as the app.
XCTAssertEqualWithAccuracy(overlayView0.frame.origin.x, app.frame.origin.x, kCompareAccuracy);
XCTAssertEqualWithAccuracy(overlayView0.frame.origin.y, app.frame.origin.x, kCompareAccuracy);
XCTAssertEqualWithAccuracy(overlayView0.frame.size.width, app.frame.size.width, kCompareAccuracy);
XCTAssertEqualWithAccuracy(overlayView0.frame.size.height, app.frame.size.height,
kCompareAccuracy);

XCUIElement* overlayView1 = app.otherElements[@"platform_view[1].overlay_view[0]"];
XCTAssertTrue(overlayView1.exists);
// Overlay should always be the same frame as the app.
XCTAssertEqualWithAccuracy(overlayView1.frame.origin.x, app.frame.origin.x, kCompareAccuracy);
XCTAssertEqualWithAccuracy(overlayView1.frame.origin.y, app.frame.origin.x, kCompareAccuracy);
XCTAssertEqualWithAccuracy(overlayView1.frame.size.width, app.frame.size.width, kCompareAccuracy);
XCTAssertEqualWithAccuracy(overlayView1.frame.size.height, app.frame.size.height,
kCompareAccuracy);
}

// More then two overlays are merged into a single layer.
Expand All @@ -246,6 +304,18 @@ - (void)testPlatformViewsMaxOverlays {
XCTAssertTrue(overlay.exists);
XCTAssertFalse(app.otherElements[@"platform_view[0].overlay[1]"].exists);
XCTAssertTrue(CGRectContainsRect(platform_view.frame, overlay.frame));

XCUIElement* overlayView0 = app.otherElements[@"platform_view[0].overlay_view[0]"];
XCTAssertTrue(overlayView0.exists);
// Overlay should always be the same frame as the app.
XCTAssertEqualWithAccuracy(overlayView0.frame.origin.x, app.frame.origin.x, kCompareAccuracy);
XCTAssertEqualWithAccuracy(overlayView0.frame.origin.y, app.frame.origin.x, kCompareAccuracy);
XCTAssertEqualWithAccuracy(overlayView0.frame.size.width, app.frame.size.width, kCompareAccuracy);
XCTAssertEqualWithAccuracy(overlayView0.frame.size.height, app.frame.size.height,
kCompareAccuracy);

XCUIElement* overlayView1 = app.otherElements[@"platform_view[0].overlay_view[1]"];
XCTAssertFalse(overlayView1.exists);
}

@end