From 1a7c9b82f4dd17a55da8a383c59b43ed06e23b85 Mon Sep 17 00:00:00 2001 From: Nicolas Dufresne Date: Wed, 15 Jul 2015 14:32:42 -0400 Subject: [PATCH] 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 --- ext/gtk/gstgtkglsink.c | 18 +++++++++++++---- ext/gtk/gstgtkglsink.h | 1 - ext/gtk/gstgtksink.c | 19 +++++++++++++++--- ext/gtk/gtkgstglwidget.c | 43 ++++++++++++++++++++++++++++------------ ext/gtk/gtkgstwidget.c | 38 ++++++++++++++++++++++++++--------- 5 files changed, 89 insertions(+), 30 deletions(-) diff --git a/ext/gtk/gstgtkglsink.c b/ext/gtk/gstgtkglsink.c index 1a98c02213..cbb0e948bb 100644 --- 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 (>k_sink->widget_destroyed, 1); + GST_OBJECT_LOCK (gtk_sink); + g_clear_object (>k_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 (>k_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 d1c65c00a8..985486fb24 100644 --- 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 2c5d3ed3eb..524ff5ab6c 100644 --- 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 2e108c9f93..3d05d37b7d 100644 --- 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 3e2f9bb286..3fbb0ca4ed 100644 --- 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; + if (!widget->priv->resize_id) { + widget->priv->resize_id = g_idle_add_full (G_PRIORITY_DEFAULT, + (GSourceFunc) _queue_resize, widget, NULL); + } + g_mutex_unlock (&widget->priv->lock); - gtk_widget_queue_resize (GTK_WIDGET (widget)); - - g_main_context_invoke (main_context, (GSourceFunc) _queue_resize, widget); - return TRUE; }