[Impeller] Ignore opt-outs on iOS devices. #55808

chinmaygarde · 2024-10-10T18:48:49Z

This does not remove Skia itself from the build. I'll stage that in a followup. Want to keep the scope of this patch small.

This is the new behavior:

Impeller is the default on iOS devices and simulators.
On iOS devices only, all flags and plist options are ignored.
On iOS simulators only, Flutter will used Impeller's Metal backend by default and fallback to the Null backend if Metal device access is not available.
On iOS simulators only, Flutter can pick Skia using the command line flags or plist flags. This is to allow users one more month to migrate as communicated by @zanderso.

The settings objects enable_impeller field is now static constexpr to avoid accidentally disabling the flag while it still exists.

zanderso · 2024-10-10T20:02:51Z

iOS scenario app tests are hitting:

2024-10-10 12:01:25.156206-0700 IosUnitTests[27328:171650] [FATAL:flutter/shell/common/shell.cc(444)] Check failed: !settings.enable_software_rendering || !settings.enable_impeller. Software rendering is incompatible with Impeller.

jonahwilliams

Did flutter/flutter#149652 get addressed yet?

jonahwilliams · 2024-10-10T20:03:38Z

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

-    }
-  }
+#if FML_OS_IOS_SIMULATOR
+  // Fallback to Skia which itself falls back to software rendering on simulators that cannot


FYI @zanderso what is the plan with the no-op backend?

I think we need to respect the enable impeller setting here and use the no-op backend if there is no mtl device. otherwise we're actually forcing folks to use skia that may have already opted in

Yeah, I hadn't considered that folks might have already removed the opt-out who are on simulator without Metal. Ignoring the flag and making them use Skia would be the opposite of what we're overall trying to do. Simulator should just continue to respect the flag for now. (But we should still remove the unit tests that explicitly try to target the software backend, I think?)

So, the no-op backend will automatically get selected on sims if we enable Impeller but there is no Metal device?

Yes, see: https://github.com/flutter/engine/blob/main/shell/platform/darwin/ios/ios_surface.mm#L19

chinmaygarde · 2024-10-10T21:38:03Z

Some updates based on our conversation. Please ignore the behavior I noted in the original commit message. I am going to amend it before committing.

Impeller is the default on iOS devices and simulators.
On iOS device only, all flags and plist options are ignored.
On iOS simulators only, Flutter will used Impeller's Metal backend by default and fallback to the Null backend if Metal device access is not available.
On iOS simulators only, Flutter can pick Skia using the command line flags or plist flags. This is to allow users one more month to migrate as communicated by @zanderso.

There was one test that explicitly tested opting into software rendering. That has been removed as that option was there for folks who found the OpenGL renderer too slow. This hasn't been an issue for a while and we should remove the option altogether.

Did I miss anything?

chinmaygarde · 2024-10-10T21:41:28Z

Did flutter/flutter#149652 get addressed yet?

Not yet @jonahwilliams. I'm working on it now. @zanderso wanted to get this patch in prep. and it a good thing too because this was more confusing than I initially expected.

zanderso · 2024-10-10T21:58:32Z

Still have some tests failing on simulator.

chinmaygarde · 2024-10-10T22:03:49Z

Yup, I'm patching now.

chinmaygarde · 2024-10-10T23:48:17Z

The subprocess.TimeoutExpired: Command '['xcodebuild', '-showsdks', '-json']' timed out after 300 seconds errors are really rough. I'm having to retry quite a bit.

chinmaygarde · 2024-10-11T00:10:52Z

I keep getting the same errors as #55808 (comment) on retries. Tried those builds locally and everything seems good. Hopefully unblocked once the CI issues are addressed.

zanderso · 2024-10-11T14:16:02Z

This should land today. Can the fix for flutter/flutter#149652 land as a follow-up if it is not ready yet?

jonahwilliams · 2024-10-11T16:23:31Z

We should fix the bugs we said we would fix before we remove the ability to work around them

zanderso · 2024-10-12T17:01:02Z

I keep getting the same errors as #55808 (comment) on retries. Tried those builds locally and everything seems good. Hopefully unblocked once the CI issues are addressed.

#55818 may help.

jonahwilliams

Discussed offline. We will still pursue the linked bug as a high priority followup.

Addresses flutter/flutter#155541 This does **not** remove Skia itself from the build. I'll stage that in a followup. Want to keep the scope of this patch small. This is the new behavior: * On iOS hardware **and** simulators, the engine flag (`--enable-impeller`) to toggle Impeller will be ignored. * On iOS hardware **and** simulators, the PList flag (`FLTEnableImpeller`) to toggle Impeller will be ignored. * On iOS hardware **and** simulators, the default renderer is Impeller. * On iOS simulators **only**, running on emulated environments without access to a Metal device will fallback to Skia with software rendering. The settings objects enable_impeller field is not static constexpr to avoid accidentally disabling the flag while it still exists.

)

flutter/engine@107ea8b...7509de8 2024-10-15 chinmaygarde@google.com [Impeller] Ignore opt-outs on iOS devices. (flutter/engine#55808) 2024-10-15 skia-flutter-autoroll@skia.org Roll Skia from baf314b72924 to 539ab9d39812 (3 revisions) (flutter/engine#55879) 2024-10-15 jonahwilliams@google.com [Impeller] remove extra copy from drawPoints. (flutter/engine#55872) 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 jonahwilliams@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://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

@zanderso

Addresses flutter#155541 This does **not** remove Skia itself from the build. I'll stage that in a followup. Want to keep the scope of this patch small. This is the new behavior: * Impeller is the default on iOS devices **and** simulators. * On iOS devices **only**, all flags and plist options are ignored. * On iOS simulators **only**, Flutter will used Impeller's Metal backend by default and fallback to the Null backend if Metal device access is not available. * On iOS simulators **only**, Flutter can pick Skia using the command line flags or plist flags. This is to allow users one more month to migrate as communicated by @zanderso. The settings objects `enable_impeller` field is now `static constexpr` to avoid accidentally disabling the flag while it still exists.

chinmaygarde requested review from jonahwilliams and zanderso October 10, 2024 18:48

github-actions bot added the platform-ios label Oct 10, 2024

chinmaygarde added the e: impeller label Oct 10, 2024

chinmaygarde mentioned this pull request Oct 10, 2024

Q4 Impeller iOS configuration changes flutter/flutter#155541

Closed

2 tasks

jonahwilliams suggested changes Oct 10, 2024

View reviewed changes

chinmaygarde force-pushed the ios_config_q4 branch from 925b1aa to fad9e8e Compare October 10, 2024 22:52

zanderso approved these changes Oct 11, 2024

View reviewed changes

zanderso force-pushed the ios_config_q4 branch from 9f6c7a9 to f602a3b Compare October 14, 2024 15:42

jonahwilliams approved these changes Oct 15, 2024

View reviewed changes

chinmaygarde added 2 commits October 15, 2024 10:25

PR.

16ea9a3

zanderso force-pushed the ios_config_q4 branch from f602a3b to 16ea9a3 Compare October 15, 2024 17:25

zanderso added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 15, 2024

auto-submit bot merged commit 7509de8 into flutter:main Oct 15, 2024

engine-flutter-autoroll mentioned this pull request Oct 15, 2024

Roll Flutter Engine from 107ea8baa4f5 to 7509de89d90c (3 revisions) flutter/flutter#156908

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 15, 2024

7509de8 [Impeller] Ignore opt-outs on iOS devices. (flutter/engine#55808

cd9b394

)

[Impeller] Ignore opt-outs on iOS devices. #55808

[Impeller] Ignore opt-outs on iOS devices. #55808

Uh oh!

Conversation

chinmaygarde commented Oct 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zanderso commented Oct 10, 2024

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

jonahwilliams Oct 10, 2024

Choose a reason for hiding this comment

Uh oh!

zanderso Oct 10, 2024 • 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!

chinmaygarde Oct 10, 2024

Choose a reason for hiding this comment

Uh oh!

jonahwilliams Oct 10, 2024

Choose a reason for hiding this comment

Uh oh!

chinmaygarde commented Oct 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chinmaygarde commented Oct 10, 2024

Uh oh!

zanderso commented Oct 10, 2024

Uh oh!

chinmaygarde commented Oct 10, 2024

Uh oh!

chinmaygarde commented Oct 10, 2024

Uh oh!

chinmaygarde commented Oct 11, 2024

Uh oh!

zanderso commented Oct 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonahwilliams commented Oct 11, 2024

Uh oh!

zanderso commented Oct 12, 2024

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

chinmaygarde commented Oct 10, 2024 •

edited

Loading

zanderso Oct 10, 2024 •

edited

Loading

chinmaygarde commented Oct 10, 2024 •

edited

Loading

zanderso commented Oct 11, 2024 •

edited

Loading