rtpjitterbuffer: Detect whether to assume equidistant spacing when loss

Assuming equidistant packet spacing when that's not true leads to more
loss than necessary in the case of reordering and jitter. Typically this
is true for video where one frame often consists of multiple packets
with the same rtp timestamp. In this case it's better to assume that the
missing packets have the same timestamp as the last received packet, so
that the scheduled lost timer does not time out too early causing the
packets to be considered lost even though they may arrive in time.

https://bugzilla.gnome.org/show_bug.cgi?id=769768

gst/rtpmanager/gstrtpjitterbuffer.c

@@ -307,6 +307,7 @@ struct _GstRtpJitterBufferPrivate
   GstClockTime ips_dts;
   guint64 ips_rtptime;
   GstClockTime packet_spacing;
+  gint equidistant;
 
   GQueue gap_packets;
 
@@ -1566,6 +1567,7 @@ gst_rtp_jitter_buffer_flush_stop (GstRtpJitterBuffer * jitterbuffer)
   priv->last_dts = -1;
   priv->last_rtptime = -1;
   priv->last_in_dts = 0;
+  priv->equidistant = 0;
   GST_DEBUG_OBJECT (jitterbuffer, "flush and reset jitterbuffer");
   rtp_jitter_buffer_flush (priv->jbuf, (GFunc) free_item, NULL);
   rtp_jitter_buffer_disable_buffering (priv->jbuf, FALSE);
@@ -2423,8 +2425,9 @@ calculate_expected (GstRtpJitterBuffer * jitterbuffer, guint32 expected,
     guint16 seqnum, GstClockTime dts, gint gap)
 {
   GstRtpJitterBufferPrivate *priv = jitterbuffer->priv;
-  GstClockTime total_duration, duration, expected_dts, delay;
+  GstClockTime duration, expected_dts, delay;
   TimerType type;
+  gboolean equidistant = priv->equidistant > 0;
 
   GST_DEBUG_OBJECT (jitterbuffer,
       "dts %" GST_TIME_FORMAT ", last %" GST_TIME_FORMAT,
@@ -2435,56 +2438,66 @@ calculate_expected (GstRtpJitterBuffer * jitterbuffer, guint32 expected,
     return;
   }
 
-  /* the total duration spanned by the missing packets */
+  if (equidistant) {
-  if (dts >= priv->last_in_dts)
+    GstClockTime total_duration;
-    total_duration = dts - priv->last_in_dts;
+    /* the total duration spanned by the missing packets */
-  else
+    if (dts >= priv->last_in_dts)
-    total_duration = 0;
+      total_duration = dts - priv->last_in_dts;
+    else
+      total_duration = 0;
 
-  /* interpolate between the current time and the last time based on
+    /* interpolate between the current time and the last time based on
-   * number of packets we are missing, this is the estimated duration
+     * number of packets we are missing, this is the estimated duration
-   * for the missing packet based on equidistant packet spacing. */
+     * for the missing packet based on equidistant packet spacing. */
-  duration = total_duration / (gap + 1);
+    duration = total_duration / (gap + 1);
 
-  GST_DEBUG_OBJECT (jitterbuffer, "duration %" GST_TIME_FORMAT,
+    GST_DEBUG_OBJECT (jitterbuffer, "duration %" GST_TIME_FORMAT,
-      GST_TIME_ARGS (duration));
+        GST_TIME_ARGS (duration));
 
-  if (total_duration > priv->latency_ns) {
+    if (total_duration > priv->latency_ns) {
-    GstClockTime gap_time;
+      GstClockTime gap_time;
-    guint lost_packets;
+      guint lost_packets;
 
-    if (duration > 0) {
+      if (duration > 0) {
-      GstClockTime gap_dur = gap * duration;
+        GstClockTime gap_dur = gap * duration;
-      if (gap_dur > priv->latency_ns)
+        if (gap_dur > priv->latency_ns)
-        gap_time = gap_dur - priv->latency_ns;
+          gap_time = gap_dur - priv->latency_ns;
-      else
+        else
-        gap_time = 0;
+          gap_time = 0;
-      lost_packets = gap_time / duration;
+        lost_packets = gap_time / duration;
-    } else {
+      } else {
-      gap_time = total_duration - priv->latency_ns;
+        gap_time = total_duration - priv->latency_ns;
-      lost_packets = gap;
+        lost_packets = gap;
+      }
+
+      /* too many lost packets, some of the missing packets are already
+       * too late and we can generate lost packet events for them. */
+      GST_INFO_OBJECT (jitterbuffer,
+          "lost packets (%d, #%d->#%d) duration too large %" GST_TIME_FORMAT
+          " > %" GST_TIME_FORMAT ", consider %u lost (%" GST_TIME_FORMAT ")",
+          gap, expected, seqnum - 1, GST_TIME_ARGS (total_duration),
+          GST_TIME_ARGS (priv->latency_ns), lost_packets,
+          GST_TIME_ARGS (gap_time));
+
+      /* this timer will fire immediately and the lost event will be pushed from
+       * the timer thread */
+      if (lost_packets > 0) {
+        add_timer (jitterbuffer, TIMER_TYPE_LOST, expected, lost_packets,
+            priv->last_in_dts + duration, 0, gap_time);
+        expected += lost_packets;
+        priv->last_in_dts += gap_time;
+      }
     }
 
-    /* too many lost packets, some of the missing packets are already
+    expected_dts = priv->last_in_dts + duration;
-     * too late and we can generate lost packet events for them. */
+  } else {
-    GST_DEBUG_OBJECT (jitterbuffer,
+    /* If we cannot assume equidistant packet spacing, the only thing we now
-        "lost packets (%d, #%d->#%d) duration too large %" GST_TIME_FORMAT
+     * for sure is that the missing packets have expected dts not later than
-        " > %" GST_TIME_FORMAT ", consider %u lost (%" GST_TIME_FORMAT ")",
+     * the last received dts. */
-        gap, expected, seqnum - 1, GST_TIME_ARGS (total_duration),
+    duration = 0;
-        GST_TIME_ARGS (priv->latency_ns), lost_packets,
+    expected_dts = dts;
-        GST_TIME_ARGS (gap_time));
-
-    /* this timer will fire immediately and the lost event will be pushed from
-     * the timer thread */
-    if (lost_packets > 0) {
-      add_timer (jitterbuffer, TIMER_TYPE_LOST, expected, lost_packets,
-          priv->last_in_dts + duration, 0, gap_time);
-      expected += lost_packets;
-      priv->last_in_dts += gap_time;
-    }
   }
 
-  expected_dts = priv->last_in_dts + duration;
   delay = 0;
 
   if (priv->do_retransmission) {
@@ -2518,7 +2531,7 @@ calculate_expected (GstRtpJitterBuffer * jitterbuffer, guint32 expected,
 
 static void
 calculate_jitter (GstRtpJitterBuffer * jitterbuffer, GstClockTime dts,
-    guint rtptime)
+    guint32 rtptime)
 {
   gint32 rtpdiff;
   GstClockTimeDiff dtsdiff, rtpdiffns, diff;
@@ -2539,6 +2552,15 @@ calculate_jitter (GstRtpJitterBuffer * jitterbuffer, GstClockTime dts,
   else
     rtpdiff = 0;
 
+  /* Guess whether stream currently uses equidistant packet spacing. If we
+   * often see identical timestamps it means the packets are not
+   * equidistant. */
+  if (rtptime == priv->last_rtptime)
+    priv->equidistant -= 2;
+  else
+    priv->equidistant += 1;
+  priv->equidistant = CLAMP (priv->equidistant, -7, 7);
+
   priv->last_dts = dts;
   priv->last_rtptime = rtptime;
 

tests/check/elements/rtpjitterbuffer.c

@@ -925,6 +925,127 @@ GST_START_TEST (test_all_packets_are_timestamped_zero)
 
 GST_END_TEST;
 
+GST_START_TEST (test_reorder_of_non_equidistant_packets)
+{
+  GstHarness *h = gst_harness_new ("rtpjitterbuffer");
+  GstTestClock *testclock;
+  gint latency_ms = 5;
+  GstClockID pending_id;
+  GstClockTime time;
+  gint seq, frame;
+  gint num_init_frames = 1;
+  const GstClockTime frame_dur = PCMU_BUF_DURATION;
+  const guint32 frame_rtp_ts_dur = PCMU_RTP_TS_DURATION;
+
+  gst_harness_set_src_caps (h, generate_caps ());
+  testclock = gst_harness_get_testclock (h);
+  g_object_set (h->element, "do-lost", TRUE, "latency", latency_ms, NULL);
+
+  for (frame = 0, seq = 0; frame < num_init_frames; frame++, seq += 2) {
+    /* Push a couple of packets with identical timestamp, typical for a video
+     * stream where one frame generates multiple packets. */
+    gst_harness_set_time (h, frame * frame_dur);
+    gst_harness_push (h, generate_test_buffer_full (frame * frame_dur, FALSE,
+            seq, frame * frame_rtp_ts_dur));
+    gst_harness_push (h, generate_test_buffer_full (frame * frame_dur, TRUE,
+            seq + 1, frame * frame_rtp_ts_dur));
+
+    if (frame == 0)
+      /* deadline for buffer 0 expires */
+      gst_harness_crank_single_clock_wait (h);
+
+    gst_buffer_unref (gst_harness_pull (h));
+    gst_buffer_unref (gst_harness_pull (h));
+  }
+
+  /* Finally push the last frame reordered */
+  gst_harness_set_time (h, frame * frame_dur);
+  gst_harness_push (h, generate_test_buffer_full (frame * frame_dur, TRUE,
+          seq + 1, frame * frame_rtp_ts_dur));
+
+  /* Check the scheduled lost timer. The expected arrival of this packet
+   * should be assumed to be the same as the last packet received since we
+   * don't know wether the missing packet belonged to this or previous
+   * frame. */
+  gst_test_clock_wait_for_next_pending_id (testclock, &pending_id);
+  time = gst_clock_id_get_time (pending_id);
+  fail_unless_equals_int64 (time, frame * frame_dur + latency_ms * GST_MSECOND);
+  gst_clock_id_unref (pending_id);
+
+  /* And then missing packet arrives just in time */
+  gst_harness_set_time (h, time - 1);
+  gst_harness_push (h, generate_test_buffer_full (time - 1, FALSE, seq,
+          frame * frame_rtp_ts_dur));
+
+  gst_buffer_unref (gst_harness_pull (h));
+  gst_buffer_unref (gst_harness_pull (h));
+
+  gst_object_unref (testclock);
+  gst_harness_teardown (h);
+}
+
+GST_END_TEST;
+
+GST_START_TEST (test_loss_equidistant_spacing_with_parameter_packets)
+{
+  GstHarness *h = gst_harness_new ("rtpjitterbuffer");
+  GstTestClock *testclock;
+  GstEvent *event;
+  gint latency_ms = 5;
+  gint seq, frame;
+  gint num_init_frames = 10;
+
+  gst_harness_set_src_caps (h, generate_caps ());
+  testclock = gst_harness_get_testclock (h);
+  g_object_set (h->element, "do-lost", TRUE, "latency", latency_ms, NULL);
+
+  /* drop stream-start, caps, segment */
+  for (int i = 0; i < 3; i++)
+    gst_event_unref (gst_harness_pull_event (h));
+
+  for (frame = 0, seq = 0; frame < num_init_frames; frame++, seq++) {
+    gst_harness_set_time (h, frame * PCMU_BUF_DURATION);
+    gst_harness_push (h, generate_test_buffer_full (frame * PCMU_BUF_DURATION,
+            TRUE, seq, frame * PCMU_RTP_TS_DURATION));
+
+    if (frame == 0)
+      /* deadline for buffer 0 expires */
+      gst_harness_crank_single_clock_wait (h);
+
+    gst_buffer_unref (gst_harness_pull (h));
+  }
+
+  /* Push three packets with same rtptime, simulating parameter packets +
+   * frame. This should not disable equidistant mode as it is common for
+   * certain audio codecs. */
+  for (gint i = 0; i < 3; i++) {
+    gst_harness_set_time (h, frame * PCMU_BUF_DURATION);
+    gst_harness_push (h, generate_test_buffer_full (frame * PCMU_BUF_DURATION,
+            i == 2, seq++, frame * PCMU_RTP_TS_DURATION));
+    gst_buffer_unref (gst_harness_pull (h));
+  }
+  frame++;
+
+  /* Finally push the last packet introducing a gap */
+  gst_harness_set_time (h, frame * PCMU_BUF_DURATION);
+  gst_harness_push (h, generate_test_buffer_full (frame * PCMU_BUF_DURATION,
+          TRUE, seq + 1, frame * PCMU_RTP_TS_DURATION));
+
+  /* Check that the lost event has been generated assuming equidistant
+   * spacing. */
+  event = gst_harness_pull_event (h);
+  verify_lost_event (event, seq,
+      frame * PCMU_BUF_DURATION - PCMU_BUF_DURATION / 2, PCMU_BUF_DURATION / 2);
+
+  gst_buffer_unref (gst_harness_pull (h));
+
+  gst_object_unref (testclock);
+  gst_harness_teardown (h);
+}
+
+GST_END_TEST;
+
+
 static void
 gst_test_clock_set_time_and_process (GstTestClock * testclock,
     GstClockTime time)
@@ -2368,6 +2489,8 @@ rtpjitterbuffer_suite (void)
   tcase_add_test (tc_chain, test_all_packets_are_timestamped_zero);
   tcase_add_loop_test (tc_chain, test_num_late_when_considered_lost_arrives, 0,
       2);
+  tcase_add_test (tc_chain, test_reorder_of_non_equidistant_packets);
+  tcase_add_test (tc_chain, test_loss_equidistant_spacing_with_parameter_packets);
 
   tcase_add_test (tc_chain, test_rtx_expected_next);
   tcase_add_test (tc_chain, test_rtx_two_missing);