From 5ae7998a8f272c662ae923836d5fb9814b5637b5 Mon Sep 17 00:00:00 2001 From: Matthew Waters Date: Tue, 3 Mar 2020 11:53:53 +1100 Subject: [PATCH] gl/wayland: use a proxy wrapper for the wl_display This allows us to remove races when setting the wl_queue on wayland objects with wl_proxy_set_queue() as each created object is created with the queue already set. We can also move all our initilization code into the window as we can retrieve all wayland objects from each window instance. This removes a possible race when integrating with external API's as we would always attempt to immediately retrieve a small set of wayland objects. That is no longer the case with the objects from each window instance. --- gst-libs/gst/gl/meson.build | 2 +- gst-libs/gst/gl/wayland/gstgldisplay_wayland.c | 74 +------------ gst-libs/gst/gl/wayland/gstgldisplay_wayland.h | 8 +- .../gst/gl/wayland/gstgldisplay_wayland_private.h | 34 ------ gst-libs/gst/gl/wayland/gstglwindow_wayland_egl.c | 116 +++++++++++++-------- gst-libs/gst/gl/wayland/gstglwindow_wayland_egl.h | 2 + gst-libs/gst/gl/wayland/wayland_event_source.c | 57 ++++++---- gst-libs/gst/gl/wayland/wayland_event_source.h | 3 +- 8 files changed, 121 insertions(+), 175 deletions(-) delete mode 100644 gst-libs/gst/gl/wayland/gstgldisplay_wayland_private.h diff --git a/gst-libs/gst/gl/meson.build b/gst-libs/gst/gl/meson.build index 2a91bc5..deca145 100644 --- a/gst-libs/gst/gl/meson.build +++ b/gst-libs/gst/gl/meson.build @@ -537,7 +537,7 @@ if need_win_wayland != 'no' message ('Could not find EGL libraries for wayland') endif else - wayland_client_dep = dependency('wayland-client', version : '>= 1.0', required : false) + wayland_client_dep = dependency('wayland-client', version : '>= 1.11', required : false) wayland_cursor_dep = dependency('wayland-cursor', version : '>= 1.0', required : false) wayland_egl_dep = dependency('wayland-egl', version : '>= 1.0', required : false) wayland_protocols_dep = dependency('wayland-protocols', version : '>= 1.15', required : false) diff --git a/gst-libs/gst/gl/wayland/gstgldisplay_wayland.c b/gst-libs/gst/gl/wayland/gstgldisplay_wayland.c index a422b23..93eb83c 100644 --- a/gst-libs/gst/gl/wayland/gstgldisplay_wayland.c +++ b/gst-libs/gst/gl/wayland/gstgldisplay_wayland.c @@ -34,7 +34,6 @@ #endif #include "gstgldisplay_wayland.h" -#include "gstgldisplay_wayland_private.h" GST_DEBUG_CATEGORY_STATIC (gst_gl_display_debug); #define GST_CAT_DEFAULT gst_gl_display_debug @@ -42,7 +41,7 @@ GST_DEBUG_CATEGORY_STATIC (gst_gl_display_debug); /* We can't define these in the public struct, or we'd break ABI */ typedef struct _GstGLDisplayWaylandPrivate { - struct xdg_wm_base *xdg_wm_base; + gint dummy; } GstGLDisplayWaylandPrivate; G_DEFINE_TYPE_WITH_PRIVATE (GstGLDisplayWayland, gst_gl_display_wayland, @@ -54,59 +53,6 @@ static gboolean gst_gl_display_wayland_get_foreign_display (GstGLDisplay * display); static void -handle_xdg_wm_base_ping (void *user_data, struct xdg_wm_base *xdg_wm_base, - uint32_t serial) -{ - xdg_wm_base_pong (xdg_wm_base, serial); -} - -static const struct xdg_wm_base_listener xdg_wm_base_listener = { - handle_xdg_wm_base_ping -}; - -static void -registry_handle_global (void *data, struct wl_registry *registry, - uint32_t name, const char *interface, uint32_t version) -{ - GstGLDisplayWayland *display = data; - GstGLDisplayWaylandPrivate *priv = - gst_gl_display_wayland_get_instance_private (display); - - GST_DEBUG_CATEGORY_GET (gst_gl_display_debug, "gldisplay"); - - GST_TRACE_OBJECT (display, "registry_handle_global with registry %p, " - "interface %s, version %u", registry, interface, version); - - if (g_strcmp0 (interface, "wl_compositor") == 0) { - display->compositor = - wl_registry_bind (registry, name, &wl_compositor_interface, 1); - } else if (g_strcmp0 (interface, "wl_subcompositor") == 0) { - display->subcompositor = - wl_registry_bind (registry, name, &wl_subcompositor_interface, 1); - } else if (g_strcmp0 (interface, "xdg_wm_base") == 0) { - priv->xdg_wm_base = - wl_registry_bind (registry, name, &xdg_wm_base_interface, 1); - xdg_wm_base_add_listener (priv->xdg_wm_base, &xdg_wm_base_listener, - display); - } else if (g_strcmp0 (interface, "wl_shell") == 0) { - display->shell = wl_registry_bind (registry, name, &wl_shell_interface, 1); - } -} - -static const struct wl_registry_listener registry_listener = { - registry_handle_global -}; - -static void -_connect_listeners (GstGLDisplayWayland * display) -{ - display->registry = wl_display_get_registry (display->display); - wl_registry_add_listener (display->registry, ®istry_listener, display); - - wl_display_roundtrip (display->display); -} - -static void gst_gl_display_wayland_class_init (GstGLDisplayWaylandClass * klass) { GST_GL_DISPLAY_CLASS (klass)->get_handle = @@ -130,11 +76,6 @@ static void gst_gl_display_wayland_finalize (GObject * object) { GstGLDisplayWayland *display_wayland = GST_GL_DISPLAY_WAYLAND (object); - GstGLDisplayWaylandPrivate *priv = - gst_gl_display_wayland_get_instance_private (display_wayland); - - g_clear_pointer (&display_wayland->shell, wl_shell_destroy); - g_clear_pointer (&priv->xdg_wm_base, xdg_wm_base_destroy); /* Cause eglTerminate() to occur before wl_display_disconnect() * https://bugzilla.gnome.org/show_bug.cgi?id=787293 */ @@ -179,8 +120,6 @@ gst_gl_display_wayland_new (const gchar * name) return NULL; } - _connect_listeners (ret); - return ret; } @@ -207,8 +146,6 @@ gst_gl_display_wayland_new_with_display (struct wl_display * display) ret->display = display; ret->foreign_display = TRUE; - _connect_listeners (ret); - return ret; } @@ -223,12 +160,3 @@ gst_gl_display_wayland_get_foreign_display (GstGLDisplay * display) { return GST_GL_DISPLAY_WAYLAND (display)->foreign_display; } - -struct xdg_wm_base * -gst_gl_display_wayland_get_xdg_wm_base (GstGLDisplayWayland * display) -{ - GstGLDisplayWaylandPrivate *priv = - gst_gl_display_wayland_get_instance_private (display); - - return priv->xdg_wm_base; -} diff --git a/gst-libs/gst/gl/wayland/gstgldisplay_wayland.h b/gst-libs/gst/gl/wayland/gstgldisplay_wayland.h index d5df71b..ea576b4 100644 --- a/gst-libs/gst/gl/wayland/gstgldisplay_wayland.h +++ b/gst-libs/gst/gl/wayland/gstgldisplay_wayland.h @@ -53,12 +53,12 @@ struct _GstGLDisplayWayland GstGLDisplay parent; struct wl_display *display; - struct wl_registry *registry; - struct wl_compositor *compositor; - struct wl_subcompositor *subcompositor; + struct wl_registry *registry; /* unset */ + struct wl_compositor *compositor; /* unset */ + struct wl_subcompositor *subcompositor; /* unset */ /* Basic shell, see private struct for others (e.g. XDG-shell) */ - struct wl_shell *shell; + struct wl_shell *shell; /* unset */ /*< private >*/ gboolean foreign_display; diff --git a/gst-libs/gst/gl/wayland/gstgldisplay_wayland_private.h b/gst-libs/gst/gl/wayland/gstgldisplay_wayland_private.h deleted file mode 100644 index 0d3ab01..0000000 --- a/gst-libs/gst/gl/wayland/gstgldisplay_wayland_private.h +++ /dev/null @@ -1,34 +0,0 @@ -/* GStreamer - * Copyright (C) 2019 Niels De Graef - * - * This library is free software; you can redistribute it and/or - * modify it under the terms of the GNU Library General Public - * License as published by the Free Software Foundation; either - * version 2 of the License, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Library General Public License for more details. - * - * You should have received a copy of the GNU Library General Public - * License along with this library; if not, write to the - * Free Software Foundation, Inc., 51 Franklin St, Fifth Floor, - * Boston, MA 02110-1301, USA. - */ - -#ifndef __GST_GL_DISPLAY_WAYLAND_PRIVATE_H__ -#define __GST_GL_DISPLAY_WAYLAND_PRIVATE_H__ - -#include -#include "xdg-shell-client-protocol.h" - -G_BEGIN_DECLS - -G_GNUC_INTERNAL -struct xdg_wm_base * -gst_gl_display_wayland_get_xdg_wm_base (GstGLDisplayWayland * display); - -G_END_DECLS - -#endif /* __GST_GL_DISPLAY_WAYLAND_PRIVATE_H__ */ diff --git a/gst-libs/gst/gl/wayland/gstglwindow_wayland_egl.c b/gst-libs/gst/gl/wayland/gstglwindow_wayland_egl.c index 43eca98..7b51c90 100644 --- a/gst-libs/gst/gl/wayland/gstglwindow_wayland_egl.c +++ b/gst-libs/gst/gl/wayland/gstglwindow_wayland_egl.c @@ -33,7 +33,6 @@ #include #include "gstgldisplay_wayland.h" -#include "gstgldisplay_wayland_private.h" #include "gstglwindow_wayland_egl.h" #include "../gstglwindow_private.h" @@ -301,35 +300,19 @@ destroy_surfaces (GstGLWindowWaylandEGL * window_egl) static void create_xdg_surface_and_toplevel (GstGLWindowWaylandEGL * window_egl) { - GstGLDisplayWayland *display = - GST_GL_DISPLAY_WAYLAND (GST_GL_WINDOW (window_egl)->display); - struct xdg_wm_base *xdg_wm_base; struct xdg_surface *xdg_surface; struct xdg_toplevel *xdg_toplevel; GST_DEBUG ("Creating surfaces XDG-shell"); /* First create the XDG surface */ - xdg_wm_base = gst_gl_display_wayland_get_xdg_wm_base (display); - if (window_egl->window.queue) { - wl_proxy_set_queue ((struct wl_proxy *) xdg_wm_base, - window_egl->window.queue); - } - xdg_surface = xdg_wm_base_get_xdg_surface (xdg_wm_base, + xdg_surface = xdg_wm_base_get_xdg_surface (window_egl->display.xdg_wm_base, window_egl->window.surface); - if (window_egl->window.queue) { - wl_proxy_set_queue ((struct wl_proxy *) xdg_surface, - window_egl->window.queue); - } xdg_surface_add_listener (xdg_surface, &xdg_surface_listener, window_egl); /* Then the XDG top-level */ xdg_toplevel = xdg_surface_get_toplevel (xdg_surface); xdg_toplevel_set_title (xdg_toplevel, "OpenGL Renderer"); - if (window_egl->window.queue) { - wl_proxy_set_queue ((struct wl_proxy *) xdg_toplevel, - window_egl->window.queue); - } xdg_toplevel_add_listener (xdg_toplevel, &xdg_toplevel_listener, window_egl); /* Commit the xdg_surface state */ @@ -343,20 +326,13 @@ create_xdg_surface_and_toplevel (GstGLWindowWaylandEGL * window_egl) static void create_wl_shell_surface (GstGLWindowWaylandEGL * window_egl) { - GstGLDisplayWayland *display = - GST_GL_DISPLAY_WAYLAND (GST_GL_WINDOW (window_egl)->display); struct wl_shell_surface *wl_shell_surface; GST_DEBUG ("Creating surfaces for wl-shell"); - wl_shell_surface = wl_shell_get_shell_surface (display->shell, + wl_shell_surface = wl_shell_get_shell_surface (window_egl->display.shell, window_egl->window.surface); - if (window_egl->window.queue) { - wl_proxy_set_queue ((struct wl_proxy *) wl_shell_surface, - window_egl->window.queue); - } - wl_shell_surface_add_listener (wl_shell_surface, &wl_shell_surface_listener, window_egl); wl_shell_surface_set_title (wl_shell_surface, "OpenGL Renderer"); @@ -368,21 +344,16 @@ create_wl_shell_surface (GstGLWindowWaylandEGL * window_egl) static void create_surfaces (GstGLWindowWaylandEGL * window_egl) { - GstGLDisplayWayland *display = - GST_GL_DISPLAY_WAYLAND (GST_GL_WINDOW (window_egl)->display); gint width, height; if (!window_egl->window.surface) { window_egl->window.surface = - wl_compositor_create_surface (display->compositor); - if (window_egl->window.queue) - wl_proxy_set_queue ((struct wl_proxy *) window_egl->window.surface, - window_egl->window.queue); + wl_compositor_create_surface (window_egl->display.compositor); } if (window_egl->window.foreign_surface) { /* (re)parent */ - if (!display->subcompositor) { + if (!window_egl->display.subcompositor) { GST_ERROR_OBJECT (window_egl, "Wayland server does not support subsurfaces"); window_egl->window.foreign_surface = NULL; @@ -391,11 +362,8 @@ create_surfaces (GstGLWindowWaylandEGL * window_egl) if (!window_egl->window.subsurface) { window_egl->window.subsurface = - wl_subcompositor_get_subsurface (display->subcompositor, + wl_subcompositor_get_subsurface (window_egl->display.subcompositor, window_egl->window.surface, window_egl->window.foreign_surface); - if (window_egl->window.queue) - wl_proxy_set_queue ((struct wl_proxy *) window_egl->window.subsurface, - window_egl->window.queue); wl_subsurface_set_position (window_egl->window.subsurface, window_egl->window.window_x, window_egl->window.window_y); @@ -403,7 +371,7 @@ create_surfaces (GstGLWindowWaylandEGL * window_egl) } } else { shell_window: - if (gst_gl_display_wayland_get_xdg_wm_base (display)) { + if (window_egl->display.xdg_wm_base) { if (!window_egl->window.xdg_surface) create_xdg_surface_and_toplevel (window_egl); } else if (!window_egl->window.wl_shell_surface) { @@ -508,6 +476,47 @@ gst_gl_window_wayland_egl_close (GstGLWindow * window) GST_GL_WINDOW_CLASS (parent_class)->close (window); } +static void +handle_xdg_wm_base_ping (void *user_data, struct xdg_wm_base *xdg_wm_base, + uint32_t serial) +{ + xdg_wm_base_pong (xdg_wm_base, serial); +} + +static const struct xdg_wm_base_listener xdg_wm_base_listener = { + handle_xdg_wm_base_ping +}; + +static void +registry_handle_global (void *data, struct wl_registry *registry, + uint32_t name, const char *interface, uint32_t version) +{ + GstGLWindowWaylandEGL *window_wayland = data; + + GST_TRACE_OBJECT (window_wayland, "registry_handle_global with registry %p, " + "interface %s, version %u", registry, interface, version); + + if (g_strcmp0 (interface, "wl_compositor") == 0) { + window_wayland->display.compositor = + wl_registry_bind (registry, name, &wl_compositor_interface, 1); + } else if (g_strcmp0 (interface, "wl_subcompositor") == 0) { + window_wayland->display.subcompositor = + wl_registry_bind (registry, name, &wl_subcompositor_interface, 1); + } else if (g_strcmp0 (interface, "xdg_wm_base") == 0) { + window_wayland->display.xdg_wm_base = + wl_registry_bind (registry, name, &xdg_wm_base_interface, 1); + xdg_wm_base_add_listener (window_wayland->display.xdg_wm_base, + &xdg_wm_base_listener, window_wayland); + } else if (g_strcmp0 (interface, "wl_shell") == 0) { + window_wayland->display.shell = + wl_registry_bind (registry, name, &wl_shell_interface, 1); + } +} + +static const struct wl_registry_listener registry_listener = { + registry_handle_global +}; + static gboolean gst_gl_window_wayland_egl_open (GstGLWindow * window, GError ** error) { @@ -529,7 +538,29 @@ gst_gl_window_wayland_egl_open (GstGLWindow * window, GError ** error) return FALSE; } - window_egl->window.queue = wl_display_create_queue (display->display); + /* we create a proxy wrapper for the display so that we can set the queue on + * it. This allows us to avoid sprinkling `wl_proxy_set_queue()` calls for + * each wayland resource we create as well as removing a race between + * creation and the `wl_proxy_set_queue()` call. */ + window_egl->display.display = wl_proxy_create_wrapper (display->display); + window_egl->window.queue = + wl_display_create_queue (window_egl->display.display); + + wl_proxy_set_queue ((struct wl_proxy *) window_egl->display.display, + window_egl->window.queue); + + window_egl->display.registry = + wl_display_get_registry (window_egl->display.display); + wl_registry_add_listener (window_egl->display.registry, ®istry_listener, + window_egl); + + if (gst_gl_wl_display_roundtrip_queue (window_egl->display.display, + display->display, window_egl->window.queue) < 0) { + g_set_error (error, GST_GL_WINDOW_ERROR, + GST_GL_WINDOW_ERROR_RESOURCE_UNAVAILABLE, + "Failed to perform a wayland roundtrip"); + return FALSE; + } window_egl->wl_source = wayland_event_source_new (display->display, window_egl->window.queue); @@ -574,14 +605,13 @@ gst_gl_window_wayland_egl_set_window_handle (GstGLWindow * window, static void _roundtrip_async (GstGLWindow * window) { - GstGLDisplayWayland *display_wayland = - GST_GL_DISPLAY_WAYLAND (window->display); + GstGLDisplayWayland *display = GST_GL_DISPLAY_WAYLAND (window->display); GstGLWindowWaylandEGL *window_egl = GST_GL_WINDOW_WAYLAND_EGL (window); create_surfaces (window_egl); - if (gst_gl_wl_display_roundtrip_queue (display_wayland->display, - window_egl->window.queue) < 0) + if (gst_gl_wl_display_roundtrip_queue (window_egl->display.display, + display->display, window_egl->window.queue) < 0) GST_WARNING_OBJECT (window, "failed a roundtrip"); } diff --git a/gst-libs/gst/gl/wayland/gstglwindow_wayland_egl.h b/gst-libs/gst/gl/wayland/gstglwindow_wayland_egl.h index 89dedd9..0212744 100644 --- a/gst-libs/gst/gl/wayland/gstglwindow_wayland_egl.h +++ b/gst-libs/gst/gl/wayland/gstglwindow_wayland_egl.h @@ -46,6 +46,7 @@ struct display { struct wl_display *display; struct wl_registry *registry; struct wl_compositor *compositor; + struct wl_subcompositor *subcompositor; struct wl_shell *shell; struct wl_seat *seat; struct wl_pointer *pointer; @@ -54,6 +55,7 @@ struct display { struct wl_cursor_theme *cursor_theme; struct wl_cursor *default_cursor; struct wl_surface *cursor_surface; + struct xdg_wm_base *xdg_wm_base; struct window *window; guint32 serial; diff --git a/gst-libs/gst/gl/wayland/wayland_event_source.c b/gst-libs/gst/gl/wayland/wayland_event_source.c index 553125e..caeb8cd 100644 --- a/gst-libs/gst/gl/wayland/wayland_event_source.c +++ b/gst-libs/gst/gl/wayland/wayland_event_source.c @@ -39,6 +39,21 @@ #include "wayland_event_source.h" +#define GST_CAT_DEFAULT gst_gl_wayland_event_source_debug +GST_DEBUG_CATEGORY_STATIC (GST_CAT_DEFAULT); + +static void +init_debug (void) +{ + static volatile gsize _debug; + + if (g_once_init_enter (&_debug)) { + GST_DEBUG_CATEGORY_INIT (GST_CAT_DEFAULT, "glwaylandeventsource", 0, + "OpenGL Wayland event source"); + g_once_init_leave (&_debug, 1); + } +} + static void sync_callback (void *data, struct wl_callback *callback, uint32_t serial) { @@ -62,46 +77,48 @@ static const struct wl_callback_listener sync_listener = { * As a concrete example, if the wayland event source (below) for a @queue is * running on a certain thread, then this function must only be called in that * thread (with that @queue). */ +/* @sync_display is the wl_display that is used to create the sync object and + * may be a proxy wrapper. + * @dispatch_display must not be a proxy wrapper. + * @queue can be NULL. */ gint -gst_gl_wl_display_roundtrip_queue (struct wl_display *display, - struct wl_event_queue *queue) +gst_gl_wl_display_roundtrip_queue (struct wl_display *sync_display, + struct wl_display *dispatch_display, struct wl_event_queue *queue) { struct wl_callback *callback; gboolean done = FALSE; gint ret = 0; - GST_TRACE ("roundtrip start for dpy %p and queue %p", display, queue); + init_debug (); - if (queue) { - /* creating a wl_proxy and setting the queue is racy with the dispatching - * of the default queue */ - while (wl_display_prepare_read_queue (display, queue) != 0) { - if ((ret = wl_display_dispatch_queue_pending (display, queue)) < 0) { - return ret; - } - } - } - if (!(callback = wl_display_sync (display))) { + GST_TRACE ("roundtrip start for dpy %p and queue %p", dispatch_display, + queue); + + if (!(callback = wl_display_sync (sync_display))) { + GST_WARNING ("creating sync callback failed"); return -1; } GST_TRACE ("create roundtrip callback %p", callback); wl_callback_add_listener (callback, &sync_listener, &done); if (queue) { - wl_proxy_set_queue ((struct wl_proxy *) callback, queue); - wl_display_cancel_read (display); while (!done && ret >= 0) { - ret = wl_display_dispatch_queue (display, queue); + ret = wl_display_dispatch_queue (dispatch_display, queue); + GST_TRACE ("dispatch ret: %i, errno: (%i, 0x%x) \'%s\'", ret, errno, + errno, g_strerror (errno)); } } else { while (!done && ret >= 0) { - ret = wl_display_dispatch (display); + ret = wl_display_dispatch (dispatch_display); + GST_TRACE ("dispatch ret: %i, errno: (%i, 0x%x) \'%s\'", ret, errno, + errno, g_strerror (errno)); } } if (ret == -1 && !done) wl_callback_destroy (callback); - GST_TRACE ("roundtrip done for dpy %p and queue %p. ret %i", display, queue, - ret); + GST_DEBUG ("roundtrip done for dpy %p and queue %p. ret %i, " + "errno: (%i, 0x%x) \'%s\'", dispatch_display, queue, ret, errno, errno, + g_strerror (errno)); return ret; } @@ -204,6 +221,8 @@ wayland_event_source_new (struct wl_display *display, { WaylandEventSource *source; + init_debug (); + source = (WaylandEventSource *) g_source_new (&wayland_event_source_funcs, sizeof (WaylandEventSource)); source->display = display; diff --git a/gst-libs/gst/gl/wayland/wayland_event_source.h b/gst-libs/gst/gl/wayland/wayland_event_source.h index fa20892..9d651ec 100644 --- a/gst-libs/gst/gl/wayland/wayland_event_source.h +++ b/gst-libs/gst/gl/wayland/wayland_event_source.h @@ -36,7 +36,8 @@ GSource * wayland_event_source_new (struct wl_display *display, struct wl_event_queue *queue); -G_GNUC_INTERNAL gint gst_gl_wl_display_roundtrip_queue (struct wl_display *display, +G_GNUC_INTERNAL gint gst_gl_wl_display_roundtrip_queue (struct wl_display *sync_display, + struct wl_display *dispatch_display, struct wl_event_queue *queue); #endif /* __WAYLAND_EVENT_SOURCE_H__ */ -- 2.7.4