Bug 205483

Summary: [WebGL] Enable ANGLE by default for Cocoa platforms
Product: WebKit Reporter: Dean Jackson <dino>
Component: New BugsAssignee: Dean Jackson <dino>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, benjamin, cdumez, cmarcelo, dbates, eric.carlson, esprehn+autocc, ews-watchlist, glenn, graouts, gyuyoung.kim, jer.noble, justin_fan, kbr, kondapallykalyan, philipj, sergio, simon.fraser, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=205627
https://bugs.webkit.org/show_bug.cgi?id=205662
https://bugs.webkit.org/show_bug.cgi?id=205665
https://bugs.webkit.org/show_bug.cgi?id=205664
https://bugs.webkit.org/show_bug.cgi?id=205663
https://bugs.webkit.org/show_bug.cgi?id=205666
https://bugs.webkit.org/show_bug.cgi?id=205667
https://bugs.webkit.org/show_bug.cgi?id=205668
https://bugs.webkit.org/show_bug.cgi?id=205707
https://bugs.webkit.org/show_bug.cgi?id=205756
https://bugs.webkit.org/show_bug.cgi?id=205739
https://bugs.webkit.org/show_bug.cgi?id=205738
https://bugs.webkit.org/show_bug.cgi?id=205737
https://bugs.webkit.org/show_bug.cgi?id=205736
https://bugs.webkit.org/show_bug.cgi?id=205735
https://bugs.webkit.org/show_bug.cgi?id=205734
https://bugs.webkit.org/show_bug.cgi?id=206318
Bug Depends on: 205707, 205843, 206318, 206625, 207080    
Bug Blocks: 126404, 198948, 205618, 205734, 207857, 210994    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
simon.fraser: review+
EWS test
none
EWS test 2
none
EWS test 3
none
EWS test 4
none
EWS test 5 none

Dean Jackson
Reported 2019-12-19 17:27:56 PST
[WebGL] Enable ANGLE by default for Cocoa platforms (except simulator)
Attachments
Patch (966.44 KB, patch)
2019-12-19 17:44 PST, Dean Jackson
no flags
Patch (965.99 KB, patch)
2019-12-20 09:43 PST, Dean Jackson
no flags
Patch (967.40 KB, patch)
2019-12-20 09:56 PST, Dean Jackson
no flags
Patch (626.33 KB, patch)
2019-12-20 15:55 PST, Dean Jackson
no flags
Patch (626.80 KB, patch)
2019-12-20 16:45 PST, Dean Jackson
simon.fraser: review+
EWS test (610.65 KB, patch)
2019-12-26 12:08 PST, Dean Jackson
no flags
EWS test 2 (611.73 KB, patch)
2019-12-27 13:28 PST, Dean Jackson
no flags
EWS test 3 (527.45 KB, patch)
2019-12-27 14:00 PST, Dean Jackson
no flags
EWS test 4 (492.30 KB, patch)
2019-12-27 16:41 PST, Dean Jackson
no flags
EWS test 5 (495.25 KB, patch)
2019-12-28 00:15 PST, Dean Jackson
no flags
Radar WebKit Bug Importer
Comment 1 2019-12-19 17:28:38 PST
Dean Jackson
Comment 2 2019-12-19 17:29:54 PST
Dean Jackson
Comment 3 2019-12-19 17:44:28 PST
Alexey Proskuryakov
Comment 4 2019-12-19 22:00:10 PST
Poor simulator! What is the plan for testing iOS port?
Dean Jackson
Comment 5 2019-12-20 09:39:47 PST
(In reply to Alexey Proskuryakov from comment #4) > Poor simulator! What is the plan for testing iOS port? Google are planning on allowing ANGLE to take a platform GL texture in place of an IOSurface.
Dean Jackson
Comment 6 2019-12-20 09:43:46 PST
EWS Watchlist
Comment 7 2019-12-20 09:44:51 PST
Note that there are important steps to take when updating ANGLE. See http://trac.webkit.org/wiki/UpdatingANGLE
Alexey Proskuryakov
Comment 8 2019-12-20 09:49:11 PST
Does the simulator not support something needed from IOSurface in this particular case? See bug 203026, Turn on IOSurface support in the iOS Simulator.
Alexey Proskuryakov
Comment 9 2019-12-20 09:51:31 PST
Comment on attachment 386219 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386219&action=review > LayoutTests/webgl/2.0.0/conformance/renderbuffers/framebuffer-object-attachment-expected.txt:13 > +[ 10: FAIL ] gl.checkFramebufferStatus(gl.FRAMEBUFFER) returned 36061 I haven't nearly reviewed much of the patch, but I scrolled around enough to find one regression. That would be a problem with zero regression policy.
Dean Jackson
Comment 10 2019-12-20 09:56:48 PST
Dean Jackson
Comment 11 2019-12-20 10:17:41 PST
Comment on attachment 386219 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386219&action=review >> LayoutTests/webgl/2.0.0/conformance/renderbuffers/framebuffer-object-attachment-expected.txt:13 >> +[ 10: FAIL ] gl.checkFramebufferStatus(gl.FRAMEBUFFER) returned 36061 > > I haven't nearly reviewed much of the patch, but I scrolled around enough to find one regression. That would be a problem with zero regression policy. There are some regressions, but there are many more progressions - so we've decided it is a net win. (Also, in this case, it is a regression in WebGL 2 which is not enabled by default)
Kenneth Russell
Comment 12 2019-12-20 11:26:04 PST
(In reply to Alexey Proskuryakov from comment #8) > Does the simulator not support something needed from IOSurface in this > particular case? > > See bug 203026, Turn on IOSurface support in the iOS Simulator. Ah - I hadn't noticed that! I'll try building for the iOS Simulator with the ANGLE/project.pbxproj build fix in this patch and see whether the IOSurface presentation path works.
Alexey Proskuryakov
Comment 13 2019-12-20 12:17:26 PST
Awesome! There could be more things disabled in WebKit on Simulator that can/should be enabled now.
Justin Fan
Comment 14 2019-12-20 12:18:36 PST
Comment on attachment 386222 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386222&action=review > LayoutTests/fast/canvas/webgl/array-bounds-clamping-expected.txt:1 > Checks that array access in a shader can not read out of bounds Why was this test left in but other fast/canvas tests removed?
Dean Jackson
Comment 15 2019-12-20 15:55:19 PST
Dean Jackson
Comment 16 2019-12-20 16:45:08 PST
Dean Jackson
Comment 17 2019-12-20 16:56:50 PST
Comment on attachment 386222 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386222&action=review >> LayoutTests/fast/canvas/webgl/array-bounds-clamping-expected.txt:1 >> Checks that array access in a shader can not read out of bounds > > Why was this test left in but other fast/canvas tests removed? This isn't in the WebGL conformance suite.
Dean Jackson
Comment 18 2019-12-20 16:59:48 PST
(In reply to Kenneth Russell from comment #12) > (In reply to Alexey Proskuryakov from comment #8) > > Does the simulator not support something needed from IOSurface in this > > particular case? > > > > See bug 203026, Turn on IOSurface support in the iOS Simulator. > > Ah - I hadn't noticed that! > > I'll try building for the iOS Simulator with the ANGLE/project.pbxproj build > fix in this patch and see whether the IOSurface presentation path works. Nope! It's not that the simulator doesn't have IOSurface, it's that @interface EAGLContext(IOSurface) - (BOOL)texImageIOSurface:(IOSurfaceRef)ioSurface target:(NSUInteger)target internalFormat:(NSUInteger)internalFormat width:(uint32_t)width height:(uint32_t)height format:(NSUInteger)format type:(NSUInteger)type plane:(uint32_t)plane NS_AVAILABLE_IOS(11_0); @end ... is not implemented.
Kenneth Russell
Comment 19 2019-12-20 18:07:13 PST
(In reply to Dean Jackson from comment #18) > (In reply to Kenneth Russell from comment #12) > > (In reply to Alexey Proskuryakov from comment #8) > > > Does the simulator not support something needed from IOSurface in this > > > particular case? > > > > > > See bug 203026, Turn on IOSurface support in the iOS Simulator. > > > > Ah - I hadn't noticed that! > > > > I'll try building for the iOS Simulator with the ANGLE/project.pbxproj build > > fix in this patch and see whether the IOSurface presentation path works. > > Nope! It's not that the simulator doesn't have IOSurface, it's that > > @interface EAGLContext(IOSurface) > - (BOOL)texImageIOSurface:(IOSurfaceRef)ioSurface target:(NSUInteger)target > internalFormat:(NSUInteger)internalFormat width:(uint32_t)width > height:(uint32_t)height format:(NSUInteger)format type:(NSUInteger)type > plane:(uint32_t)plane NS_AVAILABLE_IOS(11_0); > @end > > ... is not implemented. Thanks - I see that now, trying to enable that code path in ANGLE. I'll work on writing a (slow) emulation path for that in ANGLE and hopefully the WebKit-side code can work unmodified.
Dean Jackson
Comment 20 2019-12-21 19:03:14 PST
Weirdly, on this dual GPU device (MBP15), as soon as DumpRenderTree launches the machine swaps to the high-power GPU. This happens before the WebGL context is even created. Doesn't happen when running MiniBrowser with a WK1 window.
Dean Jackson
Comment 21 2019-12-21 19:05:06 PST
I'm trying to replicate the crash shown on the bots in WK1. It's not happening on this device when using the AMD chip in WK1, nor when using the Intel chip in WK2.
Dean Jackson
Comment 22 2019-12-21 19:08:50 PST
Actually, WebKitTestRunner triggers the high-power GPU too! MuxProbe says that the WindowServer and WebKitTestRunner/DumpRenderTree are the "bad" apps. And it doesn't matter if it is a WebGL test or not.
Dean Jackson
Comment 23 2019-12-21 19:11:19 PST
Dean Jackson
Comment 24 2019-12-26 12:08:46 PST
Created attachment 386420 [details] EWS test
Dean Jackson
Comment 25 2019-12-27 13:28:26 PST
Created attachment 386453 [details] EWS test 2
Dean Jackson
Comment 26 2019-12-27 14:00:22 PST
Created attachment 386455 [details] EWS test 3
Dean Jackson
Comment 27 2019-12-27 16:41:10 PST
Created attachment 386459 [details] EWS test 4
Dean Jackson
Comment 28 2019-12-28 00:15:59 PST
Created attachment 386464 [details] EWS test 5
Dean Jackson
Comment 29 2019-12-28 12:17:07 PST
Alexey Proskuryakov
Comment 30 2019-12-30 17:35:09 PST
This patch adds many new expectations with "FAIL", but at least some of those are simply because those tests were marked as failing overall. Can you post a list of true known regressions, if there are any, and ideally file bugs for them?
Dean Jackson
Comment 31 2020-01-03 14:18:51 PST
Reverted in r254014
Kenneth Russell
Comment 32 2020-01-16 15:20:18 PST
Tracking down, and finding a fix or workaround for, the flakiness introduced with the ANGLE backend is our team's top priority. At this point ANGLE has iOS Simulator support. Adjusting the summary.
Justin Fan
Comment 33 2020-01-21 14:45:16 PST
Dean Jackson
Comment 34 2020-02-17 15:16:43 PST
Note You need to log in before you can comment on or make changes to this bug.