From f1ff80ced0d14639beb3835970b62770e1eb74d1 Mon Sep 17 00:00:00 2001 From: Havard Graff Date: Thu, 19 Mar 2020 23:37:26 +0100 Subject: [PATCH] rtpjitterbuffer: remove the concept of "already-lost" This is a concept that only applies when a buffer arrives in the chain function, and it has already been scheduled as part of a "multi"-lost timer. However, "multi"-lost timers are now a thing of the past, making this whole concept superflous, and this buffer is now simply counted as "late", having already been pushed out (albeit as a lost-event). --- gst/rtpmanager/gstrtpjitterbuffer.c | 53 ------------------- tests/check/elements/rtpjitterbuffer.c | 73 -------------------------- 2 files changed, 126 deletions(-) diff --git a/gst/rtpmanager/gstrtpjitterbuffer.c b/gst/rtpmanager/gstrtpjitterbuffer.c index 8c2a0940bc..181395baa6 100644 --- a/gst/rtpmanager/gstrtpjitterbuffer.c +++ b/gst/rtpmanager/gstrtpjitterbuffer.c @@ -399,13 +399,11 @@ struct _GstRtpJitterBufferPrivate GstClockTime last_drop_msg_timestamp; /* accumulators; reset every time a drop message is posted */ guint num_too_late; - guint num_already_lost; guint num_drop_on_latency; }; typedef enum { REASON_TOO_LATE, - REASON_ALREADY_LOST, REASON_DROP_ON_LATENCY } DropMessageReason; @@ -597,8 +595,6 @@ gst_rtp_jitter_buffer_class_init (GstRtpJitterBufferClass * klass) * * #gstring `reason`: Reason for dropping the packet. * * #guint `num-too-late`: Number of packets arriving too late since * last drop message. - * * #guint `num-already-lost`: Number of packets already considered lost - * since drop message. * * #guint `num-drop-on-latency`: Number of packets dropped due to the * drop-on-latency property since last drop message. * @@ -1022,7 +1018,6 @@ gst_rtp_jitter_buffer_init (GstRtpJitterBuffer * jitterbuffer) priv->avg_jitter = 0; priv->last_drop_msg_timestamp = GST_CLOCK_TIME_NONE; priv->num_too_late = 0; - priv->num_already_lost = 0; priv->num_drop_on_latency = 0; priv->segment_seqnum = GST_SEQNUM_INVALID; priv->timers = rtp_timer_queue_new (); @@ -1619,7 +1614,6 @@ gst_rtp_jitter_buffer_flush_stop (GstRtpJitterBuffer * jitterbuffer) priv->segment_seqnum = GST_SEQNUM_INVALID; priv->last_drop_msg_timestamp = GST_CLOCK_TIME_NONE; priv->num_too_late = 0; - priv->num_already_lost = 0; priv->num_drop_on_latency = 0; GST_DEBUG_OBJECT (jitterbuffer, "flush and reset jitterbuffer"); rtp_jitter_buffer_flush (priv->jbuf, NULL, NULL); @@ -2047,9 +2041,6 @@ new_drop_message (GstRtpJitterBuffer * jitterbuffer, guint seqnum, if (reason == REASON_TOO_LATE) { priv->num_too_late++; reason_str = "too-late"; - } else if (reason == REASON_ALREADY_LOST) { - priv->num_already_lost++; - reason_str = "already-lost"; } else if (reason == REASON_DROP_ON_LATENCY) { priv->num_drop_on_latency++; reason_str = "drop-on-latency"; @@ -2068,12 +2059,10 @@ new_drop_message (GstRtpJitterBuffer * jitterbuffer, guint seqnum, "timestamp", GST_TYPE_CLOCK_TIME, timestamp, "reason", G_TYPE_STRING, reason_str, "num-too-late", G_TYPE_UINT, priv->num_too_late, - "num-already-lost", G_TYPE_UINT, priv->num_already_lost, "num-drop-on-latency", G_TYPE_UINT, priv->num_drop_on_latency, NULL); priv->last_drop_msg_timestamp = current_time; priv->num_too_late = 0; - priv->num_already_lost = 0; priv->num_drop_on_latency = 0; drop_msg = gst_message_new_element (GST_OBJECT (jitterbuffer), s); } @@ -2239,33 +2228,6 @@ get_rtx_delay (GstRtpJitterBufferPrivate * priv) return delay; } -/* Check if packet with seqnum is already considered definitely lost by being - * part of a "lost timer" for multiple packets */ -static gboolean -already_lost (GstRtpJitterBuffer * jitterbuffer, GstClockTime pts, - guint16 seqnum) -{ - GstRtpJitterBufferPrivate *priv = jitterbuffer->priv; - RtpTimer *test; - - test = rtp_timer_queue_peek_earliest (priv->timers); - while (test && get_pts_timeout (test) <= pts) { - gint gap = gst_rtp_buffer_compare_seqnum (test->seqnum, seqnum); - - if (test->num > 1 && test->type == RTP_TIMER_LOST && gap >= 0 && - gap < test->num) { - GST_DEBUG_OBJECT (jitterbuffer, - "seqnum #%d already considered definitely lost (#%d->#%d)", - seqnum, test->seqnum, (test->seqnum + test->num - 1) & 0xffff); - return TRUE; - } - - test = rtp_timer_get_next (test); - } - - return FALSE; -} - /* we just received a packet with seqnum and dts. * * First check for old seqnum that we are still expecting. If the gap with the @@ -3188,9 +3150,6 @@ gst_rtp_jitter_buffer_chain (GstPad * pad, GstObject * parent, } } - if (already_lost (jitterbuffer, pts, seqnum)) - goto already_lost; - /* let's drop oldest packet if the queue is already full and drop-on-latency * is set. We can only do this when there actually is a latency. When no * latency is set, we just pump it in the queue and let the other end push it @@ -3320,18 +3279,6 @@ too_late: gst_buffer_unref (buffer); goto finished; } -already_lost: - { - GST_DEBUG_OBJECT (jitterbuffer, "Packet #%d too late as it was already " - "considered lost", seqnum); - priv->num_late++; - if (priv->post_drop_messages) { - drop_msg = - new_drop_message (jitterbuffer, seqnum, pts, REASON_ALREADY_LOST); - } - gst_buffer_unref (buffer); - goto finished; - } duplicate: { GST_DEBUG_OBJECT (jitterbuffer, "Duplicate packet #%d detected, dropping", diff --git a/tests/check/elements/rtpjitterbuffer.c b/tests/check/elements/rtpjitterbuffer.c index fd4d271354..b333b1128f 100644 --- a/tests/check/elements/rtpjitterbuffer.c +++ b/tests/check/elements/rtpjitterbuffer.c @@ -2769,19 +2769,15 @@ check_drop_message (GstMessage * drop_msg, const char *reason_check, GstClockTime timestamp; guint seqnum; guint num_too_late; - guint num_already_lost; guint num_drop_on_latency; guint num_too_late_check = 0; - guint num_already_lost_check = 0; guint num_drop_on_latency_check = 0; /* Check that fields exist */ fail_unless (gst_structure_get_uint (s, "seqnum", &seqnum)); fail_unless (gst_structure_get_uint64 (s, "timestamp", ×tamp)); fail_unless (gst_structure_get_uint (s, "num-too-late", &num_too_late)); - fail_unless (gst_structure_get_uint (s, "num-already-lost", - &num_already_lost)); fail_unless (gst_structure_get_uint (s, "num-drop-on-latency", &num_drop_on_latency)); fail_unless (reason_str = gst_structure_get_string (s, "reason")); @@ -2789,8 +2785,6 @@ check_drop_message (GstMessage * drop_msg, const char *reason_check, /* Assing what to compare message fields to based on message reason */ if (g_strcmp0 (reason_check, "too-late") == 0) { num_too_late_check += num_msg; - } else if (g_strcmp0 (reason_check, "already-lost") == 0) { - num_already_lost_check += num_msg; } else if (g_strcmp0 (reason_check, "drop-on-latency") == 0) { num_drop_on_latency_check += num_msg; } else { @@ -2801,7 +2795,6 @@ check_drop_message (GstMessage * drop_msg, const char *reason_check, fail_unless (seqnum == seqnum_check); fail_unless (g_strcmp0 (reason_str, reason_check) == 0); fail_unless (num_too_late == num_too_late_check); - fail_unless (num_already_lost == num_already_lost_check); fail_unless (num_drop_on_latency == num_drop_on_latency_check); return TRUE; @@ -2855,71 +2848,6 @@ GST_START_TEST (test_drop_messages_too_late) GST_END_TEST; -GST_START_TEST (test_drop_messages_already_lost) -{ - GstHarness *h = gst_harness_new ("rtpjitterbuffer"); - GstTestClock *testclock; - GstClockID id; - gint latency_ms = 20; - guint seqnum_late; - guint seqnum_final; - GstBus *bus; - GstMessage *drop_msg; - gboolean have_message = FALSE; - - testclock = gst_harness_get_testclock (h); - g_object_set (h->element, "post-drop-messages", TRUE, NULL); - - /* Create a bus to get the drop message on */ - bus = gst_bus_new (); - gst_element_set_bus (h->element, bus); - - /* Get seqnum from initial state */ - seqnum_late = construct_deterministic_initial_state (h, latency_ms); - - /* Hop over 3 buffers and push buffer (gap of 3) */ - seqnum_final = seqnum_late + 4; - fail_unless_equals_int (GST_FLOW_OK, - gst_harness_push (h, - generate_test_buffer_full (seqnum_final * TEST_BUF_DURATION, - seqnum_final, seqnum_final * TEST_RTP_TS_DURATION))); - - /* The jitterbuffer should be waiting for the timeout of a "large gap timer" - * for buffer seqnum_late and seqnum_late+1 */ - gst_test_clock_wait_for_next_pending_id (testclock, &id); - fail_unless_equals_uint64 (seqnum_late * TEST_BUF_DURATION + - latency_ms * GST_MSECOND, gst_clock_id_get_time (id)); - - /* Now seqnum_late sneaks in before the lost event for buffer seqnum_late and seqnum_late+1 is - * processed. It will be dropped due to already having been considered lost */ - fail_unless_equals_int (GST_FLOW_OK, - gst_harness_push (h, - generate_test_buffer_full (seqnum_late * TEST_BUF_DURATION, - seqnum_late, seqnum_late * TEST_RTP_TS_DURATION))); - - /* Pop the resulting drop message and check its correctness */ - while (!have_message && - (drop_msg = gst_bus_pop_filtered (bus, GST_MESSAGE_ELEMENT)) != NULL) { - if (gst_message_has_name (drop_msg, "drop-msg")) { - fail_unless (check_drop_message (drop_msg, "already-lost", seqnum_late, - 1)); - have_message = TRUE; - } - gst_message_unref (drop_msg); - } - fail_unless (have_message); - - /* Cleanup */ - gst_clock_id_unref (id); - gst_element_set_bus (h->element, NULL); - gst_buffer_unref (gst_harness_take_all_data_as_buffer (h)); - gst_object_unref (bus); - gst_object_unref (testclock); - gst_harness_teardown (h); -} - -GST_END_TEST; - GST_START_TEST (test_drop_messages_drop_on_latency) { GstHarness *h = gst_harness_new ("rtpjitterbuffer"); @@ -3226,7 +3154,6 @@ rtpjitterbuffer_suite (void) tcase_add_test (tc_chain, test_performance); tcase_add_test (tc_chain, test_drop_messages_too_late); - tcase_skip_broken_test (tc_chain, test_drop_messages_already_lost); tcase_add_test (tc_chain, test_drop_messages_drop_on_latency); tcase_add_test (tc_chain, test_drop_messages_interval);