Bug 144517 – [GTK][EFL] Unify platform display handling

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Carlos Garcia Campos]

There are several places were we are handling the X display connection:

 - GLContext::sharedX11Display() creates a new connection.
 - X11Helper::nativeDisplay() creates a new connection.
 - BackingStoreBackendCairoX11 uses the GTK+ shared connection.
 - NetscapePlugin::x11HostDisplay() uses the GTK+/ecore shared connection
 - The rest of the GTK+ code uses the shared GTK+ connection

And then we also have WaylandDisplay and the code to check if the current display is wayland or X11.

We could unify all these to share the same connection to reduce the amount of ifdefs and ensure a single connection. That will also allow us to use "smart pointers" for the X resources that need a Display* to be freed.

[Carlos Garcia Campos]

Created attachment 252223 [details]
Patch

Zan, I haven't checked the Wayland part builds/works.

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/graphics/PlatformDisplay.h:42:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.h:36:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 18 files


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

[Martin Robinson]

Comment on attachment 252223 [details]
Patch

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

Excellent clean up.

> Source/WebCore/platform/graphics/PlatformDisplay.h:65
> +#define SPECIALIZE_TYPE_TRAITS_PLATFORM_DISPLAY(ToClassName, DisplayType) \
> +SPECIALIZE_TYPE_TRAITS_BEGIN(WebCore::ToClassName) \
> +    static bool isType(const WebCore::PlatformDisplay& display) { return display.type() == WebCore::PlatformDisplay::Type::DisplayType; } \
> +SPECIALIZE_TYPE_TRAITS_END()

This is cool.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:242
>  #if PLATFORM(X11)
> -        Display* display = GLContext::sharedX11Display();
> -        m_glDisplay = GST_GL_DISPLAY(gst_gl_display_x11_new_with_display(display));
> +        m_glDisplay = GST_GL_DISPLAY(gst_gl_display_x11_new_with_display(downcast<PlatformDisplayX11>(sharedDisplay).native()));
>  #elif PLATFORM(WAYLAND)
> -        EGLDisplay display = WaylandDisplay::instance()->eglDisplay();
> -        m_glDisplay = GST_GL_DISPLAY(gst_gl_display_egl_new_with_egl_display(display));
> +        m_glDisplay = GST_GL_DISPLAY(gst_gl_display_egl_new_with_egl_display(downcast<PlatformDisplayWayland>(sharedDisplay).native()));
>  #endif

These are #ifs but they really should both co-exist. It's unrelated to this patch though.

> Source/WebCore/platform/graphics/surfaces/glx/X11Helper.cpp:317
>  {
> -    // Display connection will only be broken at program shutdown.
> -    static DisplayConnection displayConnection;
> -    return displayConnection.display();
> +    return downcast<PlatformDisplayX11>(PlatformDisplay::sharedDisplay()).native();

Maybe these helpers can gradually move into PlatformDisplayX11.

> Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.h:36
> +#include <wayland-client.h>
> +
> +#include <wayland-egl.h>

Nit: Extra empty line here.

[Carlos Garcia Campos]

(In reply to comment #3)
> Comment on attachment 252223 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=252223&action=review
> 
> Excellent clean up.

Thanks.

> > Source/WebCore/platform/graphics/PlatformDisplay.h:65
> > +#define SPECIALIZE_TYPE_TRAITS_PLATFORM_DISPLAY(ToClassName, DisplayType) \
> > +SPECIALIZE_TYPE_TRAITS_BEGIN(WebCore::ToClassName) \
> > +    static bool isType(const WebCore::PlatformDisplay& display) { return display.type() == WebCore::PlatformDisplay::Type::DisplayType; } \
> > +SPECIALIZE_TYPE_TRAITS_END()
> 
> This is cool.

Yes :-)

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:242
> >  #if PLATFORM(X11)
> > -        Display* display = GLContext::sharedX11Display();
> > -        m_glDisplay = GST_GL_DISPLAY(gst_gl_display_x11_new_with_display(display));
> > +        m_glDisplay = GST_GL_DISPLAY(gst_gl_display_x11_new_with_display(downcast<PlatformDisplayX11>(sharedDisplay).native()));
> >  #elif PLATFORM(WAYLAND)
> > -        EGLDisplay display = WaylandDisplay::instance()->eglDisplay();
> > -        m_glDisplay = GST_GL_DISPLAY(gst_gl_display_egl_new_with_egl_display(display));
> > +        m_glDisplay = GST_GL_DISPLAY(gst_gl_display_egl_new_with_egl_display(downcast<PlatformDisplayWayland>(sharedDisplay).native()));
> >  #endif
> 
> These are #ifs but they really should both co-exist. It's unrelated to this
> patch though.

Right, I tried to not change any logic in this patch. But the PlatformDisplay class has been thought with the idea of supporting X11 and wayland support in the same binary, indeed.

> > Source/WebCore/platform/graphics/surfaces/glx/X11Helper.cpp:317
> >  {
> > -    // Display connection will only be broken at program shutdown.
> > -    static DisplayConnection displayConnection;
> > -    return displayConnection.display();
> > +    return downcast<PlatformDisplayX11>(PlatformDisplay::sharedDisplay()).native();
> 
> Maybe these helpers can gradually move into PlatformDisplayX11.

Yes, probably, but it seems to me that some of them are very specific to surfaces implementation, we could make it even more generic, though.

> > Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.h:36
> > +#include <wayland-client.h>
> > +
> > +#include <wayland-egl.h>
> 
> Nit: Extra empty line here.

I just copied this file, but yes, I'll fix it anyway.

[Zan Dobersek]

Comment on attachment 252223 [details]
Patch

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

> Source/WebCore/platform/graphics/PlatformDisplay.cpp:49
> +PlatformDisplay* PlatformDisplay::createPlatformDisplay()

Any reason why this doesn't use std::make_unique<>() and return a std::unique_ptr<>?

> Source/WebCore/platform/graphics/PlatformDisplay.cpp:77
> +std::unique_ptr<PlatformDisplay> PlatformDisplay::s_sharedDisplay;

I'd prefer this moved off the class and into the PlatformDisplay::sharedDisplay() method.
Perhaps it would be wise to also use std::call_once(), given that in the future this might be accessed from multiple threads.

[Carlos Garcia Campos]

(In reply to comment #5)
> Comment on attachment 252223 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=252223&action=review
> 
> > Source/WebCore/platform/graphics/PlatformDisplay.cpp:49
> > +PlatformDisplay* PlatformDisplay::createPlatformDisplay()
> 
> Any reason why this doesn't use std::make_unique<>() and return a
> std::unique_ptr<>?

Not really, I changed that several times, it was initially a reference until I realized it could return nullptr.

> > Source/WebCore/platform/graphics/PlatformDisplay.cpp:77
> > +std::unique_ptr<PlatformDisplay> PlatformDisplay::s_sharedDisplay;
> 
> I'd prefer this moved off the class and into the
> PlatformDisplay::sharedDisplay() method.
> Perhaps it would be wise to also use std::call_once(), given that in the
> future this might be accessed from multiple threads.

Sure, very good points, I'll update the patch. Thanks!

[Carlos Garcia Campos]

Created attachment 252265 [details]
Patch for landing

Addressed review comments

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/graphics/PlatformDisplay.h:42:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/PlatformDisplay.cpp:82:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.h:35:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3 in 18 files


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

[Carlos Garcia Campos]

Committed r183731: <http://trac.webkit.org/changeset/183731>

[Csaba Osztrogonác]

(In reply to comment #9)
> Committed r183731: <http://trac.webkit.org/changeset/183731>

It broke the WinCairo build, cc-ing the port maintainers.

..\platform\graphics\egl\GLContextEGL.cpp(69): error C2653: 'PlatformDisplay' : is not a class or namespace name [C:\Users\Alex\Documents\WinCairoBot\win-cairo-release\build\Source\WebCore\WebCore.vcxproj\WebCore.vcxproj]
..\platform\graphics\egl\GLContextEGL.cpp(69): error C3536: 'sharedDisplay': cannot be used before it is initialized [C:\Users\Alex\Documents\WinCairoBot\win-cairo-release\build\Source\WebCore\WebCore.vcxproj\WebCore.vcxproj]
..\platform\graphics\egl\GLContextEGL.cpp(69): error C2064: term does not evaluate to a function taking 0 arguments [C:\Users\Alex\Documents\WinCairoBot\win-cairo-release\build\Source\WebCore\WebCore.vcxproj\WebCore.vcxproj]