Move delay_based_bwe_ into CongestionController

webrtc/modules/congestion_controller/congestion_controller_unittest.cc

Index: webrtc/modules/congestion_controller/congestion_controller_unittest.cc
diff --git a/webrtc/modules/congestion_controller/congestion_controller_unittest.cc b/webrtc/modules/congestion_controller/congestion_controller_unittest.cc
index ca0d28fcd5c8cfbd819cdbcd9c98ac3bc62f7738..6df6381790acdd97eb96f8e89677d27acbf5ccab 100644
--- a/webrtc/modules/congestion_controller/congestion_controller_unittest.cc
+++ b/webrtc/modules/congestion_controller/congestion_controller_unittest.cc
@@ -9,12 +9,13 @@
  */
 
 #include "webrtc/logging/rtc_event_log/mock/mock_rtc_event_log.h"
-#include "webrtc/modules/bitrate_controller/include/bitrate_controller.h"
+#include "webrtc/modules/bitrate_controller/include/mock/mock_bitrate_controller.h"
 #include "webrtc/modules/congestion_controller/include/congestion_controller.h"
 #include "webrtc/modules/congestion_controller/include/mock/mock_congestion_controller.h"
 #include "webrtc/modules/pacing/mock/mock_paced_sender.h"
 #include "webrtc/modules/remote_bitrate_estimator/include/bwe_defines.h"
 #include "webrtc/modules/remote_bitrate_estimator/include/mock/mock_remote_bitrate_observer.h"
+#include "webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h"
 #include "webrtc/system_wrappers/include/clock.h"
 #include "webrtc/test/gmock.h"
 #include "webrtc/test/gtest.h"
@@ -27,6 +28,8 @@ using testing::SaveArg;
 using testing::StrictMock;
 
 namespace {
+const webrtc::PacedPacketInfo kPacingInfo0(0, 5, 2000);
+const webrtc::PacedPacketInfo kPacingInfo1(1, 8, 4000);
 
 // Helper to convert some time format to resolution used in absolute send time
 // header extension, rounded upwards. |t| is the time to convert, in some
@@ -36,6 +39,34 @@ uint32_t AbsSendTime(int64_t t, int64_t denom) {
   return (((t << 18) + (denom >> 1)) / denom) & 0x00fffffful;
 }
 
+// TODO(elad.alon): This is not meant to be landed like this. Ask reviewers

[elad.alon_webrtc.org, 2017/03/02 11:51:47]

Soliciting feedback. :-)

+// where would be a good place to put ComparePacketVectors(), so that it would
+// be visible to both unit-test suites.
+void ComparePacketVectors(const std::vector<webrtc::PacketFeedback>& truth,
+                          const std::vector<webrtc::PacketFeedback>& input) {
+  ASSERT_EQ(truth.size(), input.size());
+  size_t len = truth.size();
+  // truth contains the input data for the test, and input is what will be
+  // sent to the bandwidth estimator. truth.arrival_tims_ms is used to
+  // populate the transport feedback messages. As these times may be changed
+  // (because of resolution limits in the packets, and because of the time
+  // base adjustment performed by the TransportFeedbackAdapter at the first
+  // packet, the truth[x].arrival_time and input[x].arrival_time may not be
+  // equal. However, the difference must be the same for all x.
+  int64_t arrival_time_delta =
+      truth[0].arrival_time_ms - input[0].arrival_time_ms;
+  for (size_t i = 0; i < len; ++i) {
+    RTC_CHECK(truth[i].arrival_time_ms != webrtc::PacketFeedback::kNotReceived);
+    if (input[i].arrival_time_ms != webrtc::PacketFeedback::kNotReceived) {
+      EXPECT_EQ(truth[i].arrival_time_ms,
+                input[i].arrival_time_ms + arrival_time_delta);
+    }
+    EXPECT_EQ(truth[i].send_time_ms, input[i].send_time_ms);
+    EXPECT_EQ(truth[i].sequence_number, input[i].sequence_number);
+    EXPECT_EQ(truth[i].payload_size, input[i].payload_size);
+    EXPECT_EQ(truth[i].pacing_info, input[i].pacing_info);
+  }
+}
 }  // namespace
 
 namespace webrtc {
@@ -51,7 +82,9 @@ class CongestionControllerTest : public ::testing::Test {
     std::unique_ptr<PacedSender> pacer(pacer_);  // Passes ownership.
     controller_.reset(new CongestionController(
         &clock_, &observer_, &remote_bitrate_observer_, &event_log_,
-        &packet_router_, std::move(pacer)));
+        &packet_router_, std::move(pacer),
+        std::unique_ptr<BitrateController>(
+            BitrateController::CreateBitrateController(&clock_, &event_log_))));
     bandwidth_observer_.reset(
         controller_->GetBitrateController()->CreateRtcpBandwidthObserver());
 
@@ -62,6 +95,29 @@ class CongestionControllerTest : public ::testing::Test {
     controller_->SetBweBitrates(0, kInitialBitrateBps, 5 * kInitialBitrateBps);
   }
 
+  void OnSentPacket(const PacketFeedback& packet_feedback) {
+    controller_->AddPacket(packet_feedback.sequence_number,
+                           packet_feedback.payload_size,
+                           packet_feedback.pacing_info);
+    controller_->OnSentPacket(rtc::SentPacket(packet_feedback.sequence_number,
+                                              packet_feedback.send_time_ms));
+  }
+
+  class MockBitrateControllerAdapter : public MockBitrateController {

[stefan-webrtc, 2017/03/02 15:58:07]

I would prefer not to have this mock, and to not expose the bitrate controller
in the constructor at all.

It should be possible to write the test so that we can see that the delay-based
bwe does what it should without this. For instance, insert packets at a fixed
rate that is as now and verify that the target bitrate is high enough. Then
insert packets where the rate is lower, and where the delay is growing, and
verify that the target bitrate has decreased.

+   public:
+    explicit MockBitrateControllerAdapter(CongestionControllerTest* owner)
+        : MockBitrateController(), owner_(owner) {}
+
+    ~MockBitrateControllerAdapter() override {}
+
+    void OnDelayBasedBweResult(const DelayBasedBwe::Result& result) override {
+      owner_->target_bitrate_bps_ =
+          rtc::Optional<uint32_t>(result.target_bitrate_bps);
+    }
+
+    CongestionControllerTest* const owner_;
+  };
+
   SimulatedClock clock_;
   StrictMock<MockCongestionObserver> observer_;
   NiceMock<MockPacedSender>* pacer_;
@@ -71,6 +127,7 @@ class CongestionControllerTest : public ::testing::Test {
   PacketRouter packet_router_;
   std::unique_ptr<CongestionController> controller_;
   const uint32_t kInitialBitrateBps = 60000;
+  rtc::Optional<uint32_t> target_bitrate_bps_;
 };
 
 TEST_F(CongestionControllerTest, OnNetworkChanged) {
@@ -217,9 +274,11 @@ TEST_F(CongestionControllerTest, OnReceivedPacketWithAbsSendTime) {
   NiceMock<MockCongestionObserver> observer;
   StrictMock<MockRemoteBitrateObserver> remote_bitrate_observer;
   std::unique_ptr<PacedSender> pacer(new NiceMock<MockPacedSender>());
-  controller_.reset(
-      new CongestionController(&clock_, &observer, &remote_bitrate_observer,
-                               &event_log_, &packet_router_, std::move(pacer)));
+  controller_.reset(new CongestionController(
+      &clock_, &observer, &remote_bitrate_observer, &event_log_,
+      &packet_router_, std::move(pacer),
+      std::unique_ptr<BitrateController>(
+          BitrateController::CreateBitrateController(&clock_, &event_log_))));
 
   size_t payload_size = 1000;
   RTPHeader header;
@@ -240,5 +299,141 @@ TEST_F(CongestionControllerTest, OnReceivedPacketWithAbsSendTime) {
   ASSERT_EQ(1u, ssrcs.size());
   EXPECT_EQ(header.ssrc, ssrcs[0]);
 }
+
+TEST_F(CongestionControllerTest, UpdatesDelayBasedEstimate) {
+  NiceMock<MockCongestionObserver> observer;
+  StrictMock<MockRemoteBitrateObserver> remote_bitrate_observer;
+  std::unique_ptr<PacedSender> pacer(new NiceMock<MockPacedSender>());
+  controller_.reset(
+      new CongestionController(&clock_, &observer, &remote_bitrate_observer,
+                               &event_log_, &packet_router_, std::move(pacer),
+                               std::unique_ptr<BitrateController>(
+                                   new MockBitrateControllerAdapter(this))));
+
+  uint16_t seq_num = 0;
+  size_t kPayloadSize = 1000;
+  // The test must run and insert packets/feedback long enough that the
+  // BWE computes a valid estimate.
+  const int64_t kRunTimeMs = 6000;
+  int64_t start_time_ms = clock_.TimeInMilliseconds();
+  while (clock_.TimeInMilliseconds() - start_time_ms < kRunTimeMs) {
+    PacketFeedback packet(clock_.TimeInMilliseconds(),
+                          clock_.TimeInMilliseconds(), seq_num, kPayloadSize,
+                          PacedPacketInfo());
+    OnSentPacket(packet);
+    // Create expected feedback and send into adapter.
+    std::unique_ptr<rtcp::TransportFeedback> feedback(
+        new rtcp::TransportFeedback());
+    feedback->SetBase(packet.sequence_number, packet.arrival_time_ms * 1000);
+    EXPECT_TRUE(feedback->AddReceivedPacket(packet.sequence_number,
+                                            packet.arrival_time_ms * 1000));
+    rtc::Buffer raw_packet = feedback->Build();
+    feedback = rtcp::TransportFeedback::ParseFrom(raw_packet.data(),
+                                                  raw_packet.size());
+    EXPECT_TRUE(feedback.get() != nullptr);
+    controller_->OnTransportFeedback(*feedback.get());
+    clock_.AdvanceTimeMilliseconds(50);
+    ++seq_num;
+  }

[stefan-webrtc, 2017/03/02 15:58:07]

Keep this loop, and also add a similar after:

while (clock_.TimeInMilliseconds() - start_time_ms < kRunTimeMs) {
  // insert late packets by increasing the arrival time more than the send time.
}

Then verified decraesed bitrate.

+
+  EXPECT_TRUE(target_bitrate_bps_);
+}
+
+TEST_F(CongestionControllerTest, LongFeedbackDelays) {
+  // Custom setup
+  std::unique_ptr<PacedSender> pacer(new NiceMock<MockPacedSender>());
+  auto mock_bitrate_controller = std::unique_ptr<MockBitrateControllerAdapter>(
+      new MockBitrateControllerAdapter(this));
+  EXPECT_CALL(*mock_bitrate_controller, SetBitrates(_, _, _));
+  EXPECT_CALL(*mock_bitrate_controller, GetNetworkParameters(_, _, _));
+  EXPECT_CALL(observer_, OnNetworkChanged(_, _, _, _));
+  controller_.reset(new CongestionController(
+      &clock_, &observer_, &remote_bitrate_observer_, &event_log_,
+      &packet_router_, std::move(pacer),
+      std::unique_ptr<BitrateController>(std::move(mock_bitrate_controller))));
+  controller_->SetBweBitrates(0, kInitialBitrateBps, 5 * kInitialBitrateBps);
+
+  const int64_t kFeedbackTimeoutMs = 60001;
+  const int kMaxConsecutiveFailedLookups = 5;
+  for (int i = 0; i < kMaxConsecutiveFailedLookups; ++i) {
+    std::vector<PacketFeedback> packets;
+    packets.push_back(
+        PacketFeedback(i * 100, 2 * i * 100, 0, 1500, kPacingInfo0));
+    packets.push_back(
+        PacketFeedback(i * 100 + 10, 2 * i * 100 + 10, 1, 1500, kPacingInfo0));
+    packets.push_back(
+        PacketFeedback(i * 100 + 20, 2 * i * 100 + 20, 2, 1500, kPacingInfo0));
+    packets.push_back(
+        PacketFeedback(i * 100 + 30, 2 * i * 100 + 30, 3, 1500, kPacingInfo1));
+    packets.push_back(
+        PacketFeedback(i * 100 + 40, 2 * i * 100 + 40, 4, 1500, kPacingInfo1));
+
+    for (const PacketFeedback& packet : packets)
+      OnSentPacket(packet);
+
+    rtcp::TransportFeedback feedback;
+    feedback.SetBase(packets[0].sequence_number,
+                     packets[0].arrival_time_ms * 1000);
+
+    for (const PacketFeedback& packet : packets) {
+      EXPECT_TRUE(feedback.AddReceivedPacket(packet.sequence_number,
+                                             packet.arrival_time_ms * 1000));
+    }
+
+    feedback.Build();
+
+    clock_.AdvanceTimeMilliseconds(kFeedbackTimeoutMs);
+    PacketFeedback later_packet(kFeedbackTimeoutMs + i * 100 + 40,
+                                kFeedbackTimeoutMs + i * 200 + 40, 5, 1500,
+                                kPacingInfo1);
+    OnSentPacket(later_packet);
+
+    controller_->OnTransportFeedback(feedback);
+
+    // Check that packets have timed out.
+    for (PacketFeedback& packet : packets) {
+      packet.send_time_ms = -1;
+      packet.payload_size = 0;
+      packet.pacing_info = PacedPacketInfo();
+    }
+    ComparePacketVectors(packets, controller_->GetTransportFeedbackVector());
+  }
+
+  // Target bitrate should have halved due to feedback delays.
+  EXPECT_EQ(kInitialBitrateBps / 2, target_bitrate_bps_);
+
+  // Test with feedback that isn't late enough to time out.
+  {
+    std::vector<PacketFeedback> packets;
+    packets.push_back(PacketFeedback(100, 200, 0, 1500, kPacingInfo0));
+    packets.push_back(PacketFeedback(110, 210, 1, 1500, kPacingInfo0));
+    packets.push_back(PacketFeedback(120, 220, 2, 1500, kPacingInfo0));
+    packets.push_back(PacketFeedback(130, 230, 3, 1500, kPacingInfo1));
+    packets.push_back(PacketFeedback(140, 240, 4, 1500, kPacingInfo1));
+
+    for (const PacketFeedback& packet : packets)
+      OnSentPacket(packet);
+
+    rtcp::TransportFeedback feedback;
+    feedback.SetBase(packets[0].sequence_number,
+                     packets[0].arrival_time_ms * 1000);
+
+    for (const PacketFeedback& packet : packets) {
+      EXPECT_TRUE(feedback.AddReceivedPacket(packet.sequence_number,
+                                             packet.arrival_time_ms * 1000));
+    }
+
+    feedback.Build();
+
+    clock_.AdvanceTimeMilliseconds(kFeedbackTimeoutMs - 1);
+    PacketFeedback later_packet(kFeedbackTimeoutMs + 140,
+                                kFeedbackTimeoutMs + 240, 5, 1500,
+                                kPacingInfo1);
+    OnSentPacket(later_packet);
+
+    controller_->OnTransportFeedback(feedback);
+    ComparePacketVectors(packets, controller_->GetTransportFeedbackVector());
+  }
+}
 }  // namespace test
 }  // namespace webrtc