From f34116e3565d1f20025514dbc28603a2389533f7 Mon Sep 17 00:00:00 2001 From: Sebastian Jansson Date: Tue, 24 Sep 2019 17:55:50 +0200 Subject: [PATCH] Replacing bandwidth adaptation trial with stable target in Opus encoder. This also means that the NetworkEstimate::bandwidth can be deprecated as it's currently just a copy of the target_rate. Bug: webrtc:10981 Change-Id: I1bc57b98480bd77ce052736b19d630c775428546 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/153669 Commit-Queue: Sebastian Jansson Reviewed-by: Oskar Sundbom Reviewed-by: Per Kjellander Cr-Commit-Position: refs/heads/master@{#29288} --- api/call/bitrate_allocation.h | 2 - api/transport/BUILD.gn | 1 + api/transport/network_types.h | 2 + call/bitrate_allocator.cc | 11 --- call/bitrate_allocator.h | 1 - call/call.cc | 2 +- call/rtp_transport_controller_send.cc | 3 +- .../codecs/opus/audio_encoder_opus.cc | 68 +++++++++---------- .../codecs/opus/audio_encoder_opus.h | 3 +- .../bbr/bbr_network_controller.cc | 1 - .../goog_cc/goog_cc_network_control.cc | 2 - .../goog_cc_network_control_unittest.cc | 2 +- .../goog_cc/test/goog_cc_printer.cc | 1 - .../pcc/pcc_network_controller.cc | 1 - test/scenario/call_client.cc | 5 -- test/scenario/call_client.h | 1 - 16 files changed, 39 insertions(+), 67 deletions(-) diff --git a/api/call/bitrate_allocation.h b/api/call/bitrate_allocation.h index c52969b691..24530c9755 100644 --- a/api/call/bitrate_allocation.h +++ b/api/call/bitrate_allocation.h @@ -32,8 +32,6 @@ struct BitrateAllocationUpdate { double packet_loss_ratio = 0; // Predicted round trip time. TimeDelta round_trip_time = TimeDelta::PlusInfinity(); - // |link_capacity| is deprecated, use |stable_target_bitrate| instead. - DataRate link_capacity = DataRate::Zero(); // |bwe_period| is deprecated, use |stable_target_bitrate| allocation instead. TimeDelta bwe_period = TimeDelta::PlusInfinity(); }; diff --git a/api/transport/BUILD.gn b/api/transport/BUILD.gn index 972340713b..365e5aeeee 100644 --- a/api/transport/BUILD.gn +++ b/api/transport/BUILD.gn @@ -37,6 +37,7 @@ rtc_static_library("network_control") { deps = [ ":webrtc_key_value_config", + "../../rtc_base:deprecation", "../units:data_rate", "../units:data_size", "../units:time_delta", diff --git a/api/transport/network_types.h b/api/transport/network_types.h index c8c6d3c00d..320a7c07c2 100644 --- a/api/transport/network_types.h +++ b/api/transport/network_types.h @@ -19,6 +19,7 @@ #include "api/units/data_size.h" #include "api/units/time_delta.h" #include "api/units/timestamp.h" +#include "rtc_base/deprecation.h" namespace webrtc { @@ -179,6 +180,7 @@ struct TransportPacketsFeedback { struct NetworkEstimate { Timestamp at_time = Timestamp::PlusInfinity(); + // Deprecated, use TargetTransferRate::target_rate instead. DataRate bandwidth = DataRate::Infinity(); TimeDelta round_trip_time = TimeDelta::PlusInfinity(); TimeDelta bwe_period = TimeDelta::PlusInfinity(); diff --git a/call/bitrate_allocator.cc b/call/bitrate_allocator.cc index 5362ceec5f..989f70cb0d 100644 --- a/call/bitrate_allocator.cc +++ b/call/bitrate_allocator.cc @@ -56,7 +56,6 @@ BitrateAllocator::BitrateAllocator(LimitObserver* limit_observer) : limit_observer_(limit_observer), last_target_bps_(0), last_stable_target_bps_(0), - last_bandwidth_bps_(0), last_non_zero_bitrate_bps_(kDefaultBitrateBps), last_fraction_loss_(0), last_rtt_(0), @@ -95,7 +94,6 @@ uint8_t BitrateAllocator::GetTransmissionMaxBitrateMultiplier() { void BitrateAllocator::OnNetworkEstimateChanged(TargetTransferRate msg) { RTC_DCHECK_RUN_ON(&sequenced_checker_); last_target_bps_ = msg.target_rate.bps(); - last_bandwidth_bps_ = msg.network_estimate.bandwidth.bps(); last_stable_target_bps_ = msg.stable_target_rate.bps(); last_non_zero_bitrate_bps_ = last_target_bps_ > 0 ? last_target_bps_ : last_non_zero_bitrate_bps_; @@ -114,20 +112,16 @@ void BitrateAllocator::OnNetworkEstimateChanged(TargetTransferRate msg) { } ObserverAllocation allocation = AllocateBitrates(last_target_bps_); - ObserverAllocation bandwidth_allocation = - AllocateBitrates(last_bandwidth_bps_); ObserverAllocation stable_bitrate_allocation = AllocateBitrates(last_stable_target_bps_); for (auto& config : allocatable_tracks_) { uint32_t allocated_bitrate = allocation[config.observer]; - uint32_t allocated_bandwidth = bandwidth_allocation[config.observer]; uint32_t allocated_stable_target_rate = stable_bitrate_allocation[config.observer]; BitrateAllocationUpdate update; update.target_bitrate = DataRate::bps(allocated_bitrate); update.stable_target_bitrate = DataRate::bps(allocated_stable_target_rate); - update.link_capacity = DataRate::bps(allocated_bandwidth); update.packet_loss_ratio = last_fraction_loss_ / 256.0; update.round_trip_time = TimeDelta::ms(last_rtt_); update.bwe_period = TimeDelta::ms(last_bwe_period_ms_); @@ -183,19 +177,15 @@ void BitrateAllocator::AddObserver(BitrateAllocatorObserver* observer, // Calculate a new allocation and update all observers. ObserverAllocation allocation = AllocateBitrates(last_target_bps_); - ObserverAllocation bandwidth_allocation = - AllocateBitrates(last_bandwidth_bps_); ObserverAllocation stable_bitrate_allocation = AllocateBitrates(last_stable_target_bps_); for (auto& config : allocatable_tracks_) { uint32_t allocated_bitrate = allocation[config.observer]; uint32_t allocated_stable_bitrate = stable_bitrate_allocation[config.observer]; - uint32_t bandwidth = bandwidth_allocation[config.observer]; BitrateAllocationUpdate update; update.target_bitrate = DataRate::bps(allocated_bitrate); update.stable_target_bitrate = DataRate::bps(allocated_stable_bitrate); - update.link_capacity = DataRate::bps(bandwidth); update.packet_loss_ratio = last_fraction_loss_ / 256.0; update.round_trip_time = TimeDelta::ms(last_rtt_); update.bwe_period = TimeDelta::ms(last_bwe_period_ms_); @@ -212,7 +202,6 @@ void BitrateAllocator::AddObserver(BitrateAllocatorObserver* observer, BitrateAllocationUpdate update; update.target_bitrate = DataRate::Zero(); update.stable_target_bitrate = DataRate::Zero(); - update.link_capacity = DataRate::Zero(); update.packet_loss_ratio = last_fraction_loss_ / 256.0; update.round_trip_time = TimeDelta::ms(last_rtt_); update.bwe_period = TimeDelta::ms(last_bwe_period_ms_); diff --git a/call/bitrate_allocator.h b/call/bitrate_allocator.h index bad601624f..b7d77d9fb2 100644 --- a/call/bitrate_allocator.h +++ b/call/bitrate_allocator.h @@ -199,7 +199,6 @@ class BitrateAllocator : public BitrateAllocatorInterface { RTC_GUARDED_BY(&sequenced_checker_); uint32_t last_target_bps_ RTC_GUARDED_BY(&sequenced_checker_); uint32_t last_stable_target_bps_ RTC_GUARDED_BY(&sequenced_checker_); - uint32_t last_bandwidth_bps_ RTC_GUARDED_BY(&sequenced_checker_); uint32_t last_non_zero_bitrate_bps_ RTC_GUARDED_BY(&sequenced_checker_); uint8_t last_fraction_loss_ RTC_GUARDED_BY(&sequenced_checker_); int64_t last_rtt_ RTC_GUARDED_BY(&sequenced_checker_); diff --git a/call/call.cc b/call/call.cc index f816cb5ea5..971ebbdd1e 100644 --- a/call/call.cc +++ b/call/call.cc @@ -1065,7 +1065,7 @@ void Call::OnTargetTransferRate(TargetTransferRate msg) { RTC_DCHECK_RUN_ON(&worker_sequence_checker_); { rtc::CritScope cs(&last_bandwidth_bps_crit_); - last_bandwidth_bps_ = msg.network_estimate.bandwidth.bps(); + last_bandwidth_bps_ = msg.target_rate.bps(); } uint32_t target_bitrate_bps = msg.target_rate.bps(); diff --git a/call/rtp_transport_controller_send.cc b/call/rtp_transport_controller_send.cc index 3b3394de3b..4e8d021144 100644 --- a/call/rtp_transport_controller_send.cc +++ b/call/rtp_transport_controller_send.cc @@ -141,8 +141,7 @@ void RtpTransportControllerSend::UpdateControlState() { absl::optional update = control_handler_->GetUpdate(); if (!update) return; - retransmission_rate_limiter_.SetMaxRate( - update->network_estimate.bandwidth.bps()); + retransmission_rate_limiter_.SetMaxRate(update->target_rate.bps()); // We won't create control_handler_ until we have an observers. RTC_DCHECK(observer_ != nullptr); observer_->OnTargetTransferRate(*update); diff --git a/modules/audio_coding/codecs/opus/audio_encoder_opus.cc b/modules/audio_coding/codecs/opus/audio_encoder_opus.cc index 70081d7e19..60af6075aa 100644 --- a/modules/audio_coding/codecs/opus/audio_encoder_opus.cc +++ b/modules/audio_coding/codecs/opus/audio_encoder_opus.cc @@ -425,8 +425,8 @@ AudioEncoderOpusImpl::AudioEncoderOpusImpl( : payload_type_(payload_type), send_side_bwe_with_overhead_( webrtc::field_trial::IsEnabled("WebRTC-SendSideBwe-WithOverhead")), - use_link_capacity_for_adaptation_(webrtc::field_trial::IsEnabled( - "WebRTC-Audio-LinkCapacityAdaptation")), + use_stable_target_for_adaptation_(webrtc::field_trial::IsEnabled( + "WebRTC-Audio-StableTargetAdaptation")), adjust_bandwidth_( webrtc::field_trial::IsEnabled("WebRTC-AdjustOpusBandwidth")), bitrate_changed_(true), @@ -563,26 +563,28 @@ void AudioEncoderOpusImpl::OnReceivedUplinkRecoverablePacketLossFraction( void AudioEncoderOpusImpl::OnReceivedUplinkBandwidth( int target_audio_bitrate_bps, absl::optional bwe_period_ms, - absl::optional link_capacity_allocation_bps) { + absl::optional stable_target_bitrate_bps) { if (audio_network_adaptor_) { audio_network_adaptor_->SetTargetAudioBitrate(target_audio_bitrate_bps); - // We give smoothed bitrate allocation to audio network adaptor as - // the uplink bandwidth. - // The BWE spikes should not affect the bitrate smoother more than 25%. - // To simplify the calculations we use a step response as input signal. - // The step response of an exponential filter is - // u(t) = 1 - e^(-t / time_constant). - // In order to limit the affect of a BWE spike within 25% of its value - // before - // the next BWE update, we would choose a time constant that fulfills - // 1 - e^(-bwe_period_ms / time_constant) < 0.25 - // Then 4 * bwe_period_ms is a good choice. - if (bwe_period_ms) - bitrate_smoother_->SetTimeConstantMs(*bwe_period_ms * 4); - bitrate_smoother_->AddSample(target_audio_bitrate_bps); - - if (link_capacity_allocation_bps) - link_capacity_allocation_bps_ = link_capacity_allocation_bps; + if (use_stable_target_for_adaptation_) { + if (stable_target_bitrate_bps) + audio_network_adaptor_->SetUplinkBandwidth(*stable_target_bitrate_bps); + } else { + // We give smoothed bitrate allocation to audio network adaptor as + // the uplink bandwidth. + // The BWE spikes should not affect the bitrate smoother more than 25%. + // To simplify the calculations we use a step response as input signal. + // The step response of an exponential filter is + // u(t) = 1 - e^(-t / time_constant). + // In order to limit the affect of a BWE spike within 25% of its value + // before + // the next BWE update, we would choose a time constant that fulfills + // 1 - e^(-bwe_period_ms / time_constant) < 0.25 + // Then 4 * bwe_period_ms is a good choice. + if (bwe_period_ms) + bitrate_smoother_->SetTimeConstantMs(*bwe_period_ms * 4); + bitrate_smoother_->AddSample(target_audio_bitrate_bps); + } ApplyAudioNetworkAdaptor(); } else if (send_side_bwe_with_overhead_) { @@ -612,7 +614,7 @@ void AudioEncoderOpusImpl::OnReceivedUplinkBandwidth( void AudioEncoderOpusImpl::OnReceivedUplinkAllocation( BitrateAllocationUpdate update) { OnReceivedUplinkBandwidth(update.target_bitrate.bps(), update.bwe_period.ms(), - update.link_capacity.bps()); + update.stable_target_bitrate.bps()); } void AudioEncoderOpusImpl::OnReceivedRtt(int rtt_ms) { @@ -857,21 +859,15 @@ AudioEncoderOpusImpl::DefaultAudioNetworkAdaptorCreator( } void AudioEncoderOpusImpl::MaybeUpdateUplinkBandwidth() { - if (audio_network_adaptor_) { - if (use_link_capacity_for_adaptation_ && link_capacity_allocation_bps_) { - audio_network_adaptor_->SetUplinkBandwidth( - *link_capacity_allocation_bps_); - } else { - int64_t now_ms = rtc::TimeMillis(); - if (!bitrate_smoother_last_update_time_ || - now_ms - *bitrate_smoother_last_update_time_ >= - config_.uplink_bandwidth_update_interval_ms) { - absl::optional smoothed_bitrate = - bitrate_smoother_->GetAverage(); - if (smoothed_bitrate) - audio_network_adaptor_->SetUplinkBandwidth(*smoothed_bitrate); - bitrate_smoother_last_update_time_ = now_ms; - } + if (audio_network_adaptor_ && !use_stable_target_for_adaptation_) { + int64_t now_ms = rtc::TimeMillis(); + if (!bitrate_smoother_last_update_time_ || + now_ms - *bitrate_smoother_last_update_time_ >= + config_.uplink_bandwidth_update_interval_ms) { + absl::optional smoothed_bitrate = bitrate_smoother_->GetAverage(); + if (smoothed_bitrate) + audio_network_adaptor_->SetUplinkBandwidth(*smoothed_bitrate); + bitrate_smoother_last_update_time_ = now_ms; } } } diff --git a/modules/audio_coding/codecs/opus/audio_encoder_opus.h b/modules/audio_coding/codecs/opus/audio_encoder_opus.h index 51db6618df..1f785a446e 100644 --- a/modules/audio_coding/codecs/opus/audio_encoder_opus.h +++ b/modules/audio_coding/codecs/opus/audio_encoder_opus.h @@ -174,7 +174,7 @@ class AudioEncoderOpusImpl final : public AudioEncoder { AudioEncoderOpusConfig config_; const int payload_type_; const bool send_side_bwe_with_overhead_; - const bool use_link_capacity_for_adaptation_; + const bool use_stable_target_for_adaptation_; const bool adjust_bandwidth_; bool bitrate_changed_; float packet_loss_rate_; @@ -192,7 +192,6 @@ class AudioEncoderOpusImpl final : public AudioEncoder { absl::optional overhead_bytes_per_packet_; const std::unique_ptr bitrate_smoother_; absl::optional bitrate_smoother_last_update_time_; - absl::optional link_capacity_allocation_bps_; int consecutive_dtx_frames_; friend struct AudioEncoderOpus; diff --git a/modules/congestion_controller/bbr/bbr_network_controller.cc b/modules/congestion_controller/bbr/bbr_network_controller.cc index c64152c943..6d66af1265 100644 --- a/modules/congestion_controller/bbr/bbr_network_controller.cc +++ b/modules/congestion_controller/bbr/bbr_network_controller.cc @@ -267,7 +267,6 @@ NetworkControlUpdate BbrNetworkController::CreateRateUpdate( TargetTransferRate target_rate_msg; target_rate_msg.network_estimate.at_time = at_time; - target_rate_msg.network_estimate.bandwidth = bandwidth; target_rate_msg.network_estimate.round_trip_time = rtt; // TODO(srte): Fill in field below with proper value. 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 fea7fc341f..78b12369ea 100644 --- a/modules/congestion_controller/goog_cc/goog_cc_network_control.cc +++ b/modules/congestion_controller/goog_cc/goog_cc_network_control.cc @@ -572,7 +572,6 @@ NetworkControlUpdate GoogCcNetworkController::GetNetworkState( NetworkControlUpdate update; update.target_rate = TargetTransferRate(); update.target_rate->network_estimate.at_time = at_time; - update.target_rate->network_estimate.bandwidth = last_raw_target_rate_; update.target_rate->network_estimate.loss_rate_ratio = last_estimated_fraction_loss_ / 255.0; update.target_rate->network_estimate.round_trip_time = rtt; @@ -635,7 +634,6 @@ void GoogCcNetworkController::MaybeTriggerOnNetworkChanged( bandwidth_estimation_->GetEstimatedLinkCapacity(), target_rate); target_rate_msg.network_estimate.at_time = at_time; target_rate_msg.network_estimate.round_trip_time = TimeDelta::ms(rtt_ms); - target_rate_msg.network_estimate.bandwidth = last_raw_target_rate_; target_rate_msg.network_estimate.loss_rate_ratio = fraction_loss / 255.0f; target_rate_msg.network_estimate.bwe_period = bwe_period; diff --git a/modules/congestion_controller/goog_cc/goog_cc_network_control_unittest.cc b/modules/congestion_controller/goog_cc/goog_cc_network_control_unittest.cc index 0da341051c..4404ae80e5 100644 --- a/modules/congestion_controller/goog_cc/goog_cc_network_control_unittest.cc +++ b/modules/congestion_controller/goog_cc/goog_cc_network_control_unittest.cc @@ -509,7 +509,7 @@ TEST_F(GoogCcNetworkControllerTest, StableEstimateDoesNotVaryInSteadyState) { // Measure variation in steady state. for (int i = 0; i < 20; ++i) { auto stable_target_rate = client->stable_target_rate(); - auto target_rate = client->link_capacity(); + auto target_rate = client->target_rate(); EXPECT_LE(stable_target_rate, target_rate); min_stable_target = std::min(min_stable_target, stable_target_rate); diff --git a/modules/congestion_controller/goog_cc/test/goog_cc_printer.cc b/modules/congestion_controller/goog_cc/test/goog_cc_printer.cc index f8f984c985..a0b3f37006 100644 --- a/modules/congestion_controller/goog_cc/test/goog_cc_printer.cc +++ b/modules/congestion_controller/goog_cc/test/goog_cc_printer.cc @@ -91,7 +91,6 @@ std::deque GoogCcStatePrinter::CreateLoggers() { }; std::deque loggers({ Log("time", [=] { return target_.at_time; }), - Log("bandwidth", [=] { return target_.network_estimate.bandwidth; }), Log("rtt", [=] { return target_.network_estimate.round_trip_time; }), Log("target", [=] { return target_.target_rate; }), Log("pacing", [=] { return pacing_.data_rate(); }), diff --git a/modules/congestion_controller/pcc/pcc_network_controller.cc b/modules/congestion_controller/pcc/pcc_network_controller.cc index 169b1476b2..9f074afa62 100644 --- a/modules/congestion_controller/pcc/pcc_network_controller.cc +++ b/modules/congestion_controller/pcc/pcc_network_controller.cc @@ -104,7 +104,6 @@ NetworkControlUpdate PccNetworkController::CreateRateUpdate( target_rate_msg.at_time = at_time; target_rate_msg.network_estimate.at_time = at_time; target_rate_msg.network_estimate.round_trip_time = rtt_tracker_.GetRtt(); - target_rate_msg.network_estimate.bandwidth = bandwidth_estimate_; // TODO(koloskova): Add correct estimate. target_rate_msg.network_estimate.loss_rate_ratio = 0; target_rate_msg.network_estimate.bwe_period = diff --git a/test/scenario/call_client.cc b/test/scenario/call_client.cc index 7118e2da37..9293d0111d 100644 --- a/test/scenario/call_client.cc +++ b/test/scenario/call_client.cc @@ -251,11 +251,6 @@ DataRate CallClient::target_rate() const { return network_controller_factory_.GetUpdate().target_rate->target_rate; } -DataRate CallClient::link_capacity() const { - return network_controller_factory_.GetUpdate() - .target_rate->network_estimate.bandwidth; -} - DataRate CallClient::stable_target_rate() const { return network_controller_factory_.GetUpdate() .target_rate->stable_target_rate; diff --git a/test/scenario/call_client.h b/test/scenario/call_client.h index 78c302d5a8..19cafe8f7c 100644 --- a/test/scenario/call_client.h +++ b/test/scenario/call_client.h @@ -108,7 +108,6 @@ class CallClient : public EmulatedNetworkReceiverInterface { } DataRate target_rate() const; DataRate stable_target_rate() const; - DataRate link_capacity() const; DataRate padding_rate() const; void OnPacketReceived(EmulatedIpPacket packet) override;