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 925631b9bb1064116085e8fe275f0d1acceba96c..fae6c24c0f7c033b4229299a4fafc2ce2e0aaf9d 100644
--- a/webrtc/modules/congestion_controller/congestion_controller_unittest.cc
+++ b/webrtc/modules/congestion_controller/congestion_controller_unittest.cc
@@ -8,13 +8,15 @@
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
+#include "webrtc/modules/congestion_controller/include/congestion_controller.h"
 #include "webrtc/logging/rtc_event_log/mock/mock_rtc_event_log.h"
 #include "webrtc/modules/bitrate_controller/include/bitrate_controller.h"
-#include "webrtc/modules/congestion_controller/include/congestion_controller.h"
+#include "webrtc/modules/congestion_controller/congestion_controller_unittests_helper.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 +29,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
@@ -43,7 +47,7 @@ namespace test {
 
 class CongestionControllerTest : public ::testing::Test {
  protected:
-  CongestionControllerTest() : clock_(123456) {}
+  CongestionControllerTest() : clock_(123456), target_bitrate_observer_(this) {}
   ~CongestionControllerTest() override {}
 
   void SetUp() override {
@@ -64,8 +68,45 @@ class CongestionControllerTest : public ::testing::Test {
     controller_->SetBweBitrates(0, kInitialBitrateBps, 5 * kInitialBitrateBps);
   }
 
+  // Custom setup - use an observer that tracks the target bitrate, without
+  // prescribing on which iterations it must change (like a mock would).
+  void TargetBitrateTrackingSetup() {
+    std::unique_ptr<PacedSender> pacer(new NiceMock<MockPacedSender>());
+    controller_.reset(new CongestionController(
+        &clock_, &target_bitrate_observer_, &remote_bitrate_observer_,
+        &event_log_, &packet_router_, std::move(pacer)));
+    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));
+  }
+
+  // Allows us to track the target bitrate, without prescribing the exact
+  // iterations when this would hapen, like a mock would.
+  class TargetBitrateObserver : public CongestionController::Observer {
+   public:
+    explicit TargetBitrateObserver(CongestionControllerTest* owner)
+        : owner_(owner) {}
+    ~TargetBitrateObserver() override = default;
+    void OnNetworkChanged(uint32_t bitrate_bps,
+                          uint8_t fraction_loss,  // 0 - 255.
+                          int64_t rtt_ms,
+                          int64_t probing_interval_ms) override {
+      owner_->target_bitrate_bps_ = rtc::Optional<uint32_t>(bitrate_bps);
+    }
+
+   private:
+    CongestionControllerTest* owner_;
+  };
+
   SimulatedClock clock_;
   StrictMock<MockCongestionObserver> observer_;
+  TargetBitrateObserver target_bitrate_observer_;
   NiceMock<MockPacedSender>* pacer_;
   NiceMock<MockRemoteBitrateObserver> remote_bitrate_observer_;
   NiceMock<MockRtcEventLog> event_log_;
@@ -73,6 +114,8 @@ 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) {
@@ -253,5 +296,157 @@ TEST_F(CongestionControllerTest, ProbeOnBweReset) {
                                    20 * kInitialBitrateBps);
 }
 
+// Estimated bitrate reduced when the feedbacks arrive with such a long delay,
+// that the send-time-history no longer holds the feedbacked packets.
+TEST_F(CongestionControllerTest, LongFeedbackDelays) {
+  TargetBitrateTrackingSetup();
+
+  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());
+  }
+
+  controller_->Process();
+
+  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());
+  }
+}
+
+// Bandwidth estimation is updated when feedbacks are received.
+// Feedbacks which show an increasing delay cause the estimation to be reduced.
+TEST_F(CongestionControllerTest, UpdatesDelayBasedEstimate) {
+  TargetBitrateTrackingSetup();
+
+  uint16_t seq_num = 0;
+  constexpr 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);
+    controller_->Process();
+    ++seq_num;
+  }
+  EXPECT_TRUE(target_bitrate_bps_);
+  if (!target_bitrate_bps_)
+    return;  // Would crash the tests later otherwise.

[stefan-webrtc, 2017/03/09 09:41:02]

Change line 420 to ASSERT_TRUE instead. That basically returns if the assertion
doesn't hold, thereby avoiding crashes.

[elad.alon_webrtc.org, 2017/03/09 12:03:54]

Done.

+
+  // Repeat, but this time with a building delay, and make sure that the
+  // estimation is adjusted downwards.
+  uint32_t bitrate_before_delay = *target_bitrate_bps_;
+  int64_t delay = 0;
+  while (clock_.TimeInMilliseconds() - start_time_ms < 2 * kRunTimeMs) {

[stefan-webrtc, 2017/03/09 09:41:02]

Perhaps break this loop out into a function with delay as a parameter and use it
above too?

[elad.alon_webrtc.org, 2017/03/09 12:03:54]

Done.

+    delay += 50;  // Delay has to increase, or it's just an added RTT.

[stefan-webrtc, 2017/03/09 09:41:02]

"a longer RTT"?

[elad.alon_webrtc.org, 2017/03/09 12:03:54]

"Longer" is confusing to me, because it might be understood as buildup. I went
with "// Delay has to increase, or it's just RTT.", which also has the benefit
of fitting in one line (now that moving this to a function has increased
indentation).

+    PacketFeedback packet(clock_.TimeInMilliseconds() + delay,
+                          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);
+    controller_->Process();
+    ++seq_num;
+  }
+  EXPECT_LT(*target_bitrate_bps_, bitrate_before_delay);
+}
 }  // namespace test
 }  // namespace webrtc