[Impeller] Add Impeller Metal support in the embedder API #42411

bdero · 2023-05-30T10:59:17Z

Part of flutter/flutter#112230.

Now seemed like the right time to sneak this in:

We have large desktop refactors on the horizon (multi-window).
We're starting to land optimizations that complicate the surface behavior such as wide gamut and dirty regions.
We have plans to migrate the iOS embedder to use the embedder API ([iOS] Migrate to embedder API flutter#112232).

loic-sharma · 2023-05-30T18:19:27Z

/cc @dkwingsmt, this may be interesting for your macOS multi-view work.

dkwingsmt · 2023-05-30T19:57:36Z

@loic-sharma Thanks! They look good to me (in terms of multi-view macOS).

chinmaygarde

So cool! I left a few comments with nits and suggestions. But lgtm otherwise.

chinmaygarde · 2023-05-31T05:02:26Z

flow/embedded_views.cc

-void ExternalViewEmbedder::SubmitFrame(GrDirectContext* context,
-                                       std::unique_ptr<SurfaceFrame> frame) {
+void ExternalViewEmbedder::SubmitFrame(
+    GrDirectContext* context,


Ooh, that's a bit unfortunate. But we can clean this up later.

chinmaygarde · 2023-05-31T05:06:18Z

shell/platform/embedder/embedder_surface_metal_impeller.mm

+}
+
+std::unique_ptr<Surface> EmbedderSurfaceMetalImpeller::CreateGPUSurface() API_AVAILABLE(ios(13.0)) {
+  if (@available(iOS 13.0, *)) {


I don't see the point of this availability guard. There is no call here that the compiler will complain about. And we support targets below 13.0.

Whoops, this is because I just duplicated the Skia variation and modified relevant parts. Removed.

It turns out we need the check because CAMetalLayer is iOS 13+

The docs say 8.0. Perhaps its the iOS simulator?

Yeah, it's just the simulator

We should copy this sort of macro into formats_mtl and use it for the availability check. https://github.com/google/skia/blob/a8c3132457176d453c063247eaa6e2cab1cb700e/include/gpu/mtl/GrMtlTypes.h#L28

So:

#if TARGET_OS_SIMULATOR #define IMPELLER_METAL_AVAILABLE @available(macos(10.11), ios(13.0)) #else // TARGET_OS_SIMULATOR #define IMPELLER_METAL_AVAILABLE @available(macos(10.11), ios(8.0)) #endif // TARGET_OS_SIMULATOR

Or, have macros for IMPELLER_MACOS_METAL_VERSION, IMPELLER_IOS_METAL_VERSION set to the appropriate versions.

chinmaygarde · 2023-05-31T05:08:36Z

shell/platform/embedder/embedder_external_view.cc

+    dl_builder.SetTransform(&surface_transformation_);
+    slice_->render_into(&dl_builder);
+
+    auto dispatcher = impeller::DlDispatcher();


Aha. I've been trying to making this the default in the engine but it seems so counter to convention. My reasoning is that auto foo = FooType{}; makes it impossible to have foo uninitialized.

But the dl_builder above uses the other style? Just use the same style everywhere?

Done. I also prefer auto foo = ..., but tend to heavily bias towards whatever's nearby.

chinmaygarde · 2023-05-31T05:09:55Z

shell/platform/embedder/embedder_render_target.cc

+    FML_DCHECK(aiks_context_);
+    FML_DCHECK(!render_surface_);
+  } else {
+    FML_CHECK(render_surface_);


DCHECK turned to CHECK. Perhaps keep them the same?

chinmaygarde · 2023-05-31T05:15:31Z

shell/platform/embedder/BUILD.gn

        "embedder_external_texture_metal.mm",
        "embedder_surface_metal.h",
        "embedder_surface_metal.mm",
+        "embedder_surface_metal_impeller.h",


For testing, (in a later patch is fine) I suggest converting the impeller::EmbedderTest fixture to be a parameterized fixture with the impeller parameterization setting up a context object with the right flag set. Then in the setup, you can skip the test on non-supported backends. The updates should be minimal. Likely just patching the TEST_F calls and instantiating the suite. But you'd double your test coverage.

Just a thought. And we can attempt this in a follow up patch.

bdero · 2023-05-31T17:49:20Z

We're still not building Impeller with Fuchsia. I'm pulling on that thread now that the AiksContext is being lugged around everywhere.

chinmaygarde · 2023-05-31T18:14:20Z

now that the AiksContext is being lugged around everywhere.

We've resorted to forward declaring these. See the rasterizer.

bdero · 2023-05-31T18:39:02Z

We've resorted to forward declaring these.

Yes that seems like the smart choice. I didn't have to pull very far for a sense of foreboding to wash over me.

…lutter/engine#42411)

…128089) flutter/engine@3a453f2...02d6fbb 2023-06-01 [email protected] [Impeller] Emplace directly into host buffer (avoid VBB) for text data (flutter/engine#42484) 2023-06-01 [email protected] Ensure PlatformView engine life cycle callbacks are invoked (flutter/engine#42491) 2023-06-01 [email protected] Roll Skia from c408e8e9cc96 to 082a7d1f72f7 (8 revisions) (flutter/engine#42496) 2023-06-01 [email protected] [Windows - TextInput] Insert new line only when TextInputAction.newline (flutter/engine#42244) 2023-06-01 [email protected] Revert "Move clang tidy v2 build to prod." (flutter/engine#42495) 2023-06-01 [email protected] Add myself to AUTHORS (flutter/engine#42406) 2023-06-01 [email protected] [Impeller] Add Impeller Metal support in the embedder API (flutter/engine#42411) 2023-06-01 [email protected] Support DisposalMethod::kRestorePrevious in MultiFrameCodec and fix the apng problem. (flutter/engine#42153) 2023-06-01 [email protected] Fix crash getting spell-check suggestions (flutter/engine#42466) 2023-06-01 [email protected] Fix lint in rectangle packer (flutter/engine#42489) 2023-06-01 [email protected] Wait for GL command completion in the ExternalTextureGLRefreshedTooOften test (flutter/engine#42438) 2023-06-01 [email protected] Reland "[web] Remove the JS API for url strategy (#42134)" (flutter/engine#42486) 2023-06-01 [email protected] Roll Skia from f5bc3d12f0eb to c408e8e9cc96 (9 revisions) (flutter/engine#42487) 2023-06-01 [email protected] Clean up Skia includes around SkSurfaceCharacterization (flutter/engine#42485) 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…utter#42411)" This reverts commit 052e226.

…2411)" (#42532) This reverts commit 052e226. Expected to resolve flutter/flutter#128145

…utter#42411) This reverts commit 05f958c.

…2411) (#42545) This reverts commit 05f958c. Original PR: #42411 Part of flutter/flutter#112230.

… API (#42411) (#42545)" This reverts commit 2b353ae.

… API (#42411)" (#42593) Reverts #42545 Not sure if this is needed, but getting a revert started for the regression noted here: #42545 (comment).

…lutter#42411) This reverts commit 350a12b.

…42411) (#42597) This reverts commit 350a12b + fixes bugs. Reworked the surface wrapping to use a valid descriptor when wrapping the drawable.

bdero self-assigned this May 30, 2023

flutter-dashboard bot added embedder Related to the embedder API platform-android platform-ios labels May 30, 2023

bdero changed the title ~~[Impeller] Add Impeller support in the embedder API~~ [Impeller] Add Impeller Metal support in the embedder API May 30, 2023

bdero force-pushed the bdero/impeller-embedder branch from d108909 to 2f0eac6 Compare May 30, 2023 11:40

bdero force-pushed the bdero/impeller-embedder branch 2 times, most recently from 3fee72d to 20bc270 Compare May 31, 2023 02:16

bdero added the e: impeller label May 31, 2023

bdero requested review from chinmaygarde and zanderso May 31, 2023 02:21

bdero force-pushed the bdero/impeller-embedder branch 3 times, most recently from 028ef29 to 9521b3c Compare May 31, 2023 04:04

chinmaygarde reviewed May 31, 2023

View reviewed changes

chinmaygarde approved these changes May 31, 2023

View reviewed changes

bdero force-pushed the bdero/impeller-embedder branch 3 times, most recently from f801790 to 6d69561 Compare May 31, 2023 07:30

bdero force-pushed the bdero/impeller-embedder branch 2 times, most recently from bac7d09 to db021cd Compare June 1, 2023 06:29

bdero mentioned this pull request Jun 1, 2023

High rate of cancelled builds on the "collect test" step for mac_clang_tidy flutter/flutter#128054

Closed

bdero added 3 commits June 1, 2023 11:54

[Impeller] Support Impeller in the Metal embedder API

a356b2c

Embedder platform view progress

3475eaf

Render target wrapping

c9bcbe2

bdero added 6 commits June 1, 2023 11:54

Fuchsia

1e4be50

Fuchsia

4b9b822

Fuchsia

e7613ef

Fuchsia

14870a2

Fuchsia

52964f6

Lint

248415c

bdero force-pushed the bdero/impeller-embedder branch from db021cd to 248415c Compare June 1, 2023 18:55

chinmaygarde added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 1, 2023

auto-submit bot merged commit 052e226 into flutter:main Jun 1, 2023

engine-flutter-autoroll mentioned this pull request Jun 1, 2023

Roll Flutter Engine from 3a453f2ccb77 to 59dfac7a1eda (12 revisions) flutter/flutter#128085

Closed

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

052e22661 [Impeller] Add Impeller Metal support in the embedder API (f…

abb8a60

…lutter/engine#42411)

engine-flutter-autoroll mentioned this pull request Jun 1, 2023

Roll Flutter Engine from 3a453f2ccb77 to 59afddd4cf5b (13 revisions) flutter/flutter#128087

Closed

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

052e22661 [Impeller] Add Impeller Metal support in the embedder API (f…

8369cec

…lutter/engine#42411)

engine-flutter-autoroll mentioned this pull request Jun 1, 2023

Roll Flutter Engine from 3a453f2ccb77 to 02d6fbb68b69 (14 revisions) flutter/flutter#128089

Merged

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

052e22661 [Impeller] Add Impeller Metal support in the embedder API (f…

114a378

…lutter/engine#42411)

bdero added a commit to bdero/flutter-engine that referenced this pull request Jun 3, 2023

Revert "[Impeller] Add Impeller Metal support in the embedder API (fl…

0a0c5c6

…utter#42411)" This reverts commit 052e226.

bdero added a commit to bdero/flutter-engine that referenced this pull request Jun 3, 2023

Revert "[Impeller] Add Impeller Metal support in the embedder API (fl…

0e8056e

…utter#42411)" This reverts commit 052e226.

bdero added a commit that referenced this pull request Jun 3, 2023

Revert "[Impeller] Add Impeller Metal support in the embedder API (#4…

05f958c

…2411)" (#42532) This reverts commit 052e226. Expected to resolve flutter/flutter#128145

bdero added a commit to bdero/flutter-engine that referenced this pull request Jun 4, 2023

[Impeller] Reland: Add Impeller Metal support in the embedder API (fl…

6689cb2

…utter#42411) This reverts commit 05f958c.

bdero mentioned this pull request Jun 4, 2023

[Impeller] Reland: Add Impeller Metal support in the embedder API (#42411) #42545

Merged

bdero added a commit to bdero/flutter-engine that referenced this pull request Jun 5, 2023

[Impeller] Reland: Add Impeller Metal support in the embedder API (fl…

00e0043

…utter#42411) This reverts commit 05f958c.

bdero added a commit to bdero/flutter-engine that referenced this pull request Jun 5, 2023

[Impeller] Reland: Add Impeller Metal support in the embedder API (fl…

72fb1b8

…utter#42411) This reverts commit 05f958c.

bdero added a commit that referenced this pull request Jun 5, 2023

[Impeller] Reland: Add Impeller Metal support in the embedder API (#4…

2b353ae

…2411) (#42545) This reverts commit 05f958c. Original PR: #42411 Part of flutter/flutter#112230.

zanderso added a commit that referenced this pull request Jun 6, 2023

Revert "[Impeller] Reland: Add Impeller Metal support in the embedder…

cf90132

… API (#42411) (#42545)" This reverts commit 2b353ae.

bdero added a commit to bdero/flutter-engine that referenced this pull request Jun 6, 2023

[Impeller] Reland 2: Add Impeller Metal support in the embedder API (f…

c17843d

…lutter#42411) This reverts commit 350a12b.

bdero added a commit to bdero/flutter-engine that referenced this pull request Jun 6, 2023

[Impeller] Reland 2: Add Impeller Metal support in the embedder API (f…

896852c

…lutter#42411) This reverts commit 350a12b.

bdero added a commit to bdero/flutter-engine that referenced this pull request Jun 7, 2023

[Impeller] Reland 2: Add Impeller Metal support in the embedder API (f…

ec5d0aa

…lutter#42411) This reverts commit 350a12b.

[Impeller] Add Impeller Metal support in the embedder API #42411

[Impeller] Add Impeller Metal support in the embedder API #42411

Uh oh!

Conversation

bdero commented May 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

loic-sharma commented May 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dkwingsmt commented May 30, 2023

Uh oh!

chinmaygarde left a comment

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!

chinmaygarde May 31, 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!

bdero commented May 31, 2023

Uh oh!

chinmaygarde commented May 31, 2023

Uh oh!

bdero commented May 31, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bdero commented May 30, 2023 •

edited

Loading

loic-sharma commented May 30, 2023 •

edited

Loading

chinmaygarde May 31, 2023 •

edited

Loading