From 8d955fc32b552b2db933c67f3cfa31d987f36b81 Mon Sep 17 00:00:00 2001 From: Thomas Bluemel Date: Thu, 13 Jun 2019 15:45:28 -0600 Subject: [PATCH] rtpjitterbuffer: Only calculate skew or reset if no gap. In the case of reordered packets, calculating skew would cause pts values to be off. Only calculate skew when packets come in as expected. Also, late RTX packets should not trigger clock skew adjustments. Fixes #612 --- gst/rtpmanager/gstrtpjitterbuffer.c | 31 +++++++--- gst/rtpmanager/rtpjitterbuffer.c | 60 +++++++++++++----- gst/rtpmanager/rtpjitterbuffer.h | 3 +- tests/check/elements/rtpbin.c | 2 + tests/check/elements/rtpjitterbuffer.c | 85 ++++++++++++++++++++++++++ 5 files changed, 157 insertions(+), 24 deletions(-) diff --git a/gst/rtpmanager/gstrtpjitterbuffer.c b/gst/rtpmanager/gstrtpjitterbuffer.c index 35326ddb2e9..3baad7d58c1 100644 --- a/gst/rtpmanager/gstrtpjitterbuffer.c +++ b/gst/rtpmanager/gstrtpjitterbuffer.c @@ -3028,7 +3028,13 @@ gst_rtp_jitter_buffer_chain (GstPad * pad, GstObject * parent, /* calculate a pts based on rtptime and arrival time (dts) */ pts = rtp_jitter_buffer_calculate_pts (priv->jbuf, dts, estimated_dts, - rtptime, gst_element_get_base_time (GST_ELEMENT_CAST (jitterbuffer))); + rtptime, gst_element_get_base_time (GST_ELEMENT_CAST (jitterbuffer)), + 0, GST_BUFFER_IS_RETRANSMISSION (buffer)); + + if (G_UNLIKELY (!GST_CLOCK_TIME_IS_VALID (pts))) { + /* A valid timestamp cannot be calculated, discard packet */ + goto discard_invalid; + } /* we don't know what the next_in_seqnum should be, wait for the last * possible moment to push this buffer, maybe we get an earlier seqnum @@ -3080,13 +3086,16 @@ gst_rtp_jitter_buffer_chain (GstPad * pad, GstObject * parent, /* calculate a pts based on rtptime and arrival time (dts) */ /* If we estimated the DTS, don't consider it in the clock skew calculations */ - if (gap >= 0) { - pts = - rtp_jitter_buffer_calculate_pts (priv->jbuf, dts, estimated_dts, - rtptime, gst_element_get_base_time (GST_ELEMENT_CAST (jitterbuffer))); + pts = + rtp_jitter_buffer_calculate_pts (priv->jbuf, dts, estimated_dts, + rtptime, gst_element_get_base_time (GST_ELEMENT_CAST (jitterbuffer)), + gap, GST_BUFFER_IS_RETRANSMISSION (buffer)); + + if (G_UNLIKELY (!GST_CLOCK_TIME_IS_VALID (pts))) { + /* A valid timestamp cannot be calculated, discard packet */ + goto discard_invalid; } - /* else gap < 0 then we will drop the buffer anyway, so we don't need to - calculate it's pts */ + if (G_LIKELY (gap == 0)) { /* packet is expected */ calculate_packet_spacing (jitterbuffer, rtptime, pts); @@ -3308,6 +3317,14 @@ rtx_duplicate: gst_buffer_unref (buffer); goto finished; } +discard_invalid: + { + GST_DEBUG_OBJECT (jitterbuffer, + "cannot calculate a valid pts for #%d (rtx: %d), discard", + seqnum, GST_BUFFER_IS_RETRANSMISSION (buffer)); + gst_buffer_unref (buffer); + goto finished; + } } /* FIXME: hopefully we can do something more efficient here, especially when diff --git a/gst/rtpmanager/rtpjitterbuffer.c b/gst/rtpmanager/rtpjitterbuffer.c index 64d89a87ca5..98314613190 100644 --- a/gst/rtpmanager/rtpjitterbuffer.c +++ b/gst/rtpmanager/rtpjitterbuffer.c @@ -530,7 +530,7 @@ update_buffer_level (RTPJitterBuffer * jbuf, gint * percent) */ static GstClockTime calculate_skew (RTPJitterBuffer * jbuf, guint64 ext_rtptime, - GstClockTime gstrtptime, GstClockTime time) + GstClockTime gstrtptime, GstClockTime time, gint gap) { guint64 send_diff, recv_diff; gint64 delta; @@ -574,8 +574,14 @@ calculate_skew (RTPJitterBuffer * jbuf, guint64 ext_rtptime, rtp_jitter_buffer_resync (jbuf, time, gstrtptime, ext_rtptime, TRUE); send_diff = 0; delta = 0; + gap = 0; } + /* only do skew calculations if we didn't have a gap. if too much time + * has elapsed despite there being a gap, we resynced already. */ + if (G_UNLIKELY (gap != 0)) + goto no_skew; + pos = jbuf->window_pos; if (G_UNLIKELY (jbuf->window_filling)) { @@ -693,7 +699,8 @@ queue_do_insert (RTPJitterBuffer * jbuf, GList * list, GList * item) GstClockTime rtp_jitter_buffer_calculate_pts (RTPJitterBuffer * jbuf, GstClockTime dts, - gboolean estimated_dts, guint32 rtptime, GstClockTime base_time) + gboolean estimated_dts, guint32 rtptime, GstClockTime base_time, + gint gap, gboolean is_rtx) { guint64 ext_rtptime; GstClockTime gstrtptime, pts; @@ -714,10 +721,18 @@ rtp_jitter_buffer_calculate_pts (RTPJitterBuffer * jbuf, GstClockTime dts, ext_rtptime = gst_rtp_buffer_ext_timestamp (&ext_rtptime, rtptime); if (ext_rtptime > jbuf->last_rtptime + 3 * jbuf->clock_rate || ext_rtptime + 3 * jbuf->clock_rate < jbuf->last_rtptime) { - /* reset even if we don't have valid incoming time; - * still better than producing possibly very bogus output timestamp */ - GST_WARNING ("rtp delta too big, reset skew"); - rtp_jitter_buffer_reset_skew (jbuf); + if (!is_rtx) { + /* reset even if we don't have valid incoming time; + * still better than producing possibly very bogus output timestamp */ + GST_WARNING ("rtp delta too big, reset skew"); + rtp_jitter_buffer_reset_skew (jbuf); + } else { + GST_WARNING ("rtp delta too big: ignore rtx packet"); + media_clock = NULL; + pipeline_clock = NULL; + pts = GST_CLOCK_TIME_NONE; + goto done; + } } } @@ -743,11 +758,17 @@ rtp_jitter_buffer_calculate_pts (RTPJitterBuffer * jbuf, GstClockTime dts, if (G_LIKELY (jbuf->base_rtptime != -1)) { /* check elapsed time in RTP units */ if (gstrtptime < jbuf->base_rtptime) { - /* elapsed time at sender, timestamps can go backwards and thus be - * smaller than our base time, schedule to take a new base time in - * that case. */ - GST_WARNING ("backward timestamps at server, schedule resync"); - jbuf->need_resync = TRUE; + if (!is_rtx) { + /* elapsed time at sender, timestamps can go backwards and thus be + * smaller than our base time, schedule to take a new base time in + * that case. */ + GST_WARNING ("backward timestamps at server, schedule resync"); + jbuf->need_resync = TRUE; + } else { + GST_WARNING ("backward timestamps: ignore rtx packet"); + pts = GST_CLOCK_TIME_NONE; + goto done; + } } } @@ -777,6 +798,11 @@ rtp_jitter_buffer_calculate_pts (RTPJitterBuffer * jbuf, GstClockTime dts, /* need resync, lock on to time and gstrtptime if we can, otherwise we * do with the previous values */ if (G_UNLIKELY (jbuf->need_resync && dts != -1)) { + if (is_rtx) { + GST_DEBUG ("not resyncing on rtx packet, discard"); + pts = GST_CLOCK_TIME_NONE; + goto done; + } GST_INFO ("resync to time %" GST_TIME_FORMAT ", rtptime %" GST_TIME_FORMAT, GST_TIME_ARGS (dts), GST_TIME_ARGS (gstrtptime)); rtp_jitter_buffer_resync (jbuf, dts, gstrtptime, ext_rtptime, FALSE); @@ -899,7 +925,7 @@ rtp_jitter_buffer_calculate_pts (RTPJitterBuffer * jbuf, GstClockTime dts, /* do skew calculation by measuring the difference between rtptime and the * receive dts, this function will return the skew corrected rtptime. */ - pts = calculate_skew (jbuf, ext_rtptime, gstrtptime, dts); + pts = calculate_skew (jbuf, ext_rtptime, gstrtptime, dts, gap); } /* check if timestamps are not going backwards, we can only check this if we @@ -921,20 +947,22 @@ rtp_jitter_buffer_calculate_pts (RTPJitterBuffer * jbuf, GstClockTime dts, } } - if (dts != -1 && pts + jbuf->delay < dts) { + if (gap == 0 && dts != -1 && pts + jbuf->delay < dts) { /* if we are going to produce a timestamp that is later than the input * timestamp, we need to reset the jitterbuffer. Likely the server paused * temporarily */ GST_DEBUG ("out %" GST_TIME_FORMAT " + %" G_GUINT64_FORMAT " < time %" - GST_TIME_FORMAT ", reset jitterbuffer", GST_TIME_ARGS (pts), + GST_TIME_FORMAT ", reset jitterbuffer and discard", GST_TIME_ARGS (pts), jbuf->delay, GST_TIME_ARGS (dts)); - rtp_jitter_buffer_resync (jbuf, dts, gstrtptime, ext_rtptime, TRUE); - pts = dts; + rtp_jitter_buffer_reset_skew (jbuf); + pts = GST_CLOCK_TIME_NONE; + goto done; } jbuf->prev_out_time = pts; jbuf->prev_send_diff = gstrtptime - jbuf->base_rtptime; +done: if (media_clock) gst_object_unref (media_clock); if (pipeline_clock) diff --git a/gst/rtpmanager/rtpjitterbuffer.h b/gst/rtpmanager/rtpjitterbuffer.h index b65b6038e96..37ccd8711ea 100644 --- a/gst/rtpmanager/rtpjitterbuffer.h +++ b/gst/rtpmanager/rtpjitterbuffer.h @@ -191,7 +191,8 @@ void rtp_jitter_buffer_get_sync (RTPJitterBuffer *jbuf, guint64 *last_rtptime); GstClockTime rtp_jitter_buffer_calculate_pts (RTPJitterBuffer * jbuf, GstClockTime dts, gboolean estimated_dts, - guint32 rtptime, GstClockTime base_time); + guint32 rtptime, GstClockTime base_time, gint gap, + gboolean is_rtx); gboolean rtp_jitter_buffer_can_fast_start (RTPJitterBuffer * jbuf, gint num_packet); diff --git a/tests/check/elements/rtpbin.c b/tests/check/elements/rtpbin.c index 981cd64875e..747929fcee9 100644 --- a/tests/check/elements/rtpbin.c +++ b/tests/check/elements/rtpbin.c @@ -176,6 +176,8 @@ chain_rtp_packet (GstPad * pad, CleanupData * data) data->seqnum++; gst_buffer_unmap (buffer, &map); + GST_BUFFER_DTS (buffer) = 0; + res = gst_pad_chain (pad, buffer); return res; diff --git a/tests/check/elements/rtpjitterbuffer.c b/tests/check/elements/rtpjitterbuffer.c index 0d3c912d72d..078c5d6d76a 100644 --- a/tests/check/elements/rtpjitterbuffer.c +++ b/tests/check/elements/rtpjitterbuffer.c @@ -2289,6 +2289,90 @@ GST_START_TEST (test_rtx_large_packet_spacing_does_not_reset_jitterbuffer) GST_END_TEST; +GST_START_TEST (test_minor_reorder_does_not_skew) +{ + gint latency_ms = 20; + gint frame_dur_ms = 50; + guint rtx_min_delay_ms = 110; + gint hickup_ms = 2; + gint i, seq; + GstBuffer *buffer; + GstClockTime now; + GstClockTime frame_dur = frame_dur_ms * GST_MSECOND; + GstHarness *h = gst_harness_new ("rtpjitterbuffer"); + + gst_harness_set_src_caps (h, generate_caps ()); + g_object_set (h->element, + "do-lost", TRUE, "latency", latency_ms, "do-retransmission", TRUE, + "rtx-min-delay", rtx_min_delay_ms, NULL); + + /* Pushing 2 frames @frame_dur_ms ms apart from each other to initialize + * packet_spacing and avg jitter */ + for (seq = 0, now = 0; seq < 2; ++seq, now += frame_dur) { + gst_harness_set_time (h, now); + gst_harness_push (h, generate_test_buffer_full (now, seq, + AS_TEST_BUF_RTP_TIME (now))); + if (seq == 0) + gst_harness_crank_single_clock_wait (h); + buffer = gst_harness_pull (h); + fail_unless_equals_int64 (now, GST_BUFFER_PTS (buffer)); + gst_buffer_unref (buffer); + } + + /* drop GstEventStreamStart & GstEventCaps & GstEventSegment */ + for (i = 0; i < 3; i++) + gst_event_unref (gst_harness_pull_event (h)); + /* drop reconfigure event */ + gst_event_unref (gst_harness_pull_upstream_event (h)); + + /* Pushing packet #4 before #3, shortly after #3 would have arrived normally */ + gst_harness_set_time (h, now + hickup_ms * GST_MSECOND); + gst_harness_push (h, generate_test_buffer_full (now + hickup_ms * GST_MSECOND, + seq + 1, AS_TEST_BUF_RTP_TIME (now + frame_dur))); + + /* Pushing packet #3 after #4 when #4 would have normally arrived */ + gst_harness_set_time (h, now + frame_dur); + gst_harness_push (h, generate_test_buffer_full (now + frame_dur, seq, + AS_TEST_BUF_RTP_TIME (now))); + + /* Pulling should be retrieving #3 first */ + buffer = gst_harness_pull (h); + fail_unless_equals_int64 (now, GST_BUFFER_PTS (buffer)); + gst_buffer_unref (buffer); + + /* Pulling should be retrieving #4 second */ + buffer = gst_harness_pull (h); + fail_unless_equals_int64 (now + frame_dur, GST_BUFFER_PTS (buffer)); + gst_buffer_unref (buffer); + + now += 2 * frame_dur; + seq += 2; + + /* Pushing packet #5 normal again */ + gst_harness_set_time (h, now); + gst_harness_push (h, generate_test_buffer_full (now, seq, + AS_TEST_BUF_RTP_TIME (now))); + buffer = gst_harness_pull (h); + fail_unless_equals_int64 (now, GST_BUFFER_PTS (buffer)); + gst_buffer_unref (buffer); + + seq++; + now += frame_dur; + + /* Pushing packet #6 half a frame early to trigger clock skew */ + gst_harness_set_time (h, now); + gst_harness_push (h, generate_test_buffer_full (now, seq, + AS_TEST_BUF_RTP_TIME (now + frame_dur / 2))); + buffer = gst_harness_pull (h); + fail_unless (now + frame_dur / 2 > GST_BUFFER_PTS (buffer), + "pts should have been adjusted due to clock skew"); + gst_buffer_unref (buffer); + + gst_harness_teardown (h); +} + +GST_END_TEST; + GST_START_TEST (test_deadline_ts_offset) { GstHarness *h = gst_harness_new ("rtpjitterbuffer"); @@ -2651,6 +2735,7 @@ rtpjitterbuffer_suite (void) tcase_add_test (tc_chain, test_rtx_large_packet_spacing_and_large_rtt); tcase_add_test (tc_chain, test_rtx_large_packet_spacing_does_not_reset_jitterbuffer); + tcase_add_test (tc_chain, test_minor_reorder_does_not_skew); tcase_add_test (tc_chain, test_deadline_ts_offset); tcase_add_test (tc_chain, test_big_gap_seqnum);