Test and fix for huge bwe drop after alr state.

webrtc/modules/congestion_controller/acknowledged_bitrate_estimator_unittest.cc

Index: webrtc/modules/congestion_controller/acknowledged_bitrate_estimator_unittest.cc
diff --git a/webrtc/modules/congestion_controller/acknowledged_bitrate_estimator_unittest.cc b/webrtc/modules/congestion_controller/acknowledged_bitrate_estimator_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..6453b27768f511255b5d21b2cc5e93bd07c035fa
--- /dev/null
+++ b/webrtc/modules/congestion_controller/acknowledged_bitrate_estimator_unittest.cc
@@ -0,0 +1,151 @@
+/*
+ *  Copyright (c) 2017 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/acknowledged_bitrate_estimator.h"
+
+#include "webrtc/base/fakeclock.h"
+#include "webrtc/base/ptr_util.h"
+#include "webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h"
+#include "webrtc/test/gmock.h"
+#include "webrtc/test/gtest.h"
+
+using testing::_;
+using testing::NiceMock;
+using testing::InSequence;
+using testing::Return;
+
+namespace webrtc {
+
+namespace {
+class MockBitrateEstimator : public BitrateEstimator {
+ public:
+  ~MockBitrateEstimator() { Die(); }
+
+  MOCK_METHOD0(Die, void());
+  MOCK_METHOD2(Update, void(int64_t now_ms, int bytes));
+  MOCK_CONST_METHOD0(bitrate_bps, rtc::Optional<uint32_t>());
+};
+
+struct AcknowledgedBitrateEstimatorTestStates {

[philipel, 2017/06/14 09:34:05]

Maybe change this to a test fixture and use TEST_F instead?

[tschumi, 2017/06/14 10:57:46]

@kwiberg-webrtc

[kwiberg-webrtc, 2017/06/14 11:49:21]

Doing it this way instead of using a gtest test fixture with TEST_F() makes the
code easier to read, because you can see where the state comes from and how it's
initialized. Also, you don't force all the test cases to initialize their state
the same way.

[philipel, 2017/06/15 16:13:22]

I'm not sure I understand, when should we use test fixtures then?

IMO doing it like this or using a test fixture is equal in terms of complexity,
but since test fixtures are wildly used throughout the rest of the codebase I
think we should use those instead for the sake of a uniform style.

+  std::unique_ptr<AcknowledgedBitrateEstimator> acknowledged_bitrate_estimator;
+  MockBitrateEstimator* bitrate_estimator;
+};
+
+class MockBitrateEstimatorCreator : public BitrateEstimatorCreator {
+ public:
+  explicit MockBitrateEstimatorCreator(
+      AcknowledgedBitrateEstimatorTestStates* states)
+      : states_(states) {
+    RTC_CHECK(states_);
+  }
+  std::unique_ptr<BitrateEstimator> Create() const override {
+    auto bitrate_estimator = rtc::MakeUnique<NiceMock<MockBitrateEstimator>>();
+    states_->bitrate_estimator = bitrate_estimator.get();
+    return bitrate_estimator;
+  }
+  AcknowledgedBitrateEstimatorTestStates* states_;
+};
+
+AcknowledgedBitrateEstimatorTestStates CreateTestStates() {
+  AcknowledgedBitrateEstimatorTestStates states;
+  states.acknowledged_bitrate_estimator =
+      rtc::MakeUnique<AcknowledgedBitrateEstimator>(
+          rtc::MakeUnique<MockBitrateEstimatorCreator>(&states));
+  EXPECT_CALL(*states.bitrate_estimator, Die()).Times(1);
+  return states;
+}
+
+constexpr int64_t first_arrival_time_ms = 10;
+constexpr int64_t first_send_time_ms = 10;
+constexpr uint16_t sequence_number = 1;
+constexpr size_t payload_size = 10;

[philipel, 2017/06/14 09:34:05]

move these to the anonymous namespace

[tschumi, 2017/06/14 10:57:46]

They already are in the anonymous namespace.
The name space ends at line 82.

I moved them to the top of the  anonymous namespace now.

[philipel, 2017/06/15 16:13:22]

oops

+
+std::vector<PacketFeedback> CreateFeedbackVector() {
+  std::vector<PacketFeedback> packet_feedback_vector;
+  const PacedPacketInfo pacing_info;
+  packet_feedback_vector.push_back(
+      PacketFeedback(first_arrival_time_ms, first_send_time_ms, sequence_number,
+                     payload_size, pacing_info));
+  packet_feedback_vector.push_back(
+      PacketFeedback(first_arrival_time_ms + 10, first_send_time_ms + 10,
+                     sequence_number, payload_size + 10, pacing_info));
+  return packet_feedback_vector;
+}
+
+}  // anonymous namespace
+
+TEST(TestAcknowledgedBitrateEstimator, DontAddPacketsWhichAreNotInSendHistory) {
+  auto states = CreateTestStates();
+  std::vector<PacketFeedback> packet_feedback_vector;
+  packet_feedback_vector.push_back(
+      PacketFeedback(first_arrival_time_ms, sequence_number));
+  EXPECT_CALL(*states.bitrate_estimator, Update(_, _)).Times(0);
+  states.acknowledged_bitrate_estimator->IncomingPacketFeedbackVector(
+      packet_feedback_vector, false);
+}
+
+TEST(TestAcknowledgedBitrateEstimator, UpdateBandwith) {
+  auto states = CreateTestStates();
+  auto packet_feedback_vector = CreateFeedbackVector();
+  {
+    InSequence dummy;
+    EXPECT_CALL(*states.bitrate_estimator,
+                Update(packet_feedback_vector[0].arrival_time_ms,
+                       packet_feedback_vector[0].payload_size))
+        .Times(1);
+    EXPECT_CALL(*states.bitrate_estimator,
+                Update(packet_feedback_vector[1].arrival_time_ms,
+                       packet_feedback_vector[1].payload_size))
+        .Times(1);
+  }
+  states.acknowledged_bitrate_estimator->IncomingPacketFeedbackVector(
+      packet_feedback_vector, false);
+}
+
+TEST(TestAcknowledgedBitrateEstimator,
+     ResetAfterLeafAlrStateAndDontAddOldPackets) {
+  auto states = CreateTestStates();
+  auto packet_feedback_vector = CreateFeedbackVector();
+  states.acknowledged_bitrate_estimator->IncomingPacketFeedbackVector(
+      packet_feedback_vector, true);
+
+  rtc::ScopedFakeClock fake_clock;
+
+  fake_clock.AdvanceTime(
+      rtc::TimeDelta::FromMilliseconds(first_arrival_time_ms + 1));
+  states.acknowledged_bitrate_estimator->IncomingPacketFeedbackVector(
+      std::vector<PacketFeedback>(), false);
+  EXPECT_CALL(*states.bitrate_estimator, Die()).Times(1);
+
+  {
+    InSequence dummy;
+    EXPECT_CALL(*states.bitrate_estimator,
+                Update(packet_feedback_vector[0].arrival_time_ms,
+                       packet_feedback_vector[0].payload_size))
+        .Times(0);
+    EXPECT_CALL(*states.bitrate_estimator,
+                Update(packet_feedback_vector[1].arrival_time_ms,
+                       packet_feedback_vector[1].payload_size))
+        .Times(1);
+  }
+  states.acknowledged_bitrate_estimator->IncomingPacketFeedbackVector(
+      packet_feedback_vector, false);
+}
+
+TEST(TestAcknowledgedBitrateEstimator, ReturnBitrate) {
+  auto states = CreateTestStates();
+  rtc::Optional<uint32_t> return_value(42);
+  EXPECT_CALL(*states.bitrate_estimator, bitrate_bps())
+      .Times(1)
+      .WillOnce(Return(return_value));
+  EXPECT_EQ(return_value, states.acknowledged_bitrate_estimator->bitrate_bps());
+}
+
+}  // namespace webrtc