From f39f7d931c11045fe0ba842e4eba9b816f0288ca Mon Sep 17 00:00:00 2001
From: terelius <terelius@webrtc.org>
Date: Wed, 20 Jul 2016 03:36:19 -0700
Subject: [PATCH] Always take retransmissions into account when deciding pacing
 order

Retransmissions are supposed to be sent before normal packets by the pacer, but the current implementation will only use it if the second packet is a retransmission and the first packet is not. It misses the case where the first packet is retransmission and the second packet is not.

This CL fixes the comparator and adds a unit test.

Also changed the SendAndExpectPacket function to propagate the retransmission flag to the expectations. Previously, all packets were expected to be normal packets.

BUG=webrtc:6124

Review-Url: https://codereview.webrtc.org/2156063004
Cr-Commit-Position: refs/heads/master@{#13502}
---
 webrtc/modules/pacing/paced_sender.cc         |  4 +-
 .../modules/pacing/paced_sender_unittest.cc   | 56 ++++++++++++++++++-
 2 files changed, 57 insertions(+), 3 deletions(-)

diff --git a/webrtc/modules/pacing/paced_sender.cc b/webrtc/modules/pacing/paced_sender.cc
index c1741d6902..ae97434340 100644
--- a/webrtc/modules/pacing/paced_sender.cc
+++ b/webrtc/modules/pacing/paced_sender.cc
@@ -76,8 +76,8 @@ struct Comparator {
       return first->priority > second->priority;
 
     // Retransmissions go first.
-    if (second->retransmission && !first->retransmission)
-      return true;
+    if (second->retransmission != first->retransmission)
+      return second->retransmission;
 
     // Older frames have higher prio.
     if (first->capture_time_ms != second->capture_time_ms)
diff --git a/webrtc/modules/pacing/paced_sender_unittest.cc b/webrtc/modules/pacing/paced_sender_unittest.cc
index 51394c72d1..0fbc7cedda 100644
--- a/webrtc/modules/pacing/paced_sender_unittest.cc
+++ b/webrtc/modules/pacing/paced_sender_unittest.cc
@@ -128,7 +128,7 @@ class PacedSenderTest : public ::testing::Test {
     send_bucket_->InsertPacket(priority, ssrc, sequence_number, capture_time_ms,
                                size, retransmission);
     EXPECT_CALL(callback_, TimeToSendPacket(ssrc, sequence_number,
-                                            capture_time_ms, false, _))
+                                            capture_time_ms, retransmission, _))
         .Times(1)
         .WillRepeatedly(Return(true));
   }
@@ -493,6 +493,60 @@ TEST_F(PacedSenderTest, Priority) {
   send_bucket_->Process();
 }
 
+TEST_F(PacedSenderTest, RetransmissionPriority) {
+  uint32_t ssrc = 12345;
+  uint16_t sequence_number = 1234;
+  int64_t capture_time_ms = 45678;
+  int64_t capture_time_ms_retransmission = 56789;
+
+  // Due to the multiplicative factor we can send 5 packets during a send
+  // interval. (network capacity * multiplier / (8 bits per byte *
+  // (packet size * #send intervals per second)
+  const size_t packets_to_send_per_interval =
+      kTargetBitrateBps * PacedSender::kDefaultPaceMultiplier / (8 * 250 * 200);
+  send_bucket_->Process();
+  EXPECT_EQ(0u, send_bucket_->QueueSizePackets());
+
+  // Alternate retransmissions and normal packets.
+  for (size_t i = 0; i < packets_to_send_per_interval; ++i) {
+    send_bucket_->InsertPacket(PacedSender::kNormalPriority, ssrc,
+                               sequence_number++,
+                               capture_time_ms_retransmission, 250, true);
+    send_bucket_->InsertPacket(PacedSender::kNormalPriority, ssrc,
+                               sequence_number++, capture_time_ms, 250, false);
+  }
+  EXPECT_EQ(2 * packets_to_send_per_interval, send_bucket_->QueueSizePackets());
+
+  // Expect all retransmissions to be sent out first despite having a later
+  // capture time.
+  EXPECT_CALL(callback_, TimeToSendPadding(_, _)).Times(0);
+  EXPECT_CALL(callback_, TimeToSendPacket(_, _, _, false, _)).Times(0);
+  EXPECT_CALL(callback_, TimeToSendPacket(
+                             ssrc, _, capture_time_ms_retransmission, true, _))
+      .Times(packets_to_send_per_interval)
+      .WillRepeatedly(Return(true));
+
+  EXPECT_EQ(5, send_bucket_->TimeUntilNextProcess());
+  clock_.AdvanceTimeMilliseconds(5);
+  EXPECT_EQ(0, send_bucket_->TimeUntilNextProcess());
+  send_bucket_->Process();
+  EXPECT_EQ(packets_to_send_per_interval, send_bucket_->QueueSizePackets());
+
+  // Expect the remaining (non-retransmission) packets to be sent.
+  EXPECT_CALL(callback_, TimeToSendPadding(_, _)).Times(0);
+  EXPECT_CALL(callback_, TimeToSendPacket(_, _, _, true, _)).Times(0);
+  EXPECT_CALL(callback_, TimeToSendPacket(ssrc, _, capture_time_ms, false, _))
+      .Times(packets_to_send_per_interval)
+      .WillRepeatedly(Return(true));
+
+  EXPECT_EQ(5, send_bucket_->TimeUntilNextProcess());
+  clock_.AdvanceTimeMilliseconds(5);
+  EXPECT_EQ(0, send_bucket_->TimeUntilNextProcess());
+  send_bucket_->Process();
+
+  EXPECT_EQ(0u, send_bucket_->QueueSizePackets());
+}
+
 TEST_F(PacedSenderTest, HighPrioDoesntAffectBudget) {
   uint32_t ssrc = 12346;
   uint16_t sequence_number = 1234;