Use |probe_cluster_id| to cluster packets.

webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc

Index: webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc
diff --git a/webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc b/webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..a68ec4fd7b9269a2754072612c2cef45afd88582
--- /dev/null
+++ b/webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc
@@ -0,0 +1,242 @@
+/*
+ *  Copyright (c) 2016 The WebRTC project authors. All Rights Reserved.
+ *
+ *  Use of this source code is governed by a BSD-style license
+ *  that can be found in the LICENSE file in the root of the source
+ *  tree. An additional intellectual property rights grant can be found
+ *  in the file PATENTS.  All contributing project authors may
+ *  be found in the AUTHORS file in the root of the source tree.
+ */
+
+#include "webrtc/modules/congestion_controller/delay_based_bwe.h"
+
+#include "testing/gtest/include/gtest/gtest.h"
+#include "webrtc/system_wrappers/include/clock.h"
+
+namespace webrtc {
+
+class TestDelayBasedBwe : public ::testing::Test, public RemoteBitrateObserver {

[danilchap, 2016/06/08 13:20:07]

DelayBasedBweTest

[philipel, 2016/06/08 14:34:40]

Hmm... I usually use this format. Doing 'git grep "TEST_.(Test" | wc' gives 345
lines and 'git grep -E "TEST_.\(.+Test\)" | wc' gives 90 lines, so I don't think
it is uncommon to use Test as the prefix.

[danilchap, 2016/06/08 14:51:05]

it also good idea to do a local style search: both
RemoteBitrateEstimatorAbsSendTime you copy tests from and neighbor
CongestionController unittests use Test as suffix, not prefix.

+ public:
+  static constexpr int kArrivalTimeClockOffsetMs = 60000;
+
+  TestDelayBasedBwe()
+      : bwe_(this), clock_(0), bitrate_updated_(false), latest_bitrate_(0) {}
+
+  uint32_t AbsSendTime(int64_t t, int64_t denom) {
+    return (((t << 18) + (denom >> 1)) / denom) & 0x00fffffful;
+  }
+
+  void IncomingPacket(uint32_t ssrc,
+                      size_t payload_size,
+                      int64_t arrival_time,
+                      uint32_t rtp_timestamp,
+                      uint32_t absolute_send_time,
+                      bool was_paced,
+                      int probe_cluster_id) {
+    RTPHeader header;
+    memset(&header, 0, sizeof(header));
+    header.ssrc = ssrc;
+    header.timestamp = rtp_timestamp;
+    header.extension.hasAbsoluteSendTime = true;
+    header.extension.absoluteSendTime = absolute_send_time;
+    bwe_.IncomingPacket(arrival_time + kArrivalTimeClockOffsetMs, payload_size,
+                        header, was_paced, probe_cluster_id);
+  }
+
+  void OnReceiveBitrateChanged(const std::vector<uint32_t>& ssrcs,
+                               uint32_t bitrate) {
+    bitrate_updated_ = true;
+    latest_bitrate_ = bitrate;
+  }
+
+  bool bitrate_updated() {
+    bool res = bitrate_updated_;
+    bitrate_updated_ = false;
+    return res;
+  }
+
+  int latest_bitrate() { return latest_bitrate_; }
+
+  DelayBasedBwe bwe_;
+  SimulatedClock clock_;
+
+ private:
+  bool bitrate_updated_;
+  int latest_bitrate_;
+};
+
+TEST_F(TestDelayBasedBwe, TestProbeDetection) {
+  const int kProbeLength = 5;

[danilchap, 2016/06/08 13:20:07]

constexpr
may be name it kNumberOfProbes (ProbeLength gives false impression it's a length
of something, e.g. may be understood as size of a probe in bytes)
this constant appear in every test, may be move it into the test class

[philipel, 2016/06/08 14:34:40]

Done.

+  int64_t now_ms = clock_.TimeInMilliseconds();
+
+  // First burst sent at 8 * 1000 / 10 = 800 kbps.
+  for (int i = 0; i < kProbeLength; ++i) {
+    clock_.AdvanceTimeMilliseconds(10);
+    now_ms = clock_.TimeInMilliseconds();
+    IncomingPacket(0, 1000, now_ms, 90 * now_ms, AbsSendTime(now_ms, 1000),
+                   true, 0);
+  }
+  EXPECT_TRUE(bitrate_updated());
+
+  // Second burst sent at 8 * 1000 / 5 = 1600 kbps.
+  for (int i = 0; i < kProbeLength; ++i) {
+    clock_.AdvanceTimeMilliseconds(5);
+    now_ms = clock_.TimeInMilliseconds();
+    IncomingPacket(0, 1000, now_ms, 90 * now_ms, AbsSendTime(now_ms, 1000),
+                   true, 1);
+  }
+
+  EXPECT_TRUE(bitrate_updated());
+  EXPECT_GT(latest_bitrate(), 1500000);
+}
+
+TEST_F(TestDelayBasedBwe, TestProbeDetectionNonPacedPackets) {
+  const int kProbeLength = 5;
+  int64_t now_ms = clock_.TimeInMilliseconds();
+  // First burst sent at 8 * 1000 / 10 = 800 kbps, but with every other packet
+  // not being paced which could mess things up.
+  for (int i = 0; i < kProbeLength; ++i) {
+    clock_.AdvanceTimeMilliseconds(5);
+    now_ms = clock_.TimeInMilliseconds();
+    IncomingPacket(0, 1000, now_ms, 90 * now_ms, AbsSendTime(now_ms, 1000),
+                   true, 0);
+    // Non-paced packet, arriving 5 ms after.
+    clock_.AdvanceTimeMilliseconds(5);
+    IncomingPacket(0, 100, now_ms, 90 * now_ms, AbsSendTime(now_ms, 1000),

[danilchap, 2016/06/08 13:20:07]

may be use regular size packets for this test. (since there is a dedicated test
that small-size packets are not used for probing)

[philipel, 2016/06/08 14:34:39]

Done.

+                   false, PacketInfo::kNotAProbe);
+  }
+
+  EXPECT_TRUE(bitrate_updated());
+  EXPECT_GT(latest_bitrate(), 800000);
+}
+
+// Packets will require 5 ms to be transmitted to the receiver, causing packets
+// of the second probe to be dispersed.
+TEST_F(TestDelayBasedBwe, TestProbeDetectionTooHighBitrate) {
+  const int kProbeLength = 5;
+  int64_t now_ms = clock_.TimeInMilliseconds();
+  int64_t send_time_ms = 0;
+  // First burst sent at 8 * 1000 / 10 = 800 kbps.
+  for (int i = 0; i < kProbeLength; ++i) {
+    clock_.AdvanceTimeMilliseconds(10);
+    now_ms = clock_.TimeInMilliseconds();
+    send_time_ms += 10;
+    IncomingPacket(0, 1000, now_ms, 90 * send_time_ms,
+                   AbsSendTime(send_time_ms, 1000), true, 0);
+  }
+
+  // Second burst sent at 8 * 1000 / 5 = 1600 kbps, arriving at 8 * 1000 / 8 =
+  // 1000 kbps.
+  for (int i = 0; i < kProbeLength; ++i) {
+    clock_.AdvanceTimeMilliseconds(8);
+    now_ms = clock_.TimeInMilliseconds();
+    send_time_ms += 5;
+    IncomingPacket(0, 1000, now_ms, send_time_ms,
+                   AbsSendTime(send_time_ms, 1000), true, 1);
+  }
+
+  EXPECT_TRUE(bitrate_updated());
+  EXPECT_NEAR(latest_bitrate(), 800000, 10000);
+}
+
+TEST_F(TestDelayBasedBwe, TestProbeDetectionSlightlyFasterArrival) {
+  const int kProbeLength = 5;
+  int64_t now_ms = clock_.TimeInMilliseconds();
+  // First burst sent at 8 * 1000 / 10 = 800 kbps.
+  // Arriving at 8 * 1000 / 5 = 1600 kbps.
+  int64_t send_time_ms = 0;
+  for (int i = 0; i < kProbeLength; ++i) {
+    clock_.AdvanceTimeMilliseconds(5);
+    send_time_ms += 10;
+    now_ms = clock_.TimeInMilliseconds();
+    IncomingPacket(0, 1000, now_ms, 90 * send_time_ms,
+                   AbsSendTime(send_time_ms, 1000), true, 23);
+  }
+
+  EXPECT_TRUE(bitrate_updated());
+  EXPECT_GT(latest_bitrate(), 800000);
+}
+
+TEST_F(TestDelayBasedBwe, TestProbeDetectionFasterArrival) {
+  const int kProbeLength = 5;
+  int64_t now_ms = clock_.TimeInMilliseconds();
+  // First burst sent at 8 * 1000 / 10 = 800 kbps.
+  // Arriving at 8 * 1000 / 5 = 1600 kbps.
+  int64_t send_time_ms = 0;
+  for (int i = 0; i < kProbeLength; ++i) {
+    clock_.AdvanceTimeMilliseconds(1);
+    send_time_ms += 10;
+    now_ms = clock_.TimeInMilliseconds();
+    IncomingPacket(0, 1000, now_ms, 90 * send_time_ms,
+                   AbsSendTime(send_time_ms, 1000), true, 0);
+  }
+
+  EXPECT_FALSE(bitrate_updated());
+}
+
+TEST_F(TestDelayBasedBwe, TestProbeDetectionSlowerArrival) {
+  const int kProbeLength = 5;
+  int64_t now_ms = clock_.TimeInMilliseconds();
+  // First burst sent at 8 * 1000 / 5 = 1600 kbps.
+  // Arriving at 8 * 1000 / 7 = 1142 kbps.
+  int64_t send_time_ms = 0;
+  for (int i = 0; i < kProbeLength; ++i) {
+    clock_.AdvanceTimeMilliseconds(7);
+    send_time_ms += 5;
+    now_ms = clock_.TimeInMilliseconds();
+    IncomingPacket(0, 1000, now_ms, 90 * send_time_ms,
+                   AbsSendTime(send_time_ms, 1000), true, 1);
+  }
+
+  EXPECT_TRUE(bitrate_updated());
+  EXPECT_NEAR(latest_bitrate(), 1140000, 10000);
+}
+
+TEST_F(TestDelayBasedBwe, TestProbeDetectionSlowerArrivalHighBitrate) {

[danilchap, 2016/06/08 13:20:08]

Either all tests should start with word 'Test' (like this test) or none (like
next test), preferably the latter.

[philipel, 2016/06/08 14:34:40]

Done.

+  const int kProbeLength = 5;
+  int64_t now_ms = clock_.TimeInMilliseconds();
+  // Burst sent at 8 * 1000 / 1 = 8000 kbps.
+  // Arriving at 8 * 1000 / 2 = 4000 kbps.
+  int64_t send_time_ms = 0;
+  for (int i = 0; i < kProbeLength; ++i) {
+    clock_.AdvanceTimeMilliseconds(2);
+    send_time_ms += 1;
+    now_ms = clock_.TimeInMilliseconds();
+    IncomingPacket(0, 1000, now_ms, 90 * send_time_ms,
+                   AbsSendTime(send_time_ms, 1000), true, 1);
+  }
+
+  EXPECT_TRUE(bitrate_updated());
+  EXPECT_NEAR(latest_bitrate(), 4000000u, 10000);
+}
+
+TEST_F(TestDelayBasedBwe, ProbingIgnoresSmallPackets) {
+  const int kProbeLength = 5;
+  int64_t now_ms = clock_.TimeInMilliseconds();
+  // Probing with 200 bytes every 10 ms, should be ignored by the probe
+  // detection.
+  for (int i = 0; i < kProbeLength; ++i) {
+    clock_.AdvanceTimeMilliseconds(10);
+    now_ms = clock_.TimeInMilliseconds();
+    IncomingPacket(0, 200, now_ms, 90 * now_ms, AbsSendTime(now_ms, 1000), true,

[danilchap, 2016/06/08 13:20:08]

may be better use PacedSender::kMinProbePacketSize, instead of magical number
200

[philipel, 2016/06/08 14:34:39]

Done.

+                   1);
+  }
+
+  EXPECT_FALSE(bitrate_updated());
+
+  // Followed by a probe with 1000 bytes packets, should be detected as a
+  // probe.
+  for (int i = 0; i < kProbeLength; ++i) {
+    clock_.AdvanceTimeMilliseconds(10);
+    now_ms = clock_.TimeInMilliseconds();
+    IncomingPacket(0, 1000, now_ms, 90 * now_ms, AbsSendTime(now_ms, 1000),

[danilchap, 2016/06/08 13:20:07]

May be use (kMinProbePacketSize+1) instead of 1000 (and adjust expected bitrate
accordingly)

[philipel, 2016/06/08 14:34:40]

I agree that is a good idea, but these tests are kind of temporary. The
DelayBasedBwe class will change more and more and I think at that point it is a
good idea to clean/implement more tests.

+                   true, 1);
+  }
+
+  // Wait long enough so that we can call Process again.
+  clock_.AdvanceTimeMilliseconds(1000);
+
+  EXPECT_TRUE(bitrate_updated());
+  EXPECT_NEAR(latest_bitrate(), 800000u, 10000);
+}
+}  // namespace webrtc