Stop changing the requested max bitrate based on protection level.

With the current implementation, whenever we are toggling between
sending/not sending retransmissions, the BitrateAllocator will
toggle the total requested max bitrate that is signalled to the
probing mechanism. The result is that spurious probes are sent
mid-call, at |max_bitrate| and |2*max_bitrate|. This behaviour
is undesirable, and thus removed in this CL. Instead, whenever
the allocation limits actually change, we produce a single
set of probes at |max_bitrate| and |2*max_bitrate|.

This CL does not change how the BitrateAllocator hysteresis is
accounting for protection, since it does not relate to the
spurious probes.

Bug: webrtc:10275
TBR: sprang@webrtc.org
Change-Id: Iab3a690a500372c74772a8ad6217fb838af15ade
Reviewed-on: https://webrtc-review.googlesource.com/c/120808
Commit-Queue: Rasmus Brandt <brandtr@webrtc.org>
Reviewed-by: Sebastian Jansson <srte@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#26544}

call/bitrate_allocator.cc

@@ -27,6 +27,8 @@
 
 namespace webrtc {
 
+namespace {
+
 // Allow packets to be transmitted in up to 2 times max video bitrate if the
 // bandwidth estimate allows it.
 const uint8_t kTransmissionMaxBitrateMultiplier = 2;
@@ -38,8 +40,6 @@ const uint32_t kMinToggleBitrateBps = 20000;
 
 const int64_t kBweLogIntervalMs = 5000;
 
-namespace {
-
 double MediaRatio(uint32_t allocated_bitrate, uint32_t protection_bitrate) {
   RTC_DCHECK_GT(allocated_bitrate, 0);
   if (protection_bitrate == 0)
@@ -48,6 +48,7 @@ double MediaRatio(uint32_t allocated_bitrate, uint32_t protection_bitrate) {
   uint32_t media_bitrate = allocated_bitrate - protection_bitrate;
   return media_bitrate / static_cast<double>(allocated_bitrate);
 }
+
 }  // namespace
 
 BitrateAllocator::BitrateAllocator(LimitObserver* limit_observer)
@@ -228,14 +229,7 @@ void BitrateAllocator::UpdateAllocationLimits() {
           std::max(config.MinBitrateWithHysteresis(), stream_padding);
     }
     total_requested_padding_bitrate += stream_padding;
-    uint32_t max_bitrate_bps = config.max_bitrate_bps;
+    total_requested_max_bitrate += config.max_bitrate_bps;
-    if (config.media_ratio < 1.0) {
-      // Account for protection overhead (eg FEC). Assumption is that overhead
-      // is never more than 100%. Don't adjust based exact value as that might
-      // trigger too frequent calls to OnAllocationLimitsChanged().
-      max_bitrate_bps *= 2;
-    }
-    total_requested_max_bitrate += max_bitrate_bps;
   }
 
   if (total_requested_padding_bitrate == total_requested_padding_bitrate_ &&

call/bitrate_allocator_unittest.cc

@@ -17,8 +17,9 @@
 #include "test/gmock.h"
 #include "test/gtest.h"
 
-using ::testing::NiceMock;
 using ::testing::_;
+using ::testing::NiceMock;
+using ::testing::StrictMock;
 
 namespace webrtc {
 // Emulating old interface for test suite compatibility.
@@ -347,27 +348,21 @@ TEST_F(BitrateAllocatorTestNoEnforceMin, OneBitrateObserverWithPacketLoss) {
 
   // Add loss and use a part of the bitrate for protection.
   const double kProtectionRatio = 0.4;
-  uint32_t target_bitrate_bps = 200000;
+  const uint8_t fraction_loss = kProtectionRatio * 256;
-  const uint32_t kMaxBitrateWithProtectionBps =
-      static_cast<uint32_t>(kMaxBitrateBps * 2);
-  uint8_t fraction_loss = kProtectionRatio * 256;
   bitrate_observer.SetBitrateProtectionRatio(kProtectionRatio);
-  EXPECT_CALL(limit_observer_,
+  allocator_->OnNetworkChanged(200000, 0, fraction_loss,
-              OnAllocationLimitsChanged(0, 0, kMaxBitrateWithProtectionBps));
-  allocator_->OnNetworkChanged(target_bitrate_bps, 0, fraction_loss,
                                kDefaultProbingIntervalMs);
-  EXPECT_EQ(target_bitrate_bps, bitrate_observer.last_bitrate_bps_);
+  EXPECT_EQ(200000u, bitrate_observer.last_bitrate_bps_);
 
   // Above the min threshold, but not enough given the protection used.
   // Limits changed, as we will video is now off and we need to pad up to the
   // start bitrate.
-  target_bitrate_bps = kMinStartBitrateBps + 1000;
   // Verify the hysteresis is added for the protection.
   const uint32_t kMinStartBitrateWithProtectionBps =
       static_cast<uint32_t>(kMinStartBitrateBps * (1 + kProtectionRatio));
   EXPECT_CALL(limit_observer_,
               OnAllocationLimitsChanged(0, kMinStartBitrateWithProtectionBps,
-                                        kMaxBitrateWithProtectionBps));
+                                        kMaxBitrateBps));
   allocator_->OnNetworkChanged(kMinStartBitrateBps + 1000, 0, fraction_loss,
                                kDefaultProbingIntervalMs);
   EXPECT_EQ(0u, bitrate_observer.last_bitrate_bps_);
@@ -377,9 +372,7 @@ TEST_F(BitrateAllocatorTestNoEnforceMin, OneBitrateObserverWithPacketLoss) {
   EXPECT_EQ(0u, bitrate_observer.last_bitrate_bps_);
 
   // Just enough to enable video again.
-  target_bitrate_bps = kMinStartBitrateWithProtectionBps;
+  EXPECT_CALL(limit_observer_, OnAllocationLimitsChanged(0, 0, kMaxBitrateBps));
-  EXPECT_CALL(limit_observer_,
-              OnAllocationLimitsChanged(0, 0, kMaxBitrateWithProtectionBps));
   allocator_->OnNetworkChanged(kMinStartBitrateWithProtectionBps, 0,
                                fraction_loss, kDefaultProbingIntervalMs);
   EXPECT_EQ(kMinStartBitrateWithProtectionBps,
@@ -387,7 +380,6 @@ TEST_F(BitrateAllocatorTestNoEnforceMin, OneBitrateObserverWithPacketLoss) {
 
   // Remove all protection and make sure video is not paused as earlier.
   bitrate_observer.SetBitrateProtectionRatio(0.0);
-  EXPECT_CALL(limit_observer_, OnAllocationLimitsChanged(0, 0, kMaxBitrateBps));
   allocator_->OnNetworkChanged(kMinStartBitrateWithProtectionBps - 1000, 0, 0,
                                kDefaultProbingIntervalMs);
   EXPECT_EQ(kMinStartBitrateWithProtectionBps - 1000,
@@ -401,6 +393,40 @@ TEST_F(BitrateAllocatorTestNoEnforceMin, OneBitrateObserverWithPacketLoss) {
   allocator_->RemoveObserver(&bitrate_observer);
 }
 
+TEST_F(BitrateAllocatorTest,
+       TotalAllocationLimitsAreUnaffectedByProtectionRatio) {
+  TestBitrateObserver bitrate_observer;
+
+  const uint32_t kMinBitrateBps = 100000;
+  const uint32_t kMaxBitrateBps = 400000;
+
+  // Register |bitrate_observer| and expect total allocation limits to change.
+  EXPECT_CALL(limit_observer_,
+              OnAllocationLimitsChanged(kMinBitrateBps, 0, kMaxBitrateBps))
+      .Times(1);
+  allocator_->AddObserver(
+      &bitrate_observer,
+      {kMinBitrateBps, kMaxBitrateBps, 0, true, "", kDefaultBitratePriority});
+
+  // Observer uses 20% of it's allocated bitrate for protection.
+  bitrate_observer.SetBitrateProtectionRatio(/*protection_ratio=*/0.2);
+  // Total allocation limits are unaffected by the protection rate change.
+  EXPECT_CALL(limit_observer_, OnAllocationLimitsChanged(_, _, _)).Times(0);
+  allocator_->OnNetworkChanged(200000u, 0, 100, kDefaultProbingIntervalMs);
+
+  // Observer uses 0% of it's allocated bitrate for protection.
+  bitrate_observer.SetBitrateProtectionRatio(/*protection_ratio=*/0.0);
+  // Total allocation limits are unaffected by the protection rate change.
+  EXPECT_CALL(limit_observer_, OnAllocationLimitsChanged(_, _, _)).Times(0);
+  allocator_->OnNetworkChanged(200000u, 0, 100, kDefaultProbingIntervalMs);
+
+  // Observer again uses 20% of it's allocated bitrate for protection.
+  bitrate_observer.SetBitrateProtectionRatio(/*protection_ratio=*/0.2);
+  // Total allocation limits are unaffected by the protection rate change.
+  EXPECT_CALL(limit_observer_, OnAllocationLimitsChanged(_, _, _)).Times(0);
+  allocator_->OnNetworkChanged(200000u, 0, 100, kDefaultProbingIntervalMs);
+}
+
 TEST_F(BitrateAllocatorTestNoEnforceMin, TwoBitrateObserverWithPacketLoss) {
   TestBitrateObserver bitrate_observer_1;
   TestBitrateObserver bitrate_observer_2;

modules/congestion_controller/goog_cc/probe_controller.cc

@@ -154,7 +154,11 @@ std::vector<ProbeClusterConfig> ProbeController::OnMaxTotalAllocatedBitrate(
       (max_bitrate_bps_ <= 0 || estimated_bitrate_bps_ < max_bitrate_bps_) &&
       estimated_bitrate_bps_ < max_total_allocated_bitrate) {
     max_total_allocated_bitrate_ = max_total_allocated_bitrate;
-    return InitiateProbing(at_time_ms, {max_total_allocated_bitrate}, false);
+    // Also probe at 2x the max bitrate, to account for the transmission max
+    // bitrate multiplier functionality of the BitrateAllocator.
+    return InitiateProbing(
+        at_time_ms,
+        {max_total_allocated_bitrate, 2 * max_total_allocated_bitrate}, false);
   }
   max_total_allocated_bitrate_ = max_total_allocated_bitrate;
   return std::vector<ProbeClusterConfig>();