From: Nicolas Dufresne Date: Fri, 5 Jun 2015 18:28:41 +0000 (-0400) Subject: ximagesink: Don't share internal pool X-Git-Tag: 1.19.3~511^2~3535 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=ae20ba7ad40f903af16b5b9e460ae331ca87302a;p=platform%2Fupstream%2Fgstreamer.git ximagesink: Don't share internal pool Sharing the internal pool results in situation where the pool may have two upstream owners. This create a race upon deactivation. Instead, always offer a new pool, and keep the internal pool internal in case we absolutely need it. https://bugzilla.gnome.org/show_bug.cgi?id=748344 --- diff --git a/sys/ximage/ximagesink.c b/sys/ximage/ximagesink.c index 126e3cb..c7cd4a6 100644 --- a/sys/ximage/ximagesink.c +++ b/sys/ximage/ximagesink.c @@ -1093,6 +1093,34 @@ gst_ximagesink_getcaps (GstBaseSink * bsink, GstCaps * filter) return caps; } +static GstBufferPool * +gst_ximagesink_create_pool (GstXImageSink * ximagesink, GstCaps * caps, + gsize size, gint min) +{ + static GstAllocationParams params = { 0, 15, 0, 0, }; + GstBufferPool *pool; + GstStructure *config; + + /* create a new pool for the new configuration */ + pool = gst_ximage_buffer_pool_new (ximagesink); + + config = gst_buffer_pool_get_config (pool); + gst_buffer_pool_config_set_params (config, caps, size, min, 0); + gst_buffer_pool_config_set_allocator (config, NULL, ¶ms); + + if (!gst_buffer_pool_set_config (pool, config)) + goto config_failed; + + return pool; + +config_failed: + { + GST_WARNING_OBJECT (ximagesink, "failed setting config"); + gst_object_unref (pool); + return NULL; + } +} + static gboolean gst_ximagesink_setcaps (GstBaseSink * bsink, GstCaps * caps) { @@ -1101,8 +1129,6 @@ gst_ximagesink_setcaps (GstBaseSink * bsink, GstCaps * caps) GstVideoInfo info; GstBufferPool *newpool, *oldpool; const GValue *par; - gint size; - static GstAllocationParams params = { 0, 15, 0, 0, }; ximagesink = GST_XIMAGESINK (bsink); @@ -1120,8 +1146,6 @@ gst_ximagesink_setcaps (GstBaseSink * bsink, GstCaps * caps) if (!gst_video_info_from_caps (&info, caps)) goto invalid_format; - size = info.size; - structure = gst_caps_get_structure (caps, 0); /* if the caps contain pixel-aspect-ratio, they have to match ours, * otherwise linking should fail */ @@ -1168,26 +1192,17 @@ gst_ximagesink_setcaps (GstBaseSink * bsink, GstCaps * caps) /* Remember to draw borders for next frame */ ximagesink->draw_border = TRUE; - /* create a new pool for the new configuration */ - newpool = gst_ximage_buffer_pool_new (ximagesink); - - structure = gst_buffer_pool_get_config (newpool); - gst_buffer_pool_config_set_params (structure, caps, size, 2, 0); - gst_buffer_pool_config_set_allocator (structure, NULL, ¶ms); - if (!gst_buffer_pool_set_config (newpool, structure)) - goto config_failed; + /* create a new internal pool for the new configuration */ + newpool = gst_ximagesink_create_pool (ximagesink, caps, info.size, 2); + /* we don't activate the internal pool yet as it may not be needed */ oldpool = ximagesink->pool; - /* we don't activate the pool yet, this will be done by downstream after it - * has configured the pool. If downstream does not want our pool we will - * activate it when we render into it */ ximagesink->pool = newpool; g_mutex_unlock (&ximagesink->flow_lock); - /* unref the old sink */ + /* deactivate and unref the old internal pool */ if (oldpool) { - /* we don't deactivate, some elements might still be using it, it will be - * deactivated when the last ref is gone */ + gst_buffer_pool_set_active (oldpool, FALSE); gst_object_unref (oldpool); } @@ -1215,12 +1230,6 @@ invalid_size: ("Invalid image size.")); return FALSE; } -config_failed: - { - GST_ERROR_OBJECT (ximagesink, "failed to set config."); - g_mutex_unlock (&ximagesink->flow_lock); - return FALSE; - } } static GstStateChangeReturn @@ -1342,8 +1351,8 @@ gst_ximagesink_show_frame (GstVideoSink * vsink, GstBuffer * buf) /* if we have one... */ GST_LOG_OBJECT (ximagesink, "buffer not from our pool, copying"); - /* we should have a pool, configured in setcaps */ - if (ximagesink->pool == NULL) + /* an internal pool should have been created in setcaps */ + if (G_UNLIKELY (ximagesink->pool == NULL)) goto no_pool; if (!gst_buffer_pool_set_active (ximagesink->pool, TRUE)) @@ -1451,8 +1460,7 @@ static gboolean gst_ximagesink_propose_allocation (GstBaseSink * bsink, GstQuery * query) { GstXImageSink *ximagesink = GST_XIMAGESINK (bsink); - GstBufferPool *pool; - GstStructure *config; + GstBufferPool *pool = NULL; GstCaps *caps; guint size; gboolean need_pool; @@ -1462,45 +1470,21 @@ gst_ximagesink_propose_allocation (GstBaseSink * bsink, GstQuery * query) if (caps == NULL) goto no_caps; - g_mutex_lock (&ximagesink->flow_lock); - if ((pool = ximagesink->pool)) - gst_object_ref (pool); - g_mutex_unlock (&ximagesink->flow_lock); - - if (pool != NULL) { - GstCaps *pcaps; - - /* we had a pool, check caps */ - config = gst_buffer_pool_get_config (pool); - gst_buffer_pool_config_get_params (config, &pcaps, &size, NULL, NULL); - - GST_DEBUG_OBJECT (ximagesink, - "we had a pool with caps %" GST_PTR_FORMAT, pcaps); - if (!gst_caps_is_equal (caps, pcaps)) { - /* different caps, we can't use this pool */ - GST_DEBUG_OBJECT (ximagesink, "pool has different caps"); - gst_object_unref (pool); - pool = NULL; - } - gst_structure_free (config); - } - if (pool == NULL && need_pool) { + if (need_pool) { GstVideoInfo info; if (!gst_video_info_from_caps (&info, caps)) goto invalid_caps; - GST_DEBUG_OBJECT (ximagesink, "create new pool"); - pool = gst_ximage_buffer_pool_new (ximagesink); + pool = gst_ximagesink_create_pool (ximagesink, caps, info.size, 0); /* the normal size of a frame */ size = info.size; - config = gst_buffer_pool_get_config (pool); - gst_buffer_pool_config_set_params (config, caps, size, 0, 0); - if (!gst_buffer_pool_set_config (pool, config)) - goto config_failed; + if (pool == NULL) + goto no_pool; } + if (pool) { /* we need at least 2 buffer because we hold on to the last one */ gst_query_add_allocation_pool (query, pool, size, 2, 0); @@ -1524,10 +1508,9 @@ invalid_caps: GST_DEBUG_OBJECT (bsink, "invalid caps specified"); return FALSE; } -config_failed: +no_pool: { - GST_DEBUG_OBJECT (bsink, "failed setting config"); - gst_object_unref (pool); + /* Already warned in create_pool */ return FALSE; } }