rtpjitterbuffer: don't use RTX packets in rate-calc and reset-logic

The problem was this:

Due to the highly irregular arrival of RTX-packet the max-misorder variable
could be pushed very low. (-10).

If you then at some point get a big in the sequence-numbers (62 in the
test) you end up sending RTX-requests for some of those packets, and then
if the sender answers those requests, you are going to get a bunch of
RTX-packets arriving. (-13 and then 5 more packets in the test)

Now, if max-misorder is pushed very low at this point, these RTX-packets
will trigger the handle_big_gap_buffer() logic, and because they arriving
so neatly in order, (as they would, since they have been requested like
that), the gst_rtp_jitter_buffer_reset() will be called, and two things
will happen:
1. priv->next_seqnum will be set to the first RTX packet
2. the 5 RTX-packet will be pushed into the chain() function

However, at this point, these RTX-packets are no longer valid, the
jitterbuffer has already pushed lost-events for these, so they will now
be dropped on the floor, and never make it to the waiting loop-function.

And, since we now have a priv->next_seqnum that will never arrive
in the loop-function, the jitterbuffer is now stalled forever, and will
not push out another buffer.

The proposed fixes:
1. Don't use RTX in calculation of the packet-rate.
2. Don't use RTX in large-gap logic, as they are likely to be dropped.

diff --git a/gst/rtpmanager/gstrtpjitterbuffer.c b/gst/rtpmanager/gstrtpjitterbuffer.c
index 37fcc77..f0571f4 100644 (file)
--- a/gst/rtpmanager/gstrtpjitterbuffer.c
+++ b/gst/rtpmanager/gstrtpjitterbuffer.c
@@ -2946,16 +2946,19 @@ gst_rtp_jitter_buffer_chain (GstPad * pad, GstObject * parent,
 
   expected = priv->next_in_seqnum;
 
-  packet_rate =
-      gst_rtp_packet_rate_ctx_update (&priv->packet_rate_ctx, seqnum, rtptime);
+  /* don't update packet-rate based on RTX, as those arrive highly unregularly */
+  if (!is_rtx) {
+    packet_rate = gst_rtp_packet_rate_ctx_update (&priv->packet_rate_ctx,
+        seqnum, rtptime);
+    GST_TRACE_OBJECT (jitterbuffer, "updated packet_rate: %d", packet_rate);
+  }
   max_dropout =
       gst_rtp_packet_rate_ctx_get_max_dropout (&priv->packet_rate_ctx,
       priv->max_dropout_time);
   max_misorder =
       gst_rtp_packet_rate_ctx_get_max_misorder (&priv->packet_rate_ctx,
       priv->max_misorder_time);
-  GST_TRACE_OBJECT (jitterbuffer,
-      "packet_rate: %d, max_dropout: %d, max_misorder: %d", packet_rate,
+  GST_TRACE_OBJECT (jitterbuffer, "max_dropout: %d, max_misorder: %d",
       max_dropout, max_misorder);
 
   timer = rtp_timer_queue_find (priv->timers, seqnum);
@@ -3021,7 +3024,7 @@ gst_rtp_jitter_buffer_chain (GstPad * pad, GstObject * parent,
     }
 
     /* Special handling of large gaps */
-    if ((gap != -1 && gap < -max_misorder) || (gap >= max_dropout)) {
+    if (!is_rtx && ((gap != -1 && gap < -max_misorder) || (gap >= max_dropout))) {
       gboolean reset = handle_big_gap_buffer (jitterbuffer, buffer, pt, seqnum,
           gap, max_dropout, max_misorder);
       if (reset) {

diff --git a/tests/check/elements/rtpjitterbuffer.c b/tests/check/elements/rtpjitterbuffer.c
index f055b22..99cc57c 100644 (file)
--- a/tests/check/elements/rtpjitterbuffer.c
+++ b/tests/check/elements/rtpjitterbuffer.c
@@ -3126,6 +3126,29 @@ GST_START_TEST (test_multiple_lost_do_not_stall)
 
 GST_END_TEST;
 
+GST_START_TEST (test_reset_using_rtx_packets_does_not_stall)
+{
+  GstHarness *h = gst_harness_new ("rtpjitterbuffer");
+  BufferArrayCtx bufs[] = {
+    /* *INDENT-OFF* */
+    {  1,    1 * TEST_RTP_TS_DURATION, FALSE, 2000000},
+    {  62,  62 * TEST_RTP_TS_DURATION, FALSE, 0},
+    { -13, -13 * TEST_RTP_TS_DURATION, TRUE, 10000},
+    {   1,   1 * TEST_RTP_TS_DURATION, TRUE, 0},
+    {   1,   1 * TEST_RTP_TS_DURATION, TRUE, 0},
+    {   1,   1 * TEST_RTP_TS_DURATION, TRUE, 0},
+    {   1,   1 * TEST_RTP_TS_DURATION, TRUE, 0},
+    {   1,   1 * TEST_RTP_TS_DURATION, TRUE, 0},
+    /* *INDENT-ON* */
+  };
+
+  g_object_set (h->element, "latency", 400,
+      "do-retransmission", TRUE, "do-lost", TRUE, "max-misorder-time", 1, NULL);
+  fail_unless (check_for_stall (h, bufs, G_N_ELEMENTS (bufs)));
+  gst_harness_teardown (h);
+}
+
+GST_END_TEST;
 
 static Suite *
 rtpjitterbuffer_suite (void)
@@ -3196,6 +3219,8 @@ rtpjitterbuffer_suite (void)
 
   tcase_add_test (tc_chain, test_reset_timers_does_not_stall);
   tcase_add_test (tc_chain, test_multiple_lost_do_not_stall);
+  tcase_add_test (tc_chain, test_reset_using_rtx_packets_does_not_stall);
+
 
   return s;
 }