gtk: Fix race between queue_draw and destroy

In GTK dispose can be called before the last ref is reached. This
happens when you close the container window. The dispose will be
explicitly called, and destroyed notify will be fired. This patch
fixes this race by properly tracking the widget state.

In the sink, we now set the widget pointer to NULL, so the widget
will properly get created again if you set your pipeline to NULL
state after the widget was destroy, and set it back to PLAYING.

https://bugzilla.gnome.org/show_bug.cgi?id=751104

diff --git a/ext/gtk/gstgtkglsink.c b/ext/gtk/gstgtkglsink.c
index 1a98c02..cbb0e94 100644 (file)
--- a/ext/gtk/gstgtkglsink.c
+++ b/ext/gtk/gstgtkglsink.c
@@ -164,7 +164,9 @@ gst_gtk_gl_sink_finalize (GObject * object)
 static void
 widget_destroy_cb (GtkWidget * widget, GstGtkGLSink * gtk_sink)
 {
-  g_atomic_int_set (&gtk_sink->widget_destroyed, 1);
+  GST_OBJECT_LOCK (gtk_sink);
+  g_clear_object (&gtk_sink->widget);
+  GST_OBJECT_UNLOCK (gtk_sink);
 }
 
 static GtkGstGLWidget *
@@ -367,7 +369,10 @@ gst_gtk_gl_sink_change_state (GstElement * element, GstStateChange transition)
     case GST_STATE_CHANGE_PLAYING_TO_PAUSED:
       break;
     case GST_STATE_CHANGE_PAUSED_TO_READY:
-      gtk_gst_gl_widget_set_buffer (gtk_sink->widget, NULL);
+      GST_OBJECT_LOCK (gtk_sink);
+      if (gtk_sink->widget)
+        gtk_gst_gl_widget_set_buffer (gtk_sink->widget, NULL);
+      GST_OBJECT_UNLOCK (gtk_sink);
       break;
     case GST_STATE_CHANGE_READY_TO_NULL:
       if (gtk_sink->display) {
@@ -440,14 +445,19 @@ gst_gtk_gl_sink_show_frame (GstVideoSink * vsink, GstBuffer * buf)
 
   gtk_sink = GST_GTK_GL_SINK (vsink);
 
-  gtk_gst_gl_widget_set_buffer (gtk_sink->widget, buf);
+  GST_OBJECT_LOCK (vsink);
 
-  if (g_atomic_int_get (&gtk_sink->widget_destroyed)) {
+  if (gtk_sink->widget == NULL) {
+    GST_OBJECT_UNLOCK (vsink);
     GST_ELEMENT_ERROR (gtk_sink, RESOURCE, NOT_FOUND,
         ("%s", "Output widget was destroyed"), (NULL));
     return GST_FLOW_ERROR;
   }
 
+  gtk_gst_gl_widget_set_buffer (gtk_sink->widget, buf);
+
+  GST_OBJECT_UNLOCK (vsink);
+
   return GST_FLOW_OK;
 }
 

diff --git a/ext/gtk/gstgtkglsink.h b/ext/gtk/gstgtkglsink.h
index d1c65c0..985486f 100644 (file)
--- a/ext/gtk/gstgtkglsink.h
+++ b/ext/gtk/gstgtkglsink.h
@@ -53,7 +53,6 @@ struct _GstGtkGLSink
   GstVideoSink          parent;
 
   GtkGstGLWidget       *widget;
-  gboolean              widget_destroyed;
 
   GstVideoInfo          v_info;
   GstBufferPool        *pool;

diff --git a/ext/gtk/gstgtksink.c b/ext/gtk/gstgtksink.c
index 2c5d3ed..524ff5a 100644 (file)
--- a/ext/gtk/gstgtksink.c
+++ b/ext/gtk/gstgtksink.c
@@ -294,7 +294,10 @@ gst_gtk_sink_change_state (GstElement * element, GstStateChange transition)
     case GST_STATE_CHANGE_PLAYING_TO_PAUSED:
       break;
     case GST_STATE_CHANGE_PAUSED_TO_READY:
-      gtk_gst_widget_set_buffer (gtk_sink->widget, NULL);
+      GST_OBJECT_LOCK (gtk_sink);
+      if (gtk_sink->widget)
+        gtk_gst_widget_set_buffer (gtk_sink->widget, NULL);
+      GST_OBJECT_UNLOCK (gtk_sink);
       break;
     case GST_STATE_CHANGE_READY_TO_NULL:
       break;
@@ -353,8 +356,18 @@ gst_gtk_sink_show_frame (GstVideoSink * vsink, GstBuffer * buf)
 
   gtk_sink = GST_GTK_SINK (vsink);
 
-  if (gtk_sink->widget)
-    gtk_gst_widget_set_buffer (gtk_sink->widget, buf);
+  GST_OBJECT_LOCK (vsink);
+
+  if (gtk_sink->widget == NULL) {
+    GST_OBJECT_UNLOCK (vsink);
+    GST_ELEMENT_ERROR (gtk_sink, RESOURCE, NOT_FOUND,
+        ("%s", "Output widget was destroyed"), (NULL));
+    return GST_FLOW_ERROR;
+  }
+
+  gtk_gst_widget_set_buffer (gtk_sink->widget, buf);
+
+  GST_OBJECT_UNLOCK (vsink);
 
   return GST_FLOW_OK;
 }

diff --git a/ext/gtk/gtkgstglwidget.c b/ext/gtk/gtkgstglwidget.c
index 2e108c9..3d05d37 100644 (file)
--- a/ext/gtk/gtkgstglwidget.c
+++ b/ext/gtk/gtkgstglwidget.c
@@ -100,6 +100,10 @@ struct _GtkGstGLWidgetPrivate
   GLint attr_position;
   GLint attr_texture;
   GLuint current_tex;
+
+  /* Pending queued idles callback */
+  guint draw_id;
+  guint resize_id;
 };
 
 static void
@@ -430,15 +434,17 @@ gtk_gst_gl_widget_finalize (GObject * object)
     _invoke_on_main ((ThreadFunc) _reset_gl, widget);
   }
 
-  if (widget->priv->context) {
+  if (widget->priv->context)
     gst_object_unref (widget->priv->context);
-    widget->priv->context = NULL;
-  }
 
-  if (widget->priv->display) {
+  if (widget->priv->display)
     gst_object_unref (widget->priv->display);
-    widget->priv->display = NULL;
-  }
+
+  if (widget->priv->draw_id)
+    g_source_remove (widget->priv->draw_id);
+
+  if (widget->priv->resize_id)
+    g_source_remove (widget->priv->resize_id);
 
   G_OBJECT_CLASS (gtk_gst_gl_widget_parent_class)->finalize (object);
 }
@@ -575,6 +581,10 @@ gtk_gst_gl_widget_new (void)
 static gboolean
 _queue_draw (GtkGstGLWidget * widget)
 {
+  g_mutex_lock (&widget->priv->lock);
+  widget->priv->draw_id = 0;
+  g_mutex_unlock (&widget->priv->lock);
+
   gtk_widget_queue_draw (GTK_WIDGET (widget));
 
   return G_SOURCE_REMOVE;
@@ -583,8 +593,6 @@ _queue_draw (GtkGstGLWidget * widget)
 void
 gtk_gst_gl_widget_set_buffer (GtkGstGLWidget * widget, GstBuffer * buffer)
 {
-  GMainContext *main_context = g_main_context_default ();
-
   g_return_if_fail (GTK_IS_GST_GL_WIDGET (widget));
   g_return_if_fail (buffer == NULL || widget->priv->negotiated);
 
@@ -593,9 +601,12 @@ gtk_gst_gl_widget_set_buffer (GtkGstGLWidget * widget, GstBuffer * buffer)
   gst_buffer_replace (&widget->priv->buffer, buffer);
   widget->priv->new_buffer = TRUE;
 
-  g_mutex_unlock (&widget->priv->lock);
+  if (!widget->priv->draw_id) {
+    widget->priv->draw_id = g_idle_add_full (G_PRIORITY_DEFAULT,
+        (GSourceFunc) _queue_draw, widget, NULL);
+  }
 
-  g_main_context_invoke (main_context, (GSourceFunc) _queue_draw, widget);
+  g_mutex_unlock (&widget->priv->lock);
 }
 
 static void
@@ -664,6 +675,10 @@ _get_gl_context (GtkGstGLWidget * gst_widget)
 static gboolean
 _queue_resize (GtkGstGLWidget * widget)
 {
+  g_mutex_lock (&widget->priv->lock);
+  widget->priv->resize_id = 0;
+  g_mutex_unlock (&widget->priv->lock);
+
   gtk_widget_queue_resize (GTK_WIDGET (widget));
 
   return G_SOURCE_REMOVE;
@@ -770,7 +785,6 @@ _calculate_par (GtkGstGLWidget * widget, GstVideoInfo * info)
 gboolean
 gtk_gst_gl_widget_set_caps (GtkGstGLWidget * widget, GstCaps * caps)
 {
-  GMainContext *main_context = g_main_context_default ();
   GstVideoInfo v_info;
 
   g_return_val_if_fail (GTK_IS_GST_GL_WIDGET (widget), FALSE);
@@ -801,9 +815,12 @@ gtk_gst_gl_widget_set_caps (GtkGstGLWidget * widget, GstCaps * caps)
   widget->priv->v_info = v_info;
   widget->priv->negotiated = TRUE;
 
-  g_mutex_unlock (&widget->priv->lock);
+  if (!widget->priv->resize_id) {
+    widget->priv->resize_id = g_idle_add_full (G_PRIORITY_DEFAULT,
+        (GSourceFunc) _queue_resize, widget, NULL);
+  }
 
-  g_main_context_invoke (main_context, (GSourceFunc) _queue_resize, widget);
+  g_mutex_unlock (&widget->priv->lock);
 
   return TRUE;
 }

diff --git a/ext/gtk/gtkgstwidget.c b/ext/gtk/gtkgstwidget.c
index 3e2f9bb..3fbb0ca 100644 (file)
--- a/ext/gtk/gtkgstwidget.c
+++ b/ext/gtk/gtkgstwidget.c
@@ -69,6 +69,10 @@ struct _GtkGstWidgetPrivate
   GstBuffer *buffer;
   GstCaps *caps;
   GstVideoInfo v_info;
+
+  /* Pending queued idles callback */
+  guint draw_id;
+  guint resize_id;
 };
 
 static void
@@ -226,6 +230,12 @@ gtk_gst_widget_finalize (GObject * object)
   gst_buffer_replace (&widget->priv->buffer, NULL);
   g_mutex_clear (&widget->priv->lock);
 
+  if (widget->priv->draw_id)
+    g_source_remove (widget->priv->draw_id);
+
+  if (widget->priv->resize_id)
+    g_source_remove (widget->priv->resize_id);
+
   G_OBJECT_CLASS (gtk_gst_widget_parent_class)->finalize (object);
 }
 
@@ -331,6 +341,10 @@ gtk_gst_widget_new (void)
 static gboolean
 _queue_draw (GtkGstWidget * widget)
 {
+  g_mutex_lock (&widget->priv->lock);
+  widget->priv->draw_id = 0;
+  g_mutex_unlock (&widget->priv->lock);
+
   gtk_widget_queue_draw (GTK_WIDGET (widget));
 
   return G_SOURCE_REMOVE;
@@ -339,8 +353,6 @@ _queue_draw (GtkGstWidget * widget)
 void
 gtk_gst_widget_set_buffer (GtkGstWidget * widget, GstBuffer * buffer)
 {
-  GMainContext *main_context = g_main_context_default ();
-
   g_return_if_fail (GTK_IS_GST_WIDGET (widget));
   g_return_if_fail (buffer == NULL || widget->priv->negotiated);
 
@@ -348,14 +360,21 @@ gtk_gst_widget_set_buffer (GtkGstWidget * widget, GstBuffer * buffer)
 
   gst_buffer_replace (&widget->priv->buffer, buffer);
 
-  g_mutex_unlock (&widget->priv->lock);
+  if (!widget->priv->draw_id) {
+    widget->priv->draw_id = g_idle_add_full (G_PRIORITY_DEFAULT,
+        (GSourceFunc) _queue_draw, widget, NULL);
+  }
 
-  g_main_context_invoke (main_context, (GSourceFunc) _queue_draw, widget);
+  g_mutex_unlock (&widget->priv->lock);
 }
 
 static gboolean
 _queue_resize (GtkGstWidget * widget)
 {
+  g_mutex_lock (&widget->priv->lock);
+  widget->priv->resize_id = 0;
+  g_mutex_unlock (&widget->priv->lock);
+
   gtk_widget_queue_resize (GTK_WIDGET (widget));
 
   return G_SOURCE_REMOVE;
@@ -424,7 +443,6 @@ _calculate_par (GtkGstWidget * widget, GstVideoInfo * info)
 gboolean
 gtk_gst_widget_set_caps (GtkGstWidget * widget, GstCaps * caps)
 {
-  GMainContext *main_context = g_main_context_default ();
   GstVideoInfo v_info;
 
   g_return_val_if_fail (GTK_IS_GST_WIDGET (widget), FALSE);
@@ -455,15 +473,17 @@ gtk_gst_widget_set_caps (GtkGstWidget * widget, GstCaps * caps)
     return FALSE;
   }
 
+  gst_buffer_replace (&widget->priv->buffer, NULL);
   gst_caps_replace (&widget->priv->caps, caps);
   widget->priv->v_info = v_info;
   widget->priv->negotiated = TRUE;
 
-  g_mutex_unlock (&widget->priv->lock);
-
-  gtk_widget_queue_resize (GTK_WIDGET (widget));
+  if (!widget->priv->resize_id) {
+    widget->priv->resize_id = g_idle_add_full (G_PRIORITY_DEFAULT,
+        (GSourceFunc) _queue_resize, widget, NULL);
+  }
 
-  g_main_context_invoke (main_context, (GSourceFunc) _queue_resize, widget);
+  g_mutex_unlock (&widget->priv->lock);
 
   return TRUE;
 }