From cf44f5013a0b6bc6fb6dfbf4bf580d1907a62ace Mon Sep 17 00:00:00 2001 From: Nicolas Dufresne Date: Sat, 15 Aug 2015 15:08:11 +0200 Subject: [PATCH] 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. --- ext/gtk/gtkgstbasewidget.c | 6 +-- ext/gtk/gtkgstbasewidget.h | 2 +- ext/gtk/gtkgstglwidget.c | 87 +++++++++++++++++++++++--------------- ext/gtk/gtkgstwidget.c | 9 ++++ 4 files changed, 65 insertions(+), 39 deletions(-) diff --git a/ext/gtk/gtkgstbasewidget.c b/ext/gtk/gtkgstbasewidget.c index 4202a22ff3..5c7cda31cb 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 0a05ca54d5..13737c6322 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 998428107a..dc24535aa0 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) + if (!priv->context || !priv->other_context) + goto done; + + gst_gl_context_activate (priv->other_context, TRUE); + + if (!priv->initted) gtk_gst_gl_widget_init_redisplay (GTK_GST_GL_WIDGET (widget)); - 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); - - if (base_widget->new_buffer || priv->current_tex == 0) { - GstVideoFrame gl_frame; - GstGLSyncMeta *sync_meta; - - if (!gst_video_frame_map (&gl_frame, &base_widget->v_info, - base_widget->buffer, GST_MAP_READ | GST_MAP_GL)) { - goto error; - } - - 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); - } - - priv->current_tex = *(guint *) gl_frame.data[0]; - - gst_video_frame_unmap (&gl_frame); - } - - _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); + if (!priv->initted || !base_widget->negotiated) { + _draw_black (); + goto done; } + /* Upload latest buffer */ + if (base_widget->pending_buffer) { + GstBuffer *buffer = base_widget->pending_buffer; + GstVideoFrame gl_frame; + GstGLSyncMeta *sync_meta; + + if (!gst_video_frame_map (&gl_frame, &base_widget->v_info, buffer, + GST_MAP_READ | GST_MAP_GL)) { + _draw_black (); + goto done; + } + + + 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); + } + + 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 58326d76f9..5fe238a545 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,