Use pacing info in ProbeBitrateEstimator to validate probe results.

webrtc/modules/congestion_controller/probe_bitrate_estimator_unittest.cc

Index: webrtc/modules/congestion_controller/probe_bitrate_estimator_unittest.cc
diff --git a/webrtc/modules/congestion_controller/probe_bitrate_estimator_unittest.cc b/webrtc/modules/congestion_controller/probe_bitrate_estimator_unittest.cc
index ea8413c084b88e13021c8dec839b9271e6b6183e..c25366f74d2a6f3f003d86601254908718486ea8 100644
--- a/webrtc/modules/congestion_controller/probe_bitrate_estimator_unittest.cc
+++ b/webrtc/modules/congestion_controller/probe_bitrate_estimator_unittest.cc
@@ -19,7 +19,11 @@
 
 namespace webrtc {
 
-constexpr int INVALID_BPS = -1;
+namespace {
+constexpr int kInvalidBitrate = -1;
+constexpr int kDefaultMinProbes = 5;
+constexpr int kDefaultMinBytes = 5000;
+}  // anonymous namespace
 
 class TestProbeBitrateEstimator : public ::testing::Test {
  public:
@@ -30,15 +34,17 @@ class TestProbeBitrateEstimator : public ::testing::Test {
   void AddPacketFeedback(int probe_cluster_id,
                          size_t size_bytes,
                          int64_t send_time_ms,
-                         int64_t arrival_time_ms) {
-    PacketInfo info(arrival_time_ms, send_time_ms, 0, size_bytes,
-                    PacedPacketInfo(probe_cluster_id, -1, -1));
+                         int64_t arrival_time_ms,
+                         int min_probes = kDefaultMinProbes,
+                         int min_bytes = kDefaultMinBytes) {
+    PacedPacketInfo pacing_info(probe_cluster_id, min_probes, min_bytes);
+    PacketInfo info(arrival_time_ms, send_time_ms, 0, size_bytes, pacing_info);
     measured_bps_ =
         probe_bitrate_estimator_.HandleProbeAndEstimateBitrate(info);
   }
 
  protected:
-  int measured_bps_ = INVALID_BPS;
+  int measured_bps_ = kInvalidBitrate;
   ProbeBitrateEstimator probe_bitrate_estimator_;
 };
 
@@ -51,6 +57,49 @@ TEST_F(TestProbeBitrateEstimator, OneCluster) {
   EXPECT_NEAR(measured_bps_, 800000, 10);
 }
 
+TEST_F(TestProbeBitrateEstimator, OneClusterTooFewProbes) {
+  AddPacketFeedback(0, 1400, 0, 10);
+  AddPacketFeedback(0, 1400, 10, 20);
+  AddPacketFeedback(0, 1400, 20, 30);

[terelius, 2017/03/08 15:55:11]

Could we make this test send 5000 bytes to make it clear that we're really
failing the probe count and not the bytes count limit?

[philipel, 2017/03/09 11:47:41]

Done.

+
+  EXPECT_EQ(kInvalidBitrate, measured_bps_);
+}
+
+TEST_F(TestProbeBitrateEstimator, OneClusterTooFewBytes) {

[terelius, 2017/03/08 15:55:11]

Could we make this send 5 packets to make it clear that we fail the byte count
and not the probe count limit.

[philipel, 2017/03/09 11:47:41]

Done.

+  const int kMinBytes = 6000;
+  AddPacketFeedback(0, 1000, 0, 10, kDefaultMinProbes, kMinBytes);
+  AddPacketFeedback(0, 1000, 10, 20, kDefaultMinProbes, kMinBytes);
+  AddPacketFeedback(0, 1000, 20, 30, kDefaultMinProbes, kMinBytes);
+  AddPacketFeedback(0, 1000, 30, 40, kDefaultMinProbes, kMinBytes);
+
+  EXPECT_EQ(kInvalidBitrate, measured_bps_);
+}
+
+TEST_F(TestProbeBitrateEstimator, SmallCluster) {
+  const int kMinBytes = 1000;
+  AddPacketFeedback(0, 150, 0, 10, kDefaultMinProbes, kMinBytes);
+  AddPacketFeedback(0, 150, 10, 20, kDefaultMinProbes, kMinBytes);
+  AddPacketFeedback(0, 150, 20, 30, kDefaultMinProbes, kMinBytes);
+  AddPacketFeedback(0, 150, 30, 40, kDefaultMinProbes, kMinBytes);
+  AddPacketFeedback(0, 150, 40, 50, kDefaultMinProbes, kMinBytes);
+  AddPacketFeedback(0, 150, 50, 60, kDefaultMinProbes, kMinBytes);
+  EXPECT_NEAR(measured_bps_, 120000, 10);
+}
+
+TEST_F(TestProbeBitrateEstimator, LargeCluster) {
+  const int kMinProbes = 30;
+  const int kMinBytes = 312500;
+
+  int64_t send_time = 0;
+  int64_t receive_time = 5;
+  for (int i = 0; i < 25; ++i) {
+    AddPacketFeedback(0, 12500, send_time, receive_time, kMinProbes, kMinBytes);
+    ++send_time;
+    ++receive_time;
+  }
+  EXPECT_NEAR(measured_bps_, 100000000, 10);
+}
+
 TEST_F(TestProbeBitrateEstimator, FastReceive) {
   AddPacketFeedback(0, 1000, 0, 15);
   AddPacketFeedback(0, 1000, 10, 30);
@@ -66,7 +115,7 @@ TEST_F(TestProbeBitrateEstimator, TooFastReceive) {
   AddPacketFeedback(0, 1000, 20, 25);
   AddPacketFeedback(0, 1000, 40, 27);
 
-  EXPECT_EQ(measured_bps_, INVALID_BPS);
+  EXPECT_EQ(kInvalidBitrate, measured_bps_);

[terelius, 2017/03/08 15:55:11]

Is there a reason for switching order here? Or, more generally, do you prefer
the expected value as first of second argument? Currently there is a mix in this
file. (The style recommendation used to be EXPECT_EQ(expected, actual), but that
is no longer required.)

[philipel, 2017/03/09 11:47:41]

No reason, fixed :)

 }
 
 TEST_F(TestProbeBitrateEstimator, SlowReceive) {
@@ -84,7 +133,7 @@ TEST_F(TestProbeBitrateEstimator, BurstReceive) {
   AddPacketFeedback(0, 1000, 20, 50);
   AddPacketFeedback(0, 1000, 40, 50);
 
-  EXPECT_EQ(measured_bps_, INVALID_BPS);
+  EXPECT_EQ(measured_bps_, kInvalidBitrate);
 }
 
 TEST_F(TestProbeBitrateEstimator, MultipleClusters) {
@@ -92,11 +141,9 @@ TEST_F(TestProbeBitrateEstimator, MultipleClusters) {
   AddPacketFeedback(0, 1000, 10, 20);
   AddPacketFeedback(0, 1000, 20, 30);
   AddPacketFeedback(0, 1000, 40, 60);
-
   EXPECT_NEAR(measured_bps_, 480000, 10);
 
   AddPacketFeedback(0, 1000, 50, 60);
-
   EXPECT_NEAR(measured_bps_, 640000, 10);
 
   AddPacketFeedback(1, 1000, 60, 70);
@@ -122,7 +169,7 @@ TEST_F(TestProbeBitrateEstimator, IgnoreOldClusters) {
   // Coming in 6s later
   AddPacketFeedback(0, 1000, 40 + 6000, 60 + 6000);
 
-  EXPECT_EQ(measured_bps_, INVALID_BPS);
+  EXPECT_EQ(measured_bps_, kInvalidBitrate);
 }
 
 TEST_F(TestProbeBitrateEstimator, IgnoreSizeLastSendPacket) {