Bug 219759 - Enable Metal backend for ANGLE, add non-spirvX compiler path for GLSL to Metal translation
Summary: Enable Metal backend for ANGLE, add non-spirvX compiler path for GLSL to Meta...
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
Keywords: InRadar
Depends on: 220129
Blocks: anglemetal 220137
  Show dependency treegraph
Reported: 2020-12-10 15:13 PST by Kyle Piddington
Modified: 2021-01-06 14:52 PST (History)
8 users (show)

See Also:

Patch (1.43 MB, patch)
2020-12-10 15:31 PST, Kyle Piddington
no flags Details | Formatted Diff | Diff
Patch (2.72 MB, patch)
2020-12-10 18:53 PST, Kyle Piddington
no flags Details | Formatted Diff | Diff
Patch (2.72 MB, patch)
2020-12-11 10:46 PST, Kyle Piddington
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (2.74 MB, patch)
2020-12-11 14:13 PST, Kyle Piddington
dino: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle Piddington 2020-12-10 15:13:14 PST
Enable Metal backend for ANGLE, add non-spirvX compiler path for GLSL to Metal translation
Comment 1 Kyle Piddington 2020-12-10 15:31:05 PST
Created attachment 415937 [details]
Comment 2 EWS Watchlist 2020-12-10 15:32:36 PST
Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE
Comment 3 Radar WebKit Bug Importer 2020-12-10 15:34:55 PST
Comment 4 Kyle Piddington 2020-12-10 18:53:18 PST
Created attachment 415958 [details]
Comment 5 Kyle Piddington 2020-12-11 10:46:19 PST
Created attachment 416012 [details]
Comment 6 Kyle Piddington 2020-12-11 14:13:35 PST
Created attachment 416046 [details]
Comment 7 Kenneth Russell 2020-12-11 14:33:02 PST
This is amazing work! A couple of comments:

- Does this really enable the Metal backend for ANGLE in WebKit by default? If so, could I suggest doing that in a separate patch? That way if it needs to be turned off due to any regressions, this large patch won't be reverted.

- While the ANGLE update instructions in WebKit say to update changes.diff, at this point that file is automatically regenerated when ANGLE is rolled into WebKit via the Tools/Scripts/update-angle script. Including the changes in changes.diff doubles the size of this already-large patch, and makes it harder to review, so would you consider skipping that step? (Assuming doing so is OK with the other teammates)

- Could you document in this bug description and in the ChangeLog more precisely what this patch implements? It looks like you've implemented support for transform feedback (fantastic!) - it would be really great to document that.

- Would you consider uploading this patch upstream to the ANGLE project per https://github.com/google/angle/blob/master/doc/ContributingCode.md ? The ANGLE developers would love to work with you to get this work integrated upstream.

Will add more comments on the patch itself. Thanks!
Comment 8 Dean Jackson 2020-12-11 15:54:20 PST
We won't enable this by default yet. I've already added an Internal setting for this, which means developers can toggle it (but not people using Safari Technology Preview).
Comment 9 Dean Jackson 2020-12-11 15:56:03 PST
We'll also post this to ANGLE as soon as possible, which is where the official review will happen. Landing it here is our way of testing it on more systems as it is officially integrated into ANGLE, and then copied back to WebKit.
Comment 10 Kenneth Russell 2020-12-11 15:57:53 PST
Thanks Dean - all sounds great!
Comment 11 Dean Jackson 2020-12-12 03:01:05 PST
Seems like the patch is too big for the review tool. Here are some comments:

- don't include changes.diff (Ken mentioned this)
- We can probably leave the complete list of files out of the ChangeLog
- The GCC_PREPROCESSOR_DEFINITIONS should go in the .xcconfig files
- I don't think we should change ANGLE's ASSERT macro
- src/common/platform.h checks for iOS < 13, but I don't believe we support that any more in WebKit
- src/common/utilities.cpp should probably keep the clang #pragma

I'll fix some of these.
Comment 12 Dean Jackson 2020-12-12 03:19:59 PST
This patch broke depth textures on iOS because it removed a bit of code that was intentionally not checking for some required depth formats (because iOS OpenGL doesn't support them). I expect this backend does support them, so we'll need to make this a runtime decision based on the backend. However, it should be ok to continue NOT checking :)
Comment 13 Dean Jackson 2020-12-12 03:35:17 PST
Comment on attachment 416046 [details]

Before landing I:

- Fixed the depth texture and other failure
- removed changes.diff
- Put the ANGLE_ENABLE_METAL preprocessor definition in the configuration files
- Reverted the changes to ANGLE's ASSERT
- Reverted the change to the #pragma clang warning
Comment 14 Dean Jackson 2020-12-12 03:45:26 PST
Committed r270733: <https://trac.webkit.org/changeset/270733>