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

[Frediano Ziglio]

> 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.
We already link X11 directly (or the code won't link!) so is not a
big issue from my point of view.

> > > > +
> > > 
> > > 
> > > So, I'd ack but i still have two small worries 1. would it harm to call
> > > it also when it's not really needed?
> > > 
> > 
> > In theory it could impact slightly performances as locks are used. On the
> > other way:
> > - how can you make sure program is not using X11 from multiple threads?
> >   You safely can't!
> 
> If the program is doing GUI stuff from multiple threads, hopefully you
> can assume that it took proper care of doing whatever is needed for that
> to work.
> 

Here we are fixing a real issue, not something theoretical, the code is
not doing that. Now... is a plugin bug (vaapi ? gstreamer-vaapi ? glimagesink ?)
or is an application (in this case I would say spice-glib or spice-gtk) bug?
I don't think there's a good answer, it depends from the point of view.
GStreamer could say: "I don't know if any plugin would use X11".
vaapi could say: "I don't know if the user will be using X11".
gstreamer-vaapi: "I don't know if the application will output to the screen".
spice-gtk/spice-glib: "Maybe I'll use GStreamer which will use a plugin which
will use X11 but something in GStreamer should take care of it".
virt-viewer/other app: "I'm not using X11 multithread, why I should take care".

One good thing of doing from spice-gtk is that every program linking
spice-gtk not with dlopen (so virt-viewer, virt-manager and spicy for instance)
will be "fixed".

IMO spice-gtk is a good place, applications could be an addition.

> > - if GStreamer is used (which now should) program is likely to use multiple
> >   thread so not calling this function is more likely to be a bug;
> > - do we really need all these performance to risk program crash?
> 
> GStreamer using multiple threads is not an issue in itself. GStreamer
> doing X11 calls from a thread which is not the thread handling the rest
> of the UI is the issue.
> 
> Christophe
> 

Frediano