[vlc-devel] [PATCH v2] wayland: xdg-shell: use vlc_cond_t instead of dispatch

[Alexandre Janniaux]

Hi,

Won't a semaphore be as ugly to write as enable() can be
called multiple time and configure_cb() too, and lead to
a different behaviour?

Condition variable is a bit longer to write but the meaning
feels simpler here.

Regards,
--
Alexandre Janniaux
Videolabs

On Tue, Apr 07, 2020 at 12:39:57PM +0300, Rémi Denis-Courmont wrote:
> Without the surrounding code, I come to think that a semaphore would be far simpler here.
>
> Le 7 avril 2020 11:20:01 GMT+03:00, Alexandre Janniaux <ajanni at videolabs.io> a écrit :
> >When using wl_display_dispatch, there is a race condition with the main
> >loop, in which the condition `(!sys->wm.configured)` would have been
> >checked as true, but the event dispatched from the main thread after
> >that so wl_display_dispatch would block indefinitely.
> >
> >Said differently, wl_display_dispatch should not be called from a
> >different thread than the main thread (in wayland terms).
> >
> >By using a condition variable instead, we let the main thread unblock
> >the vout_window_Enable and prevent this deadlock when the callback for
> >the configured event is called. In addition, it adds the correct lock
> >around the updates and reads of the wm.configured boolean.
> >
> >Fixes #24586
> >---
> > modules/video_output/wayland/xdg-shell.c | 10 +++++++++-
> > 1 file changed, 9 insertions(+), 1 deletion(-)
> >
> >diff --git a/modules/video_output/wayland/xdg-shell.c
> >b/modules/video_output/wayland/xdg-shell.c
> >index 765ffb2a2f..0b0eba0be0 100644
> >--- a/modules/video_output/wayland/xdg-shell.c
> >+++ b/modules/video_output/wayland/xdg-shell.c
> >@@ -106,6 +106,7 @@ typedef struct
> >     struct wl_surface *cursor_surface;
> >
> >     vlc_mutex_t lock;
> >+    vlc_cond_t cond_configured;
> >     vlc_thread_t thread;
> > } vout_window_sys_t;
> >
> >@@ -262,8 +263,10 @@ static int Enable(vout_window_t *wnd, const
> >vout_window_cfg_t *restrict cfg)
> >     wl_display_flush(display);
> >
> > #ifdef XDG_SHELL
> >+    vlc_mutex_lock(&sys->lock);
> >     while (!sys->wm.configured)
> >-        wl_display_dispatch(display);
> >+        vlc_cond_wait(&sys->cond_configured, &sys->lock);
> >+    vlc_mutex_unlock(&sys->lock);
> > #endif
> >
> >     return VLC_SUCCESS;
> >@@ -339,7 +342,11 @@ static void xdg_surface_configure_cb(void *data,
> >struct xdg_surface *surface,
> >         vout_window_ReportWindowed(wnd);
> >
> >     xdg_surface_ack_configure(surface, serial);
> >+
> >+    vlc_mutex_lock(&sys->lock);
> >     sys->wm.configured = true;
> >+    vlc_cond_signal(&sys->cond_configured);
> >+    vlc_mutex_unlock(&sys->lock);
> > }
> >
> > static const struct xdg_surface_listener xdg_surface_cbs =
> >@@ -617,6 +624,7 @@ static int Open(vout_window_t *wnd)
> >     sys->cursor_theme = NULL;
> >     sys->cursor_surface = NULL;
> >     vlc_mutex_init(&sys->lock);
> >+    vlc_cond_init(&sys->cond_configured);
> >     wnd->sys = sys;
> >     wnd->handle.wl = NULL;
> >
> >--
> >2.26.0
> >
> >_______________________________________________
> >vlc-devel mailing list
> >To unsubscribe or modify your subscription options:
> >https://mailman.videolan.org/listinfo/vlc-devel
>
> --
> Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté.

> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel