Bug 136831 – [GTK] Add Wayland support to GLContext

• Component: New Bugs
• Status: RESOLVED DUPLICATE

[Zan Dobersek]

[GTK] Add Wayland support to GLContext

[Zan Dobersek]

Created attachment 238138 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/graphics/GLContext.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 3 files


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

[Martin Robinson]

Comment on attachment 238138 [details]
Patch

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

> Source/WebCore/platform/graphics/GLContext.cpp:75
> +#if PLATFORM(GTK) && PLATFORM(WAYLAND) && !defined(GTK_API_VERSION_2)
> +    if (!WaylandDisplay::instance())
> +        return nullptr;
> +    DEPRECATED_DEFINE_STATIC_LOCAL(OwnPtr<GLContext>, sharing, (WaylandDisplay::instance()->createSharingGLContext()));
> +#else
>      DEPRECATED_DEFINE_STATIC_LOCAL(OwnPtr<GLContext>, sharing, (createOffscreenContext()));
> +#endif
>      return sharing.get();

Can't you move this into createOffscreenContext? This seems like the wrong place, because it requires duplicating some things.

> Source/WebCore/platform/graphics/GLContext.cpp:82
> +    if (WaylandDisplay::instance())
> +        return WaylandDisplay::instance()->nativeDisplay();

Why not just use this directly instead of creating and intermediary in another class? Or create a static WaylandDisplay::nativeDisplay()?

> Source/WebCore/platform/graphics/GLContext.h:77
> +#if PLATFORM(GTK) && PLATFORM(WAYLAND) && !defined(GTK_API_VERSION_2)
> +    static struct wl_display* sharedWaylandDisplay();
> +#endif
> +

Why is this a static method? Is it used outside GLContext?

[Zan Dobersek]

Comment on attachment 238138 [details]
Patch

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

>> Source/WebCore/platform/graphics/GLContext.cpp:75
>>      return sharing.get();
> 
> Can't you move this into createOffscreenContext? This seems like the wrong place, because it requires duplicating some things.

It doesn't really fit into createOffscreenContext(). I'll modify this method to remove DEPRECATED_DEFINE_STATIC_LOCAL() but still use WaylandDisplay::createSharingGLContext() when actually running under Wayland, and fall back to createOffscreenContext() otherwise.

>> Source/WebCore/platform/graphics/GLContext.cpp:82
>> +        return WaylandDisplay::instance()->nativeDisplay();
> 
> Why not just use this directly instead of creating and intermediary in another class? Or create a static WaylandDisplay::nativeDisplay()?

As it stands this is only called in GLContextEGL, so I'll change the code there to call ::nativeDisplay() directly.

[Zan Dobersek]

Created attachment 238163 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/graphics/GLContext.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 3 files


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

[Zan Dobersek]

Created attachment 238165 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/graphics/GLContext.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 3 files


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

[Zan Dobersek]

Created attachment 238199 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/graphics/GLContext.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 3 files


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

[Zan Dobersek]

Created attachment 238249 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/graphics/GLContext.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 3 files


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

[Brent Fulgham]

Comment on attachment 238249 [details]
Patch

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

> Source/WebCore/platform/graphics/GLContext.cpp:69
> +    static OwnPtr<GLContext> context;

Could this be a unique_ptr?

[Zan Dobersek]

Comment on attachment 238249 [details]
Patch

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

>> Source/WebCore/platform/graphics/GLContext.cpp:69
>> +    static OwnPtr<GLContext> context;
> 
> Could this be a unique_ptr?

The current DEPRECATED_DEFINE_STATIC_LOCAL() code switched to using std::unique_ptr<>, so the new patch will use that as well.

[Zan Dobersek]

Created attachment 249057 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/graphics/GLContext.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 3 files


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

[Sergio Villar Senin]

Comment on attachment 249057 [details]
Patch

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

> Source/WebCore/platform/graphics/GLContext.cpp:79
> +    return context.get();

You're forgetting to remove the DEPRECATED_DEFINE_STATIC_LOCAL

Also I guess you could a static LazyNeverDestroyed + std::call_once() ?

[Zan Dobersek]

Comment on attachment 249057 [details]
Patch

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

>> Source/WebCore/platform/graphics/GLContext.cpp:79
>> +    return context.get();
> 
> You're forgetting to remove the DEPRECATED_DEFINE_STATIC_LOCAL
> 
> Also I guess you could a static LazyNeverDestroyed + std::call_once() ?

Oops.

std::call_once() isn't ideal here since this isn't cross-thread code (yet?), and you have to use a static std::once_flag (and the accompanying guard variable), so that's some additional overhead.

[Sergio Villar Senin]

(In reply to comment #18)
> Comment on attachment 249057 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=249057&action=review
> 
> >> Source/WebCore/platform/graphics/GLContext.cpp:79
> >> +    return context.get();
> > 
> > You're forgetting to remove the DEPRECATED_DEFINE_STATIC_LOCAL
> > 
> > Also I guess you could a static LazyNeverDestroyed + std::call_once() ?
> 
> Oops.
> 
> std::call_once() isn't ideal here since this isn't cross-thread code (yet?),
> and you have to use a static std::once_flag (and the accompanying guard
> variable), so that's some additional overhead.

Yeah well you can use the null check anyway, but NeverDestroyed is the natural replacement for DEPRECATED_DEFINE_STATIC_LOCAL.

[Build Bot]

Comment on attachment 249057 [details]
Patch

Attachment 249057 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/6030012943695872

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 249128 [details]
Archive of layout-test-results from ews203 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews203  Port: win-future  Platform: CYGWIN_NT-6.1-WOW64-1.7.20-0.266-5-3-i686-32bit

[Zan Dobersek]

Comment on attachment 249057 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/GLContext.cpp:79
>>>> +    return context.get();
>>> 
>>> You're forgetting to remove the DEPRECATED_DEFINE_STATIC_LOCAL
>>> 
>>> Also I guess you could a static LazyNeverDestroyed + std::call_once() ?
>> 
>> Oops.
>> 
>> std::call_once() isn't ideal here since this isn't cross-thread code (yet?), and you have to use a static std::once_flag (and the accompanying guard variable), so that's some additional overhead.
> 
> Yeah well you can use the null check anyway, but NeverDestroyed is the natural replacement for DEPRECATED_DEFINE_STATIC_LOCAL.

I'll very likely go with std::call_once(), because paranoia. NeverDestroyed should probably be used as well.

[Zan Dobersek]

Created attachment 250697 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/graphics/GLContext.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 3 files


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

[Martin Robinson]

Comment on attachment 250697 [details]
Patch

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

Looks good to me, apart from one small question:

> Source/WebCore/platform/graphics/GLContext.cpp:31
> +#include <wtf/NeverDestroyed.h>
>  #include <wtf/ThreadSpecific.h>

These should move above #if USE(EGL)

> Source/WebCore/platform/graphics/GLContext.cpp:-153
> -#if PLATFORM(GTK) && PLATFORM(WAYLAND) && !defined(GTK_API_VERSION_2) && defined(GDK_WINDOWING_WAYLAND) && USE(EGL)
> -    GdkDisplay* display = gdk_display_manager_get_default_display(gdk_display_manager_get());
> -
> -    if (GDK_IS_WAYLAND_DISPLAY(display)) {
> -        if (auto eglContext = GLContextEGL::createContext(windowHandle, sharingContext))
> -            return WTF::move(eglContext);
> -        return nullptr;
> -    }
> -#endif

This breaks things doesn't it? It looks like GLX contexts will be preferred even when a Wayland display is active...

[Carlos Alberto Lopez Perez]

Comment on attachment 250697 [details]
Patch

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

> Source/WebCore/platform/graphics/GLContext.h:33
> +#elif PLATFORM(GTK) && PLATFORM(WAYLAND) && !defined(GTK_API_VERSION_2)
> +#include <wayland-egl.h>
> +#include <EGL/eglplatform.h>
> +typedef EGLNativeWindowType GLNativeWindowType;

This 4 lines on GlContext.h were just committed on http://trac.webkit.org/changeset/185198
I guess you need to rebase the patch.

[Carlos Garcia Campos]

Let's track this in bug #115803 instead.

*** This bug has been marked as a duplicate of bug 115803 ***