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

1 files changed, 36 insertions, 1 deletions

diff --git a/gst/rtpmanager/gstrtpjitterbuffer.c b/gst/rtpmanager/gstrtpjitterbuffer.c
index b0fbef709..9077219ad 100644
--- a/gst/rtpmanager/gstrtpjitterbuffer.c
+++ b/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",