From e3a55672300af797cac012e65d260860156a3efb Mon Sep 17 00:00:00 2001 From: stefan Date: Mon, 13 Feb 2017 09:08:22 -0800 Subject: [PATCH] Reduce the BWE with 50% when feedback is received too late. Lateness is determined by the length of the send-side history, currently set to 60 seconds. BUG=webrtc:5079 Review-Url: https://codereview.webrtc.org/2684353004 Cr-Commit-Position: refs/heads/master@{#16588} --- .../congestion_controller.cc | 3 +- .../congestion_controller/delay_based_bwe.cc | 36 ++++++++- .../congestion_controller/delay_based_bwe.h | 2 + .../transport_feedback_adapter.cc | 19 +++-- .../transport_feedback_adapter_unittest.cc | 44 +++++++---- .../aimd_rate_control.cc | 75 ++++++++++--------- .../aimd_rate_control.h | 9 ++- .../aimd_rate_control_unittest.cc | 29 +++---- 8 files changed, 139 insertions(+), 78 deletions(-) diff --git a/webrtc/modules/congestion_controller/congestion_controller.cc b/webrtc/modules/congestion_controller/congestion_controller.cc index fd0d070ba0..81fe916e04 100644 --- a/webrtc/modules/congestion_controller/congestion_controller.cc +++ b/webrtc/modules/congestion_controller/congestion_controller.cc @@ -210,7 +210,8 @@ void CongestionController::SetBweBitrates(int min_bitrate_bps, remote_bitrate_estimator_.SetMinBitrate(min_bitrate_bps); min_bitrate_bps_ = min_bitrate_bps; - transport_feedback_adapter_.SetStartBitrate(start_bitrate_bps); + if (start_bitrate_bps > 0) + transport_feedback_adapter_.SetStartBitrate(start_bitrate_bps); transport_feedback_adapter_.SetMinBitrate(min_bitrate_bps_); MaybeTriggerOnNetworkChanged(); } diff --git a/webrtc/modules/congestion_controller/delay_based_bwe.cc b/webrtc/modules/congestion_controller/delay_based_bwe.cc index 00074a4b24..fb914828dc 100644 --- a/webrtc/modules/congestion_controller/delay_based_bwe.cc +++ b/webrtc/modules/congestion_controller/delay_based_bwe.cc @@ -49,6 +49,8 @@ constexpr double kDefaultTrendlineThresholdGain = 4.0; constexpr size_t kDefaultMedianSlopeWindowSize = 20; constexpr double kDefaultMedianSlopeThresholdGain = 4.0; +constexpr int kMaxConsecutiveFailedLookups = 5; + const char kBitrateEstimateExperiment[] = "WebRTC-ImprovedBitrateEstimate"; const char kBweTrendlineFilterExperiment[] = "WebRTC-BweTrendlineFilter"; const char kBweMedianSlopeFilterExperiment[] = "WebRTC-BweMedianSlopeFilter"; @@ -223,7 +225,8 @@ DelayBasedBwe::DelayBasedBwe(Clock* clock) trendline_threshold_gain_(kDefaultTrendlineThresholdGain), probing_interval_estimator_(&rate_control_), median_slope_window_size_(kDefaultMedianSlopeWindowSize), - median_slope_threshold_gain_(kDefaultMedianSlopeThresholdGain) { + median_slope_threshold_gain_(kDefaultMedianSlopeThresholdGain), + consecutive_delayed_feedbacks_(0) { if (in_trendline_experiment_) { ReadTrendlineFilterExperimentParameters(&trendline_window_size_, &trendline_smoothing_coeff_, @@ -256,14 +259,45 @@ DelayBasedBwe::Result DelayBasedBwe::IncomingPacketFeedbackVector( uma_recorded_ = true; } Result aggregated_result; + bool delayed_feedback = true; for (const auto& packet_info : packet_feedback_vector) { + if (packet_info.send_time_ms < 0) + continue; + delayed_feedback = false; Result result = IncomingPacketInfo(packet_info); if (result.updated) aggregated_result = result; } + if (delayed_feedback) { + ++consecutive_delayed_feedbacks_; + } else { + consecutive_delayed_feedbacks_ = 0; + } + if (consecutive_delayed_feedbacks_ >= kMaxConsecutiveFailedLookups) { + aggregated_result = + OnLongFeedbackDelay(packet_feedback_vector.back().arrival_time_ms); + consecutive_delayed_feedbacks_ = 0; + } return aggregated_result; } +DelayBasedBwe::Result DelayBasedBwe::OnLongFeedbackDelay( + int64_t arrival_time_ms) { + // Estimate should always be valid since a start bitrate always is set in the + // Call constructor. An alternative would be to return an empty Result here, + // or to estimate the throughput based on the feedback we received. + RTC_DCHECK(rate_control_.ValidEstimate()); + rate_control_.SetEstimate(rate_control_.LatestEstimate() / 2, + arrival_time_ms); + Result result; + result.updated = true; + result.probe = false; + result.target_bitrate_bps = rate_control_.LatestEstimate(); + LOG(LS_WARNING) << "Long feedback delay detected, reducing BWE to " + << result.target_bitrate_bps; + return result; +} + DelayBasedBwe::Result DelayBasedBwe::IncomingPacketInfo( const PacketInfo& info) { int64_t now_ms = clock_->TimeInMilliseconds(); diff --git a/webrtc/modules/congestion_controller/delay_based_bwe.h b/webrtc/modules/congestion_controller/delay_based_bwe.h index 6342d05927..d1a9676d59 100644 --- a/webrtc/modules/congestion_controller/delay_based_bwe.h +++ b/webrtc/modules/congestion_controller/delay_based_bwe.h @@ -80,6 +80,7 @@ class DelayBasedBwe { }; Result IncomingPacketInfo(const PacketInfo& info); + Result OnLongFeedbackDelay(int64_t arrival_time_ms); // Updates the current remote rate estimate and returns true if a valid // estimate exists. bool UpdateEstimate(int64_t packet_arrival_time_ms, @@ -108,6 +109,7 @@ class DelayBasedBwe { ProbingIntervalEstimator probing_interval_estimator_; size_t median_slope_window_size_; double median_slope_threshold_gain_; + int consecutive_delayed_feedbacks_; RTC_DISALLOW_IMPLICIT_CONSTRUCTORS(DelayBasedBwe); }; diff --git a/webrtc/modules/congestion_controller/transport_feedback_adapter.cc b/webrtc/modules/congestion_controller/transport_feedback_adapter.cc index 5c4bacbaf2..c4be2b1c0b 100644 --- a/webrtc/modules/congestion_controller/transport_feedback_adapter.cc +++ b/webrtc/modules/congestion_controller/transport_feedback_adapter.cc @@ -44,7 +44,8 @@ TransportFeedbackAdapter::TransportFeedbackAdapter( Clock* clock, BitrateController* bitrate_controller) : send_side_bwe_with_overhead_(webrtc::field_trial::FindFullName( - "WebRTC-SendSideBwe-WithOverhead") == "Enabled"), + "WebRTC-SendSideBwe-WithOverhead") == + "Enabled"), transport_overhead_bytes_per_packet_(0), send_time_history_(clock, kSendTimeHistoryWindowMs), clock_(clock), @@ -118,21 +119,25 @@ std::vector TransportFeedbackAdapter::GetPacketFeedbackVector( } last_timestamp_us_ = timestamp_us; + auto received_packets = feedback.GetReceivedPackets(); std::vector packet_feedback_vector; - packet_feedback_vector.reserve(feedback.GetReceivedPackets().size()); + packet_feedback_vector.reserve(received_packets.size()); + if (received_packets.empty()) { + LOG(LS_INFO) << "Empty transport feedback packet received."; + return packet_feedback_vector; + } { rtc::CritScope cs(&lock_); size_t failed_lookups = 0; int64_t offset_us = 0; + int64_t timestamp_ms = 0; for (const auto& packet : feedback.GetReceivedPackets()) { offset_us += packet.delta_us(); - int64_t timestamp_ms = current_offset_ms_ + (offset_us / 1000); + timestamp_ms = current_offset_ms_ + (offset_us / 1000); PacketInfo info(timestamp_ms, packet.sequence_number()); - if (send_time_history_.GetInfo(&info, true) && info.send_time_ms >= 0) { - packet_feedback_vector.push_back(info); - } else { + if (!send_time_history_.GetInfo(&info, true)) ++failed_lookups; - } + packet_feedback_vector.push_back(info); } std::sort(packet_feedback_vector.begin(), packet_feedback_vector.end(), PacketInfoComparator()); diff --git a/webrtc/modules/congestion_controller/transport_feedback_adapter_unittest.cc b/webrtc/modules/congestion_controller/transport_feedback_adapter_unittest.cc index d2e9eb340d..438a0fccc0 100644 --- a/webrtc/modules/congestion_controller/transport_feedback_adapter_unittest.cc +++ b/webrtc/modules/congestion_controller/transport_feedback_adapter_unittest.cc @@ -38,6 +38,7 @@ class TransportFeedbackAdapterTest : public ::testing::Test { virtual void SetUp() { adapter_.reset(new TransportFeedbackAdapter(&clock_, &bitrate_controller_)); adapter_->InitBwe(); + adapter_->SetStartBitrate(300000); } virtual void TearDown() { adapter_.reset(); } @@ -129,15 +130,16 @@ TEST_F(TransportFeedbackAdapterTest, AdaptsFeedbackAndPopulatesSendTimes) { ComparePacketVectors(packets, adapter_->GetTransportFeedbackVector()); } -TEST_F(TransportFeedbackAdapterTest, Timeout) { +TEST_F(TransportFeedbackAdapterTest, LongFeedbackDelays) { const int64_t kFeedbackTimeoutMs = 60001; - { + const int kMaxConsecutiveFailedLookups = 5; + for (int i = 0; i < kMaxConsecutiveFailedLookups; ++i) { std::vector packets; - packets.push_back(PacketInfo(100, 200, 0, 1500, 0)); - packets.push_back(PacketInfo(110, 210, 1, 1500, 0)); - packets.push_back(PacketInfo(120, 220, 2, 1500, 0)); - packets.push_back(PacketInfo(130, 230, 3, 1500, 1)); - packets.push_back(PacketInfo(140, 240, 4, 1500, 1)); + packets.push_back(PacketInfo(i * 100, 2 * i * 100, 0, 1500, 0)); + packets.push_back(PacketInfo(i * 100 + 10, 2 * i * 100 + 10, 1, 1500, 0)); + packets.push_back(PacketInfo(i * 100 + 20, 2 * i * 100 + 20, 2, 1500, 0)); + packets.push_back(PacketInfo(i * 100 + 30, 2 * i * 100 + 30, 3, 1500, 1)); + packets.push_back(PacketInfo(i * 100 + 40, 2 * i * 100 + 40, 4, 1500, 1)); for (const PacketInfo& packet : packets) OnSentPacket(packet); @@ -154,15 +156,25 @@ TEST_F(TransportFeedbackAdapterTest, Timeout) { feedback.Build(); clock_.AdvanceTimeMilliseconds(kFeedbackTimeoutMs); - PacketInfo later_packet(kFeedbackTimeoutMs + 140, kFeedbackTimeoutMs + 240, - 5, 1500, 1); + PacketInfo later_packet(kFeedbackTimeoutMs + i * 100 + 40, + kFeedbackTimeoutMs + i * 200 + 40, 5, 1500, 1); OnSentPacket(later_packet); adapter_->OnTransportFeedback(feedback); - EXPECT_EQ(0, rtc::checked_cast( - adapter_->GetTransportFeedbackVector().size())); + + // Check that packets have timed out. + for (PacketInfo& packet : packets) { + packet.send_time_ms = -1; + packet.payload_size = 0; + packet.probe_cluster_id = -1; + } + ComparePacketVectors(packets, adapter_->GetTransportFeedbackVector()); } + // Target bitrate should have halved due to feedback delays. + EXPECT_EQ(150000u, target_bitrate_bps_); + + // Test with feedback that isn't late enough to time out. { std::vector packets; packets.push_back(PacketInfo(100, 200, 0, 1500, 0)); @@ -225,8 +237,14 @@ TEST_F(TransportFeedbackAdapterTest, HandlesDroppedPackets) { feedback.Build(); std::vector expected_packets( - packets.begin() + kSendSideDropBefore, - packets.begin() + kReceiveSideDropAfter + 1); + packets.begin(), packets.begin() + kReceiveSideDropAfter + 1); + // Packets that have timed out on the send-side have lost the + // information stored on the send-side. + for (size_t i = 0; i < kSendSideDropBefore; ++i) { + expected_packets[i].send_time_ms = -1; + expected_packets[i].probe_cluster_id = -1; + expected_packets[i].payload_size = 0; + } adapter_->OnTransportFeedback(feedback); ComparePacketVectors(expected_packets, diff --git a/webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc b/webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc index fdee6df867..6b73864037 100644 --- a/webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc +++ b/webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc @@ -133,7 +133,8 @@ void AimdRateControl::Update(const RateControlInput* input, int64_t now_ms) { void AimdRateControl::SetEstimate(int bitrate_bps, int64_t now_ms) { updated_ = true; bitrate_is_initialized_ = true; - current_bitrate_bps_ = ChangeBitrate(bitrate_bps, bitrate_bps, now_ms); + current_bitrate_bps_ = ClampBitrate(bitrate_bps, bitrate_bps); + time_last_bitrate_change_ = now_ms; } int AimdRateControl::GetNearMaxIncreaseRateBps() const { @@ -153,7 +154,7 @@ rtc::Optional AimdRateControl::GetLastBitrateDecreaseBps() const { return last_decrease_; } -uint32_t AimdRateControl::ChangeBitrate(uint32_t current_bitrate_bps, +uint32_t AimdRateControl::ChangeBitrate(uint32_t new_bitrate_bps, uint32_t incoming_bitrate_bps, int64_t now_ms) { if (!updated_) { @@ -186,11 +187,11 @@ uint32_t AimdRateControl::ChangeBitrate(uint32_t current_bitrate_bps, if (rate_control_region_ == kRcNearMax) { uint32_t additive_increase_bps = AdditiveRateIncrease(now_ms, time_last_bitrate_change_); - current_bitrate_bps += additive_increase_bps; + new_bitrate_bps += additive_increase_bps; } else { uint32_t multiplicative_increase_bps = MultiplicativeRateIncrease( - now_ms, time_last_bitrate_change_, current_bitrate_bps); - current_bitrate_bps += multiplicative_increase_bps; + now_ms, time_last_bitrate_change_, new_bitrate_bps); + new_bitrate_bps += multiplicative_increase_bps; } time_last_bitrate_change_ = now_ms; @@ -198,35 +199,30 @@ uint32_t AimdRateControl::ChangeBitrate(uint32_t current_bitrate_bps, case kRcDecrease: bitrate_is_initialized_ = true; - if (incoming_bitrate_bps < min_configured_bitrate_bps_) { - current_bitrate_bps = min_configured_bitrate_bps_; - } else { - // Set bit rate to something slightly lower than max - // to get rid of any self-induced delay. - current_bitrate_bps = static_cast(beta_ * - incoming_bitrate_bps + 0.5); - if (current_bitrate_bps > current_bitrate_bps_) { - // Avoid increasing the rate when over-using. - if (rate_control_region_ != kRcMaxUnknown) { - current_bitrate_bps = static_cast( - beta_ * avg_max_bitrate_kbps_ * 1000 + 0.5f); - } - current_bitrate_bps = std::min(current_bitrate_bps, - current_bitrate_bps_); + // Set bit rate to something slightly lower than max + // to get rid of any self-induced delay. + new_bitrate_bps = + static_cast(beta_ * incoming_bitrate_bps + 0.5); + if (new_bitrate_bps > current_bitrate_bps_) { + // Avoid increasing the rate when over-using. + if (rate_control_region_ != kRcMaxUnknown) { + new_bitrate_bps = static_cast( + beta_ * avg_max_bitrate_kbps_ * 1000 + 0.5f); } - ChangeRegion(kRcNearMax); - - if (incoming_bitrate_bps < current_bitrate_bps_) { - last_decrease_ = - rtc::Optional(current_bitrate_bps_ - current_bitrate_bps); - } - if (incoming_bitrate_kbps < avg_max_bitrate_kbps_ - - 3 * std_max_bit_rate) { - avg_max_bitrate_kbps_ = -1.0f; - } - - UpdateMaxBitRateEstimate(incoming_bitrate_kbps); + new_bitrate_bps = std::min(new_bitrate_bps, current_bitrate_bps_); } + ChangeRegion(kRcNearMax); + + if (incoming_bitrate_bps < current_bitrate_bps_) { + last_decrease_ = + rtc::Optional(current_bitrate_bps_ - new_bitrate_bps); + } + if (incoming_bitrate_kbps < + avg_max_bitrate_kbps_ - 3 * std_max_bit_rate) { + avg_max_bitrate_kbps_ = -1.0f; + } + + UpdateMaxBitRateEstimate(incoming_bitrate_kbps); // Stay on hold until the pipes are cleared. ChangeState(kRcHold); time_last_bitrate_change_ = now_ms; @@ -235,17 +231,22 @@ uint32_t AimdRateControl::ChangeBitrate(uint32_t current_bitrate_bps, default: assert(false); } + return ClampBitrate(new_bitrate_bps, incoming_bitrate_bps); +} + +uint32_t AimdRateControl::ClampBitrate(uint32_t new_bitrate_bps, + uint32_t incoming_bitrate_bps) const { // Don't change the bit rate if the send side is too far off. // We allow a bit more lag at very low rates to not too easily get stuck if // the encoder produces uneven outputs. const uint32_t max_bitrate_bps = static_cast(1.5f * incoming_bitrate_bps) + 10000; - if (current_bitrate_bps > current_bitrate_bps_ && - current_bitrate_bps > max_bitrate_bps) { - current_bitrate_bps = std::max(current_bitrate_bps_, max_bitrate_bps); - time_last_bitrate_change_ = now_ms; + if (new_bitrate_bps > current_bitrate_bps_ && + new_bitrate_bps > max_bitrate_bps) { + new_bitrate_bps = std::max(current_bitrate_bps_, max_bitrate_bps); } - return current_bitrate_bps; + new_bitrate_bps = std::max(new_bitrate_bps, min_configured_bitrate_bps_); + return new_bitrate_bps; } uint32_t AimdRateControl::MultiplicativeRateIncrease( diff --git a/webrtc/modules/remote_bitrate_estimator/aimd_rate_control.h b/webrtc/modules/remote_bitrate_estimator/aimd_rate_control.h index b3c5928e05..03fc2d3f9e 100644 --- a/webrtc/modules/remote_bitrate_estimator/aimd_rate_control.h +++ b/webrtc/modules/remote_bitrate_estimator/aimd_rate_control.h @@ -58,9 +58,14 @@ class AimdRateControl { // in the "decrease" state the bitrate will be decreased to slightly below the // incoming bitrate. When in the "hold" state the bitrate will be kept // constant to allow built up queues to drain. - uint32_t ChangeBitrate(uint32_t current_bit_rate, - uint32_t incoming_bit_rate, + uint32_t ChangeBitrate(uint32_t current_bitrate, + uint32_t incoming_bitrate, int64_t now_ms); + // Clamps new_bitrate_bps to within the configured min bitrate and a linear + // function of the incoming bitrate, so that the new bitrate can't grow too + // large compared to the bitrate actually being received by the other end. + uint32_t ClampBitrate(uint32_t new_bitrate_bps, + uint32_t incoming_bitrate_bps) const; uint32_t MultiplicativeRateIncrease(int64_t now_ms, int64_t last_ms, uint32_t current_bitrate_bps) const; uint32_t AdditiveRateIncrease(int64_t now_ms, int64_t last_ms) const; diff --git a/webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc b/webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc index 878a03c4f5..c8c5121657 100644 --- a/webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc +++ b/webrtc/modules/remote_bitrate_estimator/aimd_rate_control_unittest.cc @@ -30,15 +30,6 @@ AimdRateControlStates CreateAimdRateControlStates() { return states; } -void InitBitrate(const AimdRateControlStates& states, - int bitrate, - int64_t now_ms) { - // Reduce the bitrate by 1000 to compensate for the Update after SetEstimate. - bitrate -= 1000; - - states.aimd_rate_control->SetEstimate(bitrate, now_ms); -} - void UpdateRateControl(const AimdRateControlStates& states, const BandwidthUsage& bandwidth_usage, int bitrate, @@ -54,21 +45,24 @@ void UpdateRateControl(const AimdRateControlStates& states, TEST(AimdRateControlTest, MinNearMaxIncreaseRateOnLowBandwith) { auto states = CreateAimdRateControlStates(); constexpr int kBitrate = 30000; - InitBitrate(states, kBitrate, states.simulated_clock->TimeInMilliseconds()); + states.aimd_rate_control->SetEstimate( + kBitrate, states.simulated_clock->TimeInMilliseconds()); EXPECT_EQ(4000, states.aimd_rate_control->GetNearMaxIncreaseRateBps()); } TEST(AimdRateControlTest, NearMaxIncreaseRateIs5kbpsOn90kbpsAnd200msRtt) { auto states = CreateAimdRateControlStates(); constexpr int kBitrate = 90000; - InitBitrate(states, kBitrate, states.simulated_clock->TimeInMilliseconds()); + states.aimd_rate_control->SetEstimate( + kBitrate, states.simulated_clock->TimeInMilliseconds()); EXPECT_EQ(5000, states.aimd_rate_control->GetNearMaxIncreaseRateBps()); } TEST(AimdRateControlTest, NearMaxIncreaseRateIs5kbpsOn60kbpsAnd100msRtt) { auto states = CreateAimdRateControlStates(); constexpr int kBitrate = 60000; - InitBitrate(states, kBitrate, states.simulated_clock->TimeInMilliseconds()); + states.aimd_rate_control->SetEstimate( + kBitrate, states.simulated_clock->TimeInMilliseconds()); states.aimd_rate_control->SetRtt(100); EXPECT_EQ(5000, states.aimd_rate_control->GetNearMaxIncreaseRateBps()); } @@ -82,7 +76,8 @@ TEST(AimdRateControlTest, UnknownBitrateDecreaseBeforeFirstOveruse) { TEST(AimdRateControlTest, GetLastBitrateDecrease) { auto states = CreateAimdRateControlStates(); constexpr int kBitrate = 300000; - InitBitrate(states, kBitrate, states.simulated_clock->TimeInMilliseconds()); + states.aimd_rate_control->SetEstimate( + kBitrate, states.simulated_clock->TimeInMilliseconds()); UpdateRateControl(states, kBwOverusing, kBitrate - 2000, states.simulated_clock->TimeInMilliseconds()); EXPECT_EQ(rtc::Optional(46700), @@ -92,8 +87,8 @@ TEST(AimdRateControlTest, GetLastBitrateDecrease) { TEST(AimdRateControlTest, BweLimitedByAckedBitrate) { auto states = CreateAimdRateControlStates(); constexpr int kAckedBitrate = 10000; - InitBitrate(states, kAckedBitrate, - states.simulated_clock->TimeInMilliseconds()); + states.aimd_rate_control->SetEstimate( + kAckedBitrate, states.simulated_clock->TimeInMilliseconds()); while (states.simulated_clock->TimeInMilliseconds() - kClockInitialTime < 20000) { UpdateRateControl(states, kBwNormal, kAckedBitrate, @@ -108,8 +103,8 @@ TEST(AimdRateControlTest, BweLimitedByAckedBitrate) { TEST(AimdRateControlTest, BweNotLimitedByDecreasingAckedBitrate) { auto states = CreateAimdRateControlStates(); constexpr int kAckedBitrate = 100000; - InitBitrate(states, kAckedBitrate, - states.simulated_clock->TimeInMilliseconds()); + states.aimd_rate_control->SetEstimate( + kAckedBitrate, states.simulated_clock->TimeInMilliseconds()); while (states.simulated_clock->TimeInMilliseconds() - kClockInitialTime < 20000) { UpdateRateControl(states, kBwNormal, kAckedBitrate,