RESOLVED FIXED 219759
Enable Metal backend for ANGLE, add non-spirvX compiler path for GLSL to Metal translation
https://bugs.webkit.org/show_bug.cgi?id=219759
Summary Enable Metal backend for ANGLE, add non-spirvX compiler path for GLSL to Meta...
Kyle Piddington
Reported 2020-12-10 15:13:14 PST
Enable Metal backend for ANGLE, add non-spirvX compiler path for GLSL to Metal translation
Attachments
Patch (1.43 MB, patch)
2020-12-10 15:31 PST, Kyle Piddington
no flags
Patch (2.72 MB, patch)
2020-12-10 18:53 PST, Kyle Piddington
no flags
Patch (2.72 MB, patch)
2020-12-11 10:46 PST, Kyle Piddington
ews-feeder: commit-queue-
Patch (2.74 MB, patch)
2020-12-11 14:13 PST, Kyle Piddington
dino: review+
ews-feeder: commit-queue-
Kyle Piddington
Comment 1 2020-12-10 15:31:05 PST
EWS Watchlist
Comment 2 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
Radar WebKit Bug Importer
Comment 3 2020-12-10 15:34:55 PST
Kyle Piddington
Comment 4 2020-12-10 18:53:18 PST
Kyle Piddington
Comment 5 2020-12-11 10:46:19 PST
Kyle Piddington
Comment 6 2020-12-11 14:13:35 PST
Kenneth Russell
Comment 7 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!
Dean Jackson
Comment 8 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).
Dean Jackson
Comment 9 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.
Kenneth Russell
Comment 10 2020-12-11 15:57:53 PST
Thanks Dean - all sounds great!
Dean Jackson
Comment 11 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.
Dean Jackson
Comment 12 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 :)
Dean Jackson
Comment 13 2020-12-12 03:35:17 PST
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
Comment 14 2020-12-12 03:45:26 PST
Note You need to log in before you can comment on or make changes to this bug.