[GTK] Prefer EGL over X11 where available

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Arcady Goldmints-Orlov]

In order to take advantage of the forthcoming work to use ANGLE for WebGL and use shared EGLimages to deliver rendered frames to the compositor thread, we need to be using EGL. Therefore, where both GLX and EGL are available, EGL should be preferred over GLX rather than the other way around.

[Arcady Goldmints-Orlov]

Created attachment 450945 [details]
Patch

[Patrick Griffis]

This appears to break all rendering on NVidia/X11.

[Philippe Normand]

Comment on attachment 450945 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/PlatformDisplayGStreamer.cpp:69
> +    if (is<PlatformDisplayX11>(sharedDisplay)) {

So what will happen now if EGL is enabled but X11 is disabled at build time? It seems this patch makes EGL depend on X11, which is not a thing we want on embedded platforms, I think.

[Chris Lord]

(In reply to Philippe Normand from comment #3)
> Comment on attachment 450945 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=450945&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/PlatformDisplayGStreamer.cpp:69
> > +    if (is<PlatformDisplayX11>(sharedDisplay)) {
> 
> So what will happen now if EGL is enabled but X11 is disabled at build time?
> It seems this patch makes EGL depend on X11, which is not a thing we want on
> embedded platforms, I think.

I don't think that's the case here, this block is already in a PLATFORM(X11) define and there are separate blocks for both USE(WPE_RENDERER) and PLATFORM(WAYLAND). The problem I do see though is that it doesn't handle if EGL and GLX are enabled and EGL context creation failed, which the rest of the patch seems to handle. The second #elif USE(GLX) path should just be in a separate #if USE(GLX) path, I think.

This would appear to be a pre-existing bug, but in the other direction (where if GLX and EGL are both enabled and GLX context creation failed and EGL succeeded, this will not return an EGL context).

This patch adds checking that there actually is a display for the EGL path, I don't know if there's a situation where there wouldn't be a display on the GLX path at this point in the code(?)

[Arcady Goldmints-Orlov]

Unfortunately when I try to test aquarium with this now, I get a context creation error, with or without ANGLE. I do know it worked at some point, and I still think it would be a generally good idea to prefer EGL over GLX.

[Víctor M. Jáquez L.]

Pull request: https://github.com/WebKit/WebKit/pull/3305

[Víctor M. Jáquez L.]

(In reply to Arcady Goldmints-Orlov from comment #5)
> Unfortunately when I try to test aquarium with this now, I get a context
> creation error, with or without ANGLE. I do know it worked at some point,
> and I still think it would be a generally good idea to prefer EGL over GLX.

I'm testing the patch as is, either with nvidia (515.49.09) and Mesa radeon (22.2.0 rc1), and it seems to work OK either a video and the aquarium

[Arcady Goldmints-Orlov]

That sounds like sufficient testing. I don't think there would be any new issues with multi-device setups with either NVIDIA or Mesa. Unfortunately it would be somewhat challenging for me to try to build and test my own build WebKit at this point.

[Alejandro G. Castro]

(In reply to Víctor M. Jáquez L. from comment #7)
> (In reply to Arcady Goldmints-Orlov from comment #5)
> > Unfortunately when I try to test aquarium with this now, I get a context
> > creation error, with or without ANGLE. I do know it worked at some point,
> > and I still think it would be a generally good idea to prefer EGL over GLX.
> 
> I'm testing the patch as is, either with nvidia (515.49.09) and Mesa radeon
> (22.2.0 rc1), and it seems to work OK either a video and the aquarium

Are you using X? Patrick could you check again this patch?

[Patrick Griffis]

Tested again on Nvidia 515.65.01 and still fails to render anything. No errors at all.

[Carlos Garcia Campos]

Created attachment 462354 [details]
Patch

This patch includes the following additions:
 - Simplify the gst context creation
 - Always match the window gl context to the shared one.
 - Ensure the EGL config has a visual compatible with the window
 - Several fixes in about:gpu that assumed X11 is always GLX

Patrick, could you try this with nvidia?

[Carlos Garcia Campos]

Created attachment 462388 [details]
Patch

[Víctor M. Jáquez L.]

(In reply to Carlos Garcia Campos from comment #12)
> Created attachment 462388 [details]
> Patch

Got a crash

NVIDIA 515.49.15 (beta for Vulkan) within flatpak with it respective runtime.

#0  0x00007f6807904112 in WebCore::GLContextEGL::createWindowContext(unsigned long, WebCore::PlatformDisplay&, void*)::$_3::operator()(int) const (this=0x7f67e81700c8, configVisualID=0x24) at /app/webkit/Source/
WebCore/platform/graphics/egl/GLContextEGL.cpp:203
#1  0x00007f680790408f in WTF::Detail::CallableWrapper<WebCore::GLContextEGL::createWindowContext(unsigned long, WebCore::PlatformDisplay&, void*)::$_3, bool, int>::call(int) (this=0x7f67e81700c0, in=0x24) at WT
F/Headers/wtf/Function.h:53
#2  0x00007f6807905605 in WTF::Function<bool (int)>::operator()(int) const (this=0x7f6795ffa5a8, in=0x24) at WTF/Headers/wtf/Function.h:82
#3  0x00007f6807903d0c in WebCore::GLContextEGL::getEGLConfig(void*, void**, WebCore::GLContextEGL::EGLSurfaceType, WTF::Function<bool (int)>&&)::$_1::operator()(void*) const (this=0x7f6795ffa350, value=0xcaf329
) at /app/webkit/Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:162
#4  0x00007f68079018f2 in WTF::Vector<void*, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>::findIf<WebCore::GLContextEGL::getEGLConfig(void*, void**, WebCore::GLContextEGL::EGLSurfaceType, WTF::Function<bool
 (int)>&&)::$_1>(WebCore::GLContextEGL::getEGLConfig(void*, void**, WebCore::GLContextEGL::EGLSurfaceType, WTF::Function<bool (int)>&&)::$_1 const&) const (this=0x7f6795ffa378, matches=...) at WTF/Headers/wtf/Ve
ctor.h:1069
#5  0x00007f6807901763 in WebCore::GLContextEGL::getEGLConfig(void*, void**, WebCore::GLContextEGL::EGLSurfaceType, WTF::Function<bool (int)>&&) (display=0x7f677c028a20, config=0x7f6795ffa4e0, surfaceType=WebCor
e::GLContextEGL::WindowSurface, checkCompatibleVisuals=...) at /app/webkit/Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:150
#6  0x00007f6807901ad8 in WebCore::GLContextEGL::createWindowContext(unsigned long, WebCore::PlatformDisplay&, void*) (window=0x2200004, platformDisplay=..., sharingContext=0x7f677c04cf91) at /app/webkit/Source/
WebCore/platform/graphics/egl/GLContextEGL.cpp:216
#7  0x00007f6807902a36 in WebCore::GLContextEGL::createContext(unsigned long, WebCore::PlatformDisplay&) (window=0x2200004, platformDisplay=...) at /app/webkit/Source/WebCore/platform/graphics/egl/GLContextEGL.c
pp:328
#8  0x00007f680787f4f7 in WebCore::GLContext::createContextForWindow(unsigned long, WebCore::PlatformDisplay*) (windowHandle=0x2200004, platformDisplay=0x7f67e8054220) at /app/webkit/Source/WebCore/platform/grap
hics/GLContext.cpp:91
#9  0x00007f680312084a in WebKit::ThreadedCompositor::createGLContext() (this=0x7f67e8119b80) at /app/webkit/Source/WebKit/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp:96
#10 0x00007f6803127c22 in WebKit::ThreadedCompositor::ThreadedCompositor(WebKit::ThreadedCompositor::Client&, WebKit::ThreadedDisplayRefreshMonitor::Client&, unsigned int, WebCore::IntSize const&, float, unsigne
d int)::$_8::operator()() const (this=0x7f67e8113ca8) at /app/webkit/Source/WebKit/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp:73
#11 0x00007f6803127b89 in WTF::Detail::CallableWrapper<WebKit::ThreadedCompositor::ThreadedCompositor(WebKit::ThreadedCompositor::Client&, WebKit::ThreadedDisplayRefreshMonitor::Client&, unsigned int, WebCore::I
ntSize const&, float, unsigned int)::$_8, void>::call() (this=0x7f67e8113ca0) at WTF/Headers/wtf/Function.h:53
#12 0x00007f68023b8d82 in WTF::Function<void ()>::operator()() const (this=0x7f67e8113cd0) at WTF/Headers/wtf/Function.h:82
#13 0x00007f68031273b0 in WebKit::CompositingRunLoop::performTaskSync(WTF::Function<void ()>&&)::$_6::operator()() const (this=0x7f67e8113cc8) at /app/webkit/Source/WebKit/Shared/CoordinatedGraphics/threadedcomp
ositor/CompositingRunLoop.cpp:90
#14 0x00007f6803127369 in WTF::Detail::CallableWrapper<WebKit::CompositingRunLoop::performTaskSync(WTF::Function<void ()>&&)::$_6, void>::call() (this=0x7f67e8113cc0) at WTF/Headers/wtf/Function.h:53
#15 0x00007f67f9cbdb22 in WTF::Function<void ()>::operator()() const (this=0x7f6795ffa890) at WTF/Headers/wtf/Function.h:82
#16 0x00007f67fb3212ae in WTF::RunLoop::performWork() (this=0x7f67e81600c0) at /app/webkit/Source/WTF/wtf/RunLoop.cpp:133
#17 0x00007f67fb3d5af9 in WTF::RunLoop::RunLoop()::$_1::operator()(void*) const (this=0x7f67e81600c0, userData=0x7f67e81600c0) at /app/webkit/Source/WTF/wtf/glib/RunLoopGLib.cpp:80
#18 0x00007f67fb3d5ad5 in WTF::RunLoop::RunLoop()::$_1::__invoke(void*) (userData=0x7f67e81600c0) at /app/webkit/Source/WTF/wtf/glib/RunLoopGLib.cpp:79
#19 0x00007f67fb3d5a89 in WTF::RunLoop::$_0::operator()(_GSource*, int (*)(void*), void*) const (this=0x7f677c001b30, source=0x7f677c001b30, callback=0x7f67fb3d5ac0 <WTF::RunLoop::RunLoop()::$_1::__invoke(void*)
>, userData=0x7f67e81600c0) at /app/webkit/Source/WTF/wtf/glib/RunLoopGLib.cpp:53
#20 0x00007f67fb3d4c85 in WTF::RunLoop::$_0::__invoke(_GSource*, int (*)(void*), void*) (source=0x7f677c001b30, callback=0x7f67fb3d5ac0 <WTF::RunLoop::RunLoop()::$_1::__invoke(void*)>, userData=0x7f67e81600c0) a
t /app/webkit/Source/WTF/wtf/glib/RunLoopGLib.cpp:45
#21 0x00007f67fbb08294 in g_main_dispatch (context=0x7f677c000b60) at ../glib/gmain.c:3381
#22 g_main_context_dispatch (context=0x7f677c000b60) at ../glib/gmain.c:4099
#23 0x00007f67fbb08638 in g_main_context_iterate (context=0x7f677c000b60, block=block@entry=0x1, dispatch=dispatch@entry=0x1, self=<optimized out>) at ../glib/gmain.c:4175
#24 0x00007f67fbb08943 in g_main_loop_run (loop=0x7f677c001b10) at ../glib/gmain.c:4373
#25 0x00007f67fb3d5208 in WTF::RunLoop::run() () at /app/webkit/Source/WTF/wtf/glib/RunLoopGLib.cpp:108
#26 0x00007f6803121da4 in WebKit::createRunLoop()::$_12::operator()() const (this=0x7f67e8113c48) at /app/webkit/Source/WebKit/Shared/CoordinatedGraphics/threadedcompositor/CompositingRunLoop.cpp:49
#27 0x00007f6803121d49 in WTF::Detail::CallableWrapper<WebKit::createRunLoop()::$_12, void>::call() (this=0x7f67e8113c40) at WTF/Headers/wtf/Function.h:53
#28 0x00007f67f9cbdb22 in WTF::Function<void ()>::operator()() const (this=0x7f6795ffab90) at WTF/Headers/wtf/Function.h:82
#29 0x00007f67fb32c298 in WTF::Thread::entryPoint(WTF::Thread::NewThreadContext*) (newThreadContext=0x7f67e8158ea0) at /app/webkit/Source/WTF/wtf/Threading.cpp:240
#30 0x00007f67fb3e1f15 in WTF::wtfThreadEntryPoint(void*) (context=0x7f67e8158ea0) at /app/webkit/Source/WTF/wtf/posix/ThreadingPOSIX.cpp:242
#31 0x00007f67f79e73ba in start_thread (arg=0x7f6795ffb640) at pthread_create.c:481
#32 0x00007f67f4fda7a3 in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95


It looks like if visualInfo is deallocated when checkCompatibleVisuals is called in getEGLConfig.

[Víctor M. Jáquez L.]

(In reply to Carlos Garcia Campos from comment #12)
> Created attachment 462388 [details]
> Patch

I got it fixed. Quite simple :)

With my nvidia setup works. I will update the patch in github.

[Carlos Garcia Campos]

Created attachment 462471 [details]
Patch

This fixes the crashes by transferring the ownership to the Function instead of passing captures by value. It also adds an env var to force GLX, for easily testing if a failure is due to EGL over X11.

[Carlos Garcia Campos]

Created attachment 462491 [details]
Patch

[EWS]

Committed 254751@main (5948e8b03d0c): <https://commits.webkit.org/254751@main>

Reviewed commits have been landed. Closing PR #3305 and removing active labels.