rtpjitterbuffer: fix bug in reschedule_timer

The new timeout is always going to be (timeout + delay), however, the
old behavior compared the current timeout to just (timeout), basically
being (delay) off.

This would happen if rtx-delay == rtx-retry-timeout, with the result that
a second rtx attempt for any buffers would be scheduled immediately instead
of after rtx-delay ms.

Simply calculate (new_timeout = timeout + delay) and then use that instead.

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

diff --git a/gst/rtpmanager/gstrtpjitterbuffer.c b/gst/rtpmanager/gstrtpjitterbuffer.c
index e1c315c8c8c77263a63d08d8b734cb052517269b..7add9d5afee5dfc4e23ff3ec32942c1728762dd6 100644 (file)
--- a/gst/rtpmanager/gstrtpjitterbuffer.c
+++ b/gst/rtpmanager/gstrtpjitterbuffer.c
@@ -2156,21 +2156,26 @@ reschedule_timer (GstRtpJitterBuffer * jitterbuffer, TimerData * timer,
   GstRtpJitterBufferPrivate *priv = jitterbuffer->priv;
   gboolean seqchange, timechange;
   guint16 oldseq;
+  GstClockTime new_timeout;
 
-  seqchange = timer->seqnum != seqnum;
-  timechange = timer->timeout != timeout;
+  oldseq = timer->seqnum;
+  new_timeout = timeout + delay;
+  seqchange = oldseq != seqnum;
+  timechange = timer->timeout != new_timeout;
 
-  if (!seqchange && !timechange)
+  if (!seqchange && !timechange) {
+    GST_DEBUG_OBJECT (jitterbuffer,
+        "No changes in seqnum (%d) and timeout (%" GST_TIME_FORMAT
+        "), skipping", oldseq, GST_TIME_ARGS (timer->timeout));
     return;
-
-  oldseq = timer->seqnum;
+  }
 
   GST_DEBUG_OBJECT (jitterbuffer,
       "replace timer %d for seqnum %d->%d timeout %" GST_TIME_FORMAT
       "->%" GST_TIME_FORMAT, timer->type, oldseq, seqnum,
-      GST_TIME_ARGS (timer->timeout), GST_TIME_ARGS (timeout + delay));
+      GST_TIME_ARGS (timer->timeout), GST_TIME_ARGS (new_timeout));
 
-  timer->timeout = timeout + delay;
+  timer->timeout = new_timeout;
   timer->seqnum = seqnum;
   if (reset) {
     GST_DEBUG_OBJECT (jitterbuffer, "reset rtx delay %" GST_TIME_FORMAT

diff --git a/tests/check/elements/rtpjitterbuffer.c b/tests/check/elements/rtpjitterbuffer.c
index 1a9d0e690385e466cc53795a72a92ff08cda3da4..222aa3c92b63b684a6f2a9b618c7645ea07e029f 100644 (file)
--- a/tests/check/elements/rtpjitterbuffer.c
+++ b/tests/check/elements/rtpjitterbuffer.c
@@ -2069,6 +2069,74 @@ GST_START_TEST (test_rtx_no_request_if_time_past_retry_period)
 
 GST_END_TEST;
 
+GST_START_TEST (test_rtx_same_delay_and_retry_timeout)
+{
+  GstHarness *h = gst_harness_new ("rtpjitterbuffer");
+  GstTestClock *testclock;
+  gint latency_ms = 5 * PCMU_BUF_MS;
+  gint num_init_buffers = latency_ms / PCMU_BUF_MS + 1;
+  GstClockTime last_rtx_request;
+
+  testclock = gst_harness_get_testclock (h);
+  gst_harness_set_src_caps (h, generate_caps ());
+  g_object_set (h->element, "do-retransmission", TRUE, "latency", latency_ms,
+      "rtx-max-retries", 3, "rtx-delay", 20, "rtx-retry-timeout", 20, NULL);
+
+  /* Push/pull buffers and advance time past buffer 0's timeout (in order to
+   * simplify the test) */
+  for (gint i = 0; i < num_init_buffers; i++) {
+    gst_test_clock_set_time (testclock, i * PCMU_BUF_DURATION);
+    fail_unless_equals_int (GST_FLOW_OK,
+        gst_harness_push (h, generate_test_buffer (i)));
+    gst_harness_wait_for_clock_id_waits (h, 1, 60);
+  }
+
+  gst_harness_crank_single_clock_wait (h);
+  fail_unless_equals_int64 (latency_ms * GST_MSECOND,
+      gst_clock_get_time (GST_CLOCK (testclock)));
+
+  for (gint i = 0; i < num_init_buffers; i++)
+    gst_buffer_unref (gst_harness_pull (h));
+
+  /* drop reconfigure event */
+  gst_event_unref (gst_harness_pull_upstream_event (h));
+
+  /* Crank clock to send retransmission events requesting seqnum 6 which has
+   * not arrived yet. */
+  gst_harness_crank_single_clock_wait (h);
+  verify_rtx_event (gst_harness_pull_upstream_event (h),
+      6, 6 * PCMU_BUF_DURATION, 20, PCMU_BUF_DURATION);
+  /* first rtx for seqnum 6 should arrive at 140ms */
+  last_rtx_request = gst_clock_get_time (GST_CLOCK (testclock));
+  fail_unless_equals_int64 (last_rtx_request, 140 * GST_MSECOND);
+
+  /* verify we have pulled out all rtx-events */
+  fail_unless_equals_int (0, gst_harness_upstream_events_in_queue (h));
+
+  /* now crank to get the second attempt at seqnum 6 */
+  gst_harness_crank_single_clock_wait (h);
+  verify_rtx_event (gst_harness_pull_upstream_event (h),
+      6, 6 * PCMU_BUF_DURATION, 40, PCMU_BUF_DURATION);
+  /* second rtx for seqnum 6 should arrive at 140 + 20ms */
+  last_rtx_request = gst_clock_get_time (GST_CLOCK (testclock));
+  fail_unless_equals_int64 (last_rtx_request, 160 * GST_MSECOND);
+
+  /* verify we have pulled out all rtx-events */
+  fail_unless_equals_int (0, gst_harness_upstream_events_in_queue (h));
+
+  fail_unless (verify_jb_stats (h->element,
+          gst_structure_new ("application/x-rtp-jitterbuffer-stats",
+              "num-pushed", G_TYPE_UINT64, (guint64) num_init_buffers,
+              "num-lost", G_TYPE_UINT64, (guint64) 0,
+              "rtx-count", G_TYPE_UINT64, (guint64) 2, NULL)));
+
+  gst_object_unref (testclock);
+  gst_harness_teardown (h);
+}
+
+GST_END_TEST;
+
+
 GST_START_TEST (test_gap_exceeds_latency)
 {
   GstHarness *h = gst_harness_new ("rtpjitterbuffer");
@@ -2506,6 +2574,7 @@ rtpjitterbuffer_suite (void)
       test_rtx_buffer_arrives_after_lost_updates_rtx_stats);
   tcase_add_test (tc_chain, test_rtx_rtt_larger_than_retry_timeout);
   tcase_add_test (tc_chain, test_rtx_no_request_if_time_past_retry_period);
+  tcase_add_test (tc_chain, test_rtx_same_delay_and_retry_timeout);
 
   tcase_add_test (tc_chain, test_gap_exceeds_latency);
   tcase_add_test (tc_chain, test_deadline_ts_offset);