Limit RFC8888 feedback rate to 500Kbit/s

Purpose is to allow sending feedback every 25ms even if link capacity is
unknown.

Bug: webrtc:377222395
Change-Id: I8379cab964cf1d414aab9581e34e209303bd975c
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/376701
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Per Kjellander <perkj@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#43918}

modules/remote_bitrate_estimator/congestion_control_feedback_generator.cc

@@ -28,9 +28,12 @@
 #include "modules/rtp_rtcp/source/rtcp_packet/congestion_control_feedback.h"
 #include "modules/rtp_rtcp/source/rtp_packet_received.h"
 #include "rtc_base/experiments/field_trial_parser.h"
+#include "rtc_base/logging.h"
 
 namespace webrtc {
 
+constexpr DataRate kMaxFeedbackRate = webrtc::DataRate::KilobitsPerSec(500);
+
 CongestionControlFeedbackGenerator::CongestionControlFeedbackGenerator(
     const Environment& env,
     RtcpSender rtcp_sender)
@@ -39,7 +42,7 @@ CongestionControlFeedbackGenerator::CongestionControlFeedbackGenerator(
       min_time_between_feedback_("min_send_delta", TimeDelta::Millis(25)),
       max_time_to_wait_for_packet_with_marker_("max_wait_for_marker",
                                                TimeDelta::Millis(25)),
-      max_time_between_feedback_("max_send_delta", TimeDelta::Millis(250)) {
+      max_time_between_feedback_("max_send_delta", TimeDelta::Millis(500)) {
   ParseFieldTrial(
       {&min_time_between_feedback_, &max_time_to_wait_for_packet_with_marker_,
        &max_time_between_feedback_},
@@ -82,14 +85,8 @@ TimeDelta CongestionControlFeedbackGenerator::Process(Timestamp now) {
   return NextFeedbackTime() - now;
 }
 
-void CongestionControlFeedbackGenerator::OnSendBandwidthEstimateChanged(
-    DataRate estimate) {
-  RTC_DCHECK_RUN_ON(&sequence_checker_);
-  // Feedback reports should max occupy 5% of total bandwidth.
-  max_feedback_rate_ = estimate * 0.05;
-}
-
 void CongestionControlFeedbackGenerator::SendFeedback(Timestamp now) {
+  RTC_DCHECK_GE(now, next_possible_feedback_send_time_);
   uint32_t compact_ntp =
       CompactNtp(env_.clock().ConvertTimestampToNtpTime(now));
   std::vector<rtcp::CongestionControlFeedback::PacketInfo> rtcp_packet_info;
@@ -112,15 +109,13 @@ void CongestionControlFeedbackGenerator::CalculateNextPossibleSendTime(
     DataSize feedback_size,
     Timestamp now) {
   TimeDelta time_since_last_sent = now - last_feedback_sent_time_;
-  DataSize debt_payed = time_since_last_sent * max_feedback_rate_;
+  DataSize debt_payed = time_since_last_sent * kMaxFeedbackRate;
   send_rate_debt_ = debt_payed > send_rate_debt_ ? DataSize::Zero()
                                                  : send_rate_debt_ - debt_payed;
   send_rate_debt_ += feedback_size;
   last_feedback_sent_time_ = now;
   next_possible_feedback_send_time_ =
-      now + std::clamp(max_feedback_rate_.IsZero()
-                           ? TimeDelta::PlusInfinity()
-                           : send_rate_debt_ / max_feedback_rate_,
+      now + std::clamp(send_rate_debt_ / kMaxFeedbackRate,
                        min_time_between_feedback_.Get(),
                        max_time_between_feedback_.Get());
 }

modules/remote_bitrate_estimator/congestion_control_feedback_generator.h

@@ -31,9 +31,17 @@ namespace webrtc {
 // incoming media packets. Feedback format will comply with RFC 8888.
 // https://datatracker.ietf.org/doc/rfc8888/
 
-// Feedback should not use more than 5% of the configured send bandwidth
-// estimate. Min and max duration between feedback is configurable using field
-// trials, but per default, min is 25ms and max is 250ms.
+// Min and max duration between feedback is configurable using field
+// trials, but per default, min is 25ms and max is 500ms.
+//
+// RTCP should not use more than 5% of the uplink link capacity.
+// However, there is no good way for a feedback sender to know the
+// link capacity unless media is sent in both directions. So we just assume that
+// the link capacity is 10 Mbit/s or more and allow sending 500 kbit/s of
+// feedback packets. This allows an approximate receive rate of 200
+// Mbit/s with feedback every 25ms. (200 Mbit/s with average size of 800 bytes =
+// 31250 packets/s => 40 feedback packets/s with feedback of 780 packets each)
+
 // If possible, given the other constraints, feedback will be sent when a packet
 // with marker bit is received in order to provide feedback as soon as possible
 // after receiving a complete video frame. If no packet with marker bit is
@@ -50,7 +58,7 @@ class CongestionControlFeedbackGenerator
 
   void OnReceivedPacket(const RtpPacketReceived& packet) override;
 
-  void OnSendBandwidthEstimateChanged(DataRate estimate) override;
+  void OnSendBandwidthEstimateChanged(DataRate estimate) override {}
 
   TimeDelta Process(Timestamp now) override;
 
@@ -70,7 +78,6 @@ class CongestionControlFeedbackGenerator
   FieldTrialParameter<TimeDelta> max_time_to_wait_for_packet_with_marker_;
   FieldTrialParameter<TimeDelta> max_time_between_feedback_;
 
-  DataRate max_feedback_rate_ = DataRate::KilobitsPerSec(1000);
   DataSize packet_overhead_ = DataSize::Zero();
   DataSize send_rate_debt_ = DataSize::Zero();
 

modules/remote_bitrate_estimator/congestion_control_feedback_generator_unittest.cc

@@ -26,9 +26,9 @@
 #include "modules/rtp_rtcp/source/rtcp_packet/congestion_control_feedback.h"
 #include "modules/rtp_rtcp/source/rtp_packet_received.h"
 #include "rtc_base/buffer.h"
+#include "rtc_base/logging.h"
 #include "rtc_base/network/ecn_marking.h"
 #include "system_wrappers/include/clock.h"
-#include "test/explicit_key_value_config.h"
 #include "test/gmock.h"
 #include "test/gtest.h"
 
@@ -133,122 +133,86 @@ TEST(CongestionControlFeedbackGeneratorTest,
 }
 
 TEST(CongestionControlFeedbackGeneratorTest,
-     FeedbackUtilizeMax5PercentOfConfiguredBwe) {
+     FeedbackFor30KPacketsUtilizeLessThan500kbitPerSecond) {
   MockFunction<void(std::vector<std::unique_ptr<rtcp::RtcpPacket>>)>
       rtcp_sender;
   SimulatedClock clock(123456);
-  constexpr TimeDelta kSmallTimeInterval = TimeDelta::Millis(2);
   CongestionControlFeedbackGenerator generator(CreateEnvironment(&clock),
                                                rtcp_sender.AsStdFunction());
 
-  const DataRate kSendBandwidthEstimate = DataRate::BytesPerSec(10'000);
-  generator.OnSendBandwidthEstimateChanged(kSendBandwidthEstimate);
-  TimeDelta time_to_next_process = generator.Process(clock.CurrentTime());
-  clock.AdvanceTime(kSmallTimeInterval);
-  time_to_next_process -= kSmallTimeInterval;
-
-  // Two packets with marker bit is received within a short duration.
-  // Expect the first feedback to be sent immidately and the second to be
-  // delayed. Delay depend on send bandwith estimate.
-  Timestamp expected_feedback_time = clock.CurrentTime();
+  int number_of_feedback_packets = 0;
+  DataSize total_feedback_size;
   EXPECT_CALL(rtcp_sender, Call)
-      .Times(2)
       .WillRepeatedly(
           [&](std::vector<std::unique_ptr<rtcp::RtcpPacket>> rtcp_packets) {
-            EXPECT_EQ(clock.CurrentTime(), expected_feedback_time);
             ASSERT_THAT(rtcp_packets, SizeIs(1));
-            rtcp::CongestionControlFeedback* rtcp =
-                static_cast<rtcp::CongestionControlFeedback*>(
-                    rtcp_packets[0].get());
-            int rtcp_len = rtcp->BlockLength();
-            // Expect at most 5% of send bandwidth to be used. This decide the
-            // time to next feedback.
-            expected_feedback_time +=
-                DataSize::Bytes(rtcp_len) / (0.05 * kSendBandwidthEstimate);
+            number_of_feedback_packets++;
+            total_feedback_size +=
+                DataSize::Bytes(rtcp_packets[0]->BlockLength());
           });
-  generator.OnReceivedPacket(
-      CreatePacket(clock.CurrentTime(), /*marker=*/true));
-  clock.AdvanceTime(kSmallTimeInterval);
-  time_to_next_process -= kSmallTimeInterval;
-  generator.OnReceivedPacket(
-      CreatePacket(clock.CurrentTime(), /*marker=*/true));
+  Timestamp start_time = clock.CurrentTime();
+  Timestamp last_process_time = clock.CurrentTime();
+  TimeDelta time_to_next_process = generator.Process(clock.CurrentTime());
+  uint16_t rtp_sequence_number = 0;
+  // Receive 30 packet per ms in 1s => 30'0000 packets.
+  while (clock.CurrentTime() < start_time + TimeDelta::Seconds(1)) {
+    for (int i = 0; i < 30; ++i) {
+      generator.OnReceivedPacket(CreatePacket(clock.CurrentTime(),
+                                              /*marker=*/true, /*ssrc=*/1234,
+                                              rtp_sequence_number++));
+    }
+    if (clock.CurrentTime() >= last_process_time + time_to_next_process) {
+      last_process_time = clock.CurrentTime();
+      time_to_next_process = generator.Process(clock.CurrentTime());
+    }
+    clock.AdvanceTime(TimeDelta::Millis(1));
+  }
 
-  clock.AdvanceTime(time_to_next_process);
-  time_to_next_process = generator.Process(clock.CurrentTime());
-  clock.AdvanceTime(time_to_next_process);
-  time_to_next_process = generator.Process(clock.CurrentTime());
+  EXPECT_LE(total_feedback_size / TimeDelta::Seconds(1),
+            DataRate::KilobitsPerSec(500));
+  EXPECT_EQ(number_of_feedback_packets, 40);
 }
 
 TEST(CongestionControlFeedbackGeneratorTest,
-     SendsFeedbackAfterMax250MsIfBweVeryLow) {
-  test::ExplicitKeyValueConfig field_trials(
-      "WebRTC-RFC8888CongestionControlFeedback/max_send_delta:250ms/");
+     FeedbackFor60KPacketsUtilizeApproximately500kbitPerSecond) {
   MockFunction<void(std::vector<std::unique_ptr<rtcp::RtcpPacket>>)>
       rtcp_sender;
   SimulatedClock clock(123456);
-  constexpr TimeDelta kSmallTimeInterval = TimeDelta::Millis(2);
-  CongestionControlFeedbackGenerator generator(
-      CreateEnvironment(&clock, &field_trials), rtcp_sender.AsStdFunction());
-  // Regardless of BWE, feedback is sent at least every 250ms.
-  generator.OnSendBandwidthEstimateChanged(DataRate::BytesPerSec(100));
+  CongestionControlFeedbackGenerator generator(CreateEnvironment(&clock),
+                                               rtcp_sender.AsStdFunction());
+
+  int number_of_feedback_packets = 0;
+  DataSize total_feedback_size;
+  DataSize last_feedback_size;
+  EXPECT_CALL(rtcp_sender, Call)
+      .WillRepeatedly(
+          [&](std::vector<std::unique_ptr<rtcp::RtcpPacket>> rtcp_packets) {
+            ASSERT_THAT(rtcp_packets, SizeIs(1));
+            number_of_feedback_packets++;
+            last_feedback_size =
+                DataSize::Bytes(rtcp_packets[0]->BlockLength());
+            total_feedback_size += last_feedback_size;
+          });
+  Timestamp start_time = clock.CurrentTime();
+  Timestamp last_process_time = clock.CurrentTime();
   TimeDelta time_to_next_process = generator.Process(clock.CurrentTime());
-  clock.AdvanceTime(kSmallTimeInterval);
-  time_to_next_process -= kSmallTimeInterval;
-
-  Timestamp expected_feedback_time = clock.CurrentTime();
-  EXPECT_CALL(rtcp_sender, Call).Times(2).WillRepeatedly(WithoutArgs([&] {
-    EXPECT_EQ(clock.CurrentTime(), expected_feedback_time);
-    // Next feedback is not expected to be sent until 250ms after the
-    // previouse due to low send bandwidth.
-    expected_feedback_time += TimeDelta::Millis(250);
-  }));
-  generator.OnReceivedPacket(
-      CreatePacket(clock.CurrentTime(), /*marker=*/true));
-  clock.AdvanceTime(kSmallTimeInterval);
-  time_to_next_process -= kSmallTimeInterval;
-  generator.OnReceivedPacket(
-      CreatePacket(clock.CurrentTime(), /*marker=*/true));
-
-  clock.AdvanceTime(time_to_next_process);
-  time_to_next_process = generator.Process(clock.CurrentTime());
-  clock.AdvanceTime(time_to_next_process);
-  time_to_next_process = generator.Process(clock.CurrentTime());
-}
-
-TEST(CongestionControlFeedbackGeneratorTest,
-     SendsFeedbackAfterMax250MsIfBweZero) {
-  test::ExplicitKeyValueConfig field_trials(
-      "WebRTC-RFC8888CongestionControlFeedback/max_send_delta:250ms/");
-  MockFunction<void(std::vector<std::unique_ptr<rtcp::RtcpPacket>>)>
-      rtcp_sender;
-  SimulatedClock clock(123456);
-  constexpr TimeDelta kSmallTimeInterval = TimeDelta::Millis(2);
-  CongestionControlFeedbackGenerator generator(
-      CreateEnvironment(&clock, &field_trials), rtcp_sender.AsStdFunction());
-  // Regardless of BWE, feedback is sent at least every 250ms.
-  generator.OnSendBandwidthEstimateChanged(DataRate::Zero());
-  TimeDelta time_to_next_process = generator.Process(clock.CurrentTime());
-  clock.AdvanceTime(kSmallTimeInterval);
-  time_to_next_process -= kSmallTimeInterval;
-
-  Timestamp expected_feedback_time = clock.CurrentTime();
-  EXPECT_CALL(rtcp_sender, Call).Times(2).WillRepeatedly(WithoutArgs([&] {
-    EXPECT_EQ(clock.CurrentTime(), expected_feedback_time);
-    // Next feedback is not expected to be sent until 250ms after the
-    // previouse due to low send bandwidth.
-    expected_feedback_time += TimeDelta::Millis(250);
-  }));
-  generator.OnReceivedPacket(
-      CreatePacket(clock.CurrentTime(), /*marker=*/true));
-  clock.AdvanceTime(kSmallTimeInterval);
-  time_to_next_process -= kSmallTimeInterval;
-  generator.OnReceivedPacket(
-      CreatePacket(clock.CurrentTime(), /*marker=*/true));
-
-  clock.AdvanceTime(time_to_next_process);
-  time_to_next_process = generator.Process(clock.CurrentTime());
-  clock.AdvanceTime(time_to_next_process);
+  uint16_t rtp_sequence_number = 0;
+  // Receive 60 packet per ms in 1s => 60'0000 packets.
+  while (clock.CurrentTime() < start_time + TimeDelta::Seconds(1)) {
+    for (int i = 0; i < 60; ++i) {
+      generator.OnReceivedPacket(CreatePacket(clock.CurrentTime(),
+                                              /*marker=*/true, /*ssrc=*/1234,
+                                              rtp_sequence_number++));
+    }
+    if (clock.CurrentTime() >= last_process_time + time_to_next_process) {
+      last_process_time = clock.CurrentTime();
       time_to_next_process = generator.Process(clock.CurrentTime());
+    }
+    clock.AdvanceTime(TimeDelta::Millis(1));
+  }
+  EXPECT_LE(total_feedback_size,
+            DataSize::Bytes(500'000 / 8) + last_feedback_size);
+  EXPECT_LT(number_of_feedback_packets, 40);
 }
 
 TEST(CongestionControlFeedbackGeneratorTest,

modules/remote_bitrate_estimator/congestion_control_feedback_tracker.cc

@@ -26,8 +26,16 @@
 
 namespace webrtc {
 
+constexpr int kMaxPacketsPerSsrc = 16384;
+
 void CongestionControlFeedbackTracker::ReceivedPacket(
     const RtpPacketReceived& packet) {
+  if (packets_.size() > kMaxPacketsPerSsrc) {
+    RTC_LOG(LS_VERBOSE)
+        << "Unexpected number of packets without sending reports:"
+        << packets_.size();
+    return;
+  }
   int64_t unwrapped_sequence_number =
       unwrapper_.Unwrap(packet.SequenceNumber());
   if (last_sequence_number_in_feedback_ &&
@@ -39,9 +47,9 @@ void CongestionControlFeedbackTracker::ReceivedPacket(
         << static_cast<uint16_t>(*last_sequence_number_in_feedback_);
     // TODO: bugs.webrtc.org/374550342 - According to spec, the old packets
     // should be reported again. But at the moment, we dont store history of
-    // packet we already reported and thus, they will be reported as lost. Note
-    // that this is likely not a problem in webrtc since the packets will also
-    // be removed from the send history when they are first reported as
+    // packet we already reported and thus, they will be reported as lost.
+    // Note that this is likely not a problem in webrtc since the packets will
+    // also be removed from the send history when they are first reported as
     // received.
     last_sequence_number_in_feedback_ = unwrapped_sequence_number - 1;
   }
@@ -69,7 +77,9 @@ void CongestionControlFeedbackTracker::AddPacketsToFeedback(
   auto packet_it = packets_.begin();
   uint32_t ssrc = packet_it->ssrc;
   for (int64_t sequence_number = *last_sequence_number_in_feedback_ + 1;
-       sequence_number <= packets_.back().unwrapped_sequence_number;
+       sequence_number <= packets_.back().unwrapped_sequence_number &&
+       sequence_number <=
+           *last_sequence_number_in_feedback_ + kMaxPacketsPerSsrc;
        ++sequence_number) {
     RTC_DCHECK(packet_it != packets_.end());
     RTC_DCHECK_EQ(ssrc, packet_it->ssrc);