Bug 237313 – GraphicsContextGLCocoa manages EGL native displays manually

• Component: WebGL
• Status: RESOLVED FIXED

[Kimmo Kinnunen]

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.

[Kimmo Kinnunen]

Created attachment 453961 [details]
Patch

[Kimmo Kinnunen]

Created attachment 453962 [details]
Patch

[Kenneth Russell]

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]

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]

<rdar://problem/89957264>

[Kimmo Kinnunen]

> > 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 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]

(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]

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].