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}diff --git a/call/bitrate_allocator.cc b/call/bitrate_allocator.cc
index 172f940..06b74ba 100644
--- a/call/bitrate_allocator.cc
+++ b/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 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 @@
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 @@
std::max(config.MinBitrateWithHysteresis(), stream_padding);
}
total_requested_padding_bitrate += stream_padding;
- uint32_t max_bitrate_bps = 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;
+ total_requested_max_bitrate += config.max_bitrate_bps;
}
if (total_requested_padding_bitrate == total_requested_padding_bitrate_ &&
diff --git a/call/bitrate_allocator_unittest.cc b/call/bitrate_allocator_unittest.cc
index b1df347..9956a88 100644
--- a/call/bitrate_allocator_unittest.cc
+++ b/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 @@
// Add loss and use a part of the bitrate for protection.
const double kProtectionRatio = 0.4;
- uint32_t target_bitrate_bps = 200000;
- const uint32_t kMaxBitrateWithProtectionBps =
- static_cast<uint32_t>(kMaxBitrateBps * 2);
- uint8_t fraction_loss = kProtectionRatio * 256;
+ const uint8_t fraction_loss = kProtectionRatio * 256;
bitrate_observer.SetBitrateProtectionRatio(kProtectionRatio);
- EXPECT_CALL(limit_observer_,
- OnAllocationLimitsChanged(0, 0, kMaxBitrateWithProtectionBps));
- allocator_->OnNetworkChanged(target_bitrate_bps, 0, fraction_loss,
+ allocator_->OnNetworkChanged(200000, 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 @@
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, kMaxBitrateWithProtectionBps));
+ EXPECT_CALL(limit_observer_, OnAllocationLimitsChanged(0, 0, kMaxBitrateBps));
allocator_->OnNetworkChanged(kMinStartBitrateWithProtectionBps, 0,
fraction_loss, kDefaultProbingIntervalMs);
EXPECT_EQ(kMinStartBitrateWithProtectionBps,
@@ -387,7 +380,6 @@
// 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 @@
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;
diff --git a/modules/congestion_controller/goog_cc/probe_controller.cc b/modules/congestion_controller/goog_cc/probe_controller.cc
index a71902d..fa7a9eb 100644
--- a/modules/congestion_controller/goog_cc/probe_controller.cc
+++ b/modules/congestion_controller/goog_cc/probe_controller.cc
@@ -154,7 +154,11 @@
(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>();
