rtpjitterbuffer: Fix stall when receiving already lost packet

When a packet arrives that has already been considered lost as part of a
large gap the "lost timer" for this will be cancelled. If the remaining
packets of this large gap never arrives, there will be missing entries
in the queue and the loop function will keep waiting for these packets
to arrive and never push another packet, effectively stalling the
pipeline.

The proposed fix conciders parts of a large gap definitely lost (since
they are calculated from latency) and ignores the late arrivals.

In practice the issue is rare since large gaps are scheduled immediately,
and for the stall to happen the late arrival needs to be processed
before this times out.

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

gst/rtpmanager/gstrtpjitterbuffer.c

@@ -2096,6 +2096,30 @@ 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, guint16 seqnum)
+{
+  GstRtpJitterBufferPrivate *priv = jitterbuffer->priv;
+  gint i, len;
+
+  len = priv->timers->len;
+  for (i = 0; i < len; i++) {
+    TimerData *test = &g_array_index (priv->timers, TimerData, i);
+    gint gap = gst_rtp_buffer_compare_seqnum (test->seqnum, seqnum);
+
+    if (test->num > 1 && test->type == TIMER_TYPE_LOST && gap >= 0 &&
+        gap < test->num) {
+      GST_DEBUG ("seqnum #%d already considered definitely lost (#%d->#%d)",
+          seqnum, test->seqnum, (test->seqnum + test->num - 1) & 0xffff);
+      return TRUE;
+    }
+  }
+
+  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
@@ -2302,7 +2326,7 @@ calculate_expected (GstRtpJitterBuffer * jitterbuffer, guint32 expected,
     GST_DEBUG_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, GST_TIME_ARGS (total_duration),
+        gap, expected, seqnum - 1, GST_TIME_ARGS (total_duration),
         GST_TIME_ARGS (priv->latency_ns), lost_packets,
         GST_TIME_ARGS (gap_time));
 
@@ -2814,6 +2838,9 @@ gst_rtp_jitter_buffer_chain (GstPad * pad, GstObject * parent,
       goto too_late;
   }
 
+  if (already_lost (jitterbuffer, 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
@@ -2931,6 +2958,14 @@ 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++;
+    gst_buffer_unref (buffer);
+    goto finished;
+  }
 duplicate:
   {
     GST_DEBUG_OBJECT (jitterbuffer, "Duplicate packet #%d detected, dropping",

tests/check/elements/rtpjitterbuffer.c

@@ -459,7 +459,7 @@ get_rtp_seq_num (GstBuffer * buf)
 }
 
 static void
-verify_lost_event (GstEvent * event, guint32 expected_seqnum,
+verify_lost_event (GstEvent * event, guint16 expected_seqnum,
     GstClockTime expected_timestamp, GstClockTime expected_duration)
 {
   const GstStructure *s = gst_event_get_structure (event);
@@ -1407,6 +1407,87 @@ GST_START_TEST (test_push_big_gap)
 
 GST_END_TEST;
 
+typedef struct
+{
+  guint seqnum_offset;
+  guint late_buffer;
+} TestLateArrivalInput;
+
+static const TestLateArrivalInput
+    test_considered_lost_packet_in_large_gap_arrives_input[] = {
+  {0, 1}, {0, 2}, {65535, 1}, {65535, 2}, {65534, 1}, {65534, 2}
+};
+
+GST_START_TEST (test_considered_lost_packet_in_large_gap_arrives)
+{
+  GstHarness *h = gst_harness_new ("rtpjitterbuffer");
+  GstTestClock *testclock;
+  GstClockID id;
+  GstBuffer *buffer;
+  gint jb_latency_ms = 20;
+  GstEvent *event;
+  const TestLateArrivalInput *test_input =
+      &test_considered_lost_packet_in_large_gap_arrives_input[__i__];
+  guint seq_offset = test_input->seqnum_offset;
+  guint late_buffer = test_input->late_buffer;
+
+  gst_harness_set_src_caps (h, generate_caps ());
+  testclock = gst_harness_get_testclock (h);
+  g_object_set (h->element, "do-lost", TRUE, "latency", jb_latency_ms, NULL);
+
+  /* first push buffer 0 */
+  fail_unless_equals_int (GST_FLOW_OK,
+      gst_harness_push (h, generate_test_buffer_full (0 * PCMU_BUF_DURATION,
+              TRUE, 0 + seq_offset, 0 * PCMU_RTP_TS_DURATION)));
+  fail_unless (gst_harness_crank_single_clock_wait (h));
+  gst_buffer_unref (gst_harness_pull (h));
+
+  /* drop GstEventStreamStart & GstEventCaps & GstEventSegment */
+  for (gint i = 0; i < 3; i++)
+    gst_event_unref (gst_harness_pull_event (h));
+
+  /* hop over 3 packets, and push buffer 4 (gap of 3) */
+  fail_unless_equals_int (GST_FLOW_OK,
+      gst_harness_push (h, generate_test_buffer_full (4 * PCMU_BUF_DURATION,
+              TRUE, 4 + seq_offset, 4 * PCMU_RTP_TS_DURATION)));
+
+  /* the jitterbuffer should be waiting for the timeout of a "large gap timer"
+   * for buffer 1 and 2 */
+  gst_test_clock_wait_for_next_pending_id (testclock, &id);
+  fail_unless_equals_uint64 (1 * PCMU_BUF_DURATION +
+      jb_latency_ms * GST_MSECOND, gst_clock_id_get_time (id));
+  gst_clock_id_unref (id);
+
+  /* now buffer 1 sneaks in before the lost event for buffer 1 and 2 is
+   * processed */
+  fail_unless_equals_int (GST_FLOW_OK,
+      gst_harness_push (h,
+          generate_test_buffer_full (late_buffer * PCMU_BUF_DURATION, TRUE,
+              late_buffer + seq_offset, late_buffer * PCMU_RTP_TS_DURATION)));
+
+  /* time out for lost packets 1 and 2 (one event, double duration) */
+  fail_unless (gst_harness_crank_single_clock_wait (h));
+  event = gst_harness_pull_event (h);
+  verify_lost_event (event, 1 + seq_offset, 1 * PCMU_BUF_DURATION,
+      2 * PCMU_BUF_DURATION);
+
+  /* time out for lost packets 3 */
+  fail_unless (gst_harness_crank_single_clock_wait (h));
+  event = gst_harness_pull_event (h);
+  verify_lost_event (event, 3 + seq_offset, 3 * PCMU_BUF_DURATION,
+      1 * PCMU_BUF_DURATION);
+
+  /* buffer 4 is pushed as normal */
+  buffer = gst_harness_pull (h);
+  fail_unless_equals_int ((4 + seq_offset) & 0xffff, get_rtp_seq_num (buffer));
+  gst_buffer_unref (buffer);
+
+  gst_object_unref (testclock);
+  gst_harness_teardown (h);
+}
+
+GST_END_TEST;
+
 static Suite *
 rtpjitterbuffer_suite (void)
 {
@@ -1431,6 +1512,10 @@ rtpjitterbuffer_suite (void)
   tcase_add_test (tc_chain, test_dts_gap_larger_than_latency);
   tcase_add_test (tc_chain, test_push_big_gap);
 
+  tcase_add_loop_test (tc_chain,
+      test_considered_lost_packet_in_large_gap_arrives, 0,
+      G_N_ELEMENTS (test_considered_lost_packet_in_large_gap_arrives_input));
+
   return s;
 }