[Impeller] Ensure that overlay surfaces are constructed with wide gamut settings. #48190

jonahwilliams · 2023-11-17T22:28:38Z

When constructing overlay layers when there are platform views, make sure the same pixel format and color space as the main view is used.

Add validation to impeller HAL about blitting different pixel formats.

gaaclarke · 2023-11-17T22:35:18Z

impeller/entity/entity_pass.cc

+    // If the context supports blitting and the pixel formats are identical,
+    // blit the offscreen texture to the onscreen texture. Otherwise, draw it to
+    // the parent texture using a pipeline (slower).
+    auto src_format = offscreen_target.GetRenderTarget()


Seems like the problem is that the pixel format for the offscreen_target does not match the root_render_target. Can't we just make that the case?

Can't we just make that the case

If we dont need the offscreen to be wide gamut, we probably shouldn't take the performance hit, no?

Oh I see, we do need it. that is what I fixed. We should discuss this next week. If essentially turning wide gamut is really a 4x performance improvement for backdrop blurs then we shouldn't have it on by default

Oh I see, we do need it. that is what I fixed.

No, I meant instead of making a drawing call to replace a blit making sure the blit is always possible.

WDYM? If the texture formats don't match the blit isn't possible, that is what I'm doing. There is still a blit if they match.

That offscreen texture is one that we create, right? So we should be specifying that it has the same format as the final surface. That's how this stuff originally worked as far as I recall.

CreateOffscreen has always used Capabilities.GetDefaultColorFormat(); Do we update that if the app switches into wide gamut?

If we're not doing that, then I think pretty much anything else in the entity_pass will start failing, because it doesn't seem like we've accounted for backdrop filters in with the wide gamut support?

It's been a while since I implemented it. I thought I made sure offscreen buffers were created to match the surface pixel format. If that wasn't the case it seems like we'd see some nasty bugs. At any rate, I think that is the correct fix. Fix the setup of the offscreen buffers, not create a workaround to account for them being setup incorrectly (with added runtime cost).

Investigated this and the offscreen format is updated to match, but the overlay surfaces introduced by platforms were not, which is the result of the mismatch.

bdero

Yeah this makes sense. Blitting between differently formatted Textures is never valid for Metal. Sometimes this seems to work on newer iOS devices for some format combinations, but some devices may end up outputting fuchsia... LGTM!

gaaclarke

Requesting changes until we resolve the question if we can make the blit always work.

gaaclarke · 2023-11-17T22:56:26Z

impeller/entity/entity_pass.cc

+    // If the context supports blitting and the pixel formats are identical,
+    // blit the offscreen texture to the onscreen texture. Otherwise, draw it to
+    // the parent texture using a pipeline (slower).
+    auto src_format = offscreen_target.GetRenderTarget()


Oh I see, we do need it. that is what I fixed.

No, I meant instead of making a drawing call to replace a blit making sure the blit is always possible.

jonahwilliams · 2023-11-18T02:16:40Z

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

-      overlay_view.reset([[FlutterOverlayView alloc] init]);
-      overlay_view_wrapper.reset([[FlutterOverlayView alloc] init]);
+      CGFloat screenScale = [UIScreen mainScreen].scale;
+      overlay_view.reset([[FlutterOverlayView alloc] initWithContentsScale:screenScale


Not clear to me why these weren't setting screen scale?

Also these branches seem pretty much identical...

jonahwilliams · 2023-11-20T17:59:23Z

Despite the failed builds all tests are now passing locally...

jonahwilliams · 2023-11-20T18:00:36Z

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

  if (available_layer_index_ >= layers_.size()) {
    std::shared_ptr<FlutterPlatformViewLayer> layer;
    fml::scoped_nsobject<UIView> overlay_view;
    fml::scoped_nsobject<UIView> overlay_view_wrapper;

-    if (!gr_context) {
+    bool impeller_enabled = !!ios_context->GetImpellerContext();
+    if (!gr_context && !impeller_enabled) {


It seems like the intention of this code was to run when the software backend was enabled, but it was running with Impeller enabled. I don't even know who to FYI, maybe @dnfield @chinmaygarde ?

… into fix_wide_gamut_blit

jonahwilliams · 2023-11-21T17:47:39Z

ping @gaaclarke / @chinmaygarde

gaaclarke

I think this needs an integration test. We have something similar in flutter/dev/integration_tests/wide_gamut_test. Maybe that could just be expanded to create the problem case with platform views and backdrop filters and look for the display p3 red?

This stuff is really easy to break on accident since it requires thinking of multiple flows through the code (wide gamut vs srgb).

impeller/renderer/blit_pass_unittests.cc

impeller/renderer/blit_pass.cc

impeller/renderer/blit_pass_unittests.cc

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

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

gaaclarke · 2023-11-21T17:53:39Z

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

@@ -43,6 +45,10 @@ - (UIScreen*)screen {
  return UIScreen.mainScreen;
 }

+- (MTLPixelFormat)pixelFormat {
+  return _pixelFormat;


Do we need a new ivar? Can't we use self.layer.pixelFormat?

the layer ivar isn't exposed, this seemed more restricted.

Right, you should keep your accessor. Just use:

- (MTLPixelFormat)pixelFormat { return self.layer.pixelFormat; }

By duplicating this value we are opening ourselves up to bugs where the layer's pixel format does not match the FlutterView's pixel format.

Fixed, I think!

gaaclarke

LGTM, modulo removing the duplicated ivar and filing an issue for a device lab tests, since it can't be included here.

jonahwilliams · 2023-11-21T19:24:05Z

Integration test is: flutter/flutter#138837

gaaclarke

thanks, lgtm!

bdero · 2023-11-21T20:56:47Z

Great investigation :)

…h wide gamut settings. (flutter/engine#48190)

…138844) flutter/engine@3179986...90d1ff0 2023-11-21 [email protected] Make `{shell|lib/ui}/...` compatible with `.clang_tidy`. (flutter/engine#48242) 2023-11-21 [email protected] [Impeller] Ensure that overlay surfaces are constructed with wide gamut settings. (flutter/engine#48190) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

mark8044 · 2023-11-21T23:13:53Z

@jonahwilliams
Hi, it looks like this is now merged into master, I'm using the following build:

Flutter 3.17.0-13.0.pre.15 • channel master • https://github.com/flutter/flutter.git
Framework • revision ba433ebb65 (39 minutes ago) • 2023-11-21 17:31:16 -0500
Engine • revision a8e9b8dd56
Tools • Dart 3.3.0 (build 3.3.0-152.0.dev) • DevTools 2.30.0-dev.4

IF SO:
It looks like this does in fact fix the GPU rendering crash we discussed here flutter/flutter#138615

HOWEVER, the app is very slow with alot of jank when scrolling through platform views (I'm using flutter web view to present YouTube videos within a listview). Once the view is scrolled off screen, the jank then becomes minimal

The previous builds of master, using FLTEnableWideGamut off was super fast with only minimal to no jank through the same views.

<key>FLTEnableWideGamut</key>
<false/>

jonahwilliams · 2023-11-21T23:23:05Z

does turning wide gamut off not improve things after this change?

mark8044 · 2023-11-22T00:44:29Z

does turning wide gamut off not improve things after this change?

Doesn't seem to do much anymore or maybe a minor improvement. But jank is there when scrolling though the webview. The page has some backdrop filters on it

jonahwilliams · 2023-11-22T01:08:39Z

The performance improvement you observed was likely a fluke caused by the rendering error then.

mark8044 · 2023-11-22T01:50:37Z

The performance improvement you observed was likely a fluke caused by the rendering error then.

Darn, well I enjoyed it while it lasted :)
Still some ways to go with jank it seems

mark8044 · 2023-11-22T01:57:14Z

Here's a strange but highly consistent oddity.

There is 0 jank (smooth as silk) when using the App on an older device (iPhone X) running on iOS 16 (16.2)

The jank however is consistent and easily reproducible on a newer iPhone 13 running on iOS 17

jonahwilliams · 2023-11-22T02:10:30Z

Is that due to a frame rate difference? 60hz vs 90/120hz? Depending on the model that might be the case. If you are barely hitting 60 fps, the high frame rate device isn't literally twice as fast so you may miss frames at a higher frame rate.

In any case, please consider adding more information such as your own timeline traces (from main) onto the existing issue and I'll investigate more next week.

In flutter/engine#48190 I discovered that overlay surfaces were not constructed with wide gamut settings. This adds a test that will fail until this is fixed.

Reverts #138837 Initiated by: jonahwilliams This change reverts the following previous change: Original Description: In flutter/engine#48190 I discovered that overlay surfaces were not constructed with wide gamut settings. This adds a test that will fail until this is fixed.

…lutter#138844) flutter/engine@3179986...90d1ff0 2023-11-21 [email protected] Make `{shell|lib/ui}/...` compatible with `.clang_tidy`. (flutter/engine#48242) 2023-11-21 [email protected] [Impeller] Ensure that overlay surfaces are constructed with wide gamut settings. (flutter/engine#48190) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

In flutter/engine#48190 I discovered that overlay surfaces were not constructed with wide gamut settings. This adds a test that will fail until this is fixed.

) Reverts flutter#138837 Initiated by: jonahwilliams This change reverts the following previous change: Original Description: In flutter/engine#48190 I discovered that overlay surfaces were not constructed with wide gamut settings. This adds a test that will fail until this is fixed.

[Impeller] dont blit onto wide gamut surface.

4ce1db5

github-actions bot added the e: impeller label Nov 17, 2023

jonahwilliams mentioned this pull request Nov 17, 2023

[Impeller] Janky scrolling with backdrop filter blur on 3.16. flutter/flutter#138615

Closed

1 task

jonahwilliams requested review from bdero and gaaclarke November 17, 2023 22:32

gaaclarke reviewed Nov 17, 2023

View reviewed changes

bdero approved these changes Nov 17, 2023

View reviewed changes

add excluded file.

55272b4

gaaclarke requested changes Nov 17, 2023

View reviewed changes

jonahwilliams added 2 commits November 17, 2023 18:12

overlay views should have same pixel format as original view.

5beac1e

++

4d2a209

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

jonahwilliams changed the title ~~[Impeller] dont blit onto wide gamut surface.~~ [Impeller] Ensure that overlay surfaces are constructed with wide gamut settings. Nov 18, 2023

jonahwilliams commented Nov 18, 2023

View reviewed changes

jonahwilliams added 7 commits November 18, 2023 08:54

++

98c7c7e

Update FlutterPlatformViews.mm

f63e4d5

Update FlutterPlatformViews.mm

8010349

Update FlutterOverlayView.mm

075e579

++

a9a7a9c

no temp

6dd77e6

release

17b4c29

jonahwilliams requested a review from gaaclarke November 20, 2023 17:59

jonahwilliams commented Nov 20, 2023

View reviewed changes

jonahwilliams added 3 commits November 20, 2023 10:16

Merge branch 'main' into fix_wide_gamut_blit

97a732e

++

98b553f

Merge branch 'fix_wide_gamut_blit' of github.com:jonahwilliams/engine…

0e9c87d

… into fix_wide_gamut_blit

jonahwilliams requested a review from chinmaygarde November 20, 2023 21:56

gaaclarke reviewed Nov 21, 2023

View reviewed changes

review.

d98696a

gaaclarke reviewed Nov 21, 2023

View reviewed changes

remove extra ivar

6d89959

jonahwilliams mentioned this pull request Nov 21, 2023

Wide gamut and platform view integration test. flutter/flutter#138837

Merged

gaaclarke approved these changes Nov 21, 2023

View reviewed changes

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 21, 2023

auto-submit bot merged commit e993a5c into flutter:main Nov 21, 2023
28 checks passed

jonahwilliams deleted the fix_wide_gamut_blit branch November 21, 2023 20:30

engine-flutter-autoroll mentioned this pull request Nov 21, 2023

Roll Flutter Engine from 31799868224d to 90d1ff04ae02 (2 revisions) flutter/flutter#138844

Merged

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

e993a5c19 [Impeller] Ensure that overlay surfaces are constructed wit…

3d6e63f

…h wide gamut settings. (flutter/engine#48190)

jonahwilliams mentioned this pull request Nov 22, 2023

Add FlutterMetalLayer as optional alternative to CAMetalLayer #48226

Merged

auto-submit bot mentioned this pull request Nov 27, 2023

Reverts "Wide gamut and platform view integration test." flutter/flutter#139100

Merged

jonahwilliams mentioned this pull request Nov 27, 2023

[CP Request] Fix for using Impeller with multiple blurs flutter/flutter#138865

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Impeller] Ensure that overlay surfaces are constructed with wide gamut settings. #48190

[Impeller] Ensure that overlay surfaces are constructed with wide gamut settings. #48190

jonahwilliams commented Nov 17, 2023 •

edited

Loading

gaaclarke Nov 17, 2023

jonahwilliams Nov 17, 2023

jonahwilliams Nov 17, 2023

gaaclarke Nov 17, 2023

jonahwilliams Nov 17, 2023

gaaclarke Nov 17, 2023 •

edited

Loading

jonahwilliams Nov 17, 2023

gaaclarke Nov 17, 2023

jonahwilliams Nov 18, 2023

bdero left a comment

gaaclarke left a comment

gaaclarke Nov 17, 2023

jonahwilliams Nov 18, 2023

jonahwilliams Nov 18, 2023

jonahwilliams commented Nov 20, 2023

jonahwilliams Nov 20, 2023

jonahwilliams commented Nov 21, 2023

gaaclarke left a comment

gaaclarke Nov 21, 2023

jonahwilliams Nov 21, 2023

gaaclarke Nov 21, 2023

jonahwilliams Nov 21, 2023

gaaclarke left a comment

jonahwilliams commented Nov 21, 2023

gaaclarke left a comment

bdero commented Nov 21, 2023

mark8044 commented Nov 21, 2023 •

edited

Loading

jonahwilliams commented Nov 21, 2023

mark8044 commented Nov 22, 2023 •

edited

Loading

jonahwilliams commented Nov 22, 2023

mark8044 commented Nov 22, 2023 •

edited

Loading

mark8044 commented Nov 22, 2023 •

edited

Loading

jonahwilliams commented Nov 22, 2023

[Impeller] Ensure that overlay surfaces are constructed with wide gamut settings. #48190

[Impeller] Ensure that overlay surfaces are constructed with wide gamut settings. #48190

Conversation

jonahwilliams commented Nov 17, 2023 • edited Loading

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

gaaclarke Nov 17, 2023 • edited Loading

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

bdero left a comment

Choose a reason for hiding this comment

gaaclarke left a 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

jonahwilliams commented Nov 20, 2023

Choose a reason for hiding this comment

jonahwilliams commented Nov 21, 2023

gaaclarke left a 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

gaaclarke left a comment

Choose a reason for hiding this comment

jonahwilliams commented Nov 21, 2023

gaaclarke left a comment

Choose a reason for hiding this comment

bdero commented Nov 21, 2023

mark8044 commented Nov 21, 2023 • edited Loading

jonahwilliams commented Nov 21, 2023

mark8044 commented Nov 22, 2023 • edited Loading

jonahwilliams commented Nov 22, 2023

mark8044 commented Nov 22, 2023 • edited Loading

mark8044 commented Nov 22, 2023 • edited Loading

jonahwilliams commented Nov 22, 2023

jonahwilliams commented Nov 17, 2023 •

edited

Loading

gaaclarke Nov 17, 2023 •

edited

Loading

mark8044 commented Nov 21, 2023 •

edited

Loading

mark8044 commented Nov 22, 2023 •

edited

Loading

mark8044 commented Nov 22, 2023 •

edited

Loading

mark8044 commented Nov 22, 2023 •

edited

Loading