BitrateProber: Support higher probing bitrates

webrtc/modules/pacing/paced_sender_unittest.cc

Index: webrtc/modules/pacing/paced_sender_unittest.cc
diff --git a/webrtc/modules/pacing/paced_sender_unittest.cc b/webrtc/modules/pacing/paced_sender_unittest.cc
index 861452567f8d018a070ee8987a290bffdac3d54b..c4f34ef69e4de8a8f5ff2ae6bd729e2765d876b0 100644
--- a/webrtc/modules/pacing/paced_sender_unittest.cc
+++ b/webrtc/modules/pacing/paced_sender_unittest.cc
@@ -19,6 +19,18 @@
 using testing::_;
 using testing::Return;
 
+namespace {
+constexpr unsigned kFirstClusterBps = 900000;
+constexpr int kFirstClusterCount = 6;
+constexpr unsigned kSecondClusterBps = 1800000;
+constexpr int kSecondClusterCount = 5;
+
+// The error stems from truncating the time interval of probe packets to integer
+// values. This results in probing slightly higher than the target bitrate.
+// For 1.8 Mbps, this comes to be about 120 kbps with 1200 probe packets.
+constexpr int kBitrateProbingError = 150000;
+}  // namespace
+
 namespace webrtc {
 namespace test {
 
@@ -62,46 +74,29 @@ class PacedSenderPadding : public PacedSender::PacketSender {
 
 class PacedSenderProbing : public PacedSender::PacketSender {
  public:
-  PacedSenderProbing(const std::list<int>& expected_deltas, Clock* clock)
-      : prev_packet_time_ms_(-1),
-        expected_deltas_(expected_deltas),
-        packets_sent_(0),
-        clock_(clock) {}
+  PacedSenderProbing() : packets_sent_(0), padding_sent_(0) {}
 
   bool TimeToSendPacket(uint32_t ssrc,
                         uint16_t sequence_number,
                         int64_t capture_time_ms,
                         bool retransmission,
                         int probe_cluster_id) override {
-    ExpectAndCountPacket();
+    packets_sent_++;
     return true;
   }
 
   size_t TimeToSendPadding(size_t bytes, int probe_cluster_id) override {
-    ExpectAndCountPacket();
-    return bytes;
-  }
-
-  void ExpectAndCountPacket() {
-    ++packets_sent_;
-    EXPECT_FALSE(expected_deltas_.empty());
-    if (expected_deltas_.empty())
-      return;
-    int64_t now_ms = clock_->TimeInMilliseconds();
-    if (prev_packet_time_ms_ >= 0) {
-      EXPECT_EQ(expected_deltas_.front(), now_ms - prev_packet_time_ms_);
-      expected_deltas_.pop_front();
-    }
-    prev_packet_time_ms_ = now_ms;
+    padding_sent_ += bytes;
+    return padding_sent_;
   }
 
   int packets_sent() const { return packets_sent_; }
 
+  int padding_sent() const { return padding_sent_; }
+
  private:
-  int64_t prev_packet_time_ms_;
-  std::list<int> expected_deltas_;
   int packets_sent_;
-  Clock* clock_;
+  int padding_sent_;
 };
 
 class PacedSenderTest : public ::testing::Test {
@@ -110,8 +105,8 @@ class PacedSenderTest : public ::testing::Test {
     srand(0);
     // Need to initialize PacedSender after we initialize clock.
     send_bucket_.reset(new PacedSender(&clock_, &callback_));
-    send_bucket_->CreateProbeCluster(900000, 6);
-    send_bucket_->CreateProbeCluster(1800000, 5);
+    send_bucket_->CreateProbeCluster(kFirstClusterBps, kFirstClusterCount);
+    send_bucket_->CreateProbeCluster(kSecondClusterBps, kSecondClusterCount);
     // Default to bitrate probing disabled for testing purposes. Probing tests
     // have to enable probing, either by creating a new PacedSender instance or
     // by calling SetProbingEnabled(true).
@@ -803,74 +798,118 @@ TEST_F(PacedSenderTest, QueueTimeGrowsOverTime) {
   EXPECT_EQ(0, send_bucket_->QueueInMs());
 }
 
-TEST_F(PacedSenderTest, ProbingWithInitialFrame) {
-  const int kNumPackets = 11;
-  const int kNumDeltas = kNumPackets - 1;
+TEST_F(PacedSenderTest, ProbingWithInsertedPackets) {
   const size_t kPacketSize = 1200;
   const int kInitialBitrateBps = 300000;
   uint32_t ssrc = 12346;
   uint16_t sequence_number = 1234;
 
-  const int expected_deltas[kNumDeltas] = {10, 10, 10, 10, 10, 5, 5, 5, 5, 5};
-  std::list<int> expected_deltas_list(expected_deltas,
-                                      expected_deltas + kNumDeltas);
-  PacedSenderProbing callback(expected_deltas_list, &clock_);
-  send_bucket_.reset(new PacedSender(&clock_, &callback));
-  send_bucket_->CreateProbeCluster(900000, 6);
-  send_bucket_->CreateProbeCluster(1800000, 5);
+  PacedSenderProbing packet_sender;
+  send_bucket_.reset(new PacedSender(&clock_, &packet_sender));
+  send_bucket_->CreateProbeCluster(kFirstClusterBps, kFirstClusterCount);
+  send_bucket_->CreateProbeCluster(kSecondClusterBps, kSecondClusterCount);
   send_bucket_->SetEstimatedBitrate(kInitialBitrateBps);
 
-  for (int i = 0; i < kNumPackets; ++i) {
+  for (int i = 0; i < kFirstClusterCount + kSecondClusterCount; ++i) {
     send_bucket_->InsertPacket(PacedSender::kNormalPriority, ssrc,
                                sequence_number++, clock_.TimeInMilliseconds(),
                                kPacketSize, false);
   }
 
-  while (callback.packets_sent() < kNumPackets) {
+  int64_t start = clock_.TimeInMilliseconds();
+  while (packet_sender.packets_sent() < kFirstClusterCount) {
     int time_until_process = send_bucket_->TimeUntilNextProcess();
-    if (time_until_process <= 0) {
-      send_bucket_->Process();
-    } else {
-      clock_.AdvanceTimeMilliseconds(time_until_process);
-    }
+    clock_.AdvanceTimeMilliseconds(time_until_process);
+    send_bucket_->Process();
+  }
+  int packets_sent = packet_sender.packets_sent();
+  // Validate first cluster bitrate. Note that we have to account for number
+  // of intervals and hence (packets_sent - 1) on the first cluster.
+  EXPECT_NEAR((packets_sent - 1) * kPacketSize * 8000 /
+                  (clock_.TimeInMilliseconds() - start),
+              kFirstClusterBps, kBitrateProbingError);
+
+  start = clock_.TimeInMilliseconds();
+  while (packet_sender.packets_sent() <
+         kFirstClusterCount + kSecondClusterCount) {
+    int time_until_process = send_bucket_->TimeUntilNextProcess();
+    clock_.AdvanceTimeMilliseconds(time_until_process);
+    send_bucket_->Process();
   }
+  packets_sent = packet_sender.packets_sent() - packets_sent;
+  // Validate second cluster bitrate.
+  EXPECT_NEAR(
+      packets_sent * kPacketSize * 8000 / (clock_.TimeInMilliseconds() - start),
+      kSecondClusterBps, kBitrateProbingError);
 }
 
-TEST_F(PacedSenderTest, ProbingWithTooSmallInitialFrame) {
-  const int kNumPackets = 11;
-  const int kNumDeltas = kNumPackets - 1;
+TEST_F(PacedSenderTest, ProbingWithPaddingSupport) {
   const size_t kPacketSize = 1200;
   const int kInitialBitrateBps = 300000;
   uint32_t ssrc = 12346;
   uint16_t sequence_number = 1234;
-  const int expected_deltas[kNumDeltas] = {10, 10, 10, 10, 10, 5, 5, 5, 5, 5};
-  std::list<int> expected_deltas_list(expected_deltas,
-                                      expected_deltas + kNumDeltas);
-  PacedSenderProbing callback(expected_deltas_list, &clock_);
-  send_bucket_.reset(new PacedSender(&clock_, &callback));
-  send_bucket_->CreateProbeCluster(900000, 6);
-  send_bucket_->CreateProbeCluster(1800000, 5);
+
+  PacedSenderProbing packet_sender;
+  send_bucket_.reset(new PacedSender(&clock_, &packet_sender));
+  send_bucket_->CreateProbeCluster(kFirstClusterBps, kFirstClusterCount);
   send_bucket_->SetEstimatedBitrate(kInitialBitrateBps);
 
-  for (int i = 0; i < kNumPackets - 5; ++i) {
+  for (int i = 0; i < kFirstClusterCount - 2; ++i) {
     send_bucket_->InsertPacket(PacedSender::kNormalPriority, ssrc,
                                sequence_number++, clock_.TimeInMilliseconds(),
                                kPacketSize, false);
   }
-  while (callback.packets_sent() < kNumPackets) {
+
+  int64_t start = clock_.TimeInMilliseconds();
+  int process_count = 0;
+  while (process_count < kFirstClusterCount) {
     int time_until_process = send_bucket_->TimeUntilNextProcess();
-    if (time_until_process <= 0) {
-      send_bucket_->Process();
-    } else {
-      clock_.AdvanceTimeMilliseconds(time_until_process);
-    }
+    clock_.AdvanceTimeMilliseconds(time_until_process);
+    send_bucket_->Process();
+    process_count++;

[stefan-webrtc, 2016/09/23 13:29:11]

++process_count;

[Irfan, 2016/09/26 23:00:29]

Done.

   }
+  int packets_sent = packet_sender.packets_sent();
+  int padding_sent = packet_sender.padding_sent();
+  EXPECT_GT(packets_sent, 0);
+  EXPECT_GT(padding_sent, 0);
+  // Note that the number of intervals here for kPacketSize is
+  // packets_sent due to padding in the same cluster.
+  EXPECT_NEAR((packets_sent * kPacketSize * 8000 + padding_sent) /
+                  (clock_.TimeInMilliseconds() - start),
+              kFirstClusterBps, kBitrateProbingError);
+}
 
-  // Process one more time and make sure we don't send any more probes.
-  int time_until_process = send_bucket_->TimeUntilNextProcess();
-  clock_.AdvanceTimeMilliseconds(time_until_process);
-  send_bucket_->Process();
-  EXPECT_EQ(kNumPackets, callback.packets_sent());
+TEST_F(PacedSenderTest, ProbingUsesRetransmissions) {

[stefan-webrtc, 2016/09/23 13:29:11]

This was not the test I was after in my earlier comments.

I'd like to have an integration test which verifies that probing doesn't send a
lot of padding in response to calls to TimeToSendPadding(). I simply want to
ensure that, if we do probing at high rates, we don't end up with a huge number
of padding packets for some reason. I think they should all be preemptive
retransmits.

This is not possible to test with the pacer alone, so I'd suggest implementing
it in video_send_stream_tests.cc.

[Irfan, 2016/09/26 23:00:29]

I have added a test that measures the padding overhead from an end-to-end
perspective.

The reality is that when there are no packets that are outstanding for
retransmission, probing will end up sending padded packets for measuring
bitrate. I see about 40% padding for 10 Mbps ramp in the test. This will really
depend on how many retransmissions or packets to send are available.

The good news here is that padding is only sent when we know there is bitrate
available on network and when there is nothing to send to avail that. We also
only send a fixed amount of padding in terms of time (so more bits at higher
bitrate).

[stefan-webrtc, 2016/09/27 14:18:32]

Right, that's expected in some situations. But in most situations there should
be video packets available for preemptive retransmission, right? Is that not the
case in your new test for some reason?

We don't send padding packets to probe before we've sent our first video packet,
right?

+  const size_t kPacketSize = 1200;
+  const int kInitialBitrateBps = 300000;
+  uint32_t ssrc = 12346;
+  uint16_t sequence_number = 1234;
+
+  PacedSenderProbing packet_sender;
+  send_bucket_.reset(new PacedSender(&clock_, &packet_sender));
+  send_bucket_->CreateProbeCluster(kFirstClusterBps, kFirstClusterCount);
+  send_bucket_->SetEstimatedBitrate(kInitialBitrateBps);
+
+  // All packets inserted are retransmissions.
+  for (int i = 0; i < kFirstClusterCount; ++i) {
+    send_bucket_->InsertPacket(PacedSender::kNormalPriority, ssrc,
+                               sequence_number++, clock_.TimeInMilliseconds(),
+                               kPacketSize, true);
+  }
+
+  int64_t start = clock_.TimeInMilliseconds();
+  while (packet_sender.packets_sent() < kFirstClusterCount) {
+    int time_until_process = send_bucket_->TimeUntilNextProcess();
+    clock_.AdvanceTimeMilliseconds(time_until_process);
+    send_bucket_->Process();
+  }
+  int packets_sent = packet_sender.packets_sent();
+  int padding_sent = packet_sender.padding_sent();
+  EXPECT_EQ(padding_sent, 0);
+  // Validate first cluster bitrate.
+  EXPECT_NEAR(((packets_sent - 1) * kPacketSize * 8000) /
+                  (clock_.TimeInMilliseconds() - start),
+              kFirstClusterBps, kBitrateProbingError);
 }
 
 TEST_F(PacedSenderTest, PriorityInversion) {