Created attachment 312076 [details] [review]
gtk: Wait at most 5s for calling something on the main thread

In certain situations we might deadlock, main thread waiting for the
streaming thread, streaming thread waiting for the main thread.

With this we cut the deadlock and error out properly.

Review of attachment 312076 [details] [review]:

::: ext/gtk/gstgtkbasesink.c
@@ +327,3 @@
+  gboolean timeout, ret;
+
+  ret = ! !gst_gtk_invoke_on_main ((GThreadFunc)

Was there before, but why would you want to use ! ! obscureness here, while you can write readable code ?

::: ext/gtk/gstgtkutils.c
@@ +29,3 @@
   GCond cond;
   gboolean fired;
+  gboolean timed_out;

Consistency, timeout in basesink vs timed_out here.

@@ +52,3 @@
+    g_mutex_clear (&info->lock);
+    g_cond_clear (&info->cond);
+    g_free (info);

Using g_main_context_invoke_full() with a destroy notify would avoid the duplication.

@@ +71,1 @@
+  info = g_new0 (struct invoke_context, 1);

Slice new please. I'd suggest having a new/free which will be clean with the invoke_full(),

@@ +80,1 @@
+  g_main_context_invoke (main_context, (GSourceFunc) gst_gtk_invoke_func, info);

Here.

@@ +91,3 @@
+      g_warning ("gtksink: Timed out to call something from the main thread");
+    *timeout = timed_out;
+    /* Leak everything on timeout */

The destroy notify will avoid the leak in a safe way.

(In reply to Nicolas Dufresne (stormer) from comment #2)
> Review of attachment 312076 [details] [review] [review]:
> 
> ::: ext/gtk/gstgtkbasesink.c
> @@ +327,3 @@
> +  gboolean timeout, ret;
> +
> +  ret = ! !gst_gtk_invoke_on_main ((GThreadFunc)
> 
> Was there before, but why would you want to use ! ! obscureness here, while
> you can write readable code ?

Boolean normalization.

> ::: ext/gtk/gstgtkutils.c
> @@ +29,3 @@
>    GCond cond;
>    gboolean fired;
> +  gboolean timed_out;
> 
> Consistency, timeout in basesink vs timed_out here.

Why does basesink matter here? :) And timeout is already the variable we use as out parameter.

> @@ +52,3 @@
> +    g_mutex_clear (&info->lock);
> +    g_cond_clear (&info->cond);
> +    g_free (info);
> 
> Using g_main_context_invoke_full() with a destroy notify would avoid the
> duplication.

No, because otherwise the struct is already freed after g_main_context_invoke() while we still want to get the result from it.

> @@ +71,1 @@
> +  info = g_new0 (struct invoke_context, 1);
> 
> Slice new please. I'd suggest having a new/free which will be clean with the
> invoke_full(),

No, GSlice is deprecated in newer GLib :)

Deprecated ?

(In reply to Sebastian Dröge (slomo) from comment #3)
> > @@ +52,3 @@
> > +    g_mutex_clear (&info->lock);
> > +    g_cond_clear (&info->cond);
> > +    g_free (info);
> > 
> > Using g_main_context_invoke_full() with a destroy notify would avoid the
> > duplication.
> 
> No, because otherwise the struct is already freed after
> g_main_context_invoke() while we still want to get the result from it.
> 

That is not correct. If you use a GDestroyNotify to free the structure (and correctly remove the two duplicate free blocks), the data will be destroy when gst_gtk_invoke_func () returns or if the main loop get destroyed without running the callback. You can also safely call g_source_remove() after a timeout to cleanup the callback if not run yet.

Ah wait, nevermind, I just realise what you didn't say, the mutex and condition need to stay alive. In any case, make a free function, that will be cleaner (and that's what I care here, it's all very messy right now).

And for the boolean, gst_gtk_base_sink == basesink in that context, you define boolean for when we have timed out, one is named timeout and the other timed_out, you could be more consistent.

(In reply to Nicolas Dufresne (stormer) from comment #7)
> And for the boolean, gst_gtk_base_sink == basesink in that context, you
> define boolean for when we have timed out, one is named timeout and the
> other timed_out, you could be more consistent.

I have no idea what you mean with this, but I'll just call them all timeout now :)

Created attachment 312114 [details] [review]
gtk: Wait at most 5s for calling something on the main thread

In certain situations we might deadlock, main thread waiting for the
streaming thread, streaming thread waiting for the main thread.

With this we cut the deadlock and error out properly.

Review of attachment 312114 [details] [review]:

LGTM

Review of attachment 312114 [details] [review]:

There exists possible memory corruption here.

::: ext/gtk/gstgtkutils.c
@@ +99,3 @@
+    /* Leak everything on timeout */
+  } else {
+    free_invoke_context (info);

Won't this double free if timeout_return == NULL and a timeout occurs?

Yes, because of the renaming of the timeout variables I broke it now. Thanks! I won't merge this for 1.6.0 btw, that's 1.6.1 material

Created attachment 312116 [details] [review]
gtk: Wait at most 5s for calling something on the main thread

In certain situations we might deadlock, main thread waiting for the
streaming thread, streaming thread waiting for the main thread.

With this we cut the deadlock and error out properly.

(In reply to Sebastian Dröge (slomo) from comment #12)
> Yes, because of the renaming of the timeout variables I broke it now.
> Thanks! I won't merge this for 1.6.0 btw, that's 1.6.1 material

Good call, will give us time to think about it a bit. I'm still concern about having to wait 5 second if you stop the main loop before GstPlayer.

You don't have to wait for GstPlayer, because on shutdown we only dispatch to the main context *if* gtksink created its own window :)

Except for:

http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/ext/gtk/gstgtkbasesink.c#n146

The last ref to the pipeline need to be triggered by the main thread here (or we need a dispatch to main thread, which is problematic).

What's up with this now?

Maybe we should have written code to demonstrate the issue in the first place ?

Let's get rid of this bug for the time being then until there is an actual problem someone can reproduce.