[webkit-reviews] review denied: [Bug 77921] [GTK] Add support for creating EGL contexts : [Attachment 161993] Patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied José Dapena Paz
<jdapena at igalia.com>'s request for review:
Bug 77921: [GTK] Add support for creating EGL contexts
https://bugs.webkit.org/show_bug.cgi?id=77921

Attachment 161993: Patch
https://bugs.webkit.org/attachment.cgi?id=161993&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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.