ScopedEGLDefaultDisplay should be removed It makes the use of EGLDisplays harder, especially in scenarios where there might be multiple EGLDisplays
Created attachment 439708 [details] Patch
Created attachment 439749 [details] Patch
Created attachment 439755 [details] Patch
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.
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.
Created attachment 439840 [details] Patch for landing
*** Bug 230951 has been marked as a duplicate of this bug. ***
Created attachment 439854 [details] Patch for landing
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.
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..
Tools/Scripts/svn-apply failed to apply attachment 439854 [details] to trunk. Please resolve the conflicts and upload a new patch.
Created attachment 440182 [details] Patch for landing
Tools/Scripts/svn-apply failed to apply attachment 440182 [details] to trunk. Please resolve the conflicts and upload a new patch.
Created attachment 440352 [details] Patch for landing
ChangeLog entry in Source/WebKit/ChangeLog contains OOPS!.
Created attachment 440473 [details] Patch for landing
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].
<rdar://problem/83970802>
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.