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

[Frediano Ziglio]

> 
> 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.
> 
> 
> Regards,
> Daniel

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.

Frediano