Bug 118944

Created attachment 207198 [details] All changes, RFC version Here are all the proposed changes, though they will be split between smaller patches that would then get proper review. This is an RFC patch of sorts, requesting feedback on the approach to refactoring GLContextEGL and adding the EGLPlatform interface. I'll leave more comments about the proposed changes through the reviewing tool.

Comment on attachment 207198 [details] All changes, RFC version View in context: https://bugs.webkit.org/attachment.cgi?id=207198&action=review > Source/WebCore/platform/graphics/cairo/GLContext.cpp:44 > +#if PLATFORM(WAYLAND) > +#include <wayland-client.h> > +#endif > + Eek, not necessary here, will be removed. > Source/WebCore/platform/graphics/cairo/GLContext.cpp:177 > +#endif On GTK only, the GLContextGLX is used under X11 displays if GLX support is enabled and similarly GLContextEGL, using the EGLPlatformWayland, is used under Wayland displays if EGL support is enabled. > Source/WebCore/platform/graphics/cairo/GLContext.cpp:190 > - if (OwnPtr<GLContext> eglContext = GLContextEGL::createContext(windowHandle, sharingContext)) > +#if PLATFORM(X11) > + typedef EGLPlatformX EGLPlatformClass; > +#else > + typedef EGLPlatformDefault EGLPlatformClass; > +#endif > + if (OwnPtr<GLContext> eglContext = GLContextEGL<EGLPlatformClass>::createContext(windowHandle, sharingContext)) Non-GTK ports fall back to the current behavior when the EGL support is enabled - X11-based ports use the new EGLPlatformX interface, other ports use the default EGLPlatformDefault interface as the EGL platform for the GLContextEGL. > Source/WebCore/platform/graphics/egl/EGLPlatform.cpp:13 > +EGLPlatform::EGLPlatform() > + : m_context(EGL_NO_CONTEXT) > + , m_surface(EGL_NO_SURFACE) > + , m_type(Unknown) The EGLPlatform class starts off as invalid - no context and surface are currently in use. > Source/WebCore/platform/graphics/egl/EGLPlatform.cpp:22 > +EGLBoolean EGLPlatform::chooseConfig(EGLConfig* config , EGLPlatform::SurfaceType surfaceType) const This implementation is at the moment common to all the EGLPlatforms but is still left extensible. > Source/WebCore/platform/graphics/egl/EGLPlatform.cpp:58 > +EGLenum EGLPlatform::glAPI() const Ditto. > Source/WebCore/platform/graphics/egl/EGLPlatform.cpp:67 > +EGLint* EGLPlatform::contextAttributes() const Ditto. > Source/WebCore/platform/graphics/egl/EGLPlatform.h:16 > +class EGLPlatform { This is the base EGLPlatform interface. It holds the EGLContext and EGLSurface that are in use by the GLContextEGL class that's extending it at the bottom of the inheritance stack. > Source/WebCore/platform/graphics/egl/EGLPlatform.h:32 > + virtual EGLDisplay sharedDisplay() const = 0; > + virtual EGLBoolean chooseConfig(EGLConfig*, SurfaceType) const; > + virtual EGLenum glAPI() const; > + virtual EGLint* contextAttributes() const; These four methods are overridable by the inheriting classes, but at the moment only sharedDisplay is necessary to be implemented by different EGL platforms as it controls the platform-specific way of acquiring the shared EGL display. > Source/WebCore/platform/graphics/egl/EGLPlatformDefault.h:10 > +class EGLPlatformDefault : protected EGLPlatform { EGLPlatformDefault replicates the current, non-X11 behavior of the GLContextEGL class. It basically only differs from the current X11-specific behavior (that's now located in EGLPlatformX11) that it gets the shared EGL display by passing EGL_DEFAULT_DISPLAY to eglGetDisplay and that it doesn't support creating pixmap surfaces. > Source/WebCore/platform/graphics/egl/EGLPlatformWayland.cpp:18 > +EGLPlatformWayland::EGLPlatformWayland(EGLPlatformWayland::NativeWindowType nativeWindow, EGLContext sharingContext) This is the Wayland-specific EGLPlatform implementation. It relies on the 'dummy wl_surface' approach to acquire the EGLSurface that's then used for rendering. > Source/WebCore/platform/graphics/egl/EGLPlatformWayland.cpp:54 > +#if PLATFORM(GTK) && defined(GDK_WINDOWING_WAYLAND) > + GdkDisplay* display = gdk_display_manager_get_default_display(gdk_display_manager_get()); > + ASSERT(GDK_IS_WAYLAND_DISPLAY(display)); > + struct wl_display* wlDisplay = gdk_wayland_display_get_wl_display(display); > + sharedEGLDisplay = eglGetDisplay(wlDisplay); > +#endif The GTK-specific bits to acquiring the shared EGL display, relying on the wl_display as provided by the default GdkDisplay. > Source/WebCore/platform/graphics/egl/EGLPlatformWayland.cpp:84 > +#if PLATFORM(GTK) && defined(GDK_WINDOWING_WAYLAND) > + ASSERT(!nativeWindow); > + > + GdkDisplay* gdkDisplay = gdk_display_manager_get_default_display(gdk_display_manager_get()); > + ASSERT(GDK_IS_WAYLAND_DISPLAY(gdkDisplay)); > + > + struct wl_compositor* wlCompositor = gdk_wayland_display_get_wl_compositor(gdkDisplay); > + if (!wlCompositor) > + return EGL_NO_SURFACE; > + > + struct wl_surface* wlSurface = wl_compositor_create_surface(wlCompositor); > + if (!wlSurface) > + return EGL_NO_SURFACE; > + > + struct wl_egl_window* eglWindow = wl_egl_window_create(wlSurface, 0, 0); > + if (!eglWindow) > + return EGL_NO_SURFACE; > + > + return eglCreateWindowSurface(sharedDisplay(), config, eglWindow, 0); > +#else The GTK-specific bits to creating the dummy wl_surface object and the subsequent wl_egl_window that then serves as the argument to eglCreateWindowSurface call. > Source/WebCore/platform/graphics/egl/EGLPlatformWayland.h:14 > + typedef struct wl_egl_window* NativeWindowType; These native window types are defined by each EGLPlatform* class, with the definition reflecting that of EGLNativeWindowType in <EGL/eglplatform.h>. This helps later in the GLContextEGL class. > Source/WebCore/platform/graphics/egl/EGLPlatformWayland.h:16 > + EGLPlatformWayland(NativeWindowType, EGLContext sharingContext); On a self-reviewing note, I believe that constructors in these classes could be made private. > Source/WebCore/platform/graphics/egl/EGLPlatformX.cpp:11 > +EGLPlatformX::EGLPlatformX(EGLPlatformX::NativeWindowType nativeWindow, EGLContext sharingContext) This class preserves the current X11-specific behavior of the GLContextEGL class. It gets the shared EGL display by using the GLContext::sharedX11Display() return value and can create either window, pbuffer or pixmap EGLSurfaces. > Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:64 > + OwnPtr<GLContextEGL<EGLPlatformClass> > context = adoptPtr(new GLContextEGL<EGLPlatformClass>(nativeWindow, sharingContext)); > + if (context->isValid()) > + return context.release(); This creates a new GLContextEGL object and returns it only if it's valid, i.e. the EGLPlatform* interface successfully acquired both the EGLContext and EGLSurface. > Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:169 > +#if PLATFORM(WAYLAND) > +template class GLContextEGL<EGLPlatformWayland>; > +#endif > +#if PLATFORM(X11) > +template class GLContextEGL<EGLPlatformX>; > +#endif > +#if !PLATFORM(WAYLAND) && !PLATFORM(X11) > +template class GLContextEGL<EGLPlatformDefault>; > +#endif This declares all the GLContextEGL variants that must be compiled. > Source/WebCore/platform/graphics/egl/GLContextEGL.h:-27 > -#include <EGL/egl.h> The proposed refactoring makes it possible to stop including the EGL header here. This was problematic as it also defined the EGL platform, by default that's the X11 platform. Specifying the native window type (as required by GLContextEGL::createContext) through the EGLPlatform* classes avoids that. > Source/WebCore/platform/graphics/egl/GLContextEGL.h:29 > +typedef void *EGLContext; > +typedef void *EGLSurface; Should be removed. > Source/WebCore/platform/graphics/egl/GLContextEGL.h:34 > +template<class EGLPlatformClass> > +class GLContextEGL : public GLContext, protected EGLPlatformClass { GLContextEGL now also inherits from the given EGLPlatform* class, making it possible to store the EGLContext and EGLSurface objects as members of the EGLPlatform interface and also making EGLPlatform a proper, extensible interface instead of just a class with loads of static methods. > Source/WebCore/platform/graphics/egl/GLContextEGL.h:37 > - enum EGLSurfaceType { PbufferSurface, WindowSurface, PixmapSurface }; > - static PassOwnPtr<GLContextEGL> createContext(EGLNativeWindowType, GLContext* sharingContext = 0); > - static PassOwnPtr<GLContextEGL> createWindowContext(EGLNativeWindowType, GLContext* sharingContext); > + static PassOwnPtr<GLContextEGL> createContext(typename EGLPlatformClass::NativeWindowType, GLContext* sharingContext = 0); EGLSurfaceType now lives as EGLPlatform::SurfaceType. createWindowContext was removed from the public interface as it's not currently called from anywhere. createContext now takes the EGLNativeWindowType (as redefined as EGLPlatform*::NativeWindowType in each EGLPlatform* class) as the first parameter. > Source/autotools/FindDependencies.m4:185 > + > + PKG_CHECK_MODULES([GTK_WAYLAND], [gtk+-wayland-$GTK_API_VERSION], [have_gtk_wayland=yes], [have_gtk_wayland=no]) Checks if GTK comes with the Wayland backend. > Source/autotools/FindDependencies.m4:193 > + PKG_CHECK_MODULES([WAYLAND_CLIENT], [wayland-client wayland-egl]) Might as well simply be 'WAYLAND'. > Source/autotools/SetupAutoconfHeader.m4:49 > + > + if test "$have_gtk_wayland" = "yes"; then > + AC_DEFINE([WTF_PLATFORM_WAYLAND], [1], [Define if Wayland support is present]) > + fi It's a bit weak to define the Wayland platform as supported only when using X11 as the target and having the GTK Wayland backend present, but at the moment the GTK port doesn't support building with two parallel targets and also doesn't support only building the Wayland target since the WebKit2 component fails to build due to the plugin infrastructure being tightly coupled into the build system. Of course this should be fixed and amended.

Comment on attachment 207198 [details] All changes, RFC version I like the approach you are taking. I think maybe instead of a new class we should just make GLContextEGL an abstract base class and create GLContextEGLWayland and GLContextEGLX. It's nice that you covered the non-Wayland non-X11 case here, but no such consumer exists yet, right? Isn't GLContext only used by GTK+. I think if the code will be unused you should avoid adding it.

(In reply to comment #3) > (From update of attachment 207198 [details]) > I like the approach you are taking. I think maybe instead of a new class we should just make GLContextEGL an abstract base class and create GLContextEGLWayland and GLContextEGLX. I'll look into that. > It's nice that you covered the non-Wayland non-X11 case here, but no such consumer exists yet, right? Isn't GLContext only used by GTK+. I think if the code will be unused you should avoid adding it. I think AppleWin port might be such a consumer, based on this: http://trac.webkit.org/changeset/152869

Created attachment 207411 [details] All changes, RFC version #2 RFC dump #2. Further refactored, merging the EGLPlatform interface into GLContextEGL and renaming EGLPlatformWayland, EGLPlatformX and EGLPlatformDefault into GLContextEGLWayland, GLContextEGLX11 and GLContextEGLDefault and have them inherit from GLContextEGL. This simplifies things, most significantly removing the necessity to have the complete GLContextEGL depend on a template.

Comment on attachment 207411 [details] All changes, RFC version #2 View in context: https://bugs.webkit.org/attachment.cgi?id=207411&action=review > Source/WebCore/platform/graphics/cairo/GLContext.cpp:186 > + typedef GLContextEGLX11 EGLPlatformClass; This and similar EGLPlatformClass leftovers could now use a better name, obviously. > Source/WebCore/platform/graphics/egl/GLContextEGL.h:34 > -#include <EGL/egl.h> > +typedef unsigned int EGLBoolean; > +typedef void *EGLConfig; > +typedef void *EGLContext; > +typedef void *EGLDisplay; > +typedef void *EGLSurface; > +typedef unsigned int EGLenum; > +typedef khronos_int32_t EGLint; Instead of including the EGL header which also pulls in the platform-specific type definitions, only the necessary types are defined here. > Source/WebCore/platform/graphics/egl/GLContextEGL.h:49 > + template<class EGLPlatformClass> > + static PassOwnPtr<GLContextEGL> createContext(typename EGLPlatformClass::NativeWindowType, GLContext* sharingContext = 0); This static method is the only one left templated, so it can operate on a native window of the proper type and also construct the proper platform-specific GLContextEGL object.

Shouldn't you change uint64_t to NativeWindowType in GLContext.cpp? PassOwnPtr<GLContext> GLContext::createContextForWindow(uint64_t windowHandle, GLContext* sharingContext) And I don't like this in GLContextEGLDefault.h. It should be platform-specific, like in https://bugs.webkit.org/show_bug.cgi?id=119235 + typedef uint64_t NativeWindowType;

The latest RFC version, when rebased, works well. I plan to upload a patch for review tomorrow. I plan to address additional abstractions for specific EGL platforms in a separate bug, after work on this one is finished.

(In reply to comment #7) > Shouldn't you change uint64_t to NativeWindowType in GLContext.cpp? > > PassOwnPtr<GLContext> GLContext::createContextForWindow(uint64_t windowHandle, GLContext* sharingContext) > > And I don't like this in GLContextEGLDefault.h. It should be platform-specific, like in https://bugs.webkit.org/show_bug.cgi?id=119235 > > + typedef uint64_t NativeWindowType; Just a note that this has been addressed in the linked bug.

Created attachment 209504 [details] Pre-review patch

Comment on attachment 209504 [details] Pre-review patch View in context: https://bugs.webkit.org/attachment.cgi?id=209504&action=review Not setting the r? flag yet, I'd first like to land the small fix in bug #120214. > Source/WebCore/platform/graphics/egl/GLContextEGLDefault.h:22 > +#if !PLATFORM(GTK) > + typedef EGLNativeWindowType NativeWindowType; > +#else > + typedef uint64_t NativeWindowType; > +#endif This follows the GLNativeWindowType definition in GLContext.h. > Source/autotools/Versions.m4:22 > +m4_define([gtk3_wayland_required_version], [3.9.6]) I was using 3.9.6 for testing, but this should be bumped to 3.10.0. Using 3.6.0 here wouldn't make much sense, there wasn't much support for Wayland then, and I generally think introducing Wayland support into the Debian long-term stable isn't really a good idea.

By the way, there are no working bots with WebGL enabled on Windows right now, so be careful because nobody will notice breaking the Windows WebGL build until I get back from vacation.

Created attachment 212358 [details] Patch

Attachment 212358 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/Platform/ChangeLog', u'Source/Platform/GNUmakefile.am', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/platform/graphics/GLContext.cpp', u'Source/WebCore/platform/graphics/egl/GLContextEGL.cpp', u'Source/WebCore/platform/graphics/egl/GLContextEGL.h', u'Source/WebCore/platform/graphics/egl/GLContextEGLDefault.cpp', u'Source/WebCore/platform/graphics/egl/GLContextEGLDefault.h', u'Source/WebCore/platform/graphics/egl/GLContextEGLWayland.cpp', u'Source/WebCore/platform/graphics/egl/GLContextEGLWayland.h', u'Source/WebCore/platform/graphics/egl/GLContextEGLX11.cpp', u'Source/WebCore/platform/graphics/egl/GLContextEGLX11.h', u'Source/WebKit/gtk/ChangeLog', u'Source/WebKit/gtk/GNUmakefile.am', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/GNUmakefile.am', u'Source/autotools/FindDependencies.m4', u'Tools/ChangeLog', u'Tools/GNUmakefile.am', u'Tools/TestWebKitAPI/GNUmakefile.am']" exit_code: 1 Source/WebCore/platform/graphics/egl/GLContextEGLWayland.cpp:6: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 22 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 212358 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=212358&action=review Looking better. I'm beginning to doubt the abstract base class now. I should play around with this a bit until I understand it. > Source/WebCore/platform/graphics/GLContext.cpp:164 > +#if defined(GDK_WINDOWING_X11) && USE(GLX) > + if (GDK_IS_X11_DISPLAY(display)) { > + if (OwnPtr<GLContext> glxContext = GLContextGLX::createContext(windowHandle, sharingContext)) > + return glxContext.release(); > + return nullptr; > + } > +#endif I'm not sure I understand why you can't skip this code and let line 163/176 handle it... > Source/WebCore/platform/graphics/GLContext.cpp:187 > + if (OwnPtr<GLContext> eglContext = GLContextEGL::createContext<EGLPlatformContext>(windowHandle, sharingContext)) Hrm. Not sure I'm a huge fan of the use of templates here. > Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:88 > + if (m_display != EGL_NO_DISPLAY) { Maybe ASSERT(m_display) instead of a conditional? when can m_display be null? > Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:102 > + return m_type == GLContextEGL::WindowSurface; You should not need to specify a namespace in a method. > Source/WebCore/platform/graphics/egl/GLContextEGL.h:32 > +typedef void *EGLConfig; > +typedef void *EGLContext; > +typedef void *EGLDisplay; > +typedef void *EGLSurface; Your asterisks are in the wrong place here. > Source/WebCore/platform/graphics/egl/GLContextEGL.h:41 > + enum SurfaceType { These aren't really "surface types" so much as context types. > Source/WebCore/platform/graphics/egl/GLContextEGL.h:70 > + EGLenum glAPI() const; > + EGLint* contextAttributes() const; These should be static. > Source/WebCore/platform/graphics/egl/GLContextEGLDefault.cpp:71 > +bool GLContextEGLDefault::createPbufferSurface(EGLContext sharingContext) > +{ > + ASSERT(m_display != EGL_NO_DISPLAY); > + Is there a way we can avoid duplicating this code? >> Source/WebCore/platform/graphics/egl/GLContextEGLWayland.cpp:6 >> +#include <wayland-egl.h> > > Alphabetical sorting problem. [build/include_order] [4] Please fix, if possible

Comment on attachment 212358 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=212358&action=review >> Source/WebCore/platform/graphics/GLContext.cpp:164 >> +#endif > > I'm not sure I understand why you can't skip this code and let line 163/176 handle it... This basically got written because of both the special-casing for the GTK port and the mirroring of the tidy Wayland display check. >> Source/WebCore/platform/graphics/GLContext.cpp:187 >> + if (OwnPtr<GLContext> eglContext = GLContextEGL::createContext<EGLPlatformContext>(windowHandle, sharingContext)) > > Hrm. Not sure I'm a huge fan of the use of templates here. Specifically in this case? Or generally for GLContextEGL::createContext? >> Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:88 >> + if (m_display != EGL_NO_DISPLAY) { > > Maybe ASSERT(m_display) instead of a conditional? when can m_display be null? m_display can be null if the platform-specific implementation was unable to set up the shared EGL display. >> Source/WebCore/platform/graphics/egl/GLContextEGL.h:32 >> +typedef void *EGLSurface; > > Your asterisks are in the wrong place here. Oddly the style checker doesn't complain. >> Source/WebCore/platform/graphics/egl/GLContextEGL.h:41 >> + enum SurfaceType { > > These aren't really "surface types" so much as context types. Roger. >> Source/WebCore/platform/graphics/egl/GLContextEGL.h:70 >> + EGLint* contextAttributes() const; > > These should be static. The idea here is that platform-specific implementations can override these and reimplement them as required. If that makes sense, I believe these should really be virtual. >> Source/WebCore/platform/graphics/egl/GLContextEGLDefault.cpp:71 >> + > > Is there a way we can avoid duplicating this code? I'll see if these surface methods can be moved up to GLContextEGL. >>> Source/WebCore/platform/graphics/egl/GLContextEGLWayland.cpp:6 >>> +#include <wayland-egl.h> >> >> Alphabetical sorting problem. [build/include_order] [4] > > Please fix, if possible wayland-egl.h defines the required types and macros so that egl.h chooses the Wayland EGL platform instead of the default X11 platform. So including wayland-egl.h before egl.h is necessary here.

Comment on attachment 212358 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=212358&action=review > Source/WebCore/ChangeLog:17 > + returns a new object of that type if the context is valid, and a nullptr otherwise. What if instead of using a template for this, we use a factory? That would mirror more or less what's done for creating the GraphicLayers for example and I think fits better than a template in this case.

(In reply to comment #17) > (From update of attachment 212358 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=212358&action=review > > > Source/WebCore/ChangeLog:17 > > + returns a new object of that type if the context is valid, and a nullptr otherwise. > > What if instead of using a template for this, we use a factory? That would mirror more or less what's done for creating the GraphicLayers for example and I think fits better than a template in this case. I like that approach, but I'd defer it to a separate bug that I'd address later, just to keep changes here under the scope.

Created attachment 213392 [details] Patch

(In reply to comment #19) > Created an attachment (id=213392) [details] > Patch Modified the surface types to now describe the type of context itself, moved the common pbuffer surface creation into the base GLContextEGL class, and moved the pointers in type definitions in the GLContextEGL header into the proper position.

Created attachment 218935 [details] Iteration of Zan's patch

Created attachment 219162 [details] Next iteration of Zan's patch

Created attachment 219170 [details] Patch ChangeLogs are hard.

Attachment 219170 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/Platform/ChangeLog', u'Source/Platform/GNUmakefile.am', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/platform/graphics/GLContext.cpp', u'Source/WebCore/platform/graphics/egl/GLContextEGL.cpp', u'Source/WebCore/platform/graphics/egl/GLContextEGL.h', u'Source/WebCore/platform/graphics/egl/GLContextEGLWayland.cpp', u'Source/WebCore/platform/graphics/egl/GLContextEGLX11.cpp', u'Source/WebKit/gtk/ChangeLog', u'Source/WebKit/gtk/GNUmakefile.am', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/GNUmakefile.am', u'Source/autotools/FindDependencies.m4', u'Tools/ChangeLog', u'Tools/GNUmakefile.am', u'Tools/TestWebKitAPI/GNUmakefile.am', '--commit-queue']" exit_code: 1 ERROR: Source/WebCore/platform/graphics/egl/GLContextEGLWayland.cpp:6: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 219170 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=219170&action=review > Source/WebCore/platform/graphics/egl/GLContextEGLWayland.cpp:12 > +#if PLATFORM(GTK) > +#include <gdk/gdk.h> > +#include <gdk/gdkwayland.h> > +#endif Still missing #if defined(GDK_WINDOWING_WAYLAND) check here.

Created attachment 219171 [details] Patch Better.

Attachment 219171 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/Platform/ChangeLog', u'Source/Platform/GNUmakefile.am', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/platform/graphics/GLContext.cpp', u'Source/WebCore/platform/graphics/egl/GLContextEGL.cpp', u'Source/WebCore/platform/graphics/egl/GLContextEGL.h', u'Source/WebCore/platform/graphics/egl/GLContextEGLWayland.cpp', u'Source/WebCore/platform/graphics/egl/GLContextEGLX11.cpp', u'Source/WebKit/gtk/ChangeLog', u'Source/WebKit/gtk/GNUmakefile.am', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/GNUmakefile.am', u'Source/autotools/FindDependencies.m4', u'Tools/ChangeLog', u'Tools/GNUmakefile.am', u'Tools/TestWebKitAPI/GNUmakefile.am', '--commit-queue']" exit_code: 1 ERROR: Source/WebCore/platform/graphics/egl/GLContextEGLWayland.cpp:6: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #27) > Attachment 219171 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/Platform/ChangeLog', u'Source/Platform/GNUmakefile.am', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/platform/graphics/GLContext.cpp', u'Source/WebCore/platform/graphics/egl/GLContextEGL.cpp', u'Source/WebCore/platform/graphics/egl/GLContextEGL.h', u'Source/WebCore/platform/graphics/egl/GLContextEGLWayland.cpp', u'Source/WebCore/platform/graphics/egl/GLContextEGLX11.cpp', u'Source/WebKit/gtk/ChangeLog', u'Source/WebKit/gtk/GNUmakefile.am', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/GNUmakefile.am', u'Source/autotools/FindDependencies.m4', u'Tools/ChangeLog', u'Tools/GNUmakefile.am', u'Tools/TestWebKitAPI/GNUmakefile.am', '--commit-queue']" exit_code: 1 > ERROR: Source/WebCore/platform/graphics/egl/GLContextEGLWayland.cpp:6: Alphabetical sorting problem. [build/include_order] [4] > Total errors found: 1 in 18 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. Looks like this issue isn't a false positive?

(In reply to comment #28) > Looks like this issue isn't a false positive? It's not a false positive, but it's required to include <wayland-egl.h> before <EGL/eglplatform.h> gets included through <EGL/egl.h>. The <wayland-egl.h> header defines the WL_EGL_PLATFORM macro that's checked for in the <EGL/eglplatform.h> so that Wayland EGL platform type definitions are set up, instead of falling back to definitions for the X11 EGL platform. http://cgit.freedesktop.org/wayland/wayland/tree/src/wayland-egl.h http://cgit.freedesktop.org/mesa/mesa/tree/include/EGL/eglplatform.h

Comment on attachment 219171 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=219171&action=review LGTM, just added a small suggestion. Thanks for the patch. > Source/WebCore/platform/graphics/GLContext.cpp:152 > +#if PLATFORM(NIX) > + return nullptr; > +#else I think we could just: #if !PLATFORM(NIX), if we leave the last return nullptr in the method outside of the ifdef.

Created attachment 221241 [details] Patch for landing

(In reply to comment #31) > Created an attachment (id=221241) [details] > Patch for landing Because of worries expressed in comment #12 I'll coordinate with Alex before landing this.

Is this just waiting to be landed? =)

(In reply to comment #32) > Because of worries expressed in comment #12 I'll coordinate with Alex before > landing this. Go ahead and land this and check the bots. I'll clean up any problems you cause to Windows. I'm pretty sure at least the WinCairo bot has WebGL enabled, and I will clean up anything the bots don't notice.

Created attachment 255559 [details] Updated patch against 2.8.3 Updated patch to apply against 2.8.3. Tested with weston 2.8.3 on the drm backend, and epiphany 3.16.1, and it works fine. webgl still works on X11, no regression there AFAICS. There's one problem with the patch: I commented out WaylandDisplay::createSharingGLContext() and WaylandSurface::createGLContext(). Anyway I commented them out because of the EGLNativeWindowType vs GLNativeWindowType mismatch in 2.8.3, as GLNativeWindowType is defined as uint64_t. That may be fixed in 2.9.x / master since commit fc2ad7. But since those functions seem to be unused I just commented them out for now as I'm testing on 2.8.

Created attachment 256005 [details] Patch against git master (commit 27f9f4, svn r186113) This is an updated patch against git master (commit 27f9f4, svn r186113). It fixes various issues with the previous versions, including a bug that was causing webgl to stop working on X11 because the GLContextEGL for the XUniquePixmap case wasn't setting the EGLDisplay. Also it changes the ordering in which the different context constructors are tried to not change the behavior from what is present in git master at the moment: e.g. createContext() in git master calls createWindowContext, then Pixmap and then Pbuffer, while the previous patches from Zan and me tried Window then Pbuffer then Pixmap (in the x11 codepath). This patch keeps the status quo. One question: Since I have made createContext() try to create a Pixmap context, it seems unnecessary to have the createX11Context() function, as they are basically the same. Note that Source/WebCore/platform/graphics/GLContext.cpp:GLContext::createContextForWindow is calling createContext() rather than createX11Context() in this patch, as they are pretty similar. Thoughts on this? Should we drop createX11Context(), or what? I have tested this with both X11 and Wayland support built, and run it on - Epiphany/X11 - OK - Epiphany/Weston (x11 backend) on wayland - OK - Epiphany/Weston (drm backend) on wayland - OK - Epiphany/Weston (x11 backend) on XWayland - BROKEN - MiniBrowser/X11 - OK - MiniBrowser/Weston (x11 backend) on wayland - BROKEN - MiniBrowser/Weston (x11 backend) on XWayland - BROKEN I haven't figured out why webgl in MiniBrowser isn't working on weston (whether on xwayland or natively). It seems to create a wayland context just like Ephy does. This isn't a regression, so maybe it shouldn't block landing if all else is fine. I also don't know yet why Epiphany fails on XWayland (I think it was working in the past with some 2.8 version, but I have to confirm that). Anyway, suggestions on how to figure out what's wrong with the failing cases more than welcome! And testing & reviews appreciated as well.

FWIW I'm building webkit with this command: Tools/Scripts/build-webkit --gtk --cmakeargs="-DENABLE_WAYLAND_TARGET=ON -DENABLE_GLES2=ON -DENABLE_ACCELERATED_2D_CANVAS=OFF -DENABLE_MEDIA_STREAM=OFF -DUSE_LIBHYPHEN=OFF" --makeargs="-j5"

(In reply to comment #36) > I have tested this with both X11 and Wayland support built, and run it on > - Epiphany/X11 - OK > - Epiphany/Weston (x11 backend) on wayland - OK > - Epiphany/Weston (drm backend) on wayland - OK > - Epiphany/Weston (x11 backend) on XWayland - BROKEN > - MiniBrowser/X11 - OK > - MiniBrowser/Weston (x11 backend) on wayland - BROKEN > - MiniBrowser/Weston (x11 backend) on XWayland - BROKEN > > I haven't figured out why webgl in MiniBrowser isn't working on weston > (whether on xwayland or natively). It seems to create a wayland context just > like Ephy does. This isn't a regression, so maybe it shouldn't block landing > if all else is fine. ChangSeok figured this out. It turns out MiniBrowser enables AC on Wayland, but AC isn't currently working. He'll send a patch to turn that off. With that fixed, this patch makes WebGL work on Wayland (natively) with no regressions on X11. XWayland isn't working, but that's not a regression and not very important so we shouldn't block on that.

Comment on attachment 256005 [details] Patch against git master (commit 27f9f4, svn r186113) View in context: https://bugs.webkit.org/attachment.cgi?id=256005&action=review This patch is missing a ChangeLog. You can use prepare-ChangeLogs to create a skeleton that you can fill out. > Source/WebCore/platform/graphics/GLContext.cpp:138 > +#if 0 // XXX PLATFORM(X11) && USE(EGL) > + if (auto eglContext = GLContextEGL::createX11Context(windowHandle, sharingContext)) > + return WTF::move(eglContext); > + return nullptr; > +#endif Please don't leave code commented out. > Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:38 > +// XXX move inside if USE(CAIRO) ? This can be removed as well. > Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:71 > -static const EGLint gContextAttributes[] = { > +EGLint* GLContextEGL::contextAttributes() > +{ > + static EGLint contextAttributes[] = { > #if USE(OPENGL_ES_2) > - EGL_CONTEXT_CLIENT_VERSION, 2, > + EGL_CONTEXT_CLIENT_VERSION, 2, > #endif > - EGL_NONE > -}; > + EGL_NONE > + }; > Why convert this from a static struct? > Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:164 > + EGLContext eglSharingContext = sharingContext ? static_cast<GLContextEGL*>(sharingContext)->m_context : 0; 0 should be nullptr now. > Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:-188 > +bool GLContextEGL::initializeOpenGLAPI() > { > - if (!sharedEGLDisplay()) > - return nullptr; initializeOpenGLShims already tracks whether it is initialized or not, so you can simply call initializeOpenGLShims and eliminate this entire function if you don't need to do anything else in it. > Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:218 > + if (m_display != EGL_NO_DISPLAY) { > + if (m_context != EGL_NO_CONTEXT) { Why would display be EGL_NO_DISPLAY here? > Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:-270 > - ASSERT(m_context && m_surface); > - Why remove the assertion if you are going to use the things it's asserting right after? > Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:259 > + if (m_surface) > + eglSwapBuffers(m_display, m_surface); I'm not sure I understand exactly why m_surface can be null. > Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.h:54 > - std::unique_ptr<GLContextEGL> createSharingGLContext(); > + std::unique_ptr<GLContext> createSharingGLContext(); Why have it hold the parent type instead of the GLContextEGL? > Source/WebCore/platform/graphics/wayland/WaylandSurface.h:52 > - std::unique_ptr<GLContextEGL> createGLContext(); > + std::unique_ptr<GLContext> createGLContext(); Ditto.

Comment on attachment 256005 [details] Patch against git master (commit 27f9f4, svn r186113) View in context: https://bugs.webkit.org/attachment.cgi?id=256005&action=review > Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:49 > +EGLenum GLContextEGL::glAPI() Is this used anywhere? > Source/WebCore/platform/graphics/egl/GLContextEGL.h:65 > + enum ContextType { > + WindowBased, > + PbufferBased, > + PixmapBased, > + Surfaceless, > + }; > > - GLContextEGL(EGLContext, EGLSurface, EGLSurfaceType); > + static std::unique_ptr<GLContextEGL> create(EGLDisplay display, EGLContext context, EGLSurface surface, ContextType type) > + { > + return std::make_unique<GLContextEGL>(display, context, surface, type); > + } > + > + static std::unique_ptr<GLContext> createContext(GLNativeWindowType, GLContext* sharingContext); > + static std::unique_ptr<GLContext> createWindowContext(EGLDisplay display, GLNativeWindowType nativeWindow, EGLContext sharingContext); > +#if PLATFORM(WAYLAND) > + static std::unique_ptr<GLContext> createWaylandContext(GLContext* sharingContext); > +#endif > #if PLATFORM(X11) > - GLContextEGL(EGLContext, EGLSurface, XUniquePixmap&&); > + static std::unique_ptr<GLContext> createPixmapContext(EGLDisplay display, EGLContext sharingContext); > + static std::unique_ptr<GLContext> createX11Context(GLNativeWindowType, GLContext* sharingContext); > +#endif > + > + static EGLBoolean chooseConfig(EGLDisplay, EGLConfig*, ContextType); > + static EGLenum glAPI(); > + static EGLint* contextAttributes(); > + > + GLContextEGL(EGLDisplay, EGLContext, EGLSurface, ContextType); > +#if PLATFORM(X11) > + GLContextEGL(EGLDisplay, EGLContext, EGLSurface, XUniquePixmap&&); Most of this should be private.

Created attachment 266778 [details] Patch

The new patch is against git master (f00fef598aeb5f94e71421263ee874c8ae0c8ba5) (In reply to comment #39) > Comment on attachment 256005 [details] > Patch against git master (commit 27f9f4, svn r186113) > > View in context: > https://bugs.webkit.org/attachment.cgi?id=256005&action=review > > This patch is missing a ChangeLog. You can use prepare-ChangeLogs to create > a skeleton that you can fill out. Added. > > > Source/WebCore/platform/graphics/GLContext.cpp:138 > > +#if 0 // XXX PLATFORM(X11) && USE(EGL) > > + if (auto eglContext = GLContextEGL::createX11Context(windowHandle, sharingContext)) > > + return WTF::move(eglContext); > > + return nullptr; > > +#endif > > Please don't leave code commented out. Removed. > > Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:38 > > +// XXX move inside if USE(CAIRO) ? > > This can be removed as well. Done. > > > Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:71 > > -static const EGLint gContextAttributes[] = { > > +EGLint* GLContextEGL::contextAttributes() > > +{ > > + static EGLint contextAttributes[] = { > > #if USE(OPENGL_ES_2) > > - EGL_CONTEXT_CLIENT_VERSION, 2, > > + EGL_CONTEXT_CLIENT_VERSION, 2, > > #endif > > - EGL_NONE > > -}; > > + EGL_NONE > > + }; > > > > Why convert this from a static struct? That was a carryover from Zan's patch, but I guess that is so that the other files (e.g. GLContextEGLWayland.cpp) can use it. > > Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:164 > > + EGLContext eglSharingContext = sharingContext ? static_cast<GLContextEGL*>(sharingContext)->m_context : 0; > > 0 should be nullptr now. Fixed throughout the patch. > > Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:-188 > > +bool GLContextEGL::initializeOpenGLAPI() > > { > > - if (!sharedEGLDisplay()) > > - return nullptr; > > initializeOpenGLShims already tracks whether it is initialized or not, so > you can simply call initializeOpenGLShims and eliminate this entire function > if you don't need to do anything else in it. Fixed. > > Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:218 > > + if (m_display != EGL_NO_DISPLAY) { > > + if (m_context != EGL_NO_CONTEXT) { > > Why would display be EGL_NO_DISPLAY here? Carryover. But it shouldn't. Reverted that change. > > Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:-270 > > - ASSERT(m_context && m_surface); > > - > > Why remove the assertion if you are going to use the things it's asserting > right after? Fixed. > > Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:259 > > + if (m_surface) > > + eglSwapBuffers(m_display, m_surface); > > I'm not sure I understand exactly why m_surface can be null. E.g. if created through createWaylandContext(). > > Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.h:54 > > - std::unique_ptr<GLContextEGL> createSharingGLContext(); > > + std::unique_ptr<GLContext> createSharingGLContext(); > > Why have it hold the parent type instead of the GLContextEGL? Fixed throughout the patch. (In reply to comment #40) > Comment on attachment 256005 [details] > Patch against git master (commit 27f9f4, svn r186113) > > View in context: > https://bugs.webkit.org/attachment.cgi?id=256005&action=review > > > Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:49 > > +EGLenum GLContextEGL::glAPI() > > Is this used anywhere? Nope. Dropped. > > Source/WebCore/platform/graphics/egl/GLContextEGL.h:65 > > + enum ContextType { > > + WindowBased, > > + PbufferBased, > > + PixmapBased, > > + Surfaceless, > > + }; > > > > - GLContextEGL(EGLContext, EGLSurface, EGLSurfaceType); > > + static std::unique_ptr<GLContextEGL> create(EGLDisplay display, EGLContext context, EGLSurface surface, ContextType type) > > + { > > + return std::make_unique<GLContextEGL>(display, context, surface, type); > > + } > > + > > + static std::unique_ptr<GLContext> createContext(GLNativeWindowType, GLContext* sharingContext); > > + static std::unique_ptr<GLContext> createWindowContext(EGLDisplay display, GLNativeWindowType nativeWindow, EGLContext sharingContext); > > +#if PLATFORM(WAYLAND) > > + static std::unique_ptr<GLContext> createWaylandContext(GLContext* sharingContext); > > +#endif > > #if PLATFORM(X11) > > - GLContextEGL(EGLContext, EGLSurface, XUniquePixmap&&); > > + static std::unique_ptr<GLContext> createPixmapContext(EGLDisplay display, EGLContext sharingContext); > > + static std::unique_ptr<GLContext> createX11Context(GLNativeWindowType, GLContext* sharingContext); > > +#endif > > + > > + static EGLBoolean chooseConfig(EGLDisplay, EGLConfig*, ContextType); > > + static EGLenum glAPI(); > > + static EGLint* contextAttributes(); > > + > > + GLContextEGL(EGLDisplay, EGLContext, EGLSurface, ContextType); > > +#if PLATFORM(X11) > > + GLContextEGL(EGLDisplay, EGLContext, EGLSurface, XUniquePixmap&&); > > Most of this should be private. I moved all the stuff that I could to private. Thanks for the reviews.

Comment on attachment 266778 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=266778&action=review I think you should mention in the changelog that the patch is based on the original patch of Zan Dobersek <zdobersek@igalia.com> and Martin Robinson <mrobinson@igalia.com> > Source/WebCore/platform/graphics/egl/GLContextEGLWayland.cpp:2 > + * Copyright (C) 2013 Igalia, S.L. 2013 ? > Source/WebCore/platform/graphics/egl/GLContextEGLX11.cpp:2 > + * Copyright (C) 2013 Igalia, S.L. 2013 ?

Created attachment 266870 [details] Patch

(In reply to comment #43) > Comment on attachment 266778 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=266778&action=review > > I think you should mention in the changelog that the patch is based on the > original > patch of Zan Dobersek <zdobersek@igalia.com> and Martin Robinson > <mrobinson@igalia.com> Absolutely! Thanks for pointing this. > > Source/WebCore/platform/graphics/egl/GLContextEGLWayland.cpp:2 > > + * Copyright (C) 2013 Igalia, S.L. > > 2013 ? > > > Source/WebCore/platform/graphics/egl/GLContextEGLX11.cpp:2 > > + * Copyright (C) 2013 Igalia, S.L. > > 2013 ? That's when Zan and Martin worked on this AFAICS, and that copyright notice comes unmodified from their patch. I have just added another one for my work though.

Zan, Martin, got time to look this over again?

Comment on attachment 266870 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=266870&action=review > Source/WebCore/platform/graphics/egl/GLContextEGLWayland.cpp:32 > + EGLDisplay display = PlatformDisplay::sharedDisplay().eglDisplay(); Seems this broke the build: ../../Source/WebCore/platform/graphics/egl/GLContextEGLWayland.cpp: In static member function 'static std::unique_ptr<WebCore::GLContextEGL> WebCore::GLContextEGL::createWaylandContext(WebCore::GLContext*)': ../../Source/WebCore/platform/graphics/egl/GLContextEGLWayland.cpp:32:26: error: 'PlatformDisplay' has not been declared

Comment on attachment 266870 [details] Patch Removing this from request queue since it failed EWS. Please, rebase the patch, then I'll try again to find someone who understands OpenGL to review it.

Created attachment 276630 [details] Patch

(In reply to comment #48) > Comment on attachment 266870 [details] > Patch > > Removing this from request queue since it failed EWS. Please, rebase the > patch, then I'll try again to find someone who understands OpenGL to review > it. This should build again against latest master. It also still works properly in my tests. It'd be great if this could be reviewed so we can merge it if the review is positive and get webgl support on wayland.

I've minimized the current patch to make sure it works on top of my work-in-progress wayland-ac[1] branch for bug #115803. In particular I've temporarily left out the GLContextEGL refactorings and enabled offscreen contexts by using a 1x1 surface[2] as the pixmap-based code is already doing under X11 and then introduced the EGL_KHR_surfaceless_context optimization[3] for truly surfaceless contexts, both under Wayland and X11. This way WebGL should work even where EGL_KHR_surfaceless_context is not supported. Note that the branch is still in flux, this is why I didn't attach the patches here yet. :) [1] https://github.com/em-/webkit/commits/wayland-ac [2] https://github.com/em-/webkit/commit/cbf4d5c3b2d05 [3] https://github.com/em-/webkit/commit/73512afa77cd7

Comment on attachment 276630 [details] Patch Clearing flags, similar refactoring has been done in bug #115803

Let's track this in bug #115803 instead. *** This bug has been marked as a duplicate of bug 115803 ***