From b7edb88ae2bb65b3b7ce3be31dd708470baa3575 Mon Sep 17 00:00:00 2001 From: pbos Date: Thu, 22 Oct 2015 08:52:20 -0700 Subject: [PATCH] Prevent BWE rampdowns without new loss reports. Before this change, UpdateEstimate would repeatedly decrease bitrate even though there's no fresh corresponding RTCP loss report, triggering multiple reactions to a single indication of high packet loss. BUG=webrtc:5101 R=stefan@webrtc.org Review URL: https://codereview.webrtc.org/1417723005 Cr-Commit-Position: refs/heads/master@{#10374} --- .../send_side_bandwidth_estimation.cc | 46 +++++++++-------- .../send_side_bandwidth_estimation.h | 5 +- ...send_side_bandwidth_estimation_unittest.cc | 51 +++++++++++++++++++ 3 files changed, 78 insertions(+), 24 deletions(-) diff --git a/webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc b/webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc index 0210371600..fb9941a3b1 100644 --- a/webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc +++ b/webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc @@ -43,13 +43,14 @@ const size_t kNumUmaRampupMetrics = } SendSideBandwidthEstimation::SendSideBandwidthEstimation() - : accumulate_lost_packets_Q8_(0), - accumulate_expected_packets_(0), + : lost_packets_since_last_loss_update_Q8_(0), + expected_packets_since_last_loss_update_(0), bitrate_(0), min_bitrate_configured_(kDefaultMinBitrateBps), max_bitrate_configured_(kDefaultMaxBitrateBps), last_low_bitrate_log_ms_(-1), - time_last_receiver_block_ms_(0), + has_decreased_since_last_fraction_loss_(false), + time_last_receiver_block_ms_(-1), last_fraction_loss_(0), last_round_trip_time_ms_(0), bwe_incoming_(0), @@ -58,8 +59,7 @@ SendSideBandwidthEstimation::SendSideBandwidthEstimation() initially_lost_packets_(0), bitrate_at_2_seconds_kbps_(0), uma_update_state_(kNoUpdate), - rampup_uma_stats_updated_(kNumUmaRampupMetrics, false) { -} + rampup_uma_stats_updated_(kNumUmaRampupMetrics, false) {} SendSideBandwidthEstimation::~SendSideBandwidthEstimation() {} @@ -117,21 +117,20 @@ void SendSideBandwidthEstimation::UpdateReceiverBlock(uint8_t fraction_loss, // Calculate number of lost packets. const int num_lost_packets_Q8 = fraction_loss * number_of_packets; // Accumulate reports. - accumulate_lost_packets_Q8_ += num_lost_packets_Q8; - accumulate_expected_packets_ += number_of_packets; + lost_packets_since_last_loss_update_Q8_ += num_lost_packets_Q8; + expected_packets_since_last_loss_update_ += number_of_packets; - // Report loss if the total report is based on sufficiently many packets. - if (accumulate_expected_packets_ >= kLimitNumPackets) { - last_fraction_loss_ = - accumulate_lost_packets_Q8_ / accumulate_expected_packets_; - - // Reset accumulators. - accumulate_lost_packets_Q8_ = 0; - accumulate_expected_packets_ = 0; - } else { - // Early return without updating estimate. + // Don't generate a loss rate until it can be based on enough packets. + if (expected_packets_since_last_loss_update_ < kLimitNumPackets) return; - } + + has_decreased_since_last_fraction_loss_ = false; + last_fraction_loss_ = lost_packets_since_last_loss_update_Q8_ / + expected_packets_since_last_loss_update_; + + // Reset accumulators. + lost_packets_since_last_loss_update_Q8_ = 0; + expected_packets_since_last_loss_update_ = 0; } time_last_receiver_block_ms_ = now_ms; UpdateEstimate(now_ms); @@ -186,7 +185,9 @@ void SendSideBandwidthEstimation::UpdateEstimate(int64_t now_ms) { } UpdateMinHistory(now_ms); // Only start updating bitrate when receiving receiver blocks. - if (time_last_receiver_block_ms_ != 0) { + // TODO(pbos): Handle the case when no receiver report is received for a very + // long time. + if (time_last_receiver_block_ms_ != -1) { if (last_fraction_loss_ <= 5) { // Loss < 2%: Increase rate by 8% of the min bitrate in the last // kBweIncreaseIntervalMs. @@ -207,12 +208,12 @@ void SendSideBandwidthEstimation::UpdateEstimate(int64_t now_ms) { } else if (last_fraction_loss_ <= 26) { // Loss between 2% - 10%: Do nothing. - } else { // Loss > 10%: Limit the rate decreases to once a kBweDecreaseIntervalMs + // rtt. - if ((now_ms - time_last_decrease_ms_) >= - (kBweDecreaseIntervalMs + last_round_trip_time_ms_)) { + if (!has_decreased_since_last_fraction_loss_ && + (now_ms - time_last_decrease_ms_) >= + (kBweDecreaseIntervalMs + last_round_trip_time_ms_)) { time_last_decrease_ms_ = now_ms; // Reduce rate: @@ -221,6 +222,7 @@ void SendSideBandwidthEstimation::UpdateEstimate(int64_t now_ms) { bitrate_ = static_cast( (bitrate_ * static_cast(512 - last_fraction_loss_)) / 512.0); + has_decreased_since_last_fraction_loss_ = true; } } } diff --git a/webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.h b/webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.h index f50ad5184a..74ac1fcb85 100644 --- a/webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.h +++ b/webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.h @@ -61,14 +61,15 @@ class SendSideBandwidthEstimation { std::deque > min_bitrate_history_; // incoming filters - int accumulate_lost_packets_Q8_; - int accumulate_expected_packets_; + int lost_packets_since_last_loss_update_Q8_; + int expected_packets_since_last_loss_update_; uint32_t bitrate_; uint32_t min_bitrate_configured_; uint32_t max_bitrate_configured_; int64_t last_low_bitrate_log_ms_; + bool has_decreased_since_last_fraction_loss_; int64_t time_last_receiver_block_ms_; uint8_t last_fraction_loss_; int64_t last_round_trip_time_ms_; diff --git a/webrtc/modules/bitrate_controller/send_side_bandwidth_estimation_unittest.cc b/webrtc/modules/bitrate_controller/send_side_bandwidth_estimation_unittest.cc index 75384ae284..0424d22bd6 100644 --- a/webrtc/modules/bitrate_controller/send_side_bandwidth_estimation_unittest.cc +++ b/webrtc/modules/bitrate_controller/send_side_bandwidth_estimation_unittest.cc @@ -44,4 +44,55 @@ TEST(SendSideBweTest, InitialRembWithProbing) { bwe.CurrentEstimate(&bitrate, &fraction_loss, &rtt); EXPECT_EQ(kRembBps, bitrate); } + +TEST(SendSideBweTest, DoesntReapplyBitrateDecreaseWithoutFollowingRemb) { + SendSideBandwidthEstimation bwe; + static const int kMinBitrateBps = 100000; + static const int kInitialBitrateBps = 1000000; + bwe.SetMinMaxBitrate(kMinBitrateBps, 1500000); + bwe.SetSendBitrate(kInitialBitrateBps); + + static const uint8_t kFractionLoss = 128; + static const int64_t kRttMs = 50; + + int64_t now_ms = 0; + int bitrate_bps; + uint8_t fraction_loss; + int64_t rtt_ms; + bwe.CurrentEstimate(&bitrate_bps, &fraction_loss, &rtt_ms); + EXPECT_EQ(kInitialBitrateBps, bitrate_bps); + EXPECT_EQ(0, fraction_loss); + EXPECT_EQ(0, rtt_ms); + + // Signal heavy loss to go down in bitrate. + bwe.UpdateReceiverBlock(kFractionLoss, kRttMs, 100, now_ms); + // Trigger an update 2 seconds later to not be rate limited. + now_ms += 2000; + bwe.UpdateEstimate(now_ms); + + bwe.CurrentEstimate(&bitrate_bps, &fraction_loss, &rtt_ms); + EXPECT_LT(bitrate_bps, kInitialBitrateBps); + // Verify that the obtained bitrate isn't hitting the min bitrate, or this + // test doesn't make sense. If this ever happens, update the thresholds or + // loss rates so that it doesn't hit min bitrate after one bitrate update. + EXPECT_GT(bitrate_bps, kMinBitrateBps); + EXPECT_EQ(kFractionLoss, fraction_loss); + EXPECT_EQ(kRttMs, rtt_ms); + + // Triggering an update shouldn't apply further downgrade nor upgrade since + // there's no intermediate receiver block received indicating whether this is + // currently good or not. + int last_bitrate_bps = bitrate_bps; + // Trigger an update 2 seconds later to not be rate limited (but it still + // shouldn't update). + now_ms += 2000; + bwe.UpdateEstimate(now_ms); + bwe.CurrentEstimate(&bitrate_bps, &fraction_loss, &rtt_ms); + + EXPECT_EQ(last_bitrate_bps, bitrate_bps); + // The old loss rate should still be applied though. + EXPECT_EQ(kFractionLoss, fraction_loss); + EXPECT_EQ(kRttMs, rtt_ms); +} + } // namespace webrtc