Bug 227633 - [macOS] Only use WebGL on Metal by default on safe versions of macOS
Summary: [macOS] Only use WebGL on Metal by default on safe versions of macOS
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-07-02 11:51 PDT by Brent Fulgham
Modified: 2021-07-04 18:28 PDT (History)
10 users (show)

See Also:


Attachments
Patch (2.07 KB, patch)
2021-07-02 11:59 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2021-07-02 11:51:17 PDT
Some older supported macOS releases have a version of Metal that does not work properly for implementing WebGL features. We should stick to the OpenGL backend on those releases (by default) and only activate Metal WebGL on systems we know pass all tests.

Users can still activate WebGL on Metal if they wish, but it will not be the default behavior.

<rdar://problem/78527887>
Comment 1 Brent Fulgham 2021-07-02 11:59:00 PDT
Created attachment 432809 [details]
Patch
Comment 2 Dean Jackson 2021-07-02 13:09:20 PDT
Comment on attachment 432809 [details]
Patch

Won't this disable it on watchOS, tvOS and Catalyst?
Comment 3 Brent Fulgham 2021-07-02 14:13:40 PDT
(In reply to Dean Jackson from comment #2)
> Comment on attachment 432809 [details]
> Patch
> 
> Won't this disable it on watchOS, tvOS and Catalyst?

#if PLATFORM(IOS_FAMILY) || (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 120000)
#define HAVE_WEBGL_COMPATIBLE_METAL 1
#endif

This should ENABLE it for watchOS, tvOS, Catalyst, and iPhone, and on any macOS at or above 120000.

I THINK!?!
Comment 4 Brent Fulgham 2021-07-02 14:21:47 PDT
I asked Tim to double-check and he said it looked right.
Comment 5 EWS 2021-07-02 16:21:09 PDT
Committed r279524 (239374@main): <https://commits.webkit.org/239374@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 432809 [details].
Comment 6 Kenneth Russell 2021-07-04 18:22:43 PDT
Comment on attachment 432809 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=432809&action=review

> Source/WTF/wtf/PlatformHave.h:1097
> +#if PLATFORM(IOS_FAMILY) || (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 120000)

Could you explain a bit more how these compile time guards work? I would assume that runtime checks would be needed in order to choose the Metal backend only when running on certain macOS versions.
Comment 7 Tim Horton 2021-07-04 18:28:25 PDT
(In reply to Kenneth Russell from comment #6)
> Comment on attachment 432809 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=432809&action=review
> 
> > Source/WTF/wtf/PlatformHave.h:1097
> > +#if PLATFORM(IOS_FAMILY) || (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 120000)
> 
> Could you explain a bit more how these compile time guards work? I would
> assume that runtime checks would be needed in order to choose the Metal
> backend only when running on certain macOS versions.

No need for a runtime check; we build separate binaries with the appropriate SDK for each OS, so a check like this is fine (and very common).
Comment 8 Tim Horton 2021-07-04 18:28:25 PDT
(In reply to Kenneth Russell from comment #6)
> Comment on attachment 432809 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=432809&action=review
> 
> > Source/WTF/wtf/PlatformHave.h:1097
> > +#if PLATFORM(IOS_FAMILY) || (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 120000)
> 
> Could you explain a bit more how these compile time guards work? I would
> assume that runtime checks would be needed in order to choose the Metal
> backend only when running on certain macOS versions.

No need for a runtime check; we build separate binaries with the appropriate SDK for each OS, so a check like this is fine (and very common).