Bug 77921 – [GTK] Add support for creating EGL contexts

• Component: WebKitGTK
• Status: RESOLVED FIXED

[ChangSeok Oh]

This bug is for adding a new configuration option to choose an opengl backend.

[ChangSeok Oh]

Created attachment 125867 [details]
Patch

[Eric Seidel (no email)]

You'll likely want to CC some relevant reviewers.

[Martin Robinson]

Comment on attachment 125867 [details]
Patch

I'll be removing GraphicsContext3DPrivate soon, hopefully within the next few days. Is it possible for you to hold off on this empty implementation until then?

Another issue is that EGL / OpenGL ES are not synonymous. While it is unlikely, I do believe it is possible to use vanilla desktop OpenGL with EGL. Quite likely the right approach here is to make the EGL and GLX context creation code co-exist at runtime. This happens in Cairo as well.

[ChangSeok Oh]

(In reply to comment #3)
> (From update of attachment 125867 [details])
> I'll be removing GraphicsContext3DPrivate soon, hopefully within the next few days. Is it possible for you to hold off on this empty implementation until then?
Sure. I'll hold my patches to support EGL/GLES backend for WebGL off until refactoring GraphicsContext3DPrivate. :) My small hope is, please CC me when you open a new bug to deal with it.

> Another issue is that EGL / OpenGL ES are not synonymous. While it is unlikely, I do believe it is possible to use vanilla desktop OpenGL with EGL. Quite likely the right approach here is to make the EGL and GLX context creation code co-exist at runtime. This happens in Cairo as well.
You're right. The desktop OpenGL can run with EGL. It seems like this patch is needed to update more  carefully. Let me do it after your refactoring. :)

[Martin Robinson]

Created attachment 132149 [details]
Patch

[ChangSeok Oh]

(In reply to comment #5)
> Created an attachment (id=132149) [details]
> Patch

I think this is a great patch!
This is not an official review. Please just refer below comments, Martin. :)

> Source/WebCore/platform/graphics/cairo/egl/GLContextEGL.cpp:40
> +            gSharedDisplay = 0;

Nit, We might be able to use EGL_NO_DISPLAY here instead of 0, but I don't mind. :)

> Source/WebCore/platform/graphics/cairo/egl/GLContextEGL.cpp:60
> +    EGLint attributeList[] = {
> +        EGL_RED_SIZE, 8,
> +        EGL_GREEN_SIZE, 8,
> +        EGL_BLUE_SIZE, 8,
> +        EGL_STENCIL_SIZE, 8,
> +        EGL_ALPHA_SIZE, 8,
> +        EGL_NONE, EGL_NONE,
> +        EGL_NONE
> +    };

Don't we need to define EGL_RENDERABLE_TYPE here? Or is it set at another place? I'm not sure that it works though we don't set it for at least one among EGL_OPENGL_ES_BIT, EGL_OPENVG_BIT, EGL_OPENGL_ES2_BIT and EGL_OPENGL_BIT.

[Martin Robinson]

(In reply to comment #6)

> > Source/WebCore/platform/graphics/cairo/egl/GLContextEGL.cpp:40
> > +            gSharedDisplay = 0;
> 
> Nit, We might be able to use EGL_NO_DISPLAY here instead of 0, but I don't mind. :)

Makes sense.

> > Source/WebCore/platform/graphics/cairo/egl/GLContextEGL.cpp:60
> > +    EGLint attributeList[] = {
> > +        EGL_RED_SIZE, 8,
> > +        EGL_GREEN_SIZE, 8,
> > +        EGL_BLUE_SIZE, 8,
> > +        EGL_STENCIL_SIZE, 8,
> > +        EGL_ALPHA_SIZE, 8,
> > +        EGL_NONE, EGL_NONE,
> > +        EGL_NONE
> > +    };
> 
> Don't we need to define EGL_RENDERABLE_TYPE here? Or is it set at another place? I'm not sure that it works though we don't set it for at least one among EGL_OPENGL_ES_BIT, EGL_OPENVG_BIT, EGL_OPENGL_ES2_BIT and EGL_OPENGL_BIT.

The default value is EGL_OPENGL_ES_BIT. For now I guess we should assume that EGL targets OpenGLES2. Later we should devise a way to figure out what the best target is though.

[Martin Robinson]

Created attachment 136701 [details]
Patch

[Daniel Bates]

Comment on attachment 136701 [details]
Patch

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

> Source/WebCore/platform/graphics/cairo/egl/GLContextEGL.cpp:51
> +{

We should add ASSERT(!sharedDisplay()) here and/or strengthen the return value handling of eglChooseConfig() below.

> Source/WebCore/platform/graphics/cairo/egl/GLContextEGL.cpp:53
> +    // TODO: We assume that we're creating an OpenGLES2 context for now,
> +    // but later it would be better to do something smarter here.

Nit: This should be a FIXME comment per the WebKit Code Style Guidelines, <http://www.webkit.org/coding/coding-style.html#comments-fixme>.

> Source/WebCore/platform/graphics/cairo/egl/GLContextEGL.cpp:71
> +    EGLint numberConfigsReturned;
> +    return eglChooseConfig(sharedDisplay(), attributeList, config, 1, &numberConfigsReturned) && numberConfigsReturned;

How does this code work when eglChooseConfig() returns an error code?

> Source/WebCore/platform/graphics/cairo/egl/GLContextEGL.cpp:134
> +    static bool initialized = false;
> +    static bool success = true;
> +    if (!initialized) {
> +        success = initializeOpenGLShims();
> +        initialized = true;
> +    }
> +    if (!success)
> +        return 0;

I would write this as:

static bool didInitializeOpenGLShims = initializeOpenGLShims();
if (!didInitializeOpenGLShims)
    return 0;

> Source/WebCore/platform/graphics/cairo/egl/GLContextEGL.cpp:174
> +    if (eglGetCurrentContext() == m_context)
> +        return true;
> +    return eglMakeCurrent(sharedDisplay(), m_surface, m_surface, m_context);

I would write this as:

return eglGetCurrentContext() == m_context || eglMakeCurrent(sharedDisplay(), m_surface, m_surface, m_context);

> Source/WebCore/platform/graphics/gtk/GLContextGtk.cpp:69
> +        context =  GLContextEGL::createContext(GDK_WINDOW_XID(gdkWindow));

Nit: There are two spaces to the right of the equal sign (=).

> Source/WebCore/platform/graphics/gtk/GLContextGtk.cpp:74
> +            context =  GLContextGLX::createContext(GDK_WINDOW_XID(gdkWindow));

Nit: There are two spaces to the right of the equal sign (=).

Do you expect most people to build with EGL and GLX enabled by default? If so, how often do you expect GLContextEGL::createContext() to return 0. Should we cache GDK_WINDOW_XID(gdkWindow) in a local variable? Notice, GDK_WINDOW_XID deferences a pointer twice, <http://developer.gnome.org/gdk/stable/gdk-X-Window-System-Interaction.html#GDK-WINDOW-XID:CAPS>.

[José Dapena Paz]

Created attachment 161993 [details]
Patch

New approach for EGL support, limiting the patch to only X11 support, so that we don't have to mix in the same patch all the effort for other windowing systems.

[Martin Robinson]

Comment on attachment 161993 [details]
Patch

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

Looking good! Thanks for writing this patch. I have a few suggestions for cleaning things up. I think it's also important that we think carefully about allowing GLX+EGL to be both active at compile-time. In a followup patch we should also allow the same for GLES and Desktop GL.

> Source/WebCore/platform/graphics/cairo/GLContext.cpp:56
> +// We do not want to call glXMakeContextCurrent using different Display pointers,
> +// because it might lead to crashes in some drivers (fglrx). We use a shared display
> +// pointer here.
> +static Display* gSharedDisplay = 0;
> +Display* GLContext::sharedDisplay()
> +{
> +    if (!gSharedDisplay)
> +        gSharedDisplay = XOpenDisplay(0);
> +    return gSharedDisplay;
> +}
> +
> +void GLContext::cleanupSharedDisplay()
> +{
> +    if (!gSharedDisplay)
> +        return;
> +    XCloseDisplay(gSharedDisplay);
> +    gSharedDisplay = 0;
> +}
> +

Perhaps you should hide this behind #if (USE_GLX || USE_EGL). There are types of OpenGL that do not use X11 like WGL and AGL.

> Source/WebCore/platform/graphics/cairo/GLContext.cpp:83
> +    context = GLContextEGL::createContext(0, sharingContext);
> +    if (context)
> +        return context.release();

Instead of defining the context at the top-level you may be able to do:

if (OwnPtr<GLContext> context = GLContextEGL::createContext(0, sharingContext))
    return context.release()

and reduce the number of lines in this function.

> Source/WebCore/platform/graphics/cairo/GLContext.h:28
> +typedef struct _XDisplay Display;

This should probably be behind  #if GLX || EGL, again

> Source/WebCore/platform/graphics/cairo/GLContext.h:48
> +    static Display *sharedDisplay(void);

The asterisk should go with the type name in WebKit and you don't need to include the void argument list in C++.

> Source/WebCore/platform/graphics/cairo/GraphicsContext3DCairo.cpp:39
> +#if USE(OPENGL_ES_2)
> +#include "Extensions3DOpenGLES.h"
> +#include "OpenGLESShims.h"
> +#else
>  #include "Extensions3DOpenGL.h"
> +#include "OpenGLShims.h"
> +#endif

These conditional sections should go after the non-conditional ones. Can we avoid the use of OpenGLESShims.h here?

> Source/WebCore/platform/graphics/cairo/GraphicsContext3DCairo.cpp:171
> +#if !USE(OPENGL_ES_2)
>  void GraphicsContext3D::releaseShaderCompiler()
>  {
>      notImplemented();
>  }
> +#endif

This is defined for both OPENGL_ES_2 and Desktop GL and has an empty implementation. Why hide it behind an #ifdef?

> Source/WebCore/platform/graphics/cairo/GraphicsContext3DPrivate.cpp:134
> -        glBindFramebuffer(GraphicsContext3D::FRAMEBUFFER, m_context->m_boundFBO);
> +        ::glBindFramebufferEXT(GraphicsContext3D::FRAMEBUFFER, m_context->m_boundFBO);

This seems like the wrong direction. In OpenGLESShims.h glBindFramebufferEXT is just defined to glBindFramebuffer, so why not just not use glBindFramebuffer in this file?

> Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:43
> +#if USE(OPENGL_ES_2)
> +static const EGLenum gGLApi = EGL_OPENGL_ES_API;
> +#else
> +static const EGLenum gGLApi = EGL_OPENGL_API;
> +#endif

Ideally we'd decide at runtime what type of API is accelerated, but we can tackle that later.

> Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:53
> +        if (gSharedEGLDisplay != EGL_NO_DISPLAY
> +            && (!eglInitialize(gSharedEGLDisplay, 0, 0)
> +                || !eglBindAPI(gGLApi)))

This can be one line. Lines in WebKit go to 120 characters and beyond.

> Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:68
> +static const EGLint contextAttributes[] = {

gContextAttributes?

> Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:96
> +// Because of driver bugs, exiting the program when there are active pbuffers
> +// can crash the X server (this has been observed with the official Nvidia drivers).
> +// We need to ensure that we clean everything up on exit. There are several reasons
> +// that GraphicsContext3Ds will still be alive at exit, including user error (memory
> +// leaks) and the page cache. In any case, we don't want the X server to crash.
> +typedef Vector<GLContext*> ActiveContextList;
> +static ActiveContextList& activeContextList()
> +{
> +    DEFINE_STATIC_LOCAL(ActiveContextList, activeContexts, ());
> +    return activeContexts;
> +}
> +
> +void GLContextEGL::addActiveContext(GLContextEGL* context)
> +{
> +    static bool addedAtExitHandler = false;
> +    if (!addedAtExitHandler) {
> +        atexit(&GLContextEGL::cleanupActiveContextsAtExit);
> +        addedAtExitHandler = true;
> +    }
> +    activeContextList().append(context);
> +}
> +

The Nvidia driver doesn't support EGL at the moment, so maybe we can remove this work-around?

> Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:120
> +    cleanupSharedDisplay();

I think it makes sense for GLContext to install the atexit handler to cleanup the display, since it creates it.

> Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:155
> +PassOwnPtr<GLContextEGL> GLContextEGL::createWindowContext(EGLNativeWindowType wId, GLContext* sharingContext)

windowId instead of wId. WebKit doesn't abbreviate variables unless the abbreviation is very common.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:143
> +            ::glBindRenderbufferEXT(GraphicsContext3D::RENDERBUFFER, m_depthStencilBuffer);
> +            ::glRenderbufferStorageEXT(GraphicsContext3D::RENDERBUFFER, GraphicsContext3D::DEPTH24_STENCIL8, width, height);

I think that you should avoid the use of EXT in this file. It's used in the GraphicsContext3DOpenGLCommon because not all platforms use OpenGLShims. Since we don't have to use it here, we shouldn't though.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:145
> -                ::glFramebufferTexture2D(GL_FRAMEBUFFER, GL_STENCIL_ATTACHMENT, GL_TEXTURE_2D, m_depthStencilBuffer, 0);
> +                ::glFramebufferRenderbufferEXT(GraphicsContext3D::FRAMEBUFFER, GraphicsContext3D::STENCIL_ATTACHMENT, GraphicsContext3D::RENDERBUFFER, m_depthStencilBuffer);

What's the purpose of using the GraphicsContext3D version of the GL_FRAMEBUFFER and GL_STENCIL_ATTACHMENT constants here?

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:149
> -                ::glFramebufferTexture2D(GL_FRAMEBUFFER, GL_DEPTH_ATTACHMENT, GL_TEXTURE_2D, m_depthStencilBuffer, 0);
> -            ::glBindTexture(GL_TEXTURE_2D, 0);
> +                ::glFramebufferRenderbufferEXT(GraphicsContext3D::FRAMEBUFFER, GraphicsContext3D::DEPTH_ATTACHMENT, GraphicsContext3D::RENDERBUFFER, m_depthStencilBuffer);
> +            ::glBindRenderbufferEXT(GraphicsContext3D::RENDERBUFFER, 0);
>          } else {

See above.

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:29
>  #include "Extensions3DOpenGL.h"
> +#include "OpenGLShims.h"
>  #endif
>  #include "GraphicsContext.h"
>  #include "GraphicsSurface.h"

Were these changes accidental?

> Source/WebCore/platform/graphics/texmap/TextureMapper.h:34
> +#if PLATFORM(GTK)
> +#if USE(OPENGL_ES_2) && !defined(TEXMAP_OPENGL_ES_2)
> +#define TEXMAP_OPENGL_ES_2
> +#endif

Why not just do:

#if PLATFORM(GTK) && USE(OPENGL_ES_2)
#define TEXMAP_OPENGL_ES_2
#endif

> Source/WebCore/platform/gtk/RedirectedXCompositeWindow.cpp:31
> +#include "GLContextEGL.h"
>  #include "GLContextGLX.h"

I guess we can remove both of these now.

> configure.ac:558
> +              AC_HELP_STRING([--enable-gles2], [enable support for OpenGL/ES2 [default=auto]]),

Nit: It's actually just called "OpenGL ES 2" I think instead of "OpenGL/ES2".

> configure.ac:573
> -if test "$with_target" = "x11"; then
> +if test "$enable_egl" = "yes"; then
> +    if test "$enable_gles2" = "yes"; then
> +       AC_CHECK_HEADERS([GLES2/gl2.h], [found_opengl="yes"], [])
> +    else
> +       AC_CHECK_HEADERS([GL/gl.h], [found_opengl="yes"], [])
> +    fi
> +    AC_CHECK_HEADERS([EGL/egl.h], [], [found_opengl="no"])
> +elif test "$with_target" = "x11"; then
>      AC_CHECK_HEADERS([GL/gl.h], [found_opengl="yes"], [])
> -    AC_CHECK_HEADERS([GL/glx.h], [], [found_opengl="no"])
> +    AC_CHECK_HEADERS([GL/glx.h], [enable_glx="yes"], [found_opengl="no"])
>  fi

Maybe what we can do to simplify this is to do this:

1. Enable GLX if we find the headers.
2. Enable EGL if we find the headers.
3. Remove the configuration option for both of them.
4. Pick the appropriate rendering type:
   a. If the CPU is not ARM --> Look for the GL headers and fall back to the GLES headers to choose the rendering type.
   b. If the CPU is ARM --> Look for the GLES headers and fall back to the GL headers to choose the rendering type.
5. Now if you've found (egl || glx) && ((opengl && glx) || egl) then turn on OpenGL by default (--with-acceleration-backend defaults to opengl)
6. If --with-acceleration-backend is passed the opengl option and the above conditional is false, give an error about what headers are missing.

It's important that we support both EGL and GLX turned on at compile-time now. We can worry about GLES and Desktop GL turned on at compile time in a followup patch.

[José Dapena Paz]

(In reply to comment #11)
> (From update of attachment 161993 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161993&action=review
> 
> Looking good! Thanks for writing this patch. I have a few suggestions for cleaning things up. I think it's also important that we think carefully about allowing GLX+EGL to be both active at compile-time. In a followup patch we should also allow the same for GLES and Desktop GL.

It shouldn't be hard to change the implementation to allow GLX and EGL. About GLES and Desktop GL, it's a bit more complicated, as many configurations still happen in compile time. But not impossible.

For this first set of patches I'm focusing only on TARGET_X11 (the only one we support now). In other tasks, I would extend the behavior for Wayland, and this will imply adding proper checks to avoid X11 dependency as needed.

> Perhaps you should hide this behind #if (USE_GLX || USE_EGL). There are types of OpenGL that do not use X11 like WGL and AGL.

As I told just in previous paragraph, I think it's better to keep the patch simpler and only add support for X11 target now. Later, in a different task, I'll add the cases for wayland, and that already implies refactoring code and moving X specific code to better places.

> > Source/WebCore/platform/graphics/cairo/GraphicsContext3DCairo.cpp:39
> > +#if USE(OPENGL_ES_2)
> > +#include "Extensions3DOpenGLES.h"
> > +#include "OpenGLESShims.h"
> > +#else
> >  #include "Extensions3DOpenGL.h"
> > +#include "OpenGLShims.h"
> > +#endif
> 
> These conditional sections should go after the non-conditional ones. Can we avoid the use of OpenGLESShims.h here?

As it is now, no. There are calls like glGenFramebufferEXT, that should be mapped to the proper glGenFramebuffer. But in theory the OpenGLShims are exactly to map as much as possible GL API to GLES (the one expected in ANGLE). So I'll move the methods to use GLES API.

> 
> > Source/WebCore/platform/graphics/cairo/GraphicsContext3DCairo.cpp:171
> > +#if !USE(OPENGL_ES_2)
> >  void GraphicsContext3D::releaseShaderCompiler()
> >  {
> >      notImplemented();
> >  }
> > +#endif
> 
> This is defined for both OPENGL_ES_2 and Desktop GL and has an empty implementation. Why hide it behind an #ifdef?

Because it is implemented in GraphicsContext3DOpenGLES.cpp. If I don't hide it, there will be 2 definitions: the one in *Cairo.cpp and the one in *OpenGLES.cpp.

> 
> > Source/WebCore/platform/graphics/cairo/GraphicsContext3DPrivate.cpp:134
> > -        glBindFramebuffer(GraphicsContext3D::FRAMEBUFFER, m_context->m_boundFBO);
> > +        ::glBindFramebufferEXT(GraphicsContext3D::FRAMEBUFFER, m_context->m_boundFBO);
> 
> This seems like the wrong direction. In OpenGLESShims.h glBindFramebufferEXT is just defined to glBindFramebuffer, so why not just not use glBindFramebuffer in this file?

Sure. I'll fix it.

> 
> > Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:43
> > +#if USE(OPENGL_ES_2)
> > +static const EGLenum gGLApi = EGL_OPENGL_ES_API;
> > +#else
> > +static const EGLenum gGLApi = EGL_OPENGL_API;
> > +#endif
> 
> Ideally we'd decide at runtime what type of API is accelerated, but we can tackle that later.

Yes, guess we can create a new bug for allowing GL/GLES2 API usage on runtime. It implies changes in many places, so it is better that we separate that task.


> > Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:96
> > +// Because of driver bugs, exiting the program when there are active pbuffers
> > +// can crash the X server (this has been observed with the official Nvidia drivers).
> > +// We need to ensure that we clean everything up on exit. There are several reasons
> > +// that GraphicsContext3Ds will still be alive at exit, including user error (memory
> > +// leaks) and the page cache. In any case, we don't want the X server to crash.
> > ...
> 
> The Nvidia driver doesn't support EGL at the moment, so maybe we can remove this work-around?

I'll try to check it doesn't break anything different.

> 
> > Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:120
> > +    cleanupSharedDisplay();
> 
> I think it makes sense for GLContext to install the atexit handler to cleanup the display, since it creates it.

Sure. I can refactor the context counting too, as it's present in both drivers. In case we drop the context counting for EGL (only needed for nvidia), then we could have independent atexit handlers in GLContext and GLContextGLX. With current structure, I guess we would end cleaning shared display 2 times (one for EGL and one for GLX, in case we allow to choose the context in runtime.

> 
> > Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:155
> > +PassOwnPtr<GLContextEGL> GLContextEGL::createWindowContext(EGLNativeWindowType wId, GLContext* sharingContext)
> 
> windowId instead of wId. WebKit doesn't abbreviate variables unless the abbreviation is very common.

In fact it's even worse. It shouldn't be a windowId, but a window, with the internal type expected as EGLNativeWindowType (which happens to be a Window in X11, or a wl_egl_window* in Wayland).

> > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:143
> > +            ::glBindRenderbufferEXT(GraphicsContext3D::RENDERBUFFER, m_depthStencilBuffer);
> > +            ::glRenderbufferStorageEXT(GraphicsContext3D::RENDERBUFFER, GraphicsContext3D::DEPTH24_STENCIL8, width, height);
> 

> Maybe what we can do to simplify this is to do this:
> 
> 1. Enable GLX if we find the headers.
> 2. Enable EGL if we find the headers.
> 3. Remove the configuration option for both of them.
> 4. Pick the appropriate rendering type:
>    a. If the CPU is not ARM --> Look for the GL headers and fall back to the GLES headers to choose the rendering type.
>    b. If the CPU is ARM --> Look for the GLES headers and fall back to the GL headers to choose the rendering type.
> 5. Now if you've found (egl || glx) && ((opengl && glx) || egl) then turn on OpenGL by default (--with-acceleration-backend defaults to opengl)
> 6. If --with-acceleration-backend is passed the opengl option and the above conditional is false, give an error about what headers are missing.

I would keep the configuration options. It allows to test the different backends (i.e. I'm running on X desktop, but I may want just to check egl+gles2 or even egl+desktop gl, even when the recommended setup is glx). But I'll set the defaults this way:
* Allow GLX and EGL both turned on. Having GLX at least will imply Desktop GL in this case. No GLES2 allowed (GLX only supports Desktop GL API).
* In case only EGL is enabled, give priority to gles2. If available use gles2, if not use desktop gl.
* Explicitely enabling glx and gles2, or enabling gles2 and disabling egl will show an error.
* I think we shouldn't use architecture for choosing GL or GLES2 or EGL or GLX. Now we can enable both GLX and EGL, so if detected, then it can build with both. It'll prefer GLX in this case. About GLES2 or GL, for GLX it'll choose GL. For EGL and not GLX, it'll prefer GLES2 (if found).

[Martin Robinson]

(In reply to comment #12)

> > Perhaps you should hide this behind #if (USE_GLX || USE_EGL). There are types of OpenGL that do not use X11 like WGL and AGL.
> 
> As I told just in previous paragraph, I think it's better to keep the patch simpler and only add support for X11 target now. Later, in a different task, I'll add the cases for wayland, and that already implies refactoring code and moving X specific code to better places.

I'm not talking about Wayland here, I'm talking about the Mac and Windows builds.  GLContext doesn't run there, but we shouldn't break it more if we can help it. Eventually one of these ports may want to use this code.

> As it is now, no. There are calls like glGenFramebufferEXT, that should be mapped to the proper glGenFramebuffer. But in theory the OpenGLShims are exactly to map as much as possible GL API to GLES (the one expected in ANGLE). So I'll move the methods to use GLES API.

Calls to glGenFramebufferEXT should be changed to glGenFramebuffer, except in files that do not use OpenGLShims.h for all ports. Practically this means that for files in the Cairo and GTK+ directories, you should use the EXT or ARB suffixes.

> > > Source/WebCore/platform/graphics/cairo/GraphicsContext3DCairo.cpp:171
> > > +#if !USE(OPENGL_ES_2)
> > >  void GraphicsContext3D::releaseShaderCompiler()
> > >  {
> > >      notImplemented();
> > >  }
> > > +#endif
> > 
> > This is defined for both OPENGL_ES_2 and Desktop GL and has an empty implementation. Why hide it behind an #ifdef?
> 
> Because it is implemented in GraphicsContext3DOpenGLES.cpp. If I don't hide it, there will be 2 definitions: the one in *Cairo.cpp and the one in *OpenGLES.cpp.

Perhaps this can simply be moved to GraphicsContext3DOpenGLcpp then? Do any ports have an implementation here?

> I would keep the configuration options. It allows to test the different backends (i.e. I'm running on X desktop, but I may want just to check egl+gles2 or even egl+desktop gl, even when the recommended setup is glx). But I'll set the defaults this way:
> * Allow GLX and EGL both turned on. Having GLX at least will imply Desktop GL in this case. No GLES2 allowed (GLX only supports Desktop GL API).
> * In case only EGL is enabled, give priority to gles2. If available use gles2, if not use desktop gl.
> * Explicitely enabling glx and gles2, or enabling gles2 and disabling egl will show an error.
> * I think we shouldn't use architecture for choosing GL or GLES2 or EGL or GLX. Now we can enable both GLX and EGL, so if detected, then it can build with both. It'll prefer GLX in this case. About GLES2 or GL, for GLX it'll choose GL. For EGL and not GLX, it'll prefer GLES2 (if found).

This seems reasonable for now. Time will probably tell what configuration defaults are the best.

[José Dapena Paz]

Created attachment 165571 [details]
Patch

New iteration of patch following Martin proposed changes

[Build Bot]

Comment on attachment 165571 [details]
Patch

Attachment 165571 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14002706

[Early Warning System Bot]

Comment on attachment 165571 [details]
Patch

Attachment 165571 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14033001

[Early Warning System Bot]

Comment on attachment 165571 [details]
Patch

Attachment 165571 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13962039

[José Dapena Paz]

Created attachment 165600 [details]
Patch

Fix qt and mac builds

[Martin Robinson]

Comment on attachment 165600 [details]
Patch

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

Looks good! Just a few minor thing. I'm incredibly sorry that it took me so long to review this patch. :(

> Source/WebCore/platform/graphics/cairo/GraphicsContext3DPrivate.cpp:33
> +#if USE(OPENGL_ES_2)
> +#include "OpenGLESShims.h"
> +#include <GLES2/gl2.h>
> +#include <GLES2/gl2ext.h>
> +#else
>  #include "OpenGLShims.h"
> +#endif

These blocks of includes should be after the main blocks like this:

#include "HostWindow.h"
#include "NotImplemented.h"
include "PlatformContextCairo.h"
#include <wtf/OwnArrayPtr.h>

#if USE(OPENGL_ES_2)
#include "OpenGLESShims.h"
#include <GLES2/gl2.h>
#include <GLES2/gl2ext.h>
#else
#include "OpenGLShims.h"
#endif

> Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:39
> +static const EGLenum gGLApi = EGL_OPENGL_ES_API;

Nit: This should be called gGLAPI

> Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:82
> +void GLContextEGL::addActiveContext(GLContextEGL* context)
> +{
> +    static bool addedAtExitHandler = false;
> +    if (!addedAtExitHandler) {
> +        atexit(&GLContextEGL::cleanupSharedEGLDisplay);
> +        addedAtExitHandler = true;
> +    }
> +    GLContext::addActiveContext(context);
> +}

I think this work-around could possibly be removed for EGL. That would make this patch quite a bit smaller. Did you find that it prevented crashes? It's probably okay to let the display leak here.

> Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:109
> +    if (surfaceType == GLContextEGL::PbufferSurface) {
> +        attributeList[10] = EGL_SURFACE_TYPE;
> +        attributeList[11] = EGL_PBUFFER_BIT;
> +    } else if (surfaceType == GLContextEGL::PixmapSurface) {
> +        attributeList[10] = EGL_SURFACE_TYPE;
> +        attributeList[11] = EGL_PIXMAP_BIT;
> +    } else if (surfaceType == GLContextEGL::WindowSurface) {
> +        attributeList[10] = EGL_SURFACE_TYPE;
> +        attributeList[11] = EGL_WINDOW_BIT;

It looks like you could set attributeList[10] when you create attributeList above and then do a switch statement to set the actual surface type. A switch statement is important here because it will make the compiler emit a warning if you added a new EGLSurfaceType and didn't handle it, in the future.

> Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:242
> +    removeActiveContext(this);

Assuming the patch continues to keep a list of contexts for cleaning up, why not move this call to removeActiveContext and the one for addActiveContext above to the superclass constructor and destructor?

> Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:277
> +    if (m_surface)
> +        eglSwapBuffers(sharedEGLDisplay(), m_surface);

It seems that you always have a surface, so this should be:

ASSERT(m_surface);
eglSwapBuffers(sharedEGLDisplay(), m_surface);

> Source/WebCore/platform/gtk/RedirectedXCompositeWindow.cpp:53
> -    Display* display = GLContextGLX::sharedDisplay();
> +    Display* display = GLContext::sharedX11Display();

I think this has changed now, so you may need to rebase. Sorry. :(

[José Dapena Paz]

Created attachment 167323 [details]
Patch

Patch with suggested changes.

[José Dapena Paz]

Created attachment 167324 [details]
Patch

Patch with suggested changes.

[Martin Robinson]

Comment on attachment 167324 [details]
Patch

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

Looks good, but a few minor nits. I'll fix them and land this patch.

> Source/WebCore/platform/graphics/cairo/GraphicsContext3DCairo.cpp:49
> +#include "OpenGLShims.h"

Maybe we can omit this since we don't need access to redefinitions of common functionality.

> Source/WebCore/platform/graphics/cairo/GraphicsContext3DPrivate.cpp:135
> +        ::glBindFramebufferEXT(GraphicsContext3D::FRAMEBUFFER, m_context->m_boundFBO);

Can't we go the other way here and leave glBindFramebuffer?

> Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:208
> +    : GLContext()

The compiler should automatically call the default constructor.

> Source/WebCore/platform/graphics/glx/GLContextGLX.cpp:166
> +    : GLContext()

Ditto.

> Source/WebCore/platform/graphics/glx/GLContextGLX.cpp:176
> +    : GLContext()

Ditto.

[Martin Robinson]

Committed r130525: <http://trac.webkit.org/changeset/130525>