From 681de2036b8e202d2a8a5556cc901e173a193a43 Mon Sep 17 00:00:00 2001 From: Rasmus Brandt Date: Mon, 4 Feb 2019 15:09:34 +0100 Subject: [PATCH] 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 Reviewed-by: Sebastian Jansson Cr-Commit-Position: refs/heads/master@{#26544} --- call/bitrate_allocator.cc | 14 ++--- call/bitrate_allocator_unittest.cc | 56 ++++++++++++++----- .../goog_cc/probe_controller.cc | 6 +- 3 files changed, 50 insertions(+), 26 deletions(-) diff --git a/call/bitrate_allocator.cc b/call/bitrate_allocator.cc index 172f9401f1..06b74ba4e4 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 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(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; - 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 b1df347caf..9956a88d71 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 @@ 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 uint32_t kMaxBitrateWithProtectionBps = - static_cast(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(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, kMaxBitrateWithProtectionBps)); + EXPECT_CALL(limit_observer_, OnAllocationLimitsChanged(0, 0, kMaxBitrateBps)); 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; diff --git a/modules/congestion_controller/goog_cc/probe_controller.cc b/modules/congestion_controller/goog_cc/probe_controller.cc index a71902d501..fa7a9eb9ab 100644 --- a/modules/congestion_controller/goog_cc/probe_controller.cc +++ b/modules/congestion_controller/goog_cc/probe_controller.cc @@ -154,7 +154,11 @@ std::vector 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();