[Impeller] Update CAMetalLayer to be opaque #45994

jonahwilliams · 2023-09-18T20:06:24Z

this setting improves the performance of the "CPU TO Display Latency" metric in metal system trace. On high frame rate devices, this reduces the time we spend waiting for drawable availibility. This only applies if the CAMetalLayer is fullscreen, as it allows a direct to display optimization that reduces "CPU TO Display Latency" from about 21 ms down to 14ms.

Xref: flutter/flutter#134959

flutter-dashboard · 2023-09-18T20:06:27Z

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 or stuartmorgan on the #hackers channel in Chat (don't just cc them here, they won't see it! Use 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.

jonahwilliams · 2023-09-18T22:19:10Z

impeller/renderer/backend/metal/surface_mtl.mm

+    } else {
+      [command_buffer presentDrawable:drawable_];
+      [command_buffer commit];
+    }


This should be safe if we're not presenting with transaction, but needs more testing from me.

jonahwilliams · 2023-09-18T22:20:40Z

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

+    // disable opaque, which on full screen flutter apps removes the direct to display
+    // optimization. This can increase presentation time by a frame interval, which
+    // is problematic on high frame rate devices.
+    if (!opaque) {


@luckysmg , I found that if we set the background color to any value, it resets the value of layer.opaque. If you have a fullscreen flutter view and opaque is true, then we get a direct to display optimization and the presentation delay decreases from ~21ms to ~14ms, which is particularly important for high frame rate devices.

See some findings in flutter/flutter#134959

How are you using a FlutterView? Is it an add2app case where it is not the only view in the scene?

Yes. We will change the layer's opaque to make the flutter view to be transparent and present it. The scene is that the new page is the transparent page, and it is presented on current page to do some thing like modal bottom sheet.

See https://github.com/alibaba/flutter_boost/blob/13eb9aed1dff9fcdfa6066666ba08a11b231af9f/ios/Classes/container/FBFlutterViewContainer.m#L137

Also see https://github.com/alibaba/flutter_boost/blob/13eb9aed1dff9fcdfa6066666ba08a11b231af9f/ios/Classes/container/FBFlutterViewContainer.m#L221

If !opaque and set a white color, will it make the FlutterView not transparent?

ahh ok. Right now the problem is that we're unconditionally setting this clear color here. But setting the background color resets the value of opaque to NO, even if we requested it to be YES. So I think as long as your code is setting opaque = NO, this should keep working for you?

Seems not working... Here init with delegate is too early to set bg color to white.

See
https://github.com/alibaba/flutter_boost/blob/13eb9aed1dff9fcdfa6066666ba08a11b231af9f/ios/Classes/container/FBFlutterViewContainer.m#L220

If we need the view is transparent, we will set the opaque to NO and don't set bg color(in above link's code)

Before this patch: the result FlutterView is will be not opaque and with clear color, so it will be transparent ✅
After this patch: the result FlutterView is will be not opaque and with white color, so it will not be transparent ❌

So your code is starting out with opaque = YES, then you're setting it to opaque = NO later? I'm not really familiar enough with flutter_boost to follow the rendering paths there. I can change this so that if you later set opaque = NO, then we also set the background color to the clearColor?

I can change this so that if you later set opaque = NO, then we also set the background color to the clearColor?

I think this can work. But I m not sure this is good or not... Because this has a implicit operation: Change opaquealso change the bg color. Maybe sometimes will confusing the developer..

Edited: For me this is good ^_^

Yeah good point. I need to test if setting opaque after we set the background color also resets the background color too. If not, maybe I just change the order and leave this as is.

luckysmg · 2023-09-19T01:39:03Z

shell/platform/darwin/ios/ios_surface_metal_impeller.mm

+  // Note: this setting improves the performance of the "CPU TO Display Latency" metric in
+  // metal system trace. On high frame rate devices, this reduces the time we spend waiting
+  // for drawable availibility.
+  layer.opaque = YES;


Emm If we set this opaque=YES, means our the change to opaque is invalid? Since this will be called every frame.

I'll remove this

Just testing 😄

jonahwilliams · 2023-09-19T03:28:40Z

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

    layer.allowsGroupOpacity = YES;
    layer.contentsScale = screenScale;
    layer.rasterizationScale = screenScale;
-    layer.framebufferOnly = flutter::Settings::kSurfaceDataAccessible ? NO : YES;


This value should match what we set in

engine/shell/platform/darwin/ios/ios_surface_metal_impeller.mm

Line 57 in 3303a3e

layer.framebufferOnly = NO;

luckysmg · 2023-09-19T03:43:47Z

Have a locally test for current patch. The constant junk is regression. I think the patch will also impact device not on iOS16.6. 😭

normal.video.mp4

jonahwilliams · 2023-09-19T03:46:30Z

Are you setting your FlutterViews opaque to YES or are you relying on the side effect of the bgcolor setting to make it opaque?

jonahwilliams · 2023-09-19T03:47:54Z

I wouldn't consider this a fix for 16.6, the fix for that is to update to 17. But this does help, and fixes a bug since right now we're ignoring the opaque property entirely

luckysmg · 2023-09-19T03:48:18Z

I test current patch with a pure flutter example without add to app. I didn't change any code of flutter view manually

jonahwilliams · 2023-09-19T03:55:18Z

Can you run metal system trace and share a screenshot of the LCD category? For me, this is showing ~8ms faster presentations with this change.

luckysmg · 2023-09-19T03:57:35Z

Will have a check later

luckysmg · 2023-09-19T04:17:39Z

Tested on iPhone 13Pro iOS16.6.1
At begining is ok. But after scrolling the page some time it will have constant junk. Just the issue fixed in #39359

jonahwilliams · 2023-09-19T05:06:27Z

I'm testing on 17, but even on 16.6 I don't see this causing regressions - if anything its stabilizing things and I'm seeing more throughput. But event with a fast app we'll always hit a point where we're limited by drawables, given that there are only 3.

I'm not sure what to make of this right now.

jonahwilliams · 2023-09-19T05:10:07Z

If this is the case, then I suspect that setting the background color to the clearColor in #39359 fixed things for you by way of reseting the opaque bit. But I'm not sure why that would improve performance, since that seems to disable direct to display.

I'm not sure if we use that opaque bit elsewhere, but we might be in a bit of an inconsistent state because while the layer is no opaque NO, the viewcontroller things opaque is YES 🤔

luckysmg · 2023-09-19T05:25:23Z

Yes. Confusing..... I will have a recheck to check whether my code applying your patch is correct.

jonahwilliams · 2023-09-19T05:29:37Z

You might also try patching in a change that remove the backgroundColor but makes the default opaque to NO, to see if it has consistent performance. That should verify if it is the opaque field or the background color that is helping your case.

luckysmg · 2023-09-19T05:42:41Z

Have a retest.

The patch on my iPhone indeed regress the constant junk issue.
After I set the code like this.

// This line is necessary. CoreAnimation(or UIKit) may take this to do
    // something to compute the final frame presented on screen, if we don't set this,
    // it will make it take long time for us to take next CAMetalDrawable and will
    // cause constant junk during rendering. Note: setting any background color will
    // disable opaque, which on full screen flutter apps removes the direct to display
    // optimization. This can increase presentation time by a frame interval, which
    // is problematic on high frame rate devices.
//    self.backgroundColor = UIColor.clearColor;
    // Make sure to set this value after backgroundColor above, otherwise it will
    // reset the value of opaque.
    self.layer.opaque = NO;

The screen will have jitter, but behavior is different from 1 point. I think this is because my iPhone is 16.6, I think maybe this code have no jitter on other version of iOS, but I have no other version of high FPS iPhone ... Maybe I will go to find one to take a look later

luckysmg · 2023-09-19T05:49:09Z

@jonahwilliams I have a project to fire on later, not sure how long to have next response.. (just ensure you are not waiting for my resp because it is mid night in your time haha ^_^)

jonahwilliams · 2023-09-19T06:47:17Z

No worries, I'm marking this as a draft for now while I investigate other things!

luckysmg · 2023-09-20T02:39:48Z

@jonahwilliams I retest using iPhone13 Pro Max on iOS 16.0

Using the code below:

//    self.backgroundColor = UIColor.clearColor;
    self.layer.opaque = YES;

Will regress the constant junk issue fixed in #39359

Using the code in this patch: The same as 1, will also regress constant junk.
Using the code below:

//    self.backgroundColor = UIColor.clearColor;
    self.layer.opaque = NO;

The app is smooth without any junk.

Also I see the CALayer's opaque default value is NO, I m not sure whether that is apple's suggestion and indicates the performance will be better.

jonahwilliams · 2023-09-20T02:48:59Z

OK, I think we can work with this. I suspect this explains why setting the background color improved performance then, because we were defaulting to Opaque = YES, and then changing the color reset it to NO?

jonahwilliams · 2023-09-20T02:49:36Z

I'll need to do some follow ups and figure out what the official story is on Layer.opaque.

luckysmg · 2023-09-20T02:50:38Z

because we were defaulting to Opaque = YES, and then changing the color reset it to NO?

Yes. I think so..

jonahwilliams · 2023-09-28T01:02:32Z

Sorry to ping on this again @luckysmg , but what is the CPU to display latency look like if you don't have this change patched in?

luckysmg · 2023-09-28T01:14:09Z

I think without this change, it is the same in main channel, maybe you could have a try. Because I only have a iOS16.6 device, I m afraid the behavior on my device can not be referred to....

jonahwilliams · 2023-09-28T01:15:12Z

Makes sense, no worries. Still researching this 💻 🧑‍🔬 !

jonahwilliams · 2023-11-28T18:34:39Z

Closing as this seems to be a dead end.

Update CAMetalLayer to be opaque.

c7c47b6

github-actions bot added the platform-ios label Sep 18, 2023

jonahwilliams added 3 commits September 18, 2023 14:55

remove clear.

5fbd2c6

++

9c6f720

++

7d5bcb1

github-actions bot added the e: impeller label Sep 18, 2023

jonahwilliams changed the title ~~Update CAMetalLayer to be opaque.~~ Update CAMetalLayer to be opaque, remove waitUntilScheduled for background rendering. Sep 18, 2023

jonahwilliams commented Sep 18, 2023

View reviewed changes

jonahwilliams requested a review from luckysmg September 18, 2023 22:23

chinmaygarde assigned jonahwilliams Sep 19, 2023

chinmaygarde changed the title ~~Update CAMetalLayer to be opaque, remove waitUntilScheduled for background rendering.~~ [Impeller] Update CAMetalLayer to be opaque, remove waitUntilScheduled for background rendering. Sep 19, 2023

luckysmg reviewed Sep 19, 2023

View reviewed changes

jonahwilliams added 2 commits September 18, 2023 20:21

Add unittests

42909c9

revert unrelated

ee8e4c8

github-actions bot removed the e: impeller label Sep 19, 2023

jonahwilliams commented Sep 19, 2023

View reviewed changes

chinmaygarde added the e: impeller label Sep 19, 2023

jonahwilliams changed the title ~~[Impeller] Update CAMetalLayer to be opaque, remove waitUntilScheduled for background rendering.~~ [Impeller] Update CAMetalLayer to be opaque Sep 19, 2023

jonahwilliams marked this pull request as draft September 19, 2023 06:47

jonahwilliams closed this Nov 28, 2023

[Impeller] Update CAMetalLayer to be opaque #45994

[Impeller] Update CAMetalLayer to be opaque #45994

Uh oh!

Conversation

jonahwilliams commented Sep 18, 2023

Uh oh!

flutter-dashboard bot commented Sep 18, 2023

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!

luckysmg Sep 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

luckysmg commented Sep 19, 2023

Uh oh!

jonahwilliams commented Sep 19, 2023

Uh oh!

jonahwilliams commented Sep 19, 2023

Uh oh!

luckysmg commented Sep 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonahwilliams commented Sep 19, 2023

Uh oh!

luckysmg commented Sep 19, 2023

Uh oh!

luckysmg commented Sep 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonahwilliams commented Sep 19, 2023

Uh oh!

jonahwilliams commented Sep 19, 2023

Uh oh!

luckysmg commented Sep 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonahwilliams commented Sep 19, 2023

Uh oh!

luckysmg commented Sep 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luckysmg commented Sep 19, 2023

Uh oh!

jonahwilliams commented Sep 19, 2023

Uh oh!

luckysmg commented Sep 20, 2023

Uh oh!

jonahwilliams commented Sep 20, 2023

Uh oh!

jonahwilliams commented Sep 20, 2023

Uh oh!

luckysmg Sep 19, 2023 •

edited

Loading

luckysmg commented Sep 19, 2023 •

edited

Loading

luckysmg commented Sep 19, 2023 •

edited

Loading

luckysmg commented Sep 19, 2023 •

edited

Loading

luckysmg commented Sep 19, 2023 •

edited

Loading