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

Hi,

Post by Frediano Ziglio
In order to support GStreamer overlay this is necessary as some
plugins can use X11 from a different thread.
---
src/spice-widget.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
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
+

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?

2. Wouldn't be more correct to call it by gtk/gdk interface (i think
teuf has mentioned such function)

Thanks, Snir.

Post by Frediano Ziglio
static void* prepare_streaming_mode(SpiceChannel *channel, bool streaming_mode, gpointer data)
{
#ifdef GDK_WINDOWING_X11

Frediano Ziglio

2018-11-26 11:27:48 UTC

Post by Snir Sheriber
Hi,

Post by Frediano Ziglio
In order to support GStreamer overlay this is necessary as some
plugins can use X11 from a different thread.
---
src/spice-widget.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
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
+

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 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?

Post by Snir Sheriber
2. Wouldn't be more correct to call it by gtk/gdk interface (i think
teuf has mentioned such function)

I did a search and somebody suggested to not use gdk_threads_init
which currently is deprecated.

Waiting for a comment from teuf too.

Post by Snir Sheriber
Thanks, Snir.

Post by Frediano Ziglio
static void* prepare_streaming_mode(SpiceChannel *channel, bool
streaming_mode, gpointer data)
{
#ifdef GDK_WINDOWING_X11

Frediano

Christophe Fergeau

2018-11-26 14:05:34 UTC

Post by Snir Sheriber
Hi,

Post by Frediano Ziglio
In order to support GStreamer overlay this is necessary as some
plugins can use X11 from a different thread.
---
src/spice-widget.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
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".

Post by Snir Sheriber

Post by Frediano Ziglio
+

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

Post by Frediano Ziglio
- 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 Ziglio

2018-11-26 14:37:31 UTC

Post by Snir Sheriber
Hi,

Post by Frediano Ziglio
In order to support GStreamer overlay this is necessary as some
plugins can use X11 from a different thread.
---
src/spice-widget.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
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

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.

Post by Snir Sheriber

Post by Frediano Ziglio
+

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

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

Christophe Fergeau

2018-11-27 10:52:02 UTC

Post by Frediano Ziglio
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".

virt-manager is python + gobject-introspection, and only load spice-gtk
shared libraries when you connect to a VM display.

Daniel P. Berrangé

2018-11-27 11:02:17 UTC

Hi,

Post by Frediano Ziglio
In order to support GStreamer overlay this is necessary as some
plugins can use X11 from a different thread.
---
src/spice-widget.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
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

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

--
|: 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 :|

Frediano Ziglio

2018-11-27 11:18:21 UTC

Hi,

Post by Frediano Ziglio
In order to support GStreamer overlay this is necessary as some
plugins can use X11 from a different thread.
---
src/spice-widget.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
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

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

Daniel P. Berrangé

2018-11-28 11:09:43 UTC