jitterbuffer: Speed up lost timeout handling

When downstream blocks, "lost" timers are created to notify the
outgoing thread that packets are lost.

The problem is that for high packet-rate streams, we might end up with
a big list of lost timeouts (had a use-case with ~1000...).

The problem isn't so much the amount of lost timeouts to handle, but
rather the way they were handled. All timers would first be iterated,
then the one selected would be handled ... to re-iterate the list again.

All of this is being done while the jbuf lock is taken, which in some use-cases
would return in holding that lock for 10s... blocking any buffers from
being accepted in input... which would then arrive late ... which would
create plenty of lost timers ... which would cause the same issue.

In order to avoid that situation, handle the lost timers immediately when
iterating the list of pending timers. This modifies the complexity from
a quadratic to a linear complexity.

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

diff --git a/gst/rtpmanager/gstrtpjitterbuffer.c b/gst/rtpmanager/gstrtpjitterbuffer.c
index be0a1c6..608c794 100644 (file)
--- a/gst/rtpmanager/gstrtpjitterbuffer.c
+++ b/gst/rtpmanager/gstrtpjitterbuffer.c
@@ -3522,40 +3522,56 @@ wait_next_timeout (GstRtpJitterBuffer * jitterbuffer)
         GST_TIME_ARGS (now));
 
     len = priv->timers->len;
-    for (i = 0; i < len; i++) {
+    for (i = 0; i < len;) {
       TimerData *test = &g_array_index (priv->timers, TimerData, i);
       GstClockTime test_timeout = get_timeout (jitterbuffer, test);
       gboolean save_best = FALSE;
 
-      GST_DEBUG_OBJECT (jitterbuffer, "%d, %d, %d, %" GST_TIME_FORMAT,
-          i, test->type, test->seqnum, GST_TIME_ARGS (test_timeout));
-
-      /* find the smallest timeout */
-      if (timer == NULL) {
-        save_best = TRUE;
-      } else if (timer_timeout == -1) {
-        /* we already have an immediate timeout, the new timer must be an
-         * immediate timer with smaller seqnum to become the best */
-        if (test_timeout == -1
+      GST_DEBUG_OBJECT (jitterbuffer,
+          "%d, %d, %d, %" GST_TIME_FORMAT " diff:%" GST_STIME_FORMAT, i,
+          test->type, test->seqnum, GST_TIME_ARGS (test_timeout),
+          GST_STIME_ARGS ((gint64) (test_timeout - now)));
+
+      /* Weed out anything too late */
+      if (test->type == TIMER_TYPE_LOST &&
+          (test_timeout == -1 || test_timeout <= now)) {
+        GST_DEBUG_OBJECT (jitterbuffer, "Weeding out late entry");
+        do_lost_timeout (jitterbuffer, test, now);
+        if (!priv->timer_running)
+          break;
+        /* We don't move the iterator forward since we just removed the current entry,
+         * but we update the termination condition */
+        len = priv->timers->len;
+      } else {
+        /* find the smallest timeout */
+        if (timer == NULL) {
+          save_best = TRUE;
+        } else if (timer_timeout == -1) {
+          /* we already have an immediate timeout, the new timer must be an
+           * immediate timer with smaller seqnum to become the best */
+          if (test_timeout == -1
+              && (gst_rtp_buffer_compare_seqnum (test->seqnum,
+                      timer->seqnum) > 0))
+            save_best = TRUE;
+        } else if (test_timeout == -1) {
+          /* first immediate timer */
+          save_best = TRUE;
+        } else if (test_timeout < timer_timeout) {
+          /* earlier timer */
+          save_best = TRUE;
+        } else if (test_timeout == timer_timeout
             && (gst_rtp_buffer_compare_seqnum (test->seqnum,
-                    timer->seqnum) > 0))
+                    timer->seqnum) > 0)) {
+          /* same timer, smaller seqnum */
           save_best = TRUE;
-      } else if (test_timeout == -1) {
-        /* first immediate timer */
-        save_best = TRUE;
-      } else if (test_timeout < timer_timeout) {
-        /* earlier timer */
-        save_best = TRUE;
-      } else if (test_timeout == timer_timeout
-          && (gst_rtp_buffer_compare_seqnum (test->seqnum,
-                  timer->seqnum) > 0)) {
-        /* same timer, smaller seqnum */
-        save_best = TRUE;
-      }
-      if (save_best) {
-        GST_DEBUG_OBJECT (jitterbuffer, "new best %d", i);
-        timer = test;
-        timer_timeout = test_timeout;
+        }
+
+        if (save_best) {
+          GST_DEBUG_OBJECT (jitterbuffer, "new best %d", i);
+          timer = test;
+          timer_timeout = test_timeout;
+        }
+        i++;
       }
     }
     if (timer && !priv->blocked) {
@@ -3565,6 +3581,9 @@ wait_next_timeout (GstRtpJitterBuffer * jitterbuffer)
       GstClockReturn ret;
       GstClockTimeDiff clock_jitter;
 
+      /* We have normally removed all lost timers in the loop above */
+      g_assert (timer->type != TIMER_TYPE_LOST);
+
       if (timer_timeout == -1 || timer_timeout <= now) {
         do_timeout (jitterbuffer, timer, now);
         /* check here, do_timeout could have released the lock */