Bug 59064 – [GTK] Add support for OpenGL ES and EGL

• Component: WebGL
• Status: RESOLVED FIXED

[ChangSeok Oh]

WebGL is currently running with GLX backend for GTK port on Linux based system.
This bug is for remark to add EGL backend for WebGL on same system.

[ChangSeok Oh]

Created attachment 99720 [details]
a draft patch

[ChangSeok Oh]

Created attachment 100076 [details]
Proposed patch

@Martin. Would you mind reviewing this patch? 
I think you're very familiar with WebGL for GTK port, so the best reviewer. :)

[WebKit Review Bot]

Attachment 100076 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/ChangeLog', u..." exit_code: 1

Source/WebCore/platform/graphics/gtk/egl/GraphicsContext3DInternal.cpp:23:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[ChangSeok Oh]

Created attachment 100769 [details]
updated patch

Fixed style issue.

[Martin Robinson]

Comment on attachment 100769 [details]
updated patch

This should be possible without copying all the code in GraphicsContext3DOpenGL.cpp, GraphicsContext3DInternal.cpp, and GraphicsContext3DGtk.cpp. Most of looks very similar.

[ChangSeok Oh]

(In reply to comment #5)
> (From update of attachment 100769 [details])
> This should be possible without copying all the code in GraphicsContext3DOpenGL.cpp, GraphicsContext3DInternal.cpp, and GraphicsContext3DGtk.cpp. Most of looks very similar.
You're right. They're very similar but a little different actually.

Newly added GraphicsContext3DInternal.cpp is implemented based on EGL(NOT GLX), so we can get eglContext and eglSurface.

GrahpicsContext3DGtk.cpp is in same situation like GraphicsContext3DInternal.cpp. Newly added GraphicsContext3DGtk.cpp is implemented base on OpenGLES not OpenGL. Some GL APIs need to change to fit GLES.

Each APIs of GraphicsContext3DOpenGL.cpp include extra code to convert OpenGL based parameter into OpenGLES. It means these extra codes are useless when we choose elg/opengles for gl-backend.

Why I seperate implementations for WebGL-APIs from original file is that I've thought it looks more clear and easy to understand. Otherwise, many build flags like #if USE(EGL) may be needed to distinguish between GL-base implemetation and GLES-base implementation throughout the each files. Do you think this way is better? I don't mind whatever ways.

Thanks for comment. :)

[ChangSeok Oh]

Created attachment 101722 [details]
updated patch

[Martin Robinson]

CCing cmarrin, as he might have some comments about the GraphicsContextOpenGL.cpp changes.

[Kenneth Russell]

Comment on attachment 101722 [details]
updated patch

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

The #ifdefs added throughout GraphicsContext3DOpenGL.cpp are going to be poor to maintain. I think a better way to do this would be to move most of the code from GraphicsContext3DOpenGL.cpp into a GraphicsContext3DOpenGLBase.cpp. In those routines where different behavior is needed, define implementations of the methods in GraphicsContext3DOpenGL.cpp and for example GraphicsContext3DOpenGLES.cpp.

See for example Source/WebCore/platform/graphics/cg/GraphicsContext3DCG.cpp and Source/WebCore/platform/graphics/skia/GraphicsContext3DSkia.cpp .

I'm refraining from touching the review bit in case others have differing opinions, since Chromium doesn't actually use this code.

> Source/WebCore/platform/graphics/gtk/GraphicsContext3DGtk.cpp:94
> +#if USE(EGL)
> +    ::glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE);
> +    ::glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE);
> +#else
>      ::glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP);
>      ::glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP);
> +#endif

GL_CLAMP_TO_EDGE should be used in both cases.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:1587
> +    if (type == GL_FLOAT)
> +        return false; // Not supported in GLES 2.0

If the GL_OES_texture_float extension is available, then this would be supported.

[ChangSeok Oh]

Comment on attachment 101722 [details]
updated patch

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

Thank you for review. :)

I split GraphicsContext3DOpenGL.cpp into three files, GraphicsContext3DOpenGLBase.cpp, GraphicsContext3DOpenGL.cpp and GraphicsContext3DOpenGLES.cpp as you mentioned.

>> Source/WebCore/platform/graphics/gtk/GraphicsContext3DGtk.cpp:94
>> +#endif
> 
> GL_CLAMP_TO_EDGE should be used in both cases.

Done.

>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:1587
>> +        return false; // Not supported in GLES 2.0
> 
> If the GL_OES_texture_float extension is available, then this would be supported.

Done. I removed this.

[ChangSeok Oh]

Created attachment 102630 [details]
updated patch

This patch has one style warning, but it's not mine.
I hope QT folks take care of it.

[WebKit Review Bot]

Attachment 102630 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/ChangeLog', u..." exit_code: 1

Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLBase.cpp:34:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kenneth Russell]

Comment on attachment 102630 [details]
updated patch

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

Are you sure the style queue failure is due to the presence of the Qt header

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLBase.cpp:35
> +

Are you sure the style queue failure is due to the presence of the Qt header, and not due to the name GraphicsContext3D.h (rather than GraphicsContext3DOpenGLBase.h) being used here? If it's being triggered by the latter, you could create a GraphicsContext3DOpenGLBase.h which just includes GraphicsContext3D.h as a workaround.

[ChangSeok Oh]

(In reply to comment #13)
> (From update of attachment 102630 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102630&action=review
> 
> Are you sure the style queue failure is due to the presence of the Qt header
> 
> > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLBase.cpp:35
> > +
> 
> Are you sure the style queue failure is due to the presence of the Qt header, and not due to the name GraphicsContext3D.h (rather than GraphicsContext3DOpenGLBase.h) being used here? If it's being triggered by the latter, you could create a GraphicsContext3DOpenGLBase.h which just includes GraphicsContext3D.h as a workaround.

O.K. Let me check it again.

[ChangSeok Oh]

(In reply to comment #13)
> (From update of attachment 102630 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102630&action=review
> 
> Are you sure the style queue failure is due to the presence of the Qt header
> 
> > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLBase.cpp:35
> > +
> 
> Are you sure the style queue failure is due to the presence of the Qt header, and not due to the name GraphicsContext3D.h (rather than GraphicsContext3DOpenGLBase.h) being used here? If it's being triggered by the latter, you could create a GraphicsContext3DOpenGLBase.h which just includes GraphicsContext3D.h as a workaround.
Warning is gone if I move or remove Qt header.
Whereas I change the header GraphicsContext3D.h to GraphicsContext3DOpenGLBase.h, warning is still there.
Of course, I can move Qt header to avoid style warning in this patch, but I can't convince no problem.

[Kenneth Russell]

Comment on attachment 102630 [details]
updated patch

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

The common code looks fine to me. I would suggest you similarly split the Gtk GraphicsContext3D implementation files to avoid all of the #if USE(EGL) constructs that have been added there. However, I'd be fine r+'ing this as is. I'll defer to mrobinson in case he wants to re-review, but ping me if you want me to r+ it.

>>>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLBase.cpp:35
>>>> +
>>> 
>>> Are you sure the style queue failure is due to the presence of the Qt header, and not due to the name GraphicsContext3D.h (rather than GraphicsContext3DOpenGLBase.h) being used here? If it's being triggered by the latter, you could create a GraphicsContext3DOpenGLBase.h which just includes GraphicsContext3D.h as a workaround.
>> 
>> O.K. Let me check it again.
> 
> Warning is gone if I move or remove Qt header.
> Whereas I change the header GraphicsContext3D.h to GraphicsContext3DOpenGLBase.h, warning is still there.
> Of course, I can move Qt header to avoid style warning in this patch, but I can't convince no problem.

OK. Thanks for double-checking.

[ChangSeok Oh]

(In reply to comment #16)
> (From update of attachment 102630 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102630&action=review
> 
> The common code looks fine to me. I would suggest you similarly split the Gtk GraphicsContext3D implementation files to avoid all of the #if USE(EGL) constructs that have been added there. However, I'd be fine r+'ing this as is. I'll defer to mrobinson in case he wants to re-review, but ping me if you want me to r+ it.
> 
> >>>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLBase.cpp:35
> >>>> +
> >>> 
> >>> Are you sure the style queue failure is due to the presence of the Qt header, and not due to the name GraphicsContext3D.h (rather than GraphicsContext3DOpenGLBase.h) being used here? If it's being triggered by the latter, you could create a GraphicsContext3DOpenGLBase.h which just includes GraphicsContext3D.h as a workaround.
> >> 
> >> O.K. Let me check it again.
> > 
> > Warning is gone if I move or remove Qt header.
> > Whereas I change the header GraphicsContext3D.h to GraphicsContext3DOpenGLBase.h, warning is still there.
> > Of course, I can move Qt header to avoid style warning in this patch, but I can't convince no problem.
> 
> OK. Thanks for double-checking.
Thanks for kind review! :)

> I would suggest you similarly split the Gtk GraphicsContext3D implementation files to avoid all of the #if USE(EGL) constructs that have been added there.
@mrobinson What do you think about this? Do you think it'd be better to split GraphicsContext3DInternal.cpp & GraphicsContext3DGtk.cpp?

[Martin Robinson]

Comment on attachment 102630 [details]
updated patch

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

I think some of these #ifdefs can be removed entirely. For instance in GraphicsContext3DGtk, you can eliminate almost all of them. It probably makes sense to split GraphicsContext3DInternal though.

> Source/WebCore/platform/graphics/gtk/GraphicsContext3DGtk.cpp:99
> +#if USE(EGL)
> +    ::glGenFramebuffers(1, &m_fbo);
> +    ::glBindFramebuffer(GL_FRAMEBUFFER, m_fbo);
> +#else
>      ::glGenFramebuffersEXT(1, &m_fbo);
>      ::glBindFramebufferEXT(GL_FRAMEBUFFER_EXT, m_fbo);
> +#endif

Adding an alias in OpenGLShims can eliminate this #ifdef.

> Source/WebCore/platform/graphics/gtk/GraphicsContext3DGtk.cpp:171
> +        if (m_attrs.stencil || m_attrs.depth) {
> +            ::glDeleteRenderbuffers(1, &m_depthBuffer);
> +            ::glDeleteRenderbuffers(1, &m_stencilBuffer);
> +        }
> +#else
>          if (m_attrs.stencil || m_attrs.depth)

Does EGL use the OpenGLShims.h. If so you should be able to alias glDeleteFramebuffersEXT to glDeleteFramebuffers and remove this #ifdef entirely.

> Source/WebCore/platform/graphics/gtk/GraphicsContext3DGtk.cpp:176
> +    ::glDeleteFramebuffers(1, &m_fbo);

Ditto for glDeleteFramebuffers and glDeleteFramebuffersEXT.

> Source/WebCore/platform/graphics/gtk/GraphicsContext3DInternal.cpp:113
> +#if USE(EGL)
> +    EGLDisplay eglDisplay = eglGetDisplay(sharedDisplay());
> +    if (eglDisplay == EGL_NO_DISPLAY)
> +        return nullptr;
> +    if (!initialized) {
> +        if (!eglInitialize(eglDisplay, 0, 0))
> +            return nullptr;
> +        initialized = true;
> +    }
> +#else

Instead of doing this, why not have sharedDisplay return an EGLDisplay?

> Source/WebCore/platform/graphics/gtk/GraphicsContext3DInternal.cpp:116
>          success = initializeOpenGLShims();

Why shouldn't EGL use the OpenGL shims?

> Source/WebCore/platform/graphics/gtk/GraphicsContext3DInternal.cpp:134
> +#if USE(EGL)
> +    GraphicsContext3DInternal* internal = createPbufferContext(eglDisplay);
> +#else
>      GraphicsContext3DInternal* internal = createPbufferContext();
> +#endif
>      if (!internal)
> +#if USE(EGL)
> +        internal = createPixmapContext(eglDisplay);
> +#else
>          internal = createPixmapContext();
> +#endif
>      if (!internal)

If you take my advice above you can remove these #ifdefs and just call sharedDisplay in these methods.

> Source/WebCore/platform/graphics/gtk/GraphicsContext3DInternal.cpp:413
>  } // namespace WebCore

I don't think there is any shared code in this file. Would it make sense to split it out into GraphicsContext3DInternalEGL.cpp?

> Source/WebCore/platform/graphics/gtk/GraphicsContext3DInternal.h:79
>  }

Almost nothing is shared here too.

[ChangSeok Oh]

Comment on attachment 102630 [details]
updated patch

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

Thanks for review!

>> Source/WebCore/platform/graphics/gtk/GraphicsContext3DGtk.cpp:99
>> +#endif
> 
> Adding an alias in OpenGLShims can eliminate this #ifdef.

Done.

>> Source/WebCore/platform/graphics/gtk/GraphicsContext3DGtk.cpp:171
>>          if (m_attrs.stencil || m_attrs.depth)
> 
> Does EGL use the OpenGLShims.h. If so you should be able to alias glDeleteFramebuffersEXT to glDeleteFramebuffers and remove this #ifdef entirely.

You're right and I've done as your comment.
But I think this case needs #ifdef, becuase there is no way to handle depth buffer and stencil buffer at the same time.

>> Source/WebCore/platform/graphics/gtk/GraphicsContext3DGtk.cpp:176
>> +    ::glDeleteFramebuffers(1, &m_fbo);
> 
> Ditto for glDeleteFramebuffers and glDeleteFramebuffersEXT.

Done.

>> Source/WebCore/platform/graphics/gtk/GraphicsContext3DInternal.cpp:113
>> +#else
> 
> Instead of doing this, why not have sharedDisplay return an EGLDisplay?

Done. I added sharedEGLDisplay() like sharedDisplay().

>> Source/WebCore/platform/graphics/gtk/GraphicsContext3DInternal.cpp:116
>>          success = initializeOpenGLShims();
> 
> Why shouldn't EGL use the OpenGL shims?

Hm... my understanding about OpenGLShims was not enough. :p
Now EGL uses OpenGLShims.

>> Source/WebCore/platform/graphics/gtk/GraphicsContext3DInternal.cpp:134
>>      if (!internal)
> 
> If you take my advice above you can remove these #ifdefs and just call sharedDisplay in these methods.

Done.

>> Source/WebCore/platform/graphics/gtk/GraphicsContext3DInternal.cpp:413
>>  } // namespace WebCore
> 
> I don't think there is any shared code in this file. Would it make sense to split it out into GraphicsContext3DInternalEGL.cpp?

Done. 
I added GraphicsContext3DInternalEGL.cpp and GraphicsContext3DInteranlEGL.h newly.

>> Source/WebCore/platform/graphics/gtk/GraphicsContext3DInternal.h:79
>>  }
> 
> Almost nothing is shared here too.

ditto.

[ChangSeok Oh]

Created attachment 103348 [details]
updated patch

[WebKit Review Bot]

Attachment 103348 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/ChangeLog', u..." exit_code: 1

Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLBase.cpp:34:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[ChangSeok Oh]

Created attachment 103440 [details]
updated patch

Fixed style warning.
I tried building qt port enabled WebGL. No issue found.

[ChangSeok Oh]

Created attachment 103441 [details]
updated patch

[ChangSeok Oh]

@mrobinson Review please?

[Martin Robinson]

Comment on attachment 103441 [details]
updated patch

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

It looks good to me in general, but I still would like a nod from someone more familiar with GraphicsContext3DOpenGL.cpp.

> Source/WebCore/ChangeLog:11
> +        Add build-flags like WTF_USE_GLX & WTF_USE_EGL to seperate gl-backends.
> +        Almost GL common APIs in GraphicsContext3DOpenGL.cpp are moved to GraphicsContext3DBase.cpp
> +        And the others APIs needed to seperate according to gl-backend are located at GraphicsContext3DOpenGL.cpp and GraphicsContext3DOpenGLES.cpp.
> +        Some GL extention parameters which are not defined in GLES are added newly in OpenGLShims.h

Do you mind evening out the ChangeLog lines here?

> Source/WebCore/platform/graphics/cairo/OpenGLShims.cpp:122
> +    openGLFunctionTable()->glBlitFramebuffer = ::glBlitFramebufferANGLE;

Why was this change necessary?

> Source/WebCore/platform/graphics/cairo/OpenGLShims.h:54
> +#if !defined(GL_FRAMEBUFFER_EXT)
> +#define GL_FRAMEBUFFER_EXT GL_FRAMEBUFFER
> +#endif
> +#if !defined(GL_COLOR_ATTACHMENT0_EXT)
> +#define GL_COLOR_ATTACHMENT0_EXT GL_COLOR_ATTACHMENT0
> +#endif
> +#if !defined(GL_RENDERBUFFER_EXT)
> +#define GL_RENDERBUFFER_EXT GL_RENDERBUFFER
>  #endif
> +#if !defined(GL_STENCIL_ATTACHMENT_EXT)
> +#define GL_STENCIL_ATTACHMENT_EXT GL_STENCIL_ATTACHMENT
> +#endif
> +#if !defined(GL_DEPTH_ATTACHMENT_EXT)
> +#define GL_DEPTH_ATTACHMENT_EXT GL_DEPTH_ATTACHMENT
> +#endif
> +#if !defined(GL_FRAMEBUFFER_COMPLETE_EXT)
> +#define GL_FRAMEBUFFER_COMPLETE_EXT GL_FRAMEBUFFER_COMPLETE
> +#endif
> +#endif // End of GL_ES_VERSION_2_0

Should these lines be EGL only or is it safe to enable them for normal GL as well?

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:36
> +#if PLATFORM(GTK)
> +#include "OpenGLShims.h"
> +#endif

This should be in a separate block.

[Kenneth Russell]

Comment on attachment 103441 [details]
updated patch

The GraphicsContext3DOpenGL refactoring looks good to me personally. If you want cmarrin's review I suggest pinging him on IRC. I note that the Xcode projects haven't been updated, so this change is probably going to break the Mac port. It's too bad the EWS Mac bots don't work for non-committers but the commit queue will catch any breakage. You might have to do a couple of iterations.

[Chris Marrin]

I don't like the idea of restructuring the code at the same time as adding support for EGL. They should be separated. As the code stands now, it is not well structured for extension. So I think that task should be separated and this bug be made a dependency of that. I've opened https://bugs.webkit.org/show_bug.cgi?id=66903 to discuss this.

Until then, I've given this patch an r-

[Chris Marrin]

Comment on attachment 103441 [details]
updated patch

Rejecting until issues brought up in https://bugs.webkit.org/show_bug.cgi?id=66903 are resolved.

[Martin Robinson]

It seems that this can be closed now?

[ChangSeok Oh]

Yeap. I agree :)