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

@@ -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

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)

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);
 

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;

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);