gst/rtpmanager/gstrtpjitterbuffer.c: When checking the seqnum, reset the jitterbuffer if the gap is too big, we need ...

Original commit message from CVS:
* gst/rtpmanager/gstrtpjitterbuffer.c:
(gst_rtp_jitter_buffer_chain), (gst_rtp_jitter_buffer_loop):
When checking the seqnum, reset the jitterbuffer if the gap is too big,
we need to do this so that we can better handle a restarted source.
Fix some comments.
* gst/rtpmanager/rtpjitterbuffer.c: (calculate_skew),
(rtp_jitter_buffer_insert):
Tweak the skew resync diff.
Use our working seqnum compare function in -base.
Rework the jitterbuffer insert code to make it clearer and more
performant by only retrieving the seqnum of the input buffer once and by
adding some G_LIKELY compiler hints.
Improve debugging for duplicate packets.
* gst/rtpmanager/rtpsource.c: (rtp_source_process_rtp):
Fix a comment, we don't do skew correction here..

4 files changed, 81 insertions, 38 deletions

diff --git a/ChangeLog b/ChangeLog
index 6c7365cd..b98c11cb 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,25 @@
 2008-05-26  Wim Taymans  <wim.taymans@collabora.co.uk>
 
+	* gst/rtpmanager/gstrtpjitterbuffer.c:
+	(gst_rtp_jitter_buffer_chain), (gst_rtp_jitter_buffer_loop):
+	When checking the seqnum, reset the jitterbuffer if the gap is too big,
+	we need to do this so that we can better handle a restarted source.
+	Fix some comments.
+
+	* gst/rtpmanager/rtpjitterbuffer.c: (calculate_skew),
+	(rtp_jitter_buffer_insert):
+	Tweak the skew resync diff.
+	Use our working seqnum compare function in -base.
+	Rework the jitterbuffer insert code to make it clearer and more
+	performant by only retrieving the seqnum of the input buffer once and by
+	adding some G_LIKELY compiler hints.
+	Improve debugging for duplicate packets.
+
+	* gst/rtpmanager/rtpsource.c: (rtp_source_process_rtp):
+	Fix a comment, we don't do skew correction here..
+
+2008-05-26  Wim Taymans  <wim.taymans@collabora.co.uk>
+
 	Patch by: Håvard Graff <havard dot graff at tandberg dot com>
 
 	* gst/rtpmanager/gstrtpbin.c:
diff --git a/gst/rtpmanager/gstrtpjitterbuffer.c b/gst/rtpmanager/gstrtpjitterbuffer.c
index 46439dc1..17dd4a90 100644
--- a/gst/rtpmanager/gstrtpjitterbuffer.c
+++ b/gst/rtpmanager/gstrtpjitterbuffer.c
@@ -885,12 +885,32 @@ gst_rtp_jitter_buffer_chain (GstPad * pad, GstBuffer * buffer)
   if (priv->eos)
     goto have_eos;
 
-  /* let's check if this buffer is too late, we cannot accept packets with
-   * bigger seqnum than the one we already pushed. */
+  /* let's check if this buffer is too late, we can only accept packets with
+   * bigger seqnum than the one we last pushed. */
   if (priv->last_popped_seqnum != -1) {
-    /* FIXME. isn't this supposed to be <= ? */
-    if (gst_rtp_buffer_compare_seqnum (priv->last_popped_seqnum, seqnum) < 0)
-      goto too_late;
+    gint gap;
+
+    gap = gst_rtp_buffer_compare_seqnum (priv->last_popped_seqnum, seqnum);
+
+    if (gap <= 0) {
+      /* priv->last_popped_seqnum >= seqnum, this packet is too late or the
+       * sender might have been restarted with different seqnum. */
+      if (gap < -100) {
+        GST_DEBUG_OBJECT (jitterbuffer, "reset: buffer too old %d", gap);
+        priv->last_popped_seqnum = -1;
+        priv->next_seqnum = -1;
+      } else {
+        goto too_late;
+      }
+    } else {
+      /* priv->last_popped_seqnum < seqnum, this is a new packet */
+      if (gap > 3000) {
+        GST_DEBUG_OBJECT (jitterbuffer, "reset: too many dropped packets %d",
+            gap);
+        priv->last_popped_seqnum = -1;
+        priv->next_seqnum = -1;
+      }
+    }
   }
 
   /* let's drop oldest packet if the queue is already full and drop-on-latency
@@ -1041,7 +1061,7 @@ again:
       if (priv->eos)
         goto do_eos;
     }
-    /* wait for packets or flushing now */
+    /* underrun, wait for packets or flushing now */
     priv->waiting = TRUE;
     JBUF_WAIT_CHECK (priv, flushing);
     priv->waiting = FALSE;
@@ -1187,7 +1207,7 @@ again:
     if (gap > 0) {
       GstEvent *event;
 
-      /* we had a gap and thus we lost a packet. Creat an event for this.  */
+      /* we had a gap and thus we lost a packet. Create an event for this.  */
       GST_DEBUG_OBJECT (jitterbuffer, "Packet #%d lost", next_seqnum);
       priv->num_late++;
       discont = TRUE;
diff --git a/gst/rtpmanager/rtpjitterbuffer.c b/gst/rtpmanager/rtpjitterbuffer.c
index 8aaefeb0..caca165d 100644
--- a/gst/rtpmanager/rtpjitterbuffer.c
+++ b/gst/rtpmanager/rtpjitterbuffer.c
@@ -217,8 +217,10 @@ again:
       delta_diff = jbuf->prev_send_diff - send_diff;
 
     /* server changed rtp timestamps too quickly, reset skew detection and start
-     * again. */
-    if (delta_diff > GST_SECOND / 4) {
+     * again. This value is sortof arbitrary and can be a bad measurement up if
+     * there are many packets missing because then we get a big gap that is
+     * unrelated to a timestamp switch. */
+    if (delta_diff > GST_SECOND) {
       GST_DEBUG ("delta changed too quickly %" GST_TIME_FORMAT " reset skew",
           GST_TIME_ARGS (delta_diff));
       rtp_jitter_buffer_reset_skew (jbuf);
@@ -326,23 +328,6 @@ no_skew:
   return out_time;
 }
 
-static gint
-compare_seqnum (GstBuffer * a, GstBuffer * b, RTPJitterBuffer * jbuf)
-{
-  guint16 seq1, seq2;
-
-  seq1 = gst_rtp_buffer_get_seq (a);
-  seq2 = gst_rtp_buffer_get_seq (b);
-
-  /* check if diff more than half of the 16bit range */
-  if (abs (seq2 - seq1) > (1 << 15)) {
-    /* one of a/b has wrapped */
-    return seq1 - seq2;
-  } else {
-    return seq2 - seq1;
-  }
-}
-
 /**
  * rtp_jitter_buffer_insert:
  * @jbuf: an #RTPJitterBuffer
@@ -362,22 +347,32 @@ rtp_jitter_buffer_insert (RTPJitterBuffer * jbuf, GstBuffer * buf,
     GstClockTime time, guint32 clock_rate, gboolean * tail)
 {
   GList *list;
-  gint func_ret = 1;
   guint32 rtptime;
+  guint16 seqnum;
 
   g_return_val_if_fail (jbuf != NULL, FALSE);
   g_return_val_if_fail (buf != NULL, FALSE);
 
+  seqnum = gst_rtp_buffer_get_seq (buf);
+
   /* loop the list to skip strictly smaller seqnum buffers */
-  list = jbuf->packets->head;
-  while (list
-      && (func_ret =
-          compare_seqnum (GST_BUFFER_CAST (list->data), buf, jbuf)) < 0)
-    list = list->next;
-
-  /* we hit a packet with the same seqnum, return FALSE to notify a duplicate */
-  if (func_ret == 0)
-    return FALSE;
+  for (list = jbuf->packets->head; list; list = g_list_next (list)) {
+    guint16 qseq;
+    gint gap;
+
+    qseq = gst_rtp_buffer_get_seq (GST_BUFFER_CAST (list->data));
+
+    /* compare the new seqnum to the one in the buffer */
+    gap = gst_rtp_buffer_compare_seqnum (seqnum, qseq);
+
+    /* we hit a packet with the same seqnum, notify a duplicate */
+    if (G_UNLIKELY (gap == 0))
+      goto duplicate;
+
+    /* seqnum > qseq, we can stop looking */
+    if (G_LIKELY (gap < 0))
+      break;
+  }
 
   /* do skew calculation by measuring the difference between rtptime and the
    * receive time, this function will retimestamp @buf with the skew corrected
@@ -391,11 +386,19 @@ rtp_jitter_buffer_insert (RTPJitterBuffer * jbuf, GstBuffer * buf,
   else
     g_queue_push_tail (jbuf->packets, buf);
 
-  /* tail was changed when we did not find a previous packet */
+  /* tail was changed when we did not find a previous packet, we set the return
+   * flag when requested. */
   if (tail)
     *tail = (list == NULL);
 
   return TRUE;
+
+  /* ERRORS */
+duplicate:
+  {
+    GST_WARNING ("duplicate packet %d found", (gint) seqnum);
+    return FALSE;
+  }
 }
 
 /**
diff --git a/gst/rtpmanager/rtpsource.c b/gst/rtpmanager/rtpsource.c
index 4c351a1a..7eab91e0 100644
--- a/gst/rtpmanager/rtpsource.c
+++ b/gst/rtpmanager/rtpsource.c
@@ -940,7 +940,7 @@ rtp_source_process_rtp (RTPSource * src, GstBuffer * buffer,
   GST_DEBUG ("seq %d, PC: %" G_GUINT64_FORMAT ", OC: %" G_GUINT64_FORMAT,
       seqnr, src->stats.packets_received, src->stats.octets_received);
 
-  /* calculate jitter and perform skew correction */
+  /* calculate jitter for the stats */
   calculate_jitter (src, buffer, arrival);
 
   /* we're ready to push the RTP packet now */