[Impeller] allowing enabling Impeller on macOS. #42639

jonahwilliams · 2023-06-07T20:10:04Z

Ship it?

flutter-dashboard · 2023-06-07T20:10:07Z

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.

jonahwilliams · 2023-06-07T20:10:19Z

shell/platform/embedder/embedder_surface_metal_impeller.h

@@ -62,6 +62,9 @@ class EmbedderSurfaceMetalImpeller final : public EmbedderSurface,
  // |GPUSurfaceMetalDelegate|
  bool PresentTexture(GPUMTLTextureInfo texture) const override;

+  // |EmbedderSurface|
+  std::shared_ptr<impeller::Context> CreateImpellerContext() const override;


Need to override this so we get a context for image decoding.

jonahwilliams · 2023-06-07T20:10:33Z

shell/platform/embedder/embedder.h

+  /// If true, the Impeller rendering backend will be enabled. It is the callers
+  /// responsibility to verify that Impeller supports the currently selected
+  /// backend.
+  bool enable_impeller;


TBD what sort of promise we want to make with this flag.

I thought the idea was to loudly crash if someone asks for impeller and it can't work. Ideally also stop things in the tooling if they ask for it on an unsupported platform.

That is how things already work.

I missed this on initial review. I don't think we should add this flag because it will inevitably become outdated. Perhaps just allow it to be enabled through command_line_argv?

I agree with @bdero. This is a flag that will only be valid in the interim during the transition. Piping it through the command line args makes more sense to me. We will also be able to flip the default at our convenience.

bdero · 2023-06-07T20:22:04Z

No strong opinions about landing this flag, but this is double ultra unstable until we get the embedder unittest harness working with Impeller.

jonahwilliams · 2023-06-07T20:22:37Z

Yeah we can wait to talk about this one a bit. Though we can keep it a bit more stable by enabling a CI machine with a simple unit test

…er_macos

loic-sharma · 2023-06-07T21:29:25Z

shell/platform/darwin/macos/framework/Source/FlutterEngine.mm

@@ -491,6 +491,7 @@ - (BOOL)runWithEntrypoint:(NSString*)entrypoint {

  FlutterProjectArgs flutterArguments = {};
  flutterArguments.struct_size = sizeof(FlutterProjectArgs);
+  flutterArguments.enable_impeller = _project.enableImpeller;


Should we set enable_impeller to false explicitly on Windows and Linux?

I don't think it matters. We'll keep the default false until we have all backends wired up and tested

dnfield · 2023-06-08T16:48:18Z

shell/platform/embedder/embedder_surface_metal_impeller.mm

@@ -75,6 +75,10 @@
  return surface;
 }

+std::shared_ptr<impeller::Context> EmbedderSurfaceMetalImpeller::CreateImpellerContext() const {
+  return context_;


(this is fine because it's set up in the ctor)

zanderso · 2023-06-08T20:08:10Z

From PR triage: This will wait for any feedback from @chinmaygarde when he is back from OOO.

jonahwilliams · 2023-06-12T19:08:54Z

shell/platform/darwin/macos/framework/Source/FlutterEngine.mm

@@ -481,6 +481,13 @@ - (BOOL)runWithEntrypoint:(NSString*)entrypoint {
  // The first argument of argv is required to be the executable name.
  std::vector<const char*> argv = {[self.executableName UTF8String]};
  std::vector<std::string> switches = self.switches;
+
+  // Enable Impeller only if specifically asked for from the project or cmdline arguments.
+  if (_project.enableImpeller ||


Moved to only using switches

jonahwilliams · 2023-06-12T19:09:08Z

shell/platform/embedder/embedder_surface_metal_impeller.mm

@@ -49,6 +49,7 @@
      std::make_shared<fml::SyncSwitch>(false),  // is_gpu_disabled_sync_switch
      "Impeller Library"                         // library_label
  );
+  FML_LOG(ERROR) << "Using the Impeller rendering backend (Metal).";


Added an error log here to match the other backends

jonahwilliams · 2023-06-12T19:09:23Z

embedder.h API change reverted.

…gine#42639)

…128738) flutter/engine@12def73...1714d73 2023-06-12 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from FreETK3TrhkNzCCL-... to J-zU9HGYXYU5UWJO9... (flutter/engine#42784) 2023-06-12 jonahwilliams@google.com [Impeller] allowing enabling Impeller on macOS. (flutter/engine#42639) Also rolling transitive DEPS: fuchsia/sdk/core/mac-amd64 from FreETK3TrhkN to J-zU9HGYXYU5 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 jacksongardner@google.com,rmistry@google.com,zra@google.com 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

cbracken

Fantastic! 🚀

zanderso

LGTM

jonahwilliams added 3 commits June 7, 2023 13:05

fix plumbing for impeller context, add FLTEnableImpeller

9a2ab40

++

caa1f4a

++

a9e3fa9

jonahwilliams requested review from cbracken and chinmaygarde as code owners June 7, 2023 20:10

flutter-dashboard bot added the needs tests label Jun 7, 2023

jonahwilliams commented Jun 7, 2023

View reviewed changes

bdero approved these changes Jun 7, 2023

View reviewed changes

Merge branch 'master' of github.com:flutter/engine into enable_impell…

73a2e22

…er_macos

loic-sharma reviewed Jun 7, 2023

View reviewed changes

dnfield reviewed Jun 8, 2023

View reviewed changes

dnfield approved these changes Jun 8, 2023

View reviewed changes

just use switches.

6bcb25c

jonahwilliams commented Jun 12, 2023

View reviewed changes

github-actions bot added the platform-macos label Jun 12, 2023

jonahwilliams commented Jun 12, 2023

View reviewed changes

dnfield approved these changes Jun 12, 2023

View reviewed changes

jonahwilliams mentioned this pull request Jun 12, 2023

[flutter_tools] pass through enable impeller flag to macOS. flutter/flutter#128720

Merged

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

chinmaygarde approved these changes Jun 12, 2023

View reviewed changes

auto-submit bot merged commit c142c0d into flutter:main Jun 12, 2023
30 checks passed

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

Roll Flutter Engine from 12def739b1f6 to c142c0d05563 (1 revision) flutter/flutter#128728

Closed

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

c142c0d05 [Impeller] allowing enabling Impeller on macOS. (flutter/en…

11e14ff

…gine#42639)

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

Roll Flutter Engine from 12def739b1f6 to 1714d73e681b (2 revisions) flutter/flutter#128738

Merged

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

c142c0d05 [Impeller] allowing enabling Impeller on macOS. (flutter/en…

8d25cea

…gine#42639)

cbracken reviewed Jun 13, 2023

View reviewed changes

zanderso reviewed Jun 20, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Impeller] allowing enabling Impeller on macOS. #42639

[Impeller] allowing enabling Impeller on macOS. #42639

jonahwilliams commented Jun 7, 2023

flutter-dashboard bot commented Jun 7, 2023

jonahwilliams Jun 7, 2023

jonahwilliams Jun 7, 2023

dnfield Jun 8, 2023

jonahwilliams Jun 8, 2023

bdero Jun 8, 2023

chinmaygarde Jun 12, 2023

jonahwilliams Jun 12, 2023

bdero commented Jun 7, 2023

jonahwilliams commented Jun 7, 2023

loic-sharma Jun 7, 2023

jonahwilliams Jun 8, 2023

dnfield Jun 8, 2023

zanderso commented Jun 8, 2023

jonahwilliams Jun 12, 2023

jonahwilliams Jun 12, 2023

jonahwilliams commented Jun 12, 2023

cbracken left a comment

zanderso left a comment

[Impeller] allowing enabling Impeller on macOS. #42639

[Impeller] allowing enabling Impeller on macOS. #42639

Conversation

jonahwilliams commented Jun 7, 2023

flutter-dashboard bot commented Jun 7, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdero commented Jun 7, 2023

jonahwilliams commented Jun 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zanderso commented Jun 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahwilliams commented Jun 12, 2023

cbracken left a comment

Choose a reason for hiding this comment

zanderso left a comment

Choose a reason for hiding this comment