From 35fa28022941f66fb267371995fd39b641da04ef Mon Sep 17 00:00:00 2001 From: Sebastian Jansson Date: Mon, 1 Oct 2018 09:16:12 +0200 Subject: [PATCH] Adds allocated rate without feedback to new congestion controller. When bitrate is allocated to streams that does not have packet feedback, the allocated bitrate should be included in the estimate. This was previously only implemented for the old congestion controller and not for the new task queue based version. To make the behavior more robust, the responsibility for tracking this is moved to BitrateAllocator where it's handled consistently for multiple streams without feedback. Bug: webrtc:9586, webrtc:8243 Change-Id: I8af7fec23e1bdc08cc61cf1b4ff10461c3711fb0 Reviewed-on: https://webrtc-review.googlesource.com/102681 Commit-Queue: Sebastian Jansson Reviewed-by: Christoffer Rodbro Reviewed-by: Karl Wiberg Cr-Commit-Position: refs/heads/master@{#24905} --- api/transport/network_types.h | 2 ++ audio/audio_send_stream.cc | 6 ------ audio/audio_send_stream_unittest.cc | 3 ++- call/bitrate_allocator.cc | 11 ++++++++++- call/bitrate_allocator.h | 11 +++++++---- call/bitrate_allocator_unittest.cc | 1 + call/call.cc | 5 +++++ call/rtp_transport_controller_send.cc | 8 +++++++- .../bbr/bbr_network_controller.cc | 2 ++ .../goog_cc/goog_cc_network_control.cc | 3 +++ .../pcc/pcc_network_controller.cc | 4 +++- .../rtp/send_side_congestion_controller.cc | 8 +++++++- 12 files changed, 49 insertions(+), 15 deletions(-) diff --git a/api/transport/network_types.h b/api/transport/network_types.h index eee1de0708..ad20abc53d 100644 --- a/api/transport/network_types.h +++ b/api/transport/network_types.h @@ -36,6 +36,8 @@ struct StreamsConfig { absl::optional min_pacing_rate; absl::optional max_padding_rate; absl::optional max_total_allocated_bitrate; + // The send rate of traffic for which feedback is not received. + DataRate unacknowledged_rate_allocation = DataRate::Zero(); }; struct TargetRateConstraints { diff --git a/audio/audio_send_stream.cc b/audio/audio_send_stream.cc index 0f725c4da1..eb9e8dfa8d 100644 --- a/audio/audio_send_stream.cc +++ b/audio/audio_send_stream.cc @@ -410,12 +410,6 @@ uint32_t AudioSendStream::OnBitrateUpdated(uint32_t bitrate_bps, uint8_t fraction_loss, int64_t rtt, int64_t bwe_period_ms) { - // Audio transport feedback will not be reported in this mode, instead update - // acknowledged bitrate estimator with the bitrate allocated for audio. - if (webrtc::field_trial::IsEnabled("WebRTC-Audio-ABWENoTWCC")) { - transport_->SetAllocatedBitrateWithoutFeedback(bitrate_bps); - } - // A send stream may be allocated a bitrate of zero if the allocator decides // to disable it. For now we ignore this decision and keep sending on min // bitrate. diff --git a/audio/audio_send_stream_unittest.cc b/audio/audio_send_stream_unittest.cc index f5d6796af5..a767e028a4 100644 --- a/audio/audio_send_stream_unittest.cc +++ b/audio/audio_send_stream_unittest.cc @@ -75,10 +75,11 @@ const AudioCodecSpec kCodecSpecs[] = { class MockLimitObserver : public BitrateAllocator::LimitObserver { public: - MOCK_METHOD4(OnAllocationLimitsChanged, + MOCK_METHOD5(OnAllocationLimitsChanged, void(uint32_t min_send_bitrate_bps, uint32_t max_padding_bitrate_bps, uint32_t total_bitrate_bps, + uint32_t allocated_without_feedback_bps, bool has_packet_feedback)); }; diff --git a/call/bitrate_allocator.cc b/call/bitrate_allocator.cc index c7049ba211..acb19ae6fb 100644 --- a/call/bitrate_allocator.cc +++ b/call/bitrate_allocator.cc @@ -60,6 +60,7 @@ BitrateAllocator::BitrateAllocator(LimitObserver* limit_observer) total_requested_padding_bitrate_(0), total_requested_min_bitrate_(0), total_requested_max_bitrate_(0), + allocated_without_feedback_(0), has_packet_feedback_(false), bitrate_allocation_strategy_(nullptr), transmission_max_bitrate_multiplier_( @@ -191,6 +192,7 @@ void BitrateAllocator::UpdateAllocationLimits() { uint32_t total_requested_min_bitrate = 0; uint32_t total_requested_max_bitrate = 0; bool has_packet_feedback = false; + uint32_t allocated_without_feedback = 0; for (const auto& config : bitrate_observer_configs_) { uint32_t stream_padding = config.pad_up_bitrate_bps; if (config.enforce_min_bitrate) { @@ -203,11 +205,16 @@ void BitrateAllocator::UpdateAllocationLimits() { total_requested_max_bitrate += config.max_bitrate_bps; if (config.allocated_bitrate_bps > 0 && config.has_packet_feedback) has_packet_feedback = true; + // TODO(srte): Remove field trial check. + if (!config.has_packet_feedback && + field_trial::IsEnabled("WebRTC-Audio-ABWENoTWCC")) + allocated_without_feedback += config.allocated_bitrate_bps; } if (total_requested_padding_bitrate == total_requested_padding_bitrate_ && total_requested_min_bitrate == total_requested_min_bitrate_ && total_requested_max_bitrate == total_requested_max_bitrate_ && + allocated_without_feedback == allocated_without_feedback_ && has_packet_feedback == has_packet_feedback_) { return; } @@ -215,6 +222,7 @@ void BitrateAllocator::UpdateAllocationLimits() { total_requested_min_bitrate_ = total_requested_min_bitrate; total_requested_padding_bitrate_ = total_requested_padding_bitrate; total_requested_max_bitrate_ = total_requested_max_bitrate; + allocated_without_feedback_ = allocated_without_feedback; has_packet_feedback_ = has_packet_feedback; RTC_LOG(LS_INFO) << "UpdateAllocationLimits : total_requested_min_bitrate: " @@ -225,7 +233,8 @@ void BitrateAllocator::UpdateAllocationLimits() { << total_requested_max_bitrate << "bps"; limit_observer_->OnAllocationLimitsChanged( total_requested_min_bitrate, total_requested_padding_bitrate, - total_requested_max_bitrate, has_packet_feedback); + total_requested_max_bitrate, allocated_without_feedback, + has_packet_feedback); } void BitrateAllocator::RemoveObserver(BitrateAllocatorObserver* observer) { diff --git a/call/bitrate_allocator.h b/call/bitrate_allocator.h index c29ea5e089..47df7e0151 100644 --- a/call/bitrate_allocator.h +++ b/call/bitrate_allocator.h @@ -88,10 +88,12 @@ class BitrateAllocator : public BitrateAllocatorInterface { // bitrate and max padding bitrate is changed. class LimitObserver { public: - virtual void OnAllocationLimitsChanged(uint32_t min_send_bitrate_bps, - uint32_t max_padding_bitrate_bps, - uint32_t total_bitrate_bps, - bool has_packet_feedback) = 0; + virtual void OnAllocationLimitsChanged( + uint32_t min_send_bitrate_bps, + uint32_t max_padding_bitrate_bps, + uint32_t total_bitrate_bps, + uint32_t allocated_without_feedback_bps, + bool has_packet_feedback) = 0; protected: virtual ~LimitObserver() = default; @@ -242,6 +244,7 @@ class BitrateAllocator : public BitrateAllocatorInterface { uint32_t total_requested_padding_bitrate_ RTC_GUARDED_BY(&sequenced_checker_); uint32_t total_requested_min_bitrate_ RTC_GUARDED_BY(&sequenced_checker_); uint32_t total_requested_max_bitrate_ RTC_GUARDED_BY(&sequenced_checker_); + uint32_t allocated_without_feedback_ RTC_GUARDED_BY(&sequenced_checker_); bool has_packet_feedback_ RTC_GUARDED_BY(&sequenced_checker_); std::unique_ptr bitrate_allocation_strategy_ RTC_GUARDED_BY(&sequenced_checker_); diff --git a/call/bitrate_allocator_unittest.cc b/call/bitrate_allocator_unittest.cc index 7814655358..2961fd409c 100644 --- a/call/bitrate_allocator_unittest.cc +++ b/call/bitrate_allocator_unittest.cc @@ -28,6 +28,7 @@ class LimitObserverWrapper : public BitrateAllocator::LimitObserver { void OnAllocationLimitsChanged(uint32_t min_send_bitrate_bps, uint32_t max_padding_bitrate_bps, uint32_t total_bitrate_bps, + uint32_t allocated_without_feedback_bps, bool has_packet_feedback) override { OnAllocationLimitsChanged(min_send_bitrate_bps, max_padding_bitrate_bps, total_bitrate_bps); diff --git a/call/call.cc b/call/call.cc index 6e9a57805e..1062baf49d 100644 --- a/call/call.cc +++ b/call/call.cc @@ -233,6 +233,7 @@ class Call final : public webrtc::Call, void OnAllocationLimitsChanged(uint32_t min_send_bitrate_bps, uint32_t max_padding_bitrate_bps, uint32_t total_bitrate_bps, + uint32_t allocated_without_feedback_bps, bool has_packet_feedback) override; private: @@ -1107,10 +1108,14 @@ void Call::OnTargetTransferRate(TargetTransferRate msg) { void Call::OnAllocationLimitsChanged(uint32_t min_send_bitrate_bps, uint32_t max_padding_bitrate_bps, uint32_t total_bitrate_bps, + uint32_t allocated_without_feedback_bps, bool has_packet_feedback) { transport_send_ptr_->SetAllocatedSendBitrateLimits( min_send_bitrate_bps, max_padding_bitrate_bps, total_bitrate_bps); transport_send_ptr_->SetPerPacketFeedbackAvailable(has_packet_feedback); + transport_send_ptr_->SetAllocatedBitrateWithoutFeedback( + allocated_without_feedback_bps); + rtc::CritScope lock(&bitrate_crit_); min_allocated_send_bitrate_bps_ = min_send_bitrate_bps; configured_max_padding_bitrate_bps_ = max_padding_bitrate_bps; diff --git a/call/rtp_transport_controller_send.cc b/call/rtp_transport_controller_send.cc index b0dd8d8e29..92d972911d 100644 --- a/call/rtp_transport_controller_send.cc +++ b/call/rtp_transport_controller_send.cc @@ -307,6 +307,12 @@ void RtpTransportControllerSend::SetClientBitratePreferences( void RtpTransportControllerSend::SetAllocatedBitrateWithoutFeedback( uint32_t bitrate_bps) { - send_side_cc_->SetAllocatedBitrateWithoutFeedback(bitrate_bps); + // Audio transport feedback will not be reported in this mode, instead update + // acknowledged bitrate estimator with the bitrate allocated for audio. + if (field_trial::IsEnabled("WebRTC-Audio-ABWENoTWCC")) { + // TODO(srte): Make sure it's safe to always report this and remove the + // field trial check. + send_side_cc_->SetAllocatedBitrateWithoutFeedback(bitrate_bps); + } } } // namespace webrtc diff --git a/modules/congestion_controller/bbr/bbr_network_controller.cc b/modules/congestion_controller/bbr/bbr_network_controller.cc index 618e90af1e..b82ded4220 100644 --- a/modules/congestion_controller/bbr/bbr_network_controller.cc +++ b/modules/congestion_controller/bbr/bbr_network_controller.cc @@ -323,6 +323,8 @@ NetworkControlUpdate BbrNetworkController::OnProcessInterval( } NetworkControlUpdate BbrNetworkController::OnStreamsConfig(StreamsConfig msg) { + // TODO(srte): Handle unacknowledged rate allocation. + RTC_DCHECK(msg.unacknowledged_rate_allocation.IsZero()); return NetworkControlUpdate(); } 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 a984f10f32..44e119dc6f 100644 --- a/modules/congestion_controller/goog_cc/goog_cc_network_control.cc +++ b/modules/congestion_controller/goog_cc/goog_cc_network_control.cc @@ -294,6 +294,9 @@ NetworkControlUpdate GoogCcNetworkController::OnStreamsConfig( max_padding_rate_ = *msg.max_padding_rate; pacing_changed = true; } + acknowledged_bitrate_estimator_->SetAllocatedBitrateWithoutFeedback( + msg.unacknowledged_rate_allocation.bps()); + if (pacing_changed) update.pacer_config = GetPacingRates(msg.at_time); return update; diff --git a/modules/congestion_controller/pcc/pcc_network_controller.cc b/modules/congestion_controller/pcc/pcc_network_controller.cc index e158844a89..98cda7409b 100644 --- a/modules/congestion_controller/pcc/pcc_network_controller.cc +++ b/modules/congestion_controller/pcc/pcc_network_controller.cc @@ -368,7 +368,9 @@ NetworkControlUpdate PccNetworkController::OnTransportLossReport( return NetworkControlUpdate(); } -NetworkControlUpdate PccNetworkController::OnStreamsConfig(StreamsConfig) { +NetworkControlUpdate PccNetworkController::OnStreamsConfig(StreamsConfig msg) { + // TODO(srte): Handle unacknowledged rate allocation. + RTC_DCHECK(msg.unacknowledged_rate_allocation.IsZero()); return NetworkControlUpdate(); } diff --git a/modules/congestion_controller/rtp/send_side_congestion_controller.cc b/modules/congestion_controller/rtp/send_side_congestion_controller.cc index 70a8d496bb..266a5dc740 100644 --- a/modules/congestion_controller/rtp/send_side_congestion_controller.cc +++ b/modules/congestion_controller/rtp/send_side_congestion_controller.cc @@ -752,7 +752,13 @@ void SendSideCongestionController::SetPacingFactor(float pacing_factor) { } void SendSideCongestionController::SetAllocatedBitrateWithoutFeedback( - uint32_t bitrate_bps) {} + uint32_t bitrate_bps) { + task_queue_->PostTask([this, bitrate_bps]() { + RTC_DCHECK_RUN_ON(task_queue_); + streams_config_.unacknowledged_rate_allocation = DataRate::bps(bitrate_bps); + UpdateStreamsConfig(); + }); +} void SendSideCongestionController::DisablePeriodicTasks() { task_queue_->PostTask([this]() {