From 8743db986574f6c860073cc68e27db9528b48dfe Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Mon, 25 Jul 2022 14:57:55 +0200 Subject: [PATCH] Cleanup congestion controller min bitrate Replace helper functions with the constant Remove option to set min bitrate in RemoteBitrateEstimator as unused: ReceivedSideCongestionController is the only user of the RemoteBitrateEstimator interface, and it always sets the same value right after construction that RemoteBitreateEstimators already use. Bug: None Change-Id: If179fdd72b1ded6ad1fd0a6dfffc97b302153322 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/269383 Reviewed-by: Philip Eliasson Commit-Queue: Danil Chapovalov Cr-Commit-Position: refs/heads/main@{#37613} --- .../goog_cc/goog_cc_network_control.cc | 5 ++--- .../goog_cc/loss_based_bwe_v2.cc | 2 +- .../goog_cc/send_side_bandwidth_estimation.cc | 8 +++----- .../include/receive_side_congestion_controller.h | 1 - .../receive_side_congestion_controller.cc | 4 +--- .../remote_bitrate_estimator/aimd_rate_control.cc | 2 +- modules/remote_bitrate_estimator/bwe_defines.cc | 12 ------------ .../remote_bitrate_estimator/include/bwe_defines.h | 5 +---- .../include/remote_bitrate_estimator.h | 2 -- .../remote_bitrate_estimator_abs_send_time.cc | 6 ------ .../remote_bitrate_estimator_abs_send_time.h | 1 - .../remote_bitrate_estimator_single_stream.cc | 5 ----- .../remote_bitrate_estimator_single_stream.h | 1 - 13 files changed, 9 insertions(+), 45 deletions(-) diff --git a/modules/congestion_controller/goog_cc/goog_cc_network_control.cc b/modules/congestion_controller/goog_cc/goog_cc_network_control.cc index bce192966a..87d16c0980 100644 --- a/modules/congestion_controller/goog_cc/goog_cc_network_control.cc +++ b/modules/congestion_controller/goog_cc/goog_cc_network_control.cc @@ -132,7 +132,7 @@ GoogCcNetworkController::GoogCcNetworkController(NetworkControllerConfig config, {&safe_reset_on_route_change_, &safe_reset_acknowledged_rate_}, key_value_config_->Lookup("WebRTC-Bwe-SafeResetOnRouteChange")); if (delay_based_bwe_) - delay_based_bwe_->SetMinBitrate(congestion_controller::GetMinBitrate()); + delay_based_bwe_->SetMinBitrate(kCongestionControllerMinBitrate); } GoogCcNetworkController::~GoogCcNetworkController() {} @@ -343,8 +343,7 @@ void GoogCcNetworkController::ClampConstraints() { // TODO(holmer): We should make sure the default bitrates are set to 10 kbps, // and that we don't try to set the min bitrate to 0 from any applications. // The congestion controller should allow a min bitrate of 0. - min_data_rate_ = - std::max(min_target_rate_, congestion_controller::GetMinBitrate()); + min_data_rate_ = std::max(min_target_rate_, kCongestionControllerMinBitrate); if (use_min_allocatable_as_lower_bound_) { min_data_rate_ = std::max(min_data_rate_, min_total_allocated_bitrate_); } diff --git a/modules/congestion_controller/goog_cc/loss_based_bwe_v2.cc b/modules/congestion_controller/goog_cc/loss_based_bwe_v2.cc index a0b32181af..c8c69f34b5 100644 --- a/modules/congestion_controller/goog_cc/loss_based_bwe_v2.cc +++ b/modules/congestion_controller/goog_cc/loss_based_bwe_v2.cc @@ -254,7 +254,7 @@ void LossBasedBweV2::UpdateBandwidthEstimate( recovering_after_loss_timestamp_ + config_->delayed_increase_window < last_send_time_most_recent_observation_)) { bandwidth_limit_in_current_window_ = std::max( - congestion_controller::GetMinBitrate(), + kCongestionControllerMinBitrate, best_candidate.loss_limited_bandwidth * config_->max_increase_factor); recovering_after_loss_timestamp_ = last_send_time_most_recent_observation_; } diff --git a/modules/congestion_controller/goog_cc/send_side_bandwidth_estimation.cc b/modules/congestion_controller/goog_cc/send_side_bandwidth_estimation.cc index 4a25446272..cc117b998a 100644 --- a/modules/congestion_controller/goog_cc/send_side_bandwidth_estimation.cc +++ b/modules/congestion_controller/goog_cc/send_side_bandwidth_estimation.cc @@ -204,8 +204,7 @@ SendSideBandwidthEstimation::SendSideBandwidthEstimation( expected_packets_since_last_loss_update_(0), current_target_(DataRate::Zero()), last_logged_target_(DataRate::Zero()), - min_bitrate_configured_( - DataRate::BitsPerSec(congestion_controller::GetMinBitrateBps())), + min_bitrate_configured_(kCongestionControllerMinBitrate), max_bitrate_configured_(kDefaultMaxBitrate), last_low_bitrate_log_(Timestamp::MinusInfinity()), has_decreased_since_last_fraction_loss_(false), @@ -253,8 +252,7 @@ void SendSideBandwidthEstimation::OnRouteChange() { lost_packets_since_last_loss_update_ = 0; expected_packets_since_last_loss_update_ = 0; current_target_ = DataRate::Zero(); - min_bitrate_configured_ = - DataRate::BitsPerSec(congestion_controller::GetMinBitrateBps()); + min_bitrate_configured_ = kCongestionControllerMinBitrate; max_bitrate_configured_ = kDefaultMaxBitrate; last_low_bitrate_log_ = Timestamp::MinusInfinity(); has_decreased_since_last_fraction_loss_ = false; @@ -300,7 +298,7 @@ void SendSideBandwidthEstimation::SetSendBitrate(DataRate bitrate, void SendSideBandwidthEstimation::SetMinMaxBitrate(DataRate min_bitrate, DataRate max_bitrate) { min_bitrate_configured_ = - std::max(min_bitrate, congestion_controller::GetMinBitrate()); + std::max(min_bitrate, kCongestionControllerMinBitrate); if (max_bitrate > DataRate::Zero() && max_bitrate.IsFinite()) { max_bitrate_configured_ = std::max(min_bitrate_configured_, max_bitrate); } else { diff --git a/modules/congestion_controller/include/receive_side_congestion_controller.h b/modules/congestion_controller/include/receive_side_congestion_controller.h index 8575f2456b..96ee8a6e3d 100644 --- a/modules/congestion_controller/include/receive_side_congestion_controller.h +++ b/modules/congestion_controller/include/receive_side_congestion_controller.h @@ -86,7 +86,6 @@ class ReceiveSideCongestionController : public CallStatsObserver { std::unique_ptr rbe_ RTC_GUARDED_BY(mutex_); bool using_absolute_send_time_ RTC_GUARDED_BY(mutex_); uint32_t packets_since_absolute_send_time_ RTC_GUARDED_BY(mutex_); - int min_bitrate_bps_ RTC_GUARDED_BY(mutex_); }; } // namespace webrtc diff --git a/modules/congestion_controller/receive_side_congestion_controller.cc b/modules/congestion_controller/receive_side_congestion_controller.cc index 51b96ef2d3..4f238835e4 100644 --- a/modules/congestion_controller/receive_side_congestion_controller.cc +++ b/modules/congestion_controller/receive_side_congestion_controller.cc @@ -74,7 +74,6 @@ void ReceiveSideCongestionController::PickEstimator() { rbe_ = std::make_unique( &remb_throttler_, &clock_); } - rbe_->SetMinBitrate(min_bitrate_bps_); } ReceiveSideCongestionController::ReceiveSideCongestionController( @@ -89,8 +88,7 @@ ReceiveSideCongestionController::ReceiveSideCongestionController( network_state_estimator), rbe_(new RemoteBitrateEstimatorSingleStream(&remb_throttler_, clock)), using_absolute_send_time_(false), - packets_since_absolute_send_time_(0), - min_bitrate_bps_(congestion_controller::GetMinBitrateBps()) {} + packets_since_absolute_send_time_(0) {} void ReceiveSideCongestionController::OnReceivedPacket( int64_t arrival_time_ms, diff --git a/modules/remote_bitrate_estimator/aimd_rate_control.cc b/modules/remote_bitrate_estimator/aimd_rate_control.cc index 8f9c33c7f6..b625a745df 100644 --- a/modules/remote_bitrate_estimator/aimd_rate_control.cc +++ b/modules/remote_bitrate_estimator/aimd_rate_control.cc @@ -70,7 +70,7 @@ AimdRateControl::AimdRateControl(const FieldTrialsView* key_value_config) AimdRateControl::AimdRateControl(const FieldTrialsView* key_value_config, bool send_side) - : min_configured_bitrate_(congestion_controller::GetMinBitrate()), + : min_configured_bitrate_(kCongestionControllerMinBitrate), max_configured_bitrate_(DataRate::KilobitsPerSec(30000)), current_bitrate_(max_configured_bitrate_), latest_estimated_throughput_(current_bitrate_), diff --git a/modules/remote_bitrate_estimator/bwe_defines.cc b/modules/remote_bitrate_estimator/bwe_defines.cc index 22c605e82d..db92f46717 100644 --- a/modules/remote_bitrate_estimator/bwe_defines.cc +++ b/modules/remote_bitrate_estimator/bwe_defines.cc @@ -14,18 +14,6 @@ namespace webrtc { const char kBweTypeHistogram[] = "WebRTC.BWE.Types"; -namespace congestion_controller { -int GetMinBitrateBps() { - constexpr int kMinBitrateBps = 5000; - return kMinBitrateBps; -} - -DataRate GetMinBitrate() { - return DataRate::BitsPerSec(GetMinBitrateBps()); -} - -} // namespace congestion_controller - RateControlInput::RateControlInput( BandwidthUsage bw_state, const absl::optional& estimated_throughput) diff --git a/modules/remote_bitrate_estimator/include/bwe_defines.h b/modules/remote_bitrate_estimator/include/bwe_defines.h index b3ca1846f4..d3dd96be75 100644 --- a/modules/remote_bitrate_estimator/include/bwe_defines.h +++ b/modules/remote_bitrate_estimator/include/bwe_defines.h @@ -19,10 +19,7 @@ namespace webrtc { -namespace congestion_controller { -int GetMinBitrateBps(); -DataRate GetMinBitrate(); -} // namespace congestion_controller +constexpr DataRate kCongestionControllerMinBitrate = DataRate::BitsPerSec(5000); static const int64_t kBitrateWindowMs = 1000; diff --git a/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h b/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h index d45f8708b9..0d4e15e9e1 100644 --- a/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h +++ b/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h @@ -58,8 +58,6 @@ class RemoteBitrateEstimator : public CallStatsObserver { // Returns latest estimate or DataRate::Zero() if estimation is unavailable. virtual DataRate LatestEstimate() const = 0; - virtual void SetMinBitrate(int min_bitrate_bps) = 0; - virtual TimeDelta Process() = 0; protected: diff --git a/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc b/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc index 934190c46b..e9fb1b99f6 100644 --- a/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc +++ b/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc @@ -396,10 +396,4 @@ DataRate RemoteBitrateEstimatorAbsSendTime::LatestEstimate() const { return remote_rate_.LatestEstimate(); } -void RemoteBitrateEstimatorAbsSendTime::SetMinBitrate(int min_bitrate_bps) { - // Called from both the configuration thread and the network thread. Shouldn't - // be called from the network thread in the future. - MutexLock lock(&mutex_); - remote_rate_.SetMinBitrate(DataRate::BitsPerSec(min_bitrate_bps)); -} } // namespace webrtc diff --git a/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.h b/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.h index c5ac10fe08..fd33c84b04 100644 --- a/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.h +++ b/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.h @@ -59,7 +59,6 @@ class RemoteBitrateEstimatorAbsSendTime : public RemoteBitrateEstimator { void OnRttUpdate(int64_t avg_rtt_ms, int64_t max_rtt_ms) override; void RemoveStream(uint32_t ssrc) override; DataRate LatestEstimate() const override; - void SetMinBitrate(int min_bitrate_bps) override; private: struct Probe { diff --git a/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.cc b/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.cc index 3146b9e439..6f442e5e2c 100644 --- a/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.cc +++ b/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.cc @@ -249,9 +249,4 @@ AimdRateControl* RemoteBitrateEstimatorSingleStream::GetRemoteRate() { return remote_rate_.get(); } -void RemoteBitrateEstimatorSingleStream::SetMinBitrate(int min_bitrate_bps) { - MutexLock lock(&mutex_); - remote_rate_->SetMinBitrate(DataRate::BitsPerSec(min_bitrate_bps)); -} - } // namespace webrtc diff --git a/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.h b/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.h index 4efeb3b80e..d62f922e02 100644 --- a/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.h +++ b/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.h @@ -53,7 +53,6 @@ class RemoteBitrateEstimatorSingleStream : public RemoteBitrateEstimator { void OnRttUpdate(int64_t avg_rtt_ms, int64_t max_rtt_ms) override; void RemoveStream(uint32_t ssrc) override; DataRate LatestEstimate() const override; - void SetMinBitrate(int min_bitrate_bps) override; private: struct Detector;