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

Description Kimmo Kinnunen 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.
Comment 1 Kimmo Kinnunen 2022-03-07 04:16:31 PST 
Created attachment 453961 [details]
Patch
Comment 2 Kimmo Kinnunen 2022-03-07 04:24:44 PST 
Created attachment 453962 [details]
Patch
Comment 3 Kenneth Russell 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.
Comment 4 Kimmo Kinnunen 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.
Comment 5 Radar WebKit Bug Importer 2022-03-08 02:09:16 PST 
<rdar://problem/89957264>
Comment 6 Kimmo Kinnunen 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.
Comment 7 Kenneth Russell 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+
Comment 8 Kimmo Kinnunen 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.
Comment 9 EWS 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].