Enable Metal backend for ANGLE, add non-spirvX compiler path for GLSL to Metal translation

• Component: WebGL
• Status: RESOLVED FIXED

[Kyle Piddington]

Enable Metal backend for ANGLE, add non-spirvX compiler path for GLSL to Metal translation

[Kyle Piddington]

Created attachment 415937 [details]
Patch

[EWS Watchlist]

Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE

[Radar WebKit Bug Importer]

<rdar://problem/72200222>

[Kyle Piddington]

Created attachment 415958 [details]
Patch

[Kyle Piddington]

Created attachment 416012 [details]
Patch

[Kyle Piddington]

Created attachment 416046 [details]
Patch

[Kenneth Russell]

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!

[Dean Jackson]

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).

[Dean Jackson]

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.

[Kenneth Russell]

Thanks Dean - all sounds great!

[Dean Jackson]

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.

[Dean Jackson]

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 :)

[Dean Jackson]

Comment on attachment 416046 [details]
Patch

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

[Dean Jackson]

Committed r270733: <https://trac.webkit.org/changeset/270733>