Bug 234293 – [GTK] Use libgbm and the ANGLE gbm backend to fix initialisation

• Component: WebGL
• Status: RESOLVED FIXED

[Chris Lord]

Once you build with ANGLE-WebGL enabled with GTK, the web process crashes upon initialising an ANGLE context. A couple of small fixes are need to enable its basic functionality.

[Chris Lord]

Created attachment 447122 [details]
Patch

[Kimmo Kinnunen]

Comment on attachment 447122 [details]
Patch

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

I suppose it's ok to prevent crashes. I think the commit message could be better and optimally there should be no additional ifdefs

> Source/WebCore/ChangeLog:8
> +        Fix ANGLE initialisation and drawing buffer binding on GTK.

If it's intended to initialize using EGL specifically, then the commit message could be more verbose as to explain what's happening.

So below I'll recommend using shared default initialisation, but I don't exactly know why it doesn't work for GTK ..

> Source/WebCore/platform/graphics/nicosia/texmap/NicosiaGCGLANGLELayer.cpp:77
> +        EGL_PLATFORM_ANGLE_TYPE_ANGLE, EGL_PLATFORM_ANGLE_TYPE_OPENGLES_ANGLE,

Without any further knowledge, it's hard to understand why GLES is forced. I'd imagine would be better to use the default one..

Would EGL_PLATFORM_ANGLE_TYPE_ANGLE == EGL_PLATFORM_ANGLE_TYPE_DEFAULT_ANGLE work?
if so, you can call GetPlatformDisplayExt and provide nullptr displayAttributes.

> Source/WebCore/platform/graphics/nicosia/texmap/NicosiaGCGLANGLELayer.cpp:81
> +    EGLDisplay display = EGL_GetPlatformDisplay(EGL_PLATFORM_ANGLE_ANGLE, EGL_DEFAULT_DISPLAY, displayAttributes);

Could you call GetPlatformDisplayExt?

> Source/WebCore/platform/graphics/nicosia/texmap/NicosiaGCGLANGLELayer.cpp:84
> +#endif

If the above would work, you could try if all ports using this file could share the initialisation.

EGL_GetDisplay is implemented with same code as  GetPlatformDisplayExt

[Chris Lord]

(In reply to Kimmo Kinnunen from comment #2)
> Comment on attachment 447122 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=447122&action=review
> 
> I suppose it's ok to prevent crashes. I think the commit message could be
> better and optimally there should be no additional ifdefs
> 
> > Source/WebCore/ChangeLog:8
> > +        Fix ANGLE initialisation and drawing buffer binding on GTK.
> 
> If it's intended to initialize using EGL specifically, then the commit
> message could be more verbose as to explain what's happening.
> 
> So below I'll recommend using shared default initialisation, but I don't
> exactly know why it doesn't work for GTK ..
> 
> > Source/WebCore/platform/graphics/nicosia/texmap/NicosiaGCGLANGLELayer.cpp:77
> > +        EGL_PLATFORM_ANGLE_TYPE_ANGLE, EGL_PLATFORM_ANGLE_TYPE_OPENGLES_ANGLE,
> 
> Without any further knowledge, it's hard to understand why GLES is forced.
> I'd imagine would be better to use the default one..
> 
> Would EGL_PLATFORM_ANGLE_TYPE_ANGLE == EGL_PLATFORM_ANGLE_TYPE_DEFAULT_ANGLE
> work?
> if so, you can call GetPlatformDisplayExt and provide nullptr
> displayAttributes.
> 
> > Source/WebCore/platform/graphics/nicosia/texmap/NicosiaGCGLANGLELayer.cpp:81
> > +    EGLDisplay display = EGL_GetPlatformDisplay(EGL_PLATFORM_ANGLE_ANGLE, EGL_DEFAULT_DISPLAY, displayAttributes);
> 
> Could you call GetPlatformDisplayExt?
> 
> > Source/WebCore/platform/graphics/nicosia/texmap/NicosiaGCGLANGLELayer.cpp:84
> > +#endif
> 
> If the above would work, you could try if all ports using this file could
> share the initialisation.
> 
> EGL_GetDisplay is implemented with same code as  GetPlatformDisplayExt

I couldn't get any combination of parameters using EGL_GetPlatformDisplayEXT working - there's no need to specify ANGLE_TYPE_OPENGLES_ANGLE when using GetPlatformDisplay, so that's redundant, but as you pointed out on Slack, specifying DEVICE_TYPE_EGL_ANGLE is probably forcing that choice anyway.

Without this, the X11 backend is used and that segfaults due to a prior GL error in rx::nativegl_gl::GenerateTextureFormatCaps during EGL_Initialize.

I don't know that we'd want to be using the X11 backend at all, but I haven't found out yet how to alter the build to force ANGLE not to build the X11 backend.

[Don Olmstead]

Comment on attachment 447122 [details]
Patch

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

Just a nit and an open question

> Source/WebCore/platform/graphics/angle/GraphicsContextGLANGLE.cpp:76
> -#if PLATFORM(WIN)
> +#if PLATFORM(WIN) || PLATFORM(GTK)

Wouldn't !PLATFORM(COCOA) be more appropriate here? I'm assuming WPE will eventually end up with ANGLE support.

>> Source/WebCore/platform/graphics/nicosia/texmap/NicosiaGCGLANGLELayer.cpp:84
>> +#endif
> 
> If the above would work, you could try if all ports using this file could share the initialisation.
> 
> EGL_GetDisplay is implemented with same code as  GetPlatformDisplayExt

Not specific to this patch but considering all the ANGLE specific classes and then needing even additional platform specific code hanging out I'm really wondering if we're abstracting at the right point. Right after the Blink split Brandon Jones did an article on how all the GraphicsContext classes were able to be collapsed and now we're adding ANGLE and expanding the class hierarchy. At one point I swore we had a gl:: namespace being generated from ANGLE and it really felt to me like we should have all GL calls going through that namespace and abstract at that point where its either a OpenGL ES 2/3 provided by system or provided by ANGLE. Just mentioning here in case anyone working on this would be interested in trying that route.

[Kenneth Russell]

Comment on attachment 447122 [details]
Patch

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

>>> Source/WebCore/platform/graphics/nicosia/texmap/NicosiaGCGLANGLELayer.cpp:84
>>> +#endif
>> 
>> If the above would work, you could try if all ports using this file could share the initialisation.
>> 
>> EGL_GetDisplay is implemented with same code as  GetPlatformDisplayExt
> 
> Not specific to this patch but considering all the ANGLE specific classes and then needing even additional platform specific code hanging out I'm really wondering if we're abstracting at the right point. Right after the Blink split Brandon Jones did an article on how all the GraphicsContext classes were able to be collapsed and now we're adding ANGLE and expanding the class hierarchy. At one point I swore we had a gl:: namespace being generated from ANGLE and it really felt to me like we should have all GL calls going through that namespace and abstract at that point where its either a OpenGL ES 2/3 provided by system or provided by ANGLE. Just mentioning here in case anyone working on this would be interested in trying that route.

I'd like to suggest that doing this shimming to make the system OpenGL look like ANGLE's OpenGL would obfuscate what the code is doing. During the initial development of WebKit's ANGLE port we ran into multiple situations where legacy code was accidentally punching through ANGLE's layer and talking directly with the system OpenGL - which breaks ANGLE's state tracking and can lead to crashes. Making the system OpenGL look like ANGLE will cause accidents, in my experience.

Instead, I would suggest pushing forward with this port, and once it's done, just delete the non-ANGLE code paths. ANGLE is a robust dependency; it provides a well-tested OpenGL ES implementation, and offers the chance to evolve off of underlying OpenGL implementations in favor of Direct3D, Metal and Vulkan.

[Myles C. Maxfield]

Comment on attachment 447122 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/nicosia/texmap/NicosiaGCGLANGLELayer.cpp:84
>>>> +#endif
>>> 
>>> If the above would work, you could try if all ports using this file could share the initialisation.
>>> 
>>> EGL_GetDisplay is implemented with same code as  GetPlatformDisplayExt
>> 
>> Not specific to this patch but considering all the ANGLE specific classes and then needing even additional platform specific code hanging out I'm really wondering if we're abstracting at the right point. Right after the Blink split Brandon Jones did an article on how all the GraphicsContext classes were able to be collapsed and now we're adding ANGLE and expanding the class hierarchy. At one point I swore we had a gl:: namespace being generated from ANGLE and it really felt to me like we should have all GL calls going through that namespace and abstract at that point where its either a OpenGL ES 2/3 provided by system or provided by ANGLE. Just mentioning here in case anyone working on this would be interested in trying that route.
> 
> I'd like to suggest that doing this shimming to make the system OpenGL look like ANGLE's OpenGL would obfuscate what the code is doing. During the initial development of WebKit's ANGLE port we ran into multiple situations where legacy code was accidentally punching through ANGLE's layer and talking directly with the system OpenGL - which breaks ANGLE's state tracking and can lead to crashes. Making the system OpenGL look like ANGLE will cause accidents, in my experience.
> 
> Instead, I would suggest pushing forward with this port, and once it's done, just delete the non-ANGLE code paths. ANGLE is a robust dependency; it provides a well-tested OpenGL ES implementation, and offers the chance to evolve off of underlying OpenGL implementations in favor of Direct3D, Metal and Vulkan.

> During the initial development of WebKit's ANGLE port we ran into multiple situations where legacy code was accidentally punching through ANGLE's layer

I'm not sure if having two subtly different copies of the WebGL API is the right solution to this problem. It seems like a build-time step to statically validate correct usage would work better.

[Myles C. Maxfield]

> I'm not sure if having two subtly different copies of the WebGL API is the
> right solution to this problem. It seems like a build-time step to
> statically validate correct usage would work better.

*** OpenGL (ES) API

[Don Olmstead]

(In reply to Kenneth Russell from comment #5)
> Comment on attachment 447122 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=447122&action=review
> 
> >>> Source/WebCore/platform/graphics/nicosia/texmap/NicosiaGCGLANGLELayer.cpp:84
> >>> +#endif
> >> 
> >> If the above would work, you could try if all ports using this file could share the initialisation.
> >> 
> >> EGL_GetDisplay is implemented with same code as  GetPlatformDisplayExt
> > 
> > Not specific to this patch but considering all the ANGLE specific classes and then needing even additional platform specific code hanging out I'm really wondering if we're abstracting at the right point. Right after the Blink split Brandon Jones did an article on how all the GraphicsContext classes were able to be collapsed and now we're adding ANGLE and expanding the class hierarchy. At one point I swore we had a gl:: namespace being generated from ANGLE and it really felt to me like we should have all GL calls going through that namespace and abstract at that point where its either a OpenGL ES 2/3 provided by system or provided by ANGLE. Just mentioning here in case anyone working on this would be interested in trying that route.
> 
> I'd like to suggest that doing this shimming to make the system OpenGL look
> like ANGLE's OpenGL would obfuscate what the code is doing. During the
> initial development of WebKit's ANGLE port we ran into multiple situations
> where legacy code was accidentally punching through ANGLE's layer and
> talking directly with the system OpenGL - which breaks ANGLE's state
> tracking and can lead to crashes. Making the system OpenGL look like ANGLE
> will cause accidents, in my experience.
> 
> Instead, I would suggest pushing forward with this port, and once it's done,
> just delete the non-ANGLE code paths. ANGLE is a robust dependency; it
> provides a well-tested OpenGL ES implementation, and offers the chance to
> evolve off of underlying OpenGL implementations in favor of Direct3D, Metal
> and Vulkan.

I completely understand that punching through ANGLE's layer and talking to the system OpenGL is bad news. Wouldn't making a <pal/OpenGL.h> that declared OpenGL functions in a namespace, e.g. glFoo -> gl::foo, replacing all uses of glFoo with gl::foo, and having gl::foo go through ANGLE or system GL dependent on the value of USE_ANGLE, solve this concern?

Why not just make a hard requirement to have an OpenGL ES implementation, that is either ANGLE or system provided, and make sure that all calls go through that implementation?

As an aside I do understand that all WebGL spec conformance is pushed into the ANGLE layer so if you want WebGL then you need to use ANGLE, but if you just need an OpenGL ES implementation and have a system one then there's no reason to have anything go through ANGLE.

[Fujii Hironori]

(In reply to Don Olmstead from comment #8)
> I completely understand that punching through ANGLE's layer and talking to
> the system OpenGL is bad news. Wouldn't making a <pal/OpenGL.h> that
> declared OpenGL functions in a namespace, e.g. glFoo -> gl::foo, replacing
> all uses of glFoo with gl::foo, and having gl::foo go through ANGLE or
> system GL dependent on the value of USE_ANGLE, solve this concern?

The new ANGLE no longer use 'gl::' namespace, but 'GL_' prefix.
With USE_ANGLE_WEBGL CMake option, GTK port is using ANGLE for WebGL, and system OpenGL for the compositor simultaneously.
Statically switching by USE_ANGLE macro approach can't solve it.

> Why not just make a hard requirement to have an OpenGL ES implementation,
> that is either ANGLE or system provided, and make sure that all calls go
> through that implementation?

ANGLE OpenGL backend isn't thread-safe. So, GTK and WPE ports want to
use OpenGL/ES both in the main thread and the compositor thread
of web process.

Eleni's video is the best summary what is the problem for
adopting ANGLE for GTK/WPE WebGL, and how to solve the problem.
(Bug 225563 comment 6)

[Myles C. Maxfield]

Are you willing to make every GL function virtual? This sounds like a job for inheritance.

[Kimmo Kinnunen]

(In reply to Don Olmstead from comment #4)
> Not specific to this patch but considering all the ANGLE specific classes
> and then needing even additional platform specific code hanging out I'm
> really wondering if we're abstracting at the right point. Right after the
> Blink split Brandon Jones did an article on how all the GraphicsContext
> classes were able to be collapsed and now we're adding ANGLE and expanding
> the class hierarchy.

There is no "expanding the class hierarchy" being done.
There is (most likely) ever only two working implementations of GraphicsContextGL: 
 - Remote
 - In-process via ANGLE (until this can be removed, when it can)

There are platform-specific details in these, but this is business as usual and shouldn't be seen as needless proliferation of class hierarchy. 

This bug is about fixing ANGLE initialisation for WebGL implementation purposes.

Whatever the various port-specific compositors are using are design decisions of the port specific compositor implementations. I propose these should be designed in the design bugs of the port specific compositors.

[Chris Lord]

Ok, so taking a slightly different tac that I hope is more amenable to everyone - if we use the ANGLE libgbm backend, that makes it X11/Wayland agnostic and also fixes the default display initialisation. Rather than add #ifdefs in the code, we can change the build and remove the X11-specific bits in favour of gbm/libdrm.

[Chris Lord]

Created attachment 447219 [details]
Patch

[Chris Lord]

Created attachment 447220 [details]
Patch

[EWS Watchlist]

Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE

[EWS]

Committed r287075 (245270@main): <https://commits.webkit.org/245270@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/86524504>