After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 755251 - gtksink: Rework threading around GtkWindow creation
gtksink: Rework threading around GtkWindow creation
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal normal
: 1.6.0
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-09-19 10:31 UTC by Thibault Saunier
Modified: 2015-09-24 12:19 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gtk: Marshall state change from NULL to READY in the main thread (3.05 KB, patch)
2015-09-24 08:52 UTC, Thibault Saunier
needs-work Details | Review
gtk: Marshall state changes in the main thread (4.13 KB, patch)
2015-09-24 09:22 UTC, Thibault Saunier
none Details | Review
gtk: Marshall state changes in the main thread (4.38 KB, patch)
2015-09-24 09:39 UTC, Thibault Saunier
none Details | Review
gtk: Factor out a function to run a function on main thread (9.36 KB, patch)
2015-09-24 09:39 UTC, Thibault Saunier
none Details | Review
gtk: Marshall state changes in the main thread (4.50 KB, patch)
2015-09-24 09:57 UTC, Thibault Saunier
committed Details | Review
gtk: Factor out a function to run a function on main thread (9.53 KB, patch)
2015-09-24 09:57 UTC, Thibault Saunier
committed Details | Review

Description Thibault Saunier 2015-09-19 10:31:21 UTC
Currently, in gtkglsink, we create a GtkWindow in ->start which can potentially happen from any thread, which is wrong as Gtk is not MT safe.

Also creating the window between NULL to READY is suboptimal from a user perspective as we might be doing that state change just to check whether we can use the element or not (and we do not expect a Window to raise at that point!).
Comment 1 Nicolas Dufresne (ndufresne) 2015-09-19 12:49:06 UTC
For this element, the state change MUST be done from the main thread, otherwise everything falls appart. Creating the windows early is only required for GL as we need the window to create a context.
Comment 2 Sebastian Dröge (slomo) 2015-09-19 15:56:22 UTC
(In reply to Nicolas Dufresne (stormer) from comment #1)
> For this element, the state change MUST be done from the main thread,
> otherwise everything falls appart.

I hope you mean this only for the case when the sink creates its own window?


You could do the window creation from a g_main_context_invoke() on the default main context.
Comment 3 Nicolas Dufresne (ndufresne) 2015-09-19 19:31:15 UTC
I do mean it, well the NULL to READY and READY to NULL state change (not the rest). Even unreffing GTK object is illegal outside the main thread.
Comment 4 Thibault Saunier 2015-09-24 08:25:05 UTC
(In reply to Nicolas Dufresne (stormer) from comment #1)
> For this element, the state change MUST be done from the main thread,
> otherwise everything falls appart. Creating the windows early is only
> required for GL as we need the window to create a context.

This is *really* bad as for example playsink will try the sink setting it state to READY from any streaming thread...

I had bad deadlocks and X crashes because of that and the solution I currently use is to set the sink state to READY before using it in playsink (but it is not nice...)
Comment 5 Thibault Saunier 2015-09-24 08:52:33 UTC
Created attachment 312012 [details] [review]
gtk: Marshall state change from NULL to READY in the main thread

Gtk is not MT safe thus we need to make sure that everything is done
in the main thread when working with it.
Comment 6 Sebastian Dröge (slomo) 2015-09-24 08:56:53 UTC
Review of attachment 312012 [details] [review]:

::: ext/gtk/gstgtkbasesink.c
@@ +356,3 @@
+gst_gtk_base_sink_start (GstBaseSink * bsink)
+{
+  return ! !_invoke_on_main ((GThreadFunc) gst_gtk_base_sink_start_in_main,

stop() should probably be on main too? And _show_window_cb() maybe with the _invoke_on_main() helper?

What about gst_gtk_base_sink_set_caps()? It calls into the widget from the streaming thread. And show_frame()?
Comment 7 Matthew Waters (ystreet00) 2015-09-24 09:17:14 UTC
Review of attachment 312012 [details] [review]:

Like the idea

::: ext/gtk/gstgtkbasesink.c
@@ +90,3 @@
+
+static gboolean
+_invoke_func (struct invoke_context *info)

This stuff already exists in gtkgstglwidget so you will want to share with them

@@ +258,3 @@
+      g_value_set_object (value,
+          _invoke_on_main ((GThreadFunc) gst_gtk_base_sink_get_widget,
+              gtk_sink));

This would be extremely prone to deadlocks if called from a thread that blocks the main thread in any way.

Also, performing a round trip to the main thread every g_object_get (obj, "widget") is not a great idea.  Cache and locks please with an _invoke_on_main() on a cache miss.

@@ +356,3 @@
+gst_gtk_base_sink_start (GstBaseSink * bsink)
+{
+  return ! !_invoke_on_main ((GThreadFunc) gst_gtk_base_sink_start_in_main,

stop - yes
show_window_cb - yes
set_caps - no
show_frame - no

set_caps and show_frame are designed to be called from the streaming thread already.
Comment 8 Thibault Saunier 2015-09-24 09:22:16 UTC
Created attachment 312017 [details] [review]
gtk: Marshall state changes in the main thread

Gtk is not MT safe thus we need to make sure that everything is done
in the main thread when working with it.
Comment 9 Thibault Saunier 2015-09-24 09:22:24 UTC
Review of attachment 312012 [details] [review]:

::: ext/gtk/gstgtkbasesink.c
@@ +356,3 @@
+gst_gtk_base_sink_start (GstBaseSink * bsink)
+{
+  return ! !_invoke_on_main ((GThreadFunc) gst_gtk_base_sink_start_in_main,

> stop() should probably be on main too? And _show_window_cb() maybe with the _invoke_on_main() helper?

Done in new version

> What about gst_gtk_base_sink_set_caps()? It calls into the widget from the streaming thread. And show_frame()?

set_caps only calls gtk_base_sink_set_widget_format which is MT safe (no direct call to Gtk itself).

show_frame calls gtk_gst_base_widget_set_buffer which in turns just does an g_idle_add_full to set the buffer.
Comment 10 Thibault Saunier 2015-09-24 09:39:10 UTC
Created attachment 312021 [details] [review]
gtk: Marshall state changes in the main thread

Gtk is not MT safe thus we need to make sure that everything is done
in the main thread when working with it.
Comment 11 Thibault Saunier 2015-09-24 09:39:16 UTC
Created attachment 312022 [details] [review]
gtk: Factor out a function to run a function on main thread
Comment 12 Thibault Saunier 2015-09-24 09:41:17 UTC
(In reply to Matthew Waters from comment #7)
> Review of attachment 312012 [details] [review] [review]:
> 
> ::: ext/gtk/gstgtkbasesink.c
> @@ +90,3 @@
> +
> +static gboolean
> +_invoke_func (struct invoke_context *info)
> 
> This stuff already exists in gtkgstglwidget so you will want to share with
> them

Factored out in a new gstgtkutils.c file

> @@ +258,3 @@
> +      g_value_set_object (value,
> +          _invoke_on_main ((GThreadFunc) gst_gtk_base_sink_get_widget,
> +              gtk_sink));
> 
> This would be extremely prone to deadlocks if called from a thread that
> blocks the main thread in any way.
> 
> Also, performing a round trip to the main thread every g_object_get (obj,
> "widget") is not a great idea.  Cache and locks please with an
> _invoke_on_main() on a cache miss.

Done
Comment 13 Sebastian Dröge (slomo) 2015-09-24 09:47:32 UTC
Review of attachment 312021 [details] [review]:

::: ext/gtk/gstgtkbasesink.c
@@ +418,3 @@
+      if (window) {
+        _invoke_on_main ((GThreadFunc) gtk_widget_show_all, window);
+        g_object_unref (window);

What if this was the last reference now? I'd rather unref it in whatever function you invoke on the main thread.
Comment 14 Sebastian Dröge (slomo) 2015-09-24 09:49:01 UTC
Review of attachment 312022 [details] [review]:

Looks good
Comment 15 Matthew Waters (ystreet00) 2015-09-24 09:55:06 UTC
Review of attachment 312022 [details] [review]:

Looks good.
Comment 16 Matthew Waters (ystreet00) 2015-09-24 09:55:52 UTC
Review of attachment 312021 [details] [review]:

Looks good with Sebastian's comment.
Comment 17 Thibault Saunier 2015-09-24 09:57:43 UTC
Created attachment 312028 [details] [review]
gtk: Marshall state changes in the main thread

Gtk is not MT safe thus we need to make sure that everything is done
in the main thread when working with it.
Comment 18 Thibault Saunier 2015-09-24 09:57:49 UTC
Created attachment 312029 [details] [review]
gtk: Factor out a function to run a function on main thread
Comment 19 Thibault Saunier 2015-09-24 10:03:30 UTC
Attachment 312028 [details] pushed as 5ad5f5c - gtk: Marshall state changes in the main thread
Attachment 312029 [details] pushed as 0105760 - gtk: Factor out a function to run a function on main thread
Comment 20 Nicolas Dufresne (ndufresne) 2015-09-24 12:19:56 UTC
Btw, this code that go back to main thread very often deadlock. So it's just a plaster. (e.g. If main thread try to set the pipeline to NULL while the stream thread is waiting for an operation to run on main thread it deadlock). This is why this element was not meant to be used with playsink, but by application directly. The rest was just added to ease debugging, if it's causing issues, we should just remove the windowing code entirely (or make it a property at best).