From 6d4ad80b82fb24e611768c1915a77dddde687274 Mon Sep 17 00:00:00 2001 From: Carlos Rafael Giani Date: Fri, 4 Oct 2019 16:57:29 +0200 Subject: [PATCH] queue2: Fix missing/dropped buffering messages at startup This fixes a bug that occurs when an attempt is made to post a buffering message before the queue2 was assigned a bus. One common situation where this happens is when the use-buffering property is set to TRUE before the queue2 was added to a bin. If the result of gst_element_post_message() is not checked, and the aforementioned situation occurs, then last_posted_buffering_percent and percent_changed will still be updated, as if posting the message succeeded. Later attempts to post again will not do anything because the code then assumes that a message with the same percentage was previously posted successfully and posting again is redundant. Updating these variables only if posting succeed and explicitely posting a buffering message in the READY->PAUSED state change ensure that a buffering message is posted as early as possible. Part-of: --- plugins/elements/gstqueue2.c | 50 +++++++++++++++++++++++++++++---- tests/check/elements/queue2.c | 53 +++++++++++++++++++++++++++++++++++ 2 files changed, 97 insertions(+), 6 deletions(-) diff --git a/plugins/elements/gstqueue2.c b/plugins/elements/gstqueue2.c index 9aaf49b05b..9d9fb12c23 100644 --- a/plugins/elements/gstqueue2.c +++ b/plugins/elements/gstqueue2.c @@ -49,6 +49,12 @@ * * The temp-location property will be used to notify the application of the * allocated filename. + * + * If the #GstQueue2:use-buffering property is set to TRUE, and any writable + * property is modified, #GstQueue2 will attempt to post a buffering message + * if the changes to the properties also cause the buffering percentage to be + * changed (for example, because the queue's capacity was changed and it already + * contains some data). */ #ifdef HAVE_CONFIG_H @@ -1132,10 +1138,7 @@ gst_queue2_get_buffering_message (GstQueue2 * queue) gst_message_set_buffering_stats (msg, queue->mode, queue->avg_in, queue->avg_out, queue->buffering_left); - - queue->last_posted_buffering_percent = percent; } - queue->percent_changed = FALSE; } return msg; @@ -1151,8 +1154,20 @@ gst_queue2_post_buffering (GstQueue2 * queue) msg = gst_queue2_get_buffering_message (queue); GST_QUEUE2_MUTEX_UNLOCK (queue); - if (msg != NULL) - gst_element_post_message (GST_ELEMENT_CAST (queue), msg); + if (msg != NULL) { + if (gst_element_post_message (GST_ELEMENT_CAST (queue), msg)) { + GST_QUEUE2_MUTEX_LOCK (queue); + /* Set these states only if posting the message succeeded. Otherwise, + * this post attempt failed, and the next one won't be done, because + * gst_queue2_get_buffering_message() checks these states and decides + * based on their values that it won't produce a message. */ + queue->last_posted_buffering_percent = queue->percent_changed; + queue->percent_changed = FALSE; + GST_QUEUE2_MUTEX_UNLOCK (queue); + GST_DEBUG_OBJECT (queue, "successfully posted buffering message"); + } else + GST_DEBUG_OBJECT (queue, "could not post buffering message"); + } g_mutex_unlock (&queue->buffering_post_lock); } @@ -2171,11 +2186,26 @@ gst_queue2_create_write (GstQueue2 * queue, GstBuffer * buffer) update_buffering (queue); msg = gst_queue2_get_buffering_message (queue); if (msg) { + gboolean post_ok; + GST_QUEUE2_MUTEX_UNLOCK (queue); + g_mutex_lock (&queue->buffering_post_lock); - gst_element_post_message (GST_ELEMENT_CAST (queue), msg); + post_ok = gst_element_post_message (GST_ELEMENT_CAST (queue), msg); g_mutex_unlock (&queue->buffering_post_lock); + GST_QUEUE2_MUTEX_LOCK (queue); + + if (post_ok) { + /* Set these states only if posting the message succeeded. Otherwise, + * this post attempt failed, and the next one won't be done, because + * gst_queue2_get_buffering_message() checks these states and decides + * based on their values that it won't produce a message. */ + queue->last_posted_buffering_percent = queue->percent_changed; + queue->percent_changed = FALSE; + GST_DEBUG_OBJECT (queue, "successfully posted buffering message"); + } else + GST_DEBUG_OBJECT (queue, "could not post buffering message"); } } @@ -3750,6 +3780,14 @@ gst_queue2_change_state (GstElement * element, GstStateChange transition) gst_event_replace (&queue->stream_start_event, NULL); GST_QUEUE2_MUTEX_UNLOCK (queue); query_downstream_bitrate (queue); + + /* Post a buffering message now to make sure the application receives + * a buffering message as early as possible. This prevents situations + * where the pipeline's state is set to PLAYING too early, before + * buffering actually finished. */ + update_buffering (queue); + gst_queue2_post_buffering (queue); + break; case GST_STATE_CHANGE_PAUSED_TO_PLAYING: break; diff --git a/tests/check/elements/queue2.c b/tests/check/elements/queue2.c index 5941c5d8dd..a9c56827bd 100644 --- a/tests/check/elements/queue2.c +++ b/tests/check/elements/queue2.c @@ -661,6 +661,58 @@ GST_START_TEST (test_bitrate_query) GST_END_TEST; +GST_START_TEST (test_ready_paused_buffering_message) +{ + /* This test verifies that a buffering message is posted during the + * READY->PAUSED state change. */ + + GstElement *pipe; + GstElement *fakesrc, *queue2, *fakesink; + + /* Set up simple test pipeline. */ + + pipe = gst_pipeline_new ("pipeline"); + + /* Set up the fakesrc to actually produce data. */ + fakesrc = gst_element_factory_make ("fakesrc", NULL); + fail_unless (fakesrc != NULL); + g_object_set (G_OBJECT (fakesrc), "format", (gint) 3, "filltype", (gint) 2, + "sizetype", (gint) 2, "sizemax", (gint) 4096, "datarate", (gint) 4096, + NULL); + + queue2 = gst_element_factory_make ("queue2", NULL); + fail_unless (queue2 != NULL); + /* Note that use-buffering is set _before_ the queue2 got added to pipe. + * This is intentional. queue2's set_property function attempts to post a + * buffering message. This fails silently, because without having been added + * to a bin, queue2 won't have been assigned a bus, so it cannot post that + * message anywhere. In such a case, the next attempt to post a buffering + * message must always actually be attempted. (Normally, queue2 performs + * internal checks to see whether or not the buffering message would be + * redundant because a prior message with the same percentage was already + * posted. But these checked only make sense if the previous posting attempt + * succeeded.) */ + g_object_set (queue2, "use-buffering", (gboolean) TRUE, NULL); + + fakesink = gst_element_factory_make ("fakesink", NULL); + fail_unless (fakesink != NULL); + + gst_bin_add_many (GST_BIN (pipe), fakesrc, queue2, fakesink, NULL); + + /* Set the pipeline to PAUSED. This should cause queue2 to attempt to post + * a buffering message during its READY->PAUSED state change. And this should + * succeed, since queue2 has been added to pipe by now. */ + gst_element_set_state (pipe, GST_STATE_PAUSED); + + /* Look for the expected 0% buffering message. */ + CHECK_FOR_BUFFERING_MSG (pipe, 0); + + gst_element_set_state (pipe, GST_STATE_NULL); + gst_object_unref (pipe); +} + +GST_END_TEST; + static Suite * queue2_suite (void) { @@ -678,6 +730,7 @@ queue2_suite (void) tcase_add_test (tc_chain, test_percent_overflow); tcase_add_test (tc_chain, test_small_ring_buffer); tcase_add_test (tc_chain, test_bitrate_query); + tcase_add_test (tc_chain, test_ready_paused_buffering_message); return s; }