Remove #if SHELL_ENABLE_METAL checks in iOS code #51636

jmagman · 2024-03-22T23:40:33Z

All physical iOS devices Flutter supports (iOS 12+) can run Metal. The only time Flutter doesn't use Metal afaik is for iOS simulators running < iOS 13, which is covered by a few if (@available(iOS METAL_IOS_VERSION_BASELINE, *)) checks.

engine/shell/platform/darwin/ios/rendering_api_selection.h

Lines 37 to 41 in 8a51e12

    
           #if TARGET_OS_SIMULATOR 
        
           #define METAL_IOS_VERSION_BASELINE 13.0 
        
           #else 
        
           #define METAL_IOS_VERSION_BASELINE 10.0 
        
           #endif  // TARGET_OS_SIMULATOR

Remove hardware checks for physical devices.

Remove shell_enable_metal from the gn files and the #if SHELL_ENABLE_METAL checks in the iOS embedder.

I limited this PR to just iOS, but I imagine it's safe to remove shell_enable_metal everywhere?

engine/shell/platform/darwin/macos/BUILD.gn

Line 18 in 41da00a

enable_metal = shell_enable_metal

engine/tools/gn

Lines 673 to 679 in 41da00a

    
           if sys.platform == 'darwin' and args.target_os not in ['android', 'fuchsia']: 
        
             # OpenGL is deprecated on macOS > 10.11. 
        
             # This is not necessarily needed but enabling this until we have a way to 
        
             # build a macOS metal only shell and a gl only shell. 
        
             gn_args['allow_deprecated_api_calls'] = True 
        
             gn_args['skia_use_metal'] = True 
        
             gn_args['shell_enable_metal'] = True

cbracken · 2024-04-04T06:22:46Z

I imagine it's safe to remove shell_enable_metal everywhere?

I removed GL support from the macOS embedder and we don't have the software compositing fallback in that embedder, so should be safe to remove anything in the mac embedder if it still exists.

Does the iOS embedder still support software rendering? I know we used to use it for simulators, but IIRC those moved to Metal. Or was simulator Metal adoption limited to arm64 macs?

cbracken · 2024-04-04T06:24:58Z

shell/platform/darwin/ios/ios_context.mm

 #include "flutter/shell/platform/darwin/ios/ios_context_metal_impeller.h"
 #include "flutter/shell/platform/darwin/ios/ios_context_metal_skia.h"
-#endif  // SHELL_ENABLE_METAL
+#include "flutter/shell/platform/darwin/ios/ios_context_software.h"


Curious what this is still used for.

IOSContextSoftware
https://github.com/flutter/engine/blob/03cd54d4285a44e1e082ba0d1277fbee5b3f4f48/shell/platform/darwin/ios/ios_context.mm#L34C7-L34C53

cbracken

I suspect that all devices/simulators have metal support, but if so, I'd think we should be able to kill off the software rasterisation fallback. It seems a little contradictory to require metal but also support the software fallback.

lgtm modulo that question (and the question below as to whether we're still using it on simulators on intel).

jmagman · 2024-04-04T16:45:48Z

I know we used to use it for simulators, but IIRC those moved to Metal. Or was simulator Metal adoption limited to arm64 macs?

The only time Flutter doesn't use Metal afaik is for iOS simulators running < iOS 13 #17881. However, I think to run an iOS 12 simulator you need an older version of Xcode than the current one, which itself needs to be run on an older version of macOS. It is still possible though with that setup.

engine/shell/platform/darwin/ios/rendering_api_selection.h

Lines 37 to 41 in 8a51e12

    
           #if TARGET_OS_SIMULATOR 
        
           #define METAL_IOS_VERSION_BASELINE 13.0 
        
           #else 
        
           #define METAL_IOS_VERSION_BASELINE 10.0 
        
           #endif  // TARGET_OS_SIMULATOR

jmagman · 2024-04-04T16:48:28Z

The only time Flutter doesn't use Metal afaik is for iOS simulators running < iOS 13 #17881.

Oh also when software rendering is "forced" for simulators, forgot about that. Used for... scubas?

jmagman · 2024-04-04T16:55:34Z

I'd think we should be able to kill off the software rasterisation fallback

I think we may need to continue to support softrware rendering until the Skia fallback is removed in favor of Impeller. #44346

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

Lines 29 to 33 in 03cd54d

    
           FML_CHECK(backend != IOSRenderingBackend::kImpeller) 
        
               << "Software rendering is incompatible with Impeller.\n" 
        
                  "Software rendering may have been automatically selected when running on a simulator " 
        
                  "in an environment that does not support Metal. Enabling GPU pass through in your " 
        
                  "environment may fix this. If that is not possible, then disable Impeller.";

@bdero is that a true statement?

cbracken · 2024-04-04T18:11:30Z

I think to run an iOS 12 simulator you need an older version of Xcode than the current one, which itself needs to be run on an older version of macOS. It is still possible though with that setup.

Is that still a supported deployment target? If not, I'm all for scrub scrub scrub.

Oh also when software rendering is "forced" for simulators, forgot about that. Used for... scubas?

It's possible/likely it's changed since 2016 when @goderbauer and I wrote the original scuba support but I think at that point we were using the exact same underlying mechanism for screenshotting the tool uses -- i.e. wiring through the call to [NSView drawLayer:inContext:].

But now that you mention it, maybe we did force software rasterisation? Something about reproducible results? i.e. the same reason we run our GPU screenshot tests through SwiftShader in engine tests? The sad part is I think we'd need to work that out from the code, cause 2016 is definitely further back than I clearly remember, and either way, so much has changed that even if I could, the chance it still applies is near 0%.

jmagman · 2024-04-04T18:18:22Z

Is that still a supported deployment target? If not, I'm all for scrub scrub scrub.

Technically yes, iOS 12 is the minimum supported. Though it is really tricky getting that simulator installed. We can rip out support next year when Apple (likely) drops support for it.

…ngine#51636)

…146300) flutter/engine@fa4d97e...e3104af 2024-04-04 [email protected] Remove #if SHELL_ENABLE_METAL checks in iOS code (flutter/engine#51636) 2024-04-04 [email protected] Roll Skia from d58a6dbaaadb to f9dfb0308594 (1 revision) (flutter/engine#51913) 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

jmagman · 2024-04-05T18:49:59Z

Used for... scubas?

Sigh, yes, scubas. Reverting.
b/333039358

auto-submit · 2024-04-05T18:50:19Z

A reason for requesting a revert of flutter/engine/51636 could
not be found or the reason was not properly formatted. Begin a comment with 'Reason for revert:' to tell the bot why
this issue is being reverted.

jmagman · 2024-04-05T18:51:28Z

Reason for revert: This caused a slew of unexpected Scuba changes b/333039358

This reverts commit e3104af.

@available

…51941) Reverts: #51636 Initiated by: jmagman Reason for reverting: This caused a slew of unexpected Scuba changes b/333039358 Original PR Author: jmagman Reviewed By: {cbracken} This change reverts the following previous change: All physical iOS devices Flutter supports (iOS 12+) can run Metal. The only time Flutter doesn't use Metal afaik is for iOS simulators running < iOS 13, which is covered by a few `if (@available(iOS METAL_IOS_VERSION_BASELINE, *))` checks. https://github.com/flutter/engine/blob/8a51e124fbf168295a1b05f8d66459bfb29ae8a9/shell/platform/darwin/ios/rendering_api_selection.h#L37-L41 Remove hardware checks for physical devices. Remove `shell_enable_metal` from the gn files and the `#if SHELL_ENABLE_METAL` checks in the iOS embedder. I limited this PR to just iOS, but I imagine it's safe to remove `shell_enable_metal` everywhere? https://github.com/flutter/engine/blob/41da00ac46bc38a97a63ed0635450271d71afd9f/shell/platform/darwin/macos/BUILD.gn#L18 https://github.com/flutter/engine/blob/41da00ac46bc38a97a63ed0635450271d71afd9f/tools/gn#L673-L679

jonahwilliams · 2024-04-05T20:43:02Z

shell/platform/darwin/ios/rendering_api_selection.mm

@@ -18,18 +16,6 @@

 namespace flutter {

-#if SHELL_ENABLE_METAL
-bool ShouldUseMetalRenderer() {


Removing this is likely what broke the google3 test harness. Despite running on a version of iOS sim that is required to support metal - due to the lack of supported GPU passthrough (and in defiance of all that is holy), creation of the MTLDevice will fail.

Makes sense, I'll fix it up. Thank you for looking at it!

@available

All physical iOS devices Flutter supports (iOS 12+) can run Metal. The only time Flutter doesn't use Metal afaik is for iOS simulators running < iOS 13, which is covered by a few `if (@available(iOS METAL_IOS_VERSION_BASELINE, *))` checks. https://github.com/flutter/engine/blob/8a51e124fbf168295a1b05f8d66459bfb29ae8a9/shell/platform/darwin/ios/rendering_api_selection.h#L37-L41 Remove hardware checks for physical devices. Remove `shell_enable_metal` from the gn files and the `#if SHELL_ENABLE_METAL` checks in the iOS embedder. I limited this PR to just iOS, but I imagine it's safe to remove `shell_enable_metal` everywhere? https://github.com/flutter/engine/blob/41da00ac46bc38a97a63ed0635450271d71afd9f/shell/platform/darwin/macos/BUILD.gn#L18 https://github.com/flutter/engine/blob/41da00ac46bc38a97a63ed0635450271d71afd9f/tools/gn#L673-L679

Same as #51636 except: 1. The original change caused scuba changes because those simulators are running on Macs without GPU passthrough, and so fallback to software rendering when `MTLCreateSystemDefaultDevice()` fails. Re-add that `MTLCreateSystemDefaultDevice` check https://github.com/flutter/engine/pull/51636/files#diff-1db60a8a74c1cfb40c970541dd1f7f0f6301853a2ba17451f5117f154036d732L22-L31 2. Set `enable_metal` to always true for `macos/BUILD.gn` [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

…lutter#146300) flutter/engine@fa4d97e...e3104af 2024-04-04 [email protected] Remove #if SHELL_ENABLE_METAL checks in iOS code (flutter/engine#51636) 2024-04-04 [email protected] Roll Skia from d58a6dbaaadb to f9dfb0308594 (1 revision) (flutter/engine#51913) 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

Remove #if SHELL_ENABLE_METAL checks in iOS code

704d543

jmagman self-assigned this Mar 22, 2024

github-actions bot added the platform-ios label Mar 22, 2024

jmagman added 2 commits April 3, 2024 17:00

comma

334647c

Remove ShouldUseMetalRenderer

ebd56fb

jmagman marked this pull request as ready for review April 4, 2024 03:59

jmagman requested review from chinmaygarde and cbracken April 4, 2024 04:00

cbracken reviewed Apr 4, 2024

View reviewed changes

cbracken approved these changes Apr 4, 2024

View reviewed changes

jmagman added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 4, 2024

auto-submit bot merged commit e3104af into flutter:main Apr 4, 2024

engine-flutter-autoroll mentioned this pull request Apr 4, 2024

Roll Flutter Engine from fa4d97e363ff to e3104af94328 (2 revisions) flutter/flutter#146300

Merged

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

e3104af94 Remove #if SHELL_ENABLE_METAL checks in iOS code (flutter/e…

50f9189

…ngine#51636)

jmagman deleted the enable-metal branch April 4, 2024 23:32

jmagman added the revert Label used to revert changes in a closed and merged pull request. label Apr 5, 2024

auto-submit bot removed the revert Label used to revert changes in a closed and merged pull request. label Apr 5, 2024

jmagman added the revert Label used to revert changes in a closed and merged pull request. label Apr 5, 2024

auto-submit bot pushed a commit that referenced this pull request Apr 5, 2024

Revert "Remove #if SHELL_ENABLE_METAL checks in iOS code (#51636)"

6d404bf

This reverts commit e3104af.

auto-submit bot mentioned this pull request Apr 5, 2024

Reverts "Remove #if SHELL_ENABLE_METAL checks in iOS code (#51636)" #51941

Merged

auto-submit bot removed the revert Label used to revert changes in a closed and merged pull request. label Apr 5, 2024

jonahwilliams reviewed Apr 5, 2024

View reviewed changes

jmagman mentioned this pull request Apr 5, 2024

Remove #if SHELL_ENABLE_METAL checks in darwin code #51947

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove #if SHELL_ENABLE_METAL checks in iOS code #51636

Remove #if SHELL_ENABLE_METAL checks in iOS code #51636

jmagman commented Mar 22, 2024 •

edited

Loading

cbracken commented Apr 4, 2024 •

edited

Loading

cbracken Apr 4, 2024

jmagman Apr 4, 2024

cbracken left a comment

jmagman commented Apr 4, 2024

jmagman commented Apr 4, 2024

jmagman commented Apr 4, 2024

cbracken commented Apr 4, 2024 •

edited

Loading

jmagman commented Apr 4, 2024

jmagman commented Apr 5, 2024

auto-submit bot commented Apr 5, 2024

jmagman commented Apr 5, 2024

jonahwilliams Apr 5, 2024 •

edited

Loading

jmagman Apr 5, 2024

	#if TARGET_OS_SIMULATOR
	#define METAL_IOS_VERSION_BASELINE 13.0
	#else
	#define METAL_IOS_VERSION_BASELINE 10.0
	#endif // TARGET_OS_SIMULATOR

	if sys.platform == 'darwin' and args.target_os not in ['android', 'fuchsia']:
	# OpenGL is deprecated on macOS > 10.11.
	# This is not necessarily needed but enabling this until we have a way to
	# build a macOS metal only shell and a gl only shell.
	gn_args['allow_deprecated_api_calls'] = True
	gn_args['skia_use_metal'] = True
	gn_args['shell_enable_metal'] = True

Remove #if SHELL_ENABLE_METAL checks in iOS code #51636

Remove #if SHELL_ENABLE_METAL checks in iOS code #51636

Conversation

jmagman commented Mar 22, 2024 • edited Loading

cbracken commented Apr 4, 2024 • edited Loading

cbracken Apr 4, 2024

Choose a reason for hiding this comment

jmagman Apr 4, 2024

Choose a reason for hiding this comment

cbracken left a comment

Choose a reason for hiding this comment

jmagman commented Apr 4, 2024

jmagman commented Apr 4, 2024

jmagman commented Apr 4, 2024

cbracken commented Apr 4, 2024 • edited Loading

jmagman commented Apr 4, 2024

jmagman commented Apr 5, 2024

auto-submit bot commented Apr 5, 2024

jmagman commented Apr 5, 2024

jonahwilliams Apr 5, 2024 • edited Loading

Choose a reason for hiding this comment

jmagman Apr 5, 2024

Choose a reason for hiding this comment

jmagman commented Mar 22, 2024 •

edited

Loading

cbracken commented Apr 4, 2024 •

edited

Loading

cbracken commented Apr 4, 2024 •

edited

Loading

jonahwilliams Apr 5, 2024 •

edited

Loading