Bug 220076 (anglemetal) - Enable Metal ANGLE backend for WebGL
Summary: Enable Metal ANGLE backend for WebGL
Status: RESOLVED FIXED
Alias: anglemetal
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dean Jackson
URL:
Keywords: InRadar
Depends on: 220896 223921 223926 219759 220129 220137 220877 220895 222239 222331 222624 223627 223667 223739 223767 223778 223922 223923 223925
Blocks: 224257
  Show dependency treegraph
 
Reported: 2020-12-21 15:46 PST by Dean Jackson
Modified: 2021-04-06 21:19 PDT (History)
19 users (show)

See Also:


Attachments
Patch (25.96 KB, patch)
2020-12-21 15:49 PST, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (8.58 KB, patch)
2021-02-20 15:29 PST, Dean Jackson
sam: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
EWS Test (48.73 KB, patch)
2021-03-03 14:05 PST, Dean Jackson
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Another EWS test (51.53 KB, patch)
2021-03-06 06:07 PST, Dean Jackson
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
EWS Test (54.03 KB, patch)
2021-03-19 18:31 PDT, Dean Jackson
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
EWS Test 5 or so (48.73 KB, patch)
2021-03-22 15:26 PDT, Dean Jackson
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
EWS Test 6 (48.87 KB, patch)
2021-03-23 13:31 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2020-12-21 15:46:39 PST
Enable Metal ANGLE backend for WebGL 1
Comment 1 Radar WebKit Bug Importer 2020-12-21 15:47:23 PST
<rdar://problem/72565020>
Comment 2 Dean Jackson 2020-12-21 15:49:54 PST
Created attachment 416641 [details]
Patch
Comment 3 Dean Jackson 2020-12-21 15:52:05 PST
Comment on attachment 416641 [details]
Patch

Not for review. Currently fails a bunch of tests.

e.g. webgl/1.0.3/conformance/attribs/gl-disabled-vertex-attrib.html
Comment 4 Dean Jackson 2020-12-21 15:52:29 PST
Also has some extra code in the patch that should be a separate patch.
Comment 5 Dean Jackson 2020-12-21 16:54:19 PST
The tests pass when run in MiniBrowser, but do not pass when run via WebKitTestRunner.

The failures are usually related to readPixels getting fully white output.

It does call into gl::ReadnPixelsRobustANGLE, but Xcode doesn't want to show me the values of local variables after that call.

e.g. webgl/1.0.3/conformance/attribs/gl-disabled-vertex-attrib.html

Loops through all the vertex attributes

Attribute 0
draw a green quad
before readPixels 0 0 0 0
after readPixels 0 255 0 255

OK!

Attribute 1
draw a green quad
before readPixels 0 0 0 0
after readPixels 255 255 255 255

FAIL!

(this doesn't make sense since the canvas is cleared to black, and the shader should draw red if the attribute wasn't correct)
Comment 6 Dean Jackson 2020-12-21 16:55:07 PST
So it looks like the first readPixels works, but then it fails from then on, which explains why only some tests are failing.
Comment 7 Dean Jackson 2020-12-21 17:23:50 PST
A simple test calling readPixels twice works though.
Comment 8 Dean Jackson 2020-12-21 17:47:25 PST
Hmmm.. maybe it is only tests that use bindAttribLocation.

In fact, fast/canvas/webgl/gl-bind-attrib-location-test.html is failing in MiniBrowser.

(Some of my confusion comes from the fact that the EGLDisplay is reused across contexts, so the code in this patch to allow WebGL1 and 2 with different backends is not actually working.)
Comment 9 Dean Jackson 2020-12-21 17:48:58 PST
It seems EGL_GetPlatformDisplayEXT returns the existing display even if the parameters are different.
Comment 10 Dean Jackson 2020-12-21 17:54:27 PST
Yep. Display::GetDisplayFromNativeDisplay can only have one display per native display, and only allows the display to be configured once.

This blocks us from using different backends in the same page (process, even).
Comment 11 Dean Jackson 2020-12-21 17:55:28 PST
The ANGLEPlatformDisplayMap uses the native display as the key.
Comment 12 Kenneth Russell 2020-12-23 14:52:59 PST
Something seems broken in the Metal backend related to vertex attributes and possibly binding attribute locations. Tests including:

webgl/1.0.3/conformance/attribs/gl-disabled-vertex-attrib.html
webgl/1.0.3/conformance/attribs/gl-vertex-attrib-render.html
webgl/1.0.3/conformance/attribs/gl-vertex-attrib-zero-issues.html

fail when run in WK1 in the MiniBrowser with the Metal backend.
Comment 13 Kenneth Russell 2020-12-23 23:43:58 PST
Filed Bug 220137, blocking this one, about the webgl/1.0.3/conformance/attribs/ failures. That is core functionality and those bugs must be fixed before turning on the Metal backend by default.
Comment 14 Kenneth Russell 2021-01-08 16:16:02 PST
Great work on fixing the attribute location conformance failures in Bug 220137!

There are several other more core conformance failures. Let's file sub-bugs about them.

We're also in the process of upstreaming Apple's large contributions to ANGLE's Metal backend. It would be easier to make future progress if we completed that upstreaming and rolled ANGLE into WebKit before making more changes in WebKit.
Comment 15 Dean Jackson 2021-01-23 13:55:16 PST
Blocked on 220877
Comment 16 Dean Jackson 2021-01-23 14:07:48 PST
And b220895
Comment 17 Dean Jackson 2021-01-23 14:09:52 PST
And finally 220896, which is taking the upstream back to WebKit.
Comment 18 Dean Jackson 2021-02-20 15:29:06 PST
Created attachment 421117 [details]
Patch
Comment 19 Dean Jackson 2021-02-20 16:37:39 PST
I am waiting because I think this will change some iOS results, but I don't have an SDK that can launch the simulator right now.
Comment 20 Dean Jackson 2021-02-21 11:52:09 PST
Still to do:

- avoid crashes on Catalina
- update iOS results (I can't currently test locally)


The Catalina crashes appear to be:
source texture pixelFormat (MTLPixelFormatRG8Unorm) not compatible with texture view pixelFormat (MTLPixelFormatBGRA8Unorm)
Comment 21 Dean Jackson 2021-03-03 14:05:20 PST
Created attachment 422149 [details]
EWS Test
Comment 22 Kenneth Russell 2021-03-03 16:58:05 PST
Only 4 test failures, looking really good!!!
Comment 23 Kenneth Russell 2021-03-05 15:50:16 PST
The list of failures in Bug 222239 is still significant, so let's block this on it.
Comment 24 Dean Jackson 2021-03-06 06:07:29 PST
Created attachment 422488 [details]
Another EWS test
Comment 25 Dean Jackson 2021-03-19 18:31:28 PDT
Created attachment 423795 [details]
EWS Test
Comment 26 EWS Watchlist 2021-03-22 04:44:47 PDT
Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE
Comment 27 Dean Jackson 2021-03-22 15:26:14 PDT
Created attachment 423947 [details]
EWS Test 5 or so
Comment 28 Dean Jackson 2021-03-23 13:31:14 PDT
Created attachment 424056 [details]
EWS Test 6
Comment 29 Dean Jackson 2021-03-23 17:55:41 PDT
Committed r274927 (235685@main): <https://commits.webkit.org/235685@main>
Comment 30 Aakash Jain 2021-03-24 07:28:37 PDT
(In reply to Dean Jackson from comment #29)
> Committed r274927 (235685@main): <https://commits.webkit.org/235685@main>
This seems to have broken inspector/canvas/updateShader-webgl.html on 

History: https://results.webkit.org/?suite=layout-tests&test=inspector%2Fcanvas%2FupdateShader-webgl.html
Comment 31 Aakash Jain 2021-03-24 07:30:15 PDT
Example: https://build.webkit.org/results/Apple-BigSur-Release-WK2-Tests/r274927%20(1227)/results.html

Crash: https://build.webkit.org/results/Apple-BigSur-Release-WK2-Tests/r274927%20(1227)/inspector/canvas/updateShader-webgl-crash-log.txt

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   libobjc.A.dylib               	0x00007fff201d6d1d objc_msgSend + 29
1   com.apple.Metal               	0x00007fff28380555 -[MTLCompileOptionsInternal setPreprocessorMacros:] + 54
2   libANGLE-shared.dylib         	0x000000010ca59cad rx::mtl::CreateShaderLibrary(id<MTLDevice>, char const*, unsigned long, NSDictionary<NSString*, NSObject*>*, rx::mtl::AutoObjCPtr<NSError*>*) + 502
3   libANGLE-shared.dylib         	0x000000010ca59aad rx::mtl::CreateShaderLibrary(id<MTLDevice>, std::__1::basic_string<char, id<MTLDevice>::char_traits<char>, id<MTLDevice>::allocator<char> > const&, NSDictionary<NSString*, NSObject*>*, rx::mtl::AutoObjCPtr<NSError*>*) + 48
4   libANGLE-shared.dylib         	0x000000010ca94244 rx::ProgramMtl::createMslShaderLib(rx::mtl::Context*, gl::ShaderType, gl::InfoLog&, rx::mtl::TranslatedShaderInfo*, NSDictionary<NSString*, NSObject*>*) + 80
5   libANGLE-shared.dylib         	0x000000010ca93d53 rx::ProgramMtl::linkImplDirect(gl::Context const*, gl::ProgramLinkedResources const&, gl::InfoLog&) + 329
6   libANGLE-shared.dylib         	0x000000010ca93bd2 rx::ProgramMtl::link(gl::Context const*, gl::ProgramLinkedResources const&, gl::InfoLog&, std::__1::vector<gl::ProgramVaryingRef, std::__1::allocator<gl::ProgramVaryingRef> > const&) + 82
7   libANGLE-shared.dylib         	0x000000010ca77b13 gl::Program::linkImpl(gl::Context const*) + 1569
8   libANGLE-shared.dylib         	0x000000010ca77482 gl::Program::link(gl::Context const*) + 18
9   libANGLE-shared.dylib         	0x000000010c8fdf7f gl::Context::linkProgram(gl::ShaderProgramID) + 29
10  libANGLE-shared.dylib         	0x000000010c9663f2 gl::LinkProgram(unsigned int) + 111
11  com.apple.WebCore             	0x0000000106ae7306 WebCore::WebGLRenderingContextBase::linkProgramWithoutInvalidatingAttribLocations(WebCore::WebGLProgram*) + 102 (WebGLRenderingContextBase.cpp:3987)
12  com.apple.WebCore             	0x0000000106bb9216 operator() + 139 (InspectorShaderProgram.cpp:197) [inlined]
13  com.apple.WebCore             	0x0000000106bb9216 bool WTF::__visitor_table<WTF::Visitor<WebCore::InspectorShaderProgram::updateShader(Inspector::Protocol::Canvas::ShaderType, WTF::String const&)::$_11, WebCore::InspectorShaderProgram::updateShader(Inspector::Protocol::Canvas::ShaderType, WTF::String const&)::$_12, WebCore::InspectorShaderProgram::updateShader(Inspector::Protocol::Canvas::ShaderType, WTF::String const&)::$_13>, std::__1::reference_wrapper<WebCore::WebGLProgram>, std::__1::reference_wrapper<WebCore::WebGPUPipeline>, WTF::Monostate>::__trampoline_func<std::__1::reference_wrapper<WebCore::WebGLProgram> >(WTF::Visitor<WebCore::InspectorShaderProgram::updateShader(Inspector::Protocol::Canvas::ShaderType, WTF::String const&)::$_11, WebCore::InspectorShaderProgram::updateShader(Inspector::Protocol::Canvas::ShaderType, WTF::String const&)::$_12, WebCore::InspectorShaderProgram::updateShader(Inspector::Protocol::Canvas::ShaderType, WTF::String const&)::$_13>&, WTF::Variant<std::__1::reference_wrapper<WebCore::WebGLProgram>, std::__1::reference_wrapper<WebCore::WebGPUPipeline>, WTF::Monostate>&) + 166 (Variant.h:1873)
Comment 32 Truitt Savell 2021-03-24 10:42:01 PDT
Reverted r274927 for reason:

Broke many tests in WebGL

Committed r274948 (235705@main): <https://commits.webkit.org/235705@main>
Comment 33 Kenneth Russell 2021-03-24 13:49:30 PDT
Looks like there were multiple WebGL layout test failures in:
https://build.webkit.org/results/Apple-BigSur-Release-WK2-Tests/r274927%20(1227)/results.html

crashes:
inspector/canvas/updateShader-webgl.html
webgl/2.0.0/conformance2/textures/image_bitmap_from_image_data/tex-2d-rgba4-rgba-unsigned_byte.html

failures:
webgl/1.0.3/conformance/limits/gl-max-texture-dimensions.html
webgl/1.0.3/conformance/textures/texture-size.html
webgl/2.0.0/conformance/limits/gl-max-texture-dimensions.html
webgl/2.0.0/conformance/textures/misc/texture-size.html
Comment 34 Kenneth Russell 2021-03-24 13:49:57 PDT
Were there other failures on other bots that prompted the revert?
Comment 35 Kyle Piddington 2021-03-24 16:27:41 PDT
(In reply to Kenneth Russell from comment #34)
> Were there other failures on other bots that prompted the revert?

We saw a large number of failures on Intel machines, which prompted the revert.
Comment 36 Dean Jackson 2021-03-26 03:29:16 PDT
Committed r275088 (235798@main): <https://commits.webkit.org/235798@main>