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

[Christophe Fergeau]

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".

> > > +
> > 
> > 
> > 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.

> - 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20181126/67ad329d/attachment.sig>