diff --git a/gst/rtpmanager/gstrtpjitterbuffer.c b/gst/rtpmanager/gstrtpjitterbuffer.c index 35326ddb2e..3baad7d58c 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 64d89a87ca..9831461319 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 b65b6038e9..37ccd8711e 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 981cd64875..747929fcee 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 0d3c912d72..078c5d6d76 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);