rtpjitterbuffer: don't try and calculate packet-rate if seqnum are jumping

Turns out that the "big-gap"-logic of the jitterbuffer has been horribly
broken.

For people using lost-events, an RTP-stream with a gap in sequencenumbers,
would produce exactly that many lost-events immediately.
So if your sequence-numbers jumped 20000, you would get 20000 lost-events
in your pipeline...

The test that looks after this logic "test_push_big_gap", basically
incremented the DTS of the buffer equal to the gap that was introduced,
so that in fact this would be more of a "large pause" test, than an
actual gap/discontinuity in the sequencenumbers.

Once the test was modified to not increment DTS (buffer arrival time) with
a similar gap, all sorts of crazy started happening, including adding
thousands of timers, and the logic that should have kicked in, the
"handle_big_gap_buffer"-logic, was not called at all, why?

Because the number max_dropout is calculated using the packet-rate, and
the packet-rate logic would, in this particular test, report that
the new packet rate was over 400000 packets per second!!!

I believe the right fix is to don't try and update the packet-rate if
there is any jumps in the sequence-numbers, and only do these calculations
for nice, sequential streams.

diff --git a/gst/rtpmanager/rtpstats.c b/gst/rtpmanager/rtpstats.c
index 73bd189..27063ee 100644 (file)
--- a/gst/rtpmanager/rtpstats.c
+++ b/gst/rtpmanager/rtpstats.c
@@ -46,15 +46,13 @@ gst_rtp_packet_rate_ctx_update (RTPPacketRateCtx * ctx, guint16 seqnum,
   gst_rtp_buffer_ext_timestamp (&new_ts, ts);
 
   if (!ctx->probed) {
-    ctx->last_seqnum = seqnum;
-    ctx->last_ts = new_ts;
     ctx->probed = TRUE;
-    return ctx->avg_packet_rate;
+    goto done;
   }
 
   diff_seqnum = gst_rtp_buffer_compare_seqnum (ctx->last_seqnum, seqnum);
-  if (diff_seqnum <= 0 || new_ts <= ctx->last_ts) {
-    return ctx->avg_packet_rate;
+  if (diff_seqnum <= 0 || new_ts <= ctx->last_ts || diff_seqnum > 1) {
+    goto done;
   }
 
   diff_ts = new_ts - ctx->last_ts;
@@ -74,6 +72,7 @@ gst_rtp_packet_rate_ctx_update (RTPPacketRateCtx * ctx, guint16 seqnum,
     ctx->avg_packet_rate = (ctx->avg_packet_rate + new_packet_rate + 1) / 2;
   }
 
+done:
   ctx->last_seqnum = seqnum;
   ctx->last_ts = new_ts;
 
@@ -89,7 +88,7 @@ gst_rtp_packet_rate_ctx_get (RTPPacketRateCtx * ctx)
 guint32
 gst_rtp_packet_rate_ctx_get_max_dropout (RTPPacketRateCtx * ctx, gint32 time_ms)
 {
-  if (time_ms <= 0 || !ctx->probed) {
+  if (time_ms <= 0 || !ctx->probed || ctx->avg_packet_rate == -1) {
     return RTP_DEF_DROPOUT;
   }
 
@@ -100,7 +99,7 @@ guint32
 gst_rtp_packet_rate_ctx_get_max_misorder (RTPPacketRateCtx * ctx,
     gint32 time_ms)
 {
-  if (time_ms <= 0 || !ctx->probed) {
+  if (time_ms <= 0 || !ctx->probed || ctx->avg_packet_rate == -1) {
     return RTP_DEF_MISORDER;
   }
 

diff --git a/tests/check/elements/rtpjitterbuffer.c b/tests/check/elements/rtpjitterbuffer.c
index 393b875..f2a6ec1 100644 (file)
--- a/tests/check/elements/rtpjitterbuffer.c
+++ b/tests/check/elements/rtpjitterbuffer.c
@@ -2042,47 +2042,87 @@ GST_START_TEST (test_deadline_ts_offset)
 
 GST_END_TEST;
 
-GST_START_TEST (test_push_big_gap)
+GST_START_TEST (test_big_gap_seqnum)
 {
   GstHarness *h = gst_harness_new ("rtpjitterbuffer");
-  GstBuffer *buf;
   const gint num_consecutive = 5;
+  const guint gap = 20000;
   gint i;
+  guint seqnum_org;
+  GstClockTime dts_base;
+  guint seqnum_base;
+  guint32 rtpts_base;
+  GstClockTime expected_ts;
 
-  gst_harness_set_src_caps (h, generate_caps ());
+  g_object_set (h->element, "do-lost", TRUE, "do-retransmission", TRUE, NULL);
+  seqnum_org = construct_deterministic_initial_state (h, 100);
 
-  for (i = 0; i < num_consecutive; i++)
-    fail_unless_equals_int (GST_FLOW_OK,
-        gst_harness_push (h, generate_test_buffer (1000 + i)));
+  /* a sudden jump in sequence-numbers (and rtptime), but packets keep arriving
+     at the same pace */
+  dts_base = seqnum_org * TEST_BUF_DURATION;
+  seqnum_base = seqnum_org + gap;
+  rtpts_base = seqnum_base * TEST_RTP_TS_DURATION;
 
-  fail_unless (gst_harness_crank_single_clock_wait (h));
+  for (i = 0; i < num_consecutive; i++) {
+    fail_unless_equals_int (GST_FLOW_OK, gst_harness_push (h,
+            generate_test_buffer_full (dts_base + i * TEST_BUF_DURATION,
+                seqnum_base + i, rtpts_base + i * TEST_RTP_TS_DURATION)));
+  }
 
   for (i = 0; i < num_consecutive; i++) {
     GstBuffer *buf = gst_harness_pull (h);
-    fail_unless_equals_int (1000 + i, get_rtp_seq_num (buf));
+    guint expected_seqnum = seqnum_base + i;
+    fail_unless_equals_int (expected_seqnum, get_rtp_seq_num (buf));
+
+    expected_ts = dts_base + i * TEST_BUF_DURATION;
+    fail_unless_equals_int (expected_ts, GST_BUFFER_PTS (buf));
     gst_buffer_unref (buf);
   }
 
-  /* Push more packets from a different sequence number domain
-   * to trigger "big gap" logic. */
-  for (i = 0; i < num_consecutive; i++)
-    fail_unless_equals_int (GST_FLOW_OK,
-        gst_harness_push (h, generate_test_buffer (20000 + i)));
+  fail_unless_equals_int (0, gst_harness_events_in_queue (h));
 
-  fail_unless (gst_harness_crank_single_clock_wait (h));
+  gst_harness_teardown (h);
+}
+
+GST_END_TEST;
+
+GST_START_TEST (test_big_gap_arrival_time)
+{
+  GstHarness *h = gst_harness_new ("rtpjitterbuffer");
+  const gint num_consecutive = 5;
+  const guint gap = 20000;
+  gint i;
+  guint seqnum_org;
+  GstClockTime dts_base;
+  guint seqnum_base;
+  guint32 rtpts_base;
+  GstClockTime expected_ts;
+
+  g_object_set (h->element, "do-lost", TRUE, "do-retransmission", TRUE, NULL);
+  seqnum_org = construct_deterministic_initial_state (h, 100);
+
+  /* packets are being held back on the wire, then continues */
+  dts_base = (seqnum_org + gap) * TEST_BUF_DURATION;
+  seqnum_base = seqnum_org;
+  rtpts_base = seqnum_base * TEST_RTP_TS_DURATION;
+
+  for (i = 0; i < num_consecutive; i++) {
+    fail_unless_equals_int (GST_FLOW_OK, gst_harness_push (h,
+            generate_test_buffer_full (dts_base + i * TEST_BUF_DURATION,
+                seqnum_base + i, rtpts_base + i * TEST_RTP_TS_DURATION)));
+  }
 
   for (i = 0; i < num_consecutive; i++) {
     GstBuffer *buf = gst_harness_pull (h);
-    fail_unless_equals_int (20000 + i, get_rtp_seq_num (buf));
+    guint expected_seqnum = seqnum_base + i;
+    fail_unless_equals_int (expected_seqnum, get_rtp_seq_num (buf));
+
+    expected_ts = dts_base + i * TEST_BUF_DURATION;
+    fail_unless_equals_int (expected_ts, GST_BUFFER_PTS (buf));
     gst_buffer_unref (buf);
   }
 
-  /* Final buffer should be pushed straight through */
-  fail_unless_equals_int (GST_FLOW_OK,
-      gst_harness_push (h, generate_test_buffer (20000 + num_consecutive)));
-  buf = gst_harness_pull (h);
-  fail_unless_equals_int (20000 + num_consecutive, get_rtp_seq_num (buf));
-  gst_buffer_unref (buf);
+  fail_unless_equals_int (0, gst_harness_events_in_queue (h));
 
   gst_harness_teardown (h);
 }
@@ -2316,7 +2356,8 @@ rtpjitterbuffer_suite (void)
   tcase_add_test (tc_chain, test_rtx_timer_reuse);
 
   tcase_add_test (tc_chain, test_deadline_ts_offset);
-  tcase_add_test (tc_chain, test_push_big_gap);
+  tcase_add_test (tc_chain, test_big_gap_seqnum);
+  tcase_add_test (tc_chain, test_big_gap_arrival_time);
   tcase_add_test (tc_chain, test_fill_queue);
 
   tcase_add_loop_test (tc_chain,