Bug 231011 – ScopedEGLDefaultDisplay should be removed

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Kimmo Kinnunen]

ScopedEGLDefaultDisplay should be removed

It makes the use of EGLDisplays harder, especially in scenarios where there might be multiple EGLDisplays

[Kimmo Kinnunen]

Created attachment 439708 [details]
Patch

[Kimmo Kinnunen]

Created attachment 439749 [details]
Patch

[Kimmo Kinnunen]

Created attachment 439755 [details]
Patch

[Kenneth Russell]

Comment on attachment 439755 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439755&action=review

Looks good overall, and a good step toward better supporting multiple EGLDisplays. A couple of small naming suggestions, but feel free to take/ignore as you see fit. r+

> Source/WebCore/platform/graphics/angle/GraphicsContextGLANGLE.cpp:107
> +            EGL_Terminate(display);

If TerminateAndReleaseThreadResources is passed, this code calls EGL_Terminate before falling through to the EGL_ReleaseThread call below. Is that the correct order in which to call these fairly low-level APIs? It looks like it probably is - just asking for a double-check. I hope this is tested in UIWebView (?) tests on the ios-wk2 bot and not just in the wild.

> Source/WebCore/platform/graphics/cocoa/GraphicsContextGLOpenGLCocoa.mm:64
>      return !!EGL_Initialize;

How does this turn into a function pointer that can be tested in this way? Via some of the weak-linked dylib magic that was added recently?

Worth adding a comment?

> Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGL.h:121
> +    enum class ReleaseResourcesBehavior {

Reading this code afresh, the term "ReleaseResources" in this enum and in the method call is a little confusing - it sounds like OpenGL resources like textures or buffers are being released, where in fact thread-related resources like the current context or access to the EGL API are being released.

I wonder whether a different name could be chosen like:
  ReleaseThreadResourcesBehavior
  ReleasePerThreadResourcesBehavior
  ReleaseEGLResourcesBehavior
(and releasePerThreadResources(), releaseEGLResources(), or similar.)

> Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGL.h:584
> +    static void platformReleaseResources();

If releaseResources is renamed, let's rename this to match.

[Kimmo Kinnunen]

Thanks for the thoughts!

(In reply to Kenneth Russell from comment #4)
> If TerminateAndReleaseThreadResources is passed, this code calls
> EGL_Terminate before falling through to the EGL_ReleaseThread call below. Is
> that the correct order in which to call these fairly low-level APIs? It
> looks like it probably is - just asking for a double-check.

Yeah. First destroy the display, then free driver internal structures, like potential TLS slots.

> I hope this is
> tested in UIWebView (?) tests on the ios-wk2 bot and not just in the wild.

Yeah, I actually wrote a test for it year ago, WebGLNoCrashOnOnOtherThreadAccess, though it starts to be a bit outdated since EAGL isn't used on iOS anymore.

> 
> > Source/WebCore/platform/graphics/cocoa/GraphicsContextGLOpenGLCocoa.mm:64
> >      return !!EGL_Initialize;
> 
> How does this turn into a function pointer that can be tested in this way?
> Via some of the weak-linked dylib magic that was added recently?
> Worth adding a comment?

Correct. Added.

> 
> > Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGL.h:121
> > +    enum class ReleaseResourcesBehavior {
> 
> Reading this code afresh, the term "ReleaseResources" in this enum and in
> the method call is a little confusing - it sounds like OpenGL resources like
> textures or buffers are being released, where in fact thread-related
> resources like the current context or access to the EGL API are being
> released.
> 
> I wonder whether a different name could be chosen like:
>   ReleaseThreadResourcesBehavior

Used this.

[Kimmo Kinnunen]

Created attachment 439840 [details]
Patch for landing

[Kimmo Kinnunen]

*** Bug 230951 has been marked as a duplicate of this bug. ***

[Kimmo Kinnunen]

Created attachment 439854 [details]
Patch for landing

[Kenneth Russell]

Still looks good. Did this land? Note that if you fix up the "Reviewed by" lines in the Changelog, you don't need to set the r+ flag yourself.

[Kimmo Kinnunen]

Comment on attachment 439854 [details]
Patch for landing

User error: first I waited on the mac-debug-wk1 which never showed up, then I set the plus on wrong field..

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 439854 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Kimmo Kinnunen]

Created attachment 440182 [details]
Patch for landing

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 440182 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Kimmo Kinnunen]

Created attachment 440352 [details]
Patch for landing

[EWS]

ChangeLog entry in Source/WebKit/ChangeLog contains OOPS!.

[Kimmo Kinnunen]

Created attachment 440473 [details]
Patch for landing

[EWS]

Committed r283703 (242629@main): <https://commits.webkit.org/242629@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 440473 [details].

[Radar WebKit Bug Importer]

<rdar://problem/83970802>

[Michael Catanzaro]

Hi, the non-ANGLE implementation of GraphicsContextGLOpenGL::releaseThreadResources is broken, it's missing its return value:

bool GraphicsContextGLOpenGL::releaseThreadResources(ReleaseThreadResourceBehavior)
{
}

The ANGLE implementation carefully chooses between true or false, but I don't understand how or why. It's called from only two places, WebCoreThread.mm and ScopedWebGLRenderingResourcesRequest.cpp, and neither caller checks the return value, so it's somewhat academic, I suppose. I'm going to have it always return false, but you might consider converting this to return void.