[Spice-devel] [PATCH spice-gtk] spice-widget: Make sure we can use X11 from different thread

[Daniel P. Berrangé]

On Tue, Nov 27, 2018 at 06:18:21AM -0500, Frediano Ziglio wrote:
> > 
> > On Mon, Nov 26, 2018 at 09:37:31AM -0500, Frediano Ziglio wrote:
> > > > On Mon, Nov 26, 2018 at 06:27:48AM -0500, Frediano Ziglio wrote:
> > > > > > 
> > > > > > Hi,
> > > > > > 
> > > > > > On 11/26/18 10:16 AM, Frediano Ziglio wrote:
> > > > > > > In order to support GStreamer overlay this is necessary as some
> > > > > > > plugins can use X11 from a different thread.
> > > > > > >
> > > > > > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > > > > > ---
> > > > > > >   src/spice-widget.c | 13 +++++++++++++
> > > > > > >   1 file changed, 13 insertions(+)
> > > > > > >
> > > > > > > CI result:
> > > > > > > https://gitlab.freedesktop.org/fziglio/spice-gtk/pipelines/9559
> > > > > > >
> > > > > > > diff --git a/src/spice-widget.c b/src/spice-widget.c
> > > > > > > index 312c640a..cca0867e 100644
> > > > > > > --- a/src/spice-widget.c
> > > > > > > +++ b/src/spice-widget.c
> > > > > > > @@ -2547,6 +2547,19 @@ static void queue_draw_area(SpiceDisplay
> > > > > > > *display,
> > > > > > > gint x, gint y,
> > > > > > >                                  x, y, width, height);
> > > > > > >   }
> > > > > > >   
> > > > > > > +#ifdef GDK_WINDOWING_X11
> > > > > > > +/* See
> > > > > > > https://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-base-libs/html/GstGLDisplay.html
> > > > > > > + * (look for XInitThreads). We call it into a constructor to make
> > > > > > > sure
> > > > > > > + * we call before any X11 function.
> > > > > > > + * In case of GStreamer some plugins will use X11 from different
> > > > > > > + * threads.
> > > > > > > + */
> > > > > > > +SPICE_CONSTRUCTOR_FUNC(x11_threads_init)
> > > > > > > +{
> > > > > > > +    XInitThreads();
> > > > > > > +}
> > > > > > > +#endif
> > > > 
> > > > Can this be done at all from a shared library, or should this be the
> > > > responsibility of the library user? For example, if spice-gtk is
> > > > dlopened for some reason, XInitThreads will be called too late.
> > > > #ifdef GDK_WINDOWING_X11 is not enough, you also need some runtime check
> > > > as you don't want to call this when running "GDK_BACKEND=wayland spicy",
> > > > but you want to call this with "GDK_BACKEND=x11 spicy".
> > > > 
> > > 
> > > In this case also the application should do it, but does not harm
> > > to do also in the shared object which likely is going to be loaded
> > > at program start as this is usually the way is done.
> > 
> > If the application already does call XInitThreads, then there is no
> > benefit to calling it in spice-gtk constructors, as threading is already
> > initialized.
> > 
> > If the application does *not* call XInitThreads, then calling it in
> > spice-gtk will cause possible crashes or deadlocks or memory corruption
> > when dlopen'd.
> > 
> > Per the man page the XInitThreads call *must* be the very X11 call
> > made by an application, before *ANY* other X11 call.
> > 
> > Consider if you have a thread A which is in the middle of an X11
> > call. Now thread B triggers dlopen of spice-gtk which calls XInitThreads.
> > When thread A finishes its X11 call it will trigger an XUnlockThreads()
> > call, despite there being no original XLockThreads call. This is very
> > bad as at the very least you will corrupt the mutex state by having too
> > many unlock calls.
> 
> I'm currently trying another option. I'm using an additional X connection to
> handle the overlay. It seems to work perfectly. So potentially each overlay
> requires an additional X11 connection to avoid thread issues.

I looked at the stndard gstreamer X11 video sink plugin, and they use a
dedicated X11 connection too, so that looks like the right approach to
deal with this problem.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|