Bug 237313

Summary: GraphicsContextGLCocoa manages EGL native displays manually
Product: WebKit Reporter: Kimmo Kinnunen <kkinnunen>
Component: WebGLAssignee: Kimmo Kinnunen <kkinnunen>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, dino, ews-watchlist, kbr, kkinnunen, kondapallykalyan, luiz, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=236030
Bug Depends on:    
Bug Blocks: 236487    
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch none

Kimmo Kinnunen
Reported 2022-03-01 02:09:01 PST
GraphicsContextGLCocoa manages EGL native displays manually bug 236030 > > EGL_GetPlatformDisplayEXT should key on all passed in attributes that affect the display behaviour. > The ANGLE-side fix is ready to be downstreamed: https://chromium-review.googlesource.com/c/angle/angle/+/3440780 Note: there are WK1 behavior where we need to iterate all the existing native displays and release thread on them. If we move away from manual native display handling, we must cache all the known displays for this use-case.
Attachments
Patch (7.81 KB, patch)
2022-03-07 04:16 PST, Kimmo Kinnunen 		ews-feeder: commit-queue-
DetailsFormatted DiffDiff
Patch (8.33 KB, patch)
2022-03-07 04:24 PST, Kimmo Kinnunen 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Kimmo Kinnunen
Comment 1 2022-03-07 04:16:31 PST
Created attachment 453961 [details] Patch
Kimmo Kinnunen
Comment 2 2022-03-07 04:24:44 PST
Created attachment 453962 [details] Patch
Kenneth Russell
Comment 3 2022-03-07 13:08:35 PST
Comment on attachment 453962 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453962&action=review > Source/WebCore/platform/graphics/angle/GraphicsContextGLANGLE.cpp:64 > +static HashSet<GCGLDisplay>& usedDisplays() Is any locking needed around this data structure? In WK1 mode GraphicsContextGLANGLE is used from multiple threads, though exclusively by construction. Slight concern that other ports may start using this in an unsafe manner - for example, for OffscreenCanvas on web workers.
Kimmo Kinnunen
Comment 4 2022-03-07 23:37:01 PST
Thanks for looking! (In reply to Kenneth Russell from comment #3) > Comment on attachment 453962 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=453962&action=review > > > Source/WebCore/platform/graphics/angle/GraphicsContextGLANGLE.cpp:64 > > +static HashSet<GCGLDisplay>& usedDisplays() > > Is any locking needed around this data structure? In WK1 mode > GraphicsContextGLANGLE is used from multiple threads, though exclusively by > construction. WK1 is protected by Web thread lock. I've added comments for that in other cases, but declined here due to below.. > Slight concern that other ports may start using this in an > unsafe manner - for example, for OffscreenCanvas on web workers. It would be concerning, if ANGLE was multithreaded. Currently I think the verdict still is that ANGLE is single threaded, and as such the it would be more confusing than reassuring if ANGLE-using code would contain locking.
Radar WebKit Bug Importer
Comment 5 2022-03-08 02:09:16 PST
<rdar://problem/89957264>
Kimmo Kinnunen
Comment 6 2022-03-08 05:25:02 PST
> > Slight concern that other ports may start using this in an > > unsafe manner - for example, for OffscreenCanvas on web workers. BTW: previous patch that guarded offscreencanvas use with a lock in WebGLRenderingContextBase.cpp assumed that maybe GTK is interested in running OffscreenCanvas on top of WebGL on real OpenGL. And perhaps future GPUP WebGL.
Kenneth Russell
Comment 7 2022-03-08 13:08:55 PST
Comment on attachment 453962 [details] Patch OK. We do hope to have full multithreading support in ANGLE's Metal backend in the not too distant future. In the meantime this looks fine. r+
Kimmo Kinnunen
Comment 8 2022-03-08 23:51:14 PST
(In reply to Kenneth Russell from comment #7) > Comment on attachment 453962 [details] > Patch > > OK. We do hope to have full multithreading support in ANGLE's Metal backend > in the not too distant future. In the meantime this looks fine. r+ Sure, but ANGLE frontend isn't thread-safe either. Like GetANGLEPlatformDisplayMap() just returning a static that is then mutated.
EWS
Comment 9 2022-03-09 00:38:05 PST
Committed r291035 (248209@main): <https://commits.webkit.org/248209@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 453962 [details].
Note You need to log in before you can comment on or make changes to this bug.