gtkglsink: Fix unsafe handling of buffer life time

We need to keep the active buffer (the one we have retreive a
texture id from) otherwise it's racy and upstream may upload
new content before we have rendered or during later redisplay.

4 files changed, 58 insertions, 32 deletions

diff --git a/ext/gtk/gtkgstbasewidget.c b/ext/gtk/gtkgstbasewidget.c
index 4202a22ff..5c7cda31c 100644
--- a/ext/gtk/gtkgstbasewidget.c
+++ b/ext/gtk/gtkgstbasewidget.c
@@ -190,7 +190,6 @@ _apply_par (GtkGstBaseWidget * widget)
   GST_DEBUG ("scaling to %dx%d", widget->display_width, widget->display_height);
 }
 
-/* note, buffer is not refrence, it's only passed for pointer comparision */
 static gboolean
 _queue_draw (GtkGstBaseWidget * widget)
 {
@@ -202,7 +201,6 @@ _queue_draw (GtkGstBaseWidget * widget)
 
     widget->v_info = widget->pending_v_info;
     widget->negotiated = TRUE;
-    widget->new_buffer = TRUE;
 
     _apply_par (widget);
 
@@ -434,6 +432,7 @@ gtk_gst_base_widget_finalize (GObject * object)
 {
   GtkGstBaseWidget *widget = GTK_GST_BASE_WIDGET (object);
 
+  gst_buffer_replace (&widget->pending_buffer, NULL);
   gst_buffer_replace (&widget->buffer, NULL);
   g_mutex_clear (&widget->lock);
   g_weak_ref_clear (&widget->element);
@@ -481,8 +480,7 @@ gtk_gst_base_widget_set_buffer (GtkGstBaseWidget * widget, GstBuffer * buffer)
 
   GTK_GST_BASE_WIDGET_LOCK (widget);
 
-  gst_buffer_replace (&widget->buffer, buffer);
-  widget->new_buffer = TRUE;
+  gst_buffer_replace (&widget->pending_buffer, buffer);
 
   if (!widget->draw_id) {
     widget->draw_id = g_idle_add_full (G_PRIORITY_DEFAULT,
diff --git a/ext/gtk/gtkgstbasewidget.h b/ext/gtk/gtkgstbasewidget.h
index 0a05ca54d..13737c632 100644
--- a/ext/gtk/gtkgstbasewidget.h
+++ b/ext/gtk/gtkgstbasewidget.h
@@ -53,9 +53,9 @@ struct _GtkGstBaseWidget
   gint display_height;
 
   gboolean negotiated;
+  GstBuffer *pending_buffer;
   GstBuffer *buffer;
   GstVideoInfo v_info;
-  gboolean new_buffer;
 
   /* resize */
   gboolean pending_resize;
diff --git a/ext/gtk/gtkgstglwidget.c b/ext/gtk/gtkgstglwidget.c
index 998428107..dc24535aa 100644
--- a/ext/gtk/gtkgstglwidget.c
+++ b/ext/gtk/gtkgstglwidget.c
@@ -197,6 +197,13 @@ _redraw_texture (GtkGstGLWidget * gst_widget, guint tex)
   gl->BindTexture (GL_TEXTURE_2D, 0);
 }
 
+static inline void
+_draw_black (void)
+{
+  glClearColor (0.0, 0.0, 0.0, 0.0);
+  glClear (GL_COLOR_BUFFER_BIT);
+}
+
 static gboolean
 gtk_gst_gl_widget_render (GtkGLArea * widget, GdkGLContext * context)
 {
@@ -205,44 +212,56 @@ gtk_gst_gl_widget_render (GtkGLArea * widget, GdkGLContext * context)
 
   GTK_GST_BASE_WIDGET_LOCK (widget);
 
-  if (!priv->initted && priv->context)
-    gtk_gst_gl_widget_init_redisplay (GTK_GST_GL_WIDGET (widget));
+  if (!priv->context || !priv->other_context)
+    goto done;
 
-  if (priv->initted && base_widget->negotiated && base_widget->buffer) {
-    GST_DEBUG ("rendering buffer %p with gdk context %p",
-        base_widget->buffer, context);
+  gst_gl_context_activate (priv->other_context, TRUE);
 
-    gst_gl_context_activate (priv->other_context, TRUE);
+  if (!priv->initted)
+    gtk_gst_gl_widget_init_redisplay (GTK_GST_GL_WIDGET (widget));
 
-    if (base_widget->new_buffer || priv->current_tex == 0) {
-      GstVideoFrame gl_frame;
-      GstGLSyncMeta *sync_meta;
+  if (!priv->initted || !base_widget->negotiated) {
+    _draw_black ();
+    goto done;
+  }
 
-      if (!gst_video_frame_map (&gl_frame, &base_widget->v_info,
-              base_widget->buffer, GST_MAP_READ | GST_MAP_GL)) {
-        goto error;
-      }
+  /* Upload latest buffer */
+  if (base_widget->pending_buffer) {
+    GstBuffer *buffer = base_widget->pending_buffer;
+    GstVideoFrame gl_frame;
+    GstGLSyncMeta *sync_meta;
 
-      sync_meta = gst_buffer_get_gl_sync_meta (base_widget->buffer);
-      if (sync_meta) {
-        gst_gl_sync_meta_set_sync_point (sync_meta, priv->context);
-        gst_gl_sync_meta_wait (sync_meta, priv->other_context);
-      }
+    if (!gst_video_frame_map (&gl_frame, &base_widget->v_info, buffer,
+            GST_MAP_READ | GST_MAP_GL)) {
+      _draw_black ();
+      goto done;
+    }
 
-      priv->current_tex = *(guint *) gl_frame.data[0];
 
-      gst_video_frame_unmap (&gl_frame);
+    sync_meta = gst_buffer_get_gl_sync_meta (buffer);
+    if (sync_meta) {
+      gst_gl_sync_meta_set_sync_point (sync_meta, priv->context);
+      gst_gl_sync_meta_wait (sync_meta, priv->other_context);
     }
 
-    _redraw_texture (GTK_GST_GL_WIDGET (widget), priv->current_tex);
-    base_widget->new_buffer = FALSE;
-  } else {
-  error:
-    /* FIXME: nothing to display */
-    glClearColor (0.0, 0.0, 0.0, 0.0);
-    glClear (GL_COLOR_BUFFER_BIT);
+    priv->current_tex = *(guint *) gl_frame.data[0];
+
+    gst_video_frame_unmap (&gl_frame);
+
+    if (base_widget->buffer)
+      gst_buffer_unref (base_widget->buffer);
+
+    /* Keep the buffer to ensure current_tex stay valid */
+    base_widget->buffer = buffer;
+    base_widget->pending_buffer = NULL;
   }
 
+  GST_DEBUG ("rendering buffer %p with gdk context %p",
+      base_widget->buffer, context);
+
+  _redraw_texture (GTK_GST_GL_WIDGET (widget), priv->current_tex);
+
+done:
   if (priv->other_context)
     gst_gl_context_activate (priv->other_context, FALSE);
 
diff --git a/ext/gtk/gtkgstwidget.c b/ext/gtk/gtkgstwidget.c
index 58326d76f..5fe238a54 100644
--- a/ext/gtk/gtkgstwidget.c
+++ b/ext/gtk/gtkgstwidget.c
@@ -50,6 +50,15 @@ gtk_gst_widget_draw (GtkWidget * widget, cairo_t * cr)
 
   GTK_GST_BASE_WIDGET_LOCK (gst_widget);
 
+  /* There is not much to optimize in term of redisplay, so simply swap the
+   * pending_buffer with the active buffer */
+  if (gst_widget->pending_buffer) {
+    if (gst_widget->buffer)
+      gst_buffer_unref (gst_widget->buffer);
+    gst_widget->buffer = gst_widget->pending_buffer;
+    gst_widget->pending_buffer = NULL;
+  }
+
   /* failed to map the video frame */
   if (gst_widget->negotiated && gst_widget->buffer
       && gst_video_frame_map (&frame, &gst_widget->v_info,