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@xxxxxxxxxx> > > > > > > > --- > > > > > > > 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 :| _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel