Bug 236664 – [GTK][WPE] Make proper ANGLE context configuration for some of the expected features

• Component: ANGLE
• Status: RESOLVED FIXED

[Alejandro G. Castro]

This solves multiple layout tests and makes some other pass that were not expected to pass in the previous situation, we are already in better shape than current OpenGL context. Some of the failing test is because expected result files are wrong and expecting some failures that now are passing.

[Alejandro G. Castro]

Created attachment 452223 [details]
Patch

[Zan Dobersek]

Comment on attachment 452223 [details]
Patch

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

> Source/WebCore/platform/graphics/nicosia/texmap/NicosiaGCGLANGLELayer.cpp:124
> +std::unique_ptr<GCGLANGLELayer::ANGLEContext> GCGLANGLELayer::ANGLEContext::createContext(const WebCore::GraphicsContextGLANGLE& webkitContext)

Could just receive the GraphicsContextGLWebGLVersion value.

> Source/WebCore/platform/graphics/nicosia/texmap/NicosiaGCGLANGLELayer.cpp:166
> +    if (webkitContext.m_isForWebGL2) {

Not the scope of this patch, but what happens when WebGL2 context is requested but the underlying GL library doesn't support it?

> Source/WebCore/platform/graphics/texmap/GraphicsContextGLTextureMapperANGLE.cpp:59
> +    // We require this extension for the COORDINATED_GRAPHICS texture rendering.

More correct phrasing is we require this to render into the dmabuf-backed EGLImage. Doesn't mean COORDINATED_GRAPHICS will be using it exclusively.

> Source/WebCore/platform/graphics/texmap/GraphicsContextGLTextureMapperANGLE.cpp:61
> +    getExtensions().ensureEnabled("GL_OES_EGL_image");

This was removed in bug #236490. GL_RequestExtensionANGLE() should be used directly anyway, but you need to use it after the context is made current below.

[Alejandro G. Castro]

Thanks for the review!

(In reply to Zan Dobersek from comment #2)
> Comment on attachment 452223 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=452223&action=review
> 
> > Source/WebCore/platform/graphics/nicosia/texmap/NicosiaGCGLANGLELayer.cpp:124
> > +std::unique_ptr<GCGLANGLELayer::ANGLEContext> GCGLANGLELayer::ANGLEContext::createContext(const WebCore::GraphicsContextGLANGLE& webkitContext)
> 
> Could just receive the GraphicsContextGLWebGLVersion value.
> 

I think so.

> > Source/WebCore/platform/graphics/nicosia/texmap/NicosiaGCGLANGLELayer.cpp:166
> > +    if (webkitContext.m_isForWebGL2) {
> 
> Not the scope of this patch, but what happens when WebGL2 context is
> requested but the underlying GL library doesn't support it?
> 

It is a good question, I'm not sure what ANGLE does in that situation, we have to find out.

> > Source/WebCore/platform/graphics/texmap/GraphicsContextGLTextureMapperANGLE.cpp:59
> > +    // We require this extension for the COORDINATED_GRAPHICS texture rendering.
> 
> More correct phrasing is we require this to render into the dmabuf-backed
> EGLImage. Doesn't mean COORDINATED_GRAPHICS will be using it exclusively.
> 

Right.

> > Source/WebCore/platform/graphics/texmap/GraphicsContextGLTextureMapperANGLE.cpp:61
> > +    getExtensions().ensureEnabled("GL_OES_EGL_image");
> 
> This was removed in bug #236490. GL_RequestExtensionANGLE() should be used
> directly anyway, but you need to use it after the context is made current
> below.

Wow, last Friday :-), thanks for pointing it out, I'll replace the call with whatever they used as replacement.

[Zan Dobersek]

Comment on attachment 452223 [details]
Patch

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

>>> Source/WebCore/platform/graphics/texmap/GraphicsContextGLTextureMapperANGLE.cpp:61
>>> +    getExtensions().ensureEnabled("GL_OES_EGL_image");
>> 
>> This was removed in bug #236490. GL_RequestExtensionANGLE() should be used directly anyway, but you need to use it after the context is made current below.
> 
> Wow, last Friday :-), thanks for pointing it out, I'll replace the call with whatever they used as replacement.

Again, just use GL_RequestExtensionANGLE() after the context is made current. GraphicsContextGLCVCocoa does this already. getExtensions().ensureEnabled() did the equivalent of it, with some overhead.

[Kimmo Kinnunen]

Alex, I see you're copying the Cocoa properties.
If you have time, I filed bug 236769 so that we would work on merging the different code-paths as much as possible. ATM it's a bit hard for me since I don't have a WPE/WC compile environment yet.

[Alejandro G. Castro]

(In reply to Kimmo Kinnunen from comment #5)
> Alex, I see you're copying the Cocoa properties.
> If you have time, I filed bug 236769 so that we would work on merging the
> different code-paths as much as possible. ATM it's a bit hard for me since I
> don't have a WPE/WC compile environment yet.

That would be great! I'm interested, we did not spend the time to check each one in detail so it would help a lot to make sure we are doing the same thing. We just want to land this patch this week because we have some release milestone but I'll help with the refactor in out side.

[Alejandro G. Castro]

Created attachment 452355 [details]
Patch

[Zan Dobersek (Reviews)]

Comment on attachment 452355 [details]
Patch

LGTM.

[EWS]

Committed r290064 (247426@main): <https://commits.webkit.org/247426@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/89104959>