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

cyanglaz · 2022-05-11T16:51:31Z

This PR makes the OverlayView(which is masked by OverlayWrapper) to be at the correct frame. The new frame of the OverlayView is the same as the FlutterView. Because the Overlay is at the correct frame, we don't need to add a transform matrix to the skia canvas, so that skia doesn't draw off canvas.

This somehow fixes the flickering issue in flutter/flutter#103076, I'm not sure the exact reason behind it but it seems like it is related to Skia trying to draw off canvas.

Makes the thread merging happen at the end of the last frame on raster thread. This prevents a new frame starts on the raster thread before the thread merging is completed.

This fixes some occasional unsynchronized frame, also observed in flutter/flutter#103076

I have observed another thread synchronization issue during thread un-merging. It seems a little more complicated so I'll create a separate PR for it .

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

flutter-dashboard · 2022-05-11T16:52:14Z

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 (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

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

cyanglaz · 2022-05-11T19:32:08Z

@zanderso Here's the fix for the PlatformView flickering issue. How do mark this to be a CP candidate?

zanderso · 2022-05-11T19:33:07Z

Use the template here https://github.com/flutter/flutter/issues/new?assignees=&labels=cp%3A+review&template=7_cherry_pick.yml&title=%5BCP%5D+%3Ctitle%3E

zanderso · 2022-05-11T19:35:36Z

This will be easier to approve for a cherry-pick if it can have a test.

cyanglaz · 2022-05-11T19:36:57Z

This will be easier to approve for a cherry-pick if it can have a test.

I'm adding tests now. Building the test locally took some time and I just put the PR up before it :)

cyanglaz · 2022-05-11T21:08:09Z

@iskakaushik Do you mind take a look at code for both update?
@blasten Do you mind take a look at the code, specifically related to unobstructed platform views?

blasten · 2022-05-11T22:25:31Z

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

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


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

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,

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?

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

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

no more suggestions. Thanks for looking!

cyanglaz · 2022-05-12T19:43:06Z

@zanderso Looks like the original issue on the app was magically fixed and the issue was closed. Since it is not a regression and requires a specific device/steps to reproduce, I would suggest to not cherry-pick. WDTY?

zanderso · 2022-05-12T19:51:30Z

I'm not sure that I trust that it was magically fixed. Is there a version of the app where it repros locally for you without your fix?

draft test code format revert revert testing code cleanup clea nup revert scenario test for overlay view frame test thread merge at end frame add scenario tests fix typo

cyanglaz · 2022-05-12T20:15:53Z

I'm not sure that I trust that it was magically fixed. Is there a version of the app where it repros locally for you without your fix?

I wasn't able to repro the same exact issue they were experiencing. Not with local build, flutter stable/master, nor the TestFlight build.

Mine looks a little bit different as shown in flutter/flutter#103076 and it repros even in stable. Their video capture, however, does look like some sort of syncing issue between PlatformView and Overlay. Since the fix on the thread merging in this PR fixed the flickering issue that I was experiencing (caused by frames not synchronized during thread merging), I guess the same code might fix the issue they saw.

I asked them to test their app with this PR and see if it fixes the glitch that they saw. but it seem they couldn't reproduce the issue anymore.

zanderso · 2022-05-12T21:40:25Z

Got it. Not sure I completely follow the timeline, but if the issue was possibly introduced by the 120Hz change, then we should queue this up for a cherry-pick. Otherwise, that is if it's just a fix for a pre-existing issue, then I don't think we need a cherry-pick.

iskakaushik · 2022-05-12T21:41:55Z

This wasn't introduced by the 120hz work, rendering at high frame rates only made a previously present issue more apparent. I was able to reproduce this with the 120hz change reverted as well.

cyanglaz · 2022-05-12T23:00:03Z

Got it. Not sure I completely follow the timeline, but if the issue was possibly introduced by the 120Hz change, then we should queue this up for a cherry-pick. Otherwise, that is if it's just a fix for a pre-existing issue, then I don't think we need a cherry-pick.

Yes, the issue was not related to 120Hz. The issue happens randomly, I think it is a little more reproducible with 120hz enabled just because we draw more frames with 120Hz.
And sounds good, let's not cherry-pick this. I'm going to lower the priority of the issue.

cyanglaz · 2022-05-12T23:02:45Z

Also note that the issue only happens on iPhone 13pro. I'm not sure why, but It is possible that Skia drawing outside of the canvas worked in previous iPhones but not the iPhone 13pro.

zanderso · 2022-05-13T19:49:01Z

Chatted with folks offline. It sounds like a cherry-pick isn't needed. Is this ready to land, though?

cyanglaz · 2022-05-16T17:21:57Z

friendly ping @iskakaushik for review

iskakaushik · 2022-05-18T16:46:53Z

shell/platform/darwin/ios/ios_external_view_embedder.mm

 void IOSExternalViewEmbedder::EndFrame(bool should_resubmit_frame,
                                       fml::RefPtr<fml::RasterThreadMerger> raster_thread_merger) {
  TRACE_EVENT0("flutter", "IOSExternalViewEmbedder::EndFrame");
-  FML_CHECK(platform_views_controller_);


why is this deleted?

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.

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?

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.

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.

friendly ping @iskakaushik

iskakaushik · 2022-05-18T16:48:51Z

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

    // 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);


why is this removed?

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

iskakaushik

LGTM

jmagman · 2022-05-31T21:16:09Z

@cyanglaz is this ready to merge?

blasten

LGTM

…ead at the endFrame (flutter/engine#33262)

…he endFrame (flutter#33262)

flutter-dashboard bot added the platform-ios label May 11, 2022

flutter-dashboard bot added the needs tests label May 11, 2022

cyanglaz force-pushed the platform_view_overlay_frame branch from cae8d76 to 63ed36f Compare May 11, 2022 16:53

cyanglaz requested review from blasten and iskakaushik May 11, 2022 19:31

blasten reviewed May 11, 2022

View reviewed changes

Chris Yang added 2 commits May 12, 2022 13:00

thraed merge to end frame

5720b8a

draft test code format revert revert testing code cleanup clea nup revert scenario test for overlay view frame test thread merge at end frame add scenario tests fix typo

typo

eb94a6b

cyanglaz force-pushed the platform_view_overlay_frame branch from 56eb0de to eb94a6b Compare May 12, 2022 20:07

cyanglaz mentioned this pull request May 12, 2022

[iOS] Create Explicit Transation Avoid Implicit Checker #33225

Closed

8 tasks

cyanglaz mentioned this pull request May 12, 2022

iOS: PlatformView pixel glitches when going underneath a widget (iPhone 13 pro) flutter/flutter#103076

Closed

iskakaushik reviewed May 18, 2022

View reviewed changes

iskakaushik approved these changes May 26, 2022

View reviewed changes

blasten approved these changes May 31, 2022

View reviewed changes

cyanglaz merged commit 6dfbd54 into flutter:main May 31, 2022

cyanglaz deleted the platform_view_overlay_frame branch May 31, 2022 21:42

engine-flutter-autoroll mentioned this pull request May 31, 2022

Roll Flutter Engine from a5eefc80f9f9 to 6dfbd5464f1e (16 revisions) flutter/flutter#105070

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 31, 2022

6dfbd54 PlatformView to set the correct overlay view frame, merge thr…

6f2607c

…ead at the endFrame (flutter/engine#33262)

houhuayong pushed a commit to houhuayong/engine that referenced this pull request Jun 21, 2022

PlatformView to set the correct overlay view frame, merge thread at t…

595a613

…he endFrame (flutter#33262)

cyanglaz mentioned this pull request Aug 5, 2022

[Impeller] Flickering at edges of embedded view while scrolling. flutter/flutter#109007

Closed

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

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

Uh oh!

Conversation

cyanglaz commented May 11, 2022

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented May 11, 2022

Uh oh!

cyanglaz commented May 11, 2022

Uh oh!

zanderso commented May 11, 2022

Uh oh!

zanderso commented May 11, 2022

Uh oh!

cyanglaz commented May 11, 2022

Uh oh!

cyanglaz commented May 11, 2022

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 May 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zanderso commented May 12, 2022

Uh oh!

cyanglaz commented May 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zanderso commented May 12, 2022

Uh oh!

iskakaushik commented May 12, 2022

Uh oh!

cyanglaz commented May 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cyanglaz commented May 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zanderso commented May 13, 2022

Uh oh!

cyanglaz commented May 16, 2022

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!

iskakaushik left a comment

Choose a reason for hiding this comment

Uh oh!

jmagman commented May 31, 2022

Uh oh!

blasten left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

cyanglaz commented May 12, 2022 •

edited

Loading

cyanglaz commented May 12, 2022 •

edited

Loading

cyanglaz commented May 12, 2022 •

edited

Loading

cyanglaz commented May 12, 2022 •

edited

Loading