From c3eb9fd49f7343ab7ea2ea49ae1fa576aae5231d Mon Sep 17 00:00:00 2001 From: Sebastian Jansson Date: Wed, 29 Jan 2020 17:42:52 +0100 Subject: [PATCH] Reland "Reland "Only include overhead if using send side bandwidth estimation."" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a reland of 086055d0fd9b9b9efe8bcf85884324a019e9bd33 ANA was accitendly disabled even when transport sequence numbers were negotiated due to a bug in how the audio send stream is configured. To solve this we simply continue to always allow enabling ANA and leave it up to the application to ensure that it's not used together with receive side estimation. Original change's description: > Reland "Only include overhead if using send side bandwidth estimation." > > This is a reland of 8c79c6e1af354c526497082c79ccbe12af03a33e > > Original change's description: > > Only include overhead if using send side bandwidth estimation. > > > > Bug: webrtc:11298 > > Change-Id: Ia2daf690461b55d394c1b964d6a7977a98be8be2 > > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/166820 > > Reviewed-by: Oskar Sundbom > > Reviewed-by: Sam Zackrisson > > Reviewed-by: Ali Tofigh > > Reviewed-by: Erik Språng > > Commit-Queue: Sebastian Jansson > > Cr-Commit-Position: refs/heads/master@{#30382} > > Bug: webrtc:11298 > Change-Id: I33205e869a8ae27c15ffe991f6d985973ed6d15a > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/167524 > Reviewed-by: Ali Tofigh > Reviewed-by: Sam Zackrisson > Reviewed-by: Erik Språng > Reviewed-by: Oskar Sundbom > Commit-Queue: Sebastian Jansson > Cr-Commit-Position: refs/heads/master@{#30390} Bug: webrtc:11298 Change-Id: If2ad91e17ebfc85dc51edcd9607996e18c5d1f13 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/167883 Commit-Queue: Sebastian Jansson Reviewed-by: Sebastian Jansson Reviewed-by: Karl Wiberg Cr-Commit-Position: refs/heads/master@{#30413} --- audio/audio_send_stream.cc | 4 +++ audio/audio_send_stream_unittest.cc | 2 ++ call/rtp_transport_controller_send.cc | 4 +++ call/rtp_transport_controller_send.h | 1 + .../rtp_transport_controller_send_interface.h | 1 + call/rtp_video_sender.cc | 12 ++++++-- call/rtp_video_sender.h | 1 + .../test/mock_rtp_transport_controller_send.h | 1 + .../codecs/opus/audio_encoder_opus.cc | 5 ++++ .../codecs/opus/audio_encoder_opus.h | 1 + modules/pacing/paced_sender.cc | 5 ++++ modules/pacing/paced_sender.h | 2 ++ modules/pacing/pacing_controller.cc | 18 +++++++----- modules/pacing/pacing_controller.h | 3 +- modules/pacing/round_robin_packet_queue.cc | 28 ++++++++++++++++--- modules/pacing/round_robin_packet_queue.h | 13 +++++++-- modules/pacing/rtp_packet_pacer.h | 1 + modules/pacing/task_queue_paced_sender.cc | 7 +++++ modules/pacing/task_queue_paced_sender.h | 1 + 19 files changed, 94 insertions(+), 16 deletions(-) diff --git a/audio/audio_send_stream.cc b/audio/audio_send_stream.cc index 5e3b9ffc9c..ba13fcbe8b 100644 --- a/audio/audio_send_stream.cc +++ b/audio/audio_send_stream.cc @@ -342,6 +342,8 @@ void AudioSendStream::Start() { config_.max_bitrate_bps != -1 && (allocate_audio_without_feedback_ || TransportSeqNumId(config_) != 0)) { rtp_transport_->AccountForAudioPacketsInPacedSender(true); + if (send_side_bwe_with_overhead_) + rtp_transport_->IncludeOverheadInPacedSender(); rtp_rtcp_module_->SetAsPartOfAllocation(true); rtc::Event thread_sync_event; worker_queue_->PostTask([&] { @@ -765,6 +767,8 @@ void AudioSendStream::ReconfigureBitrateObserver( if (!new_config.has_dscp && new_config.min_bitrate_bps != -1 && new_config.max_bitrate_bps != -1 && TransportSeqNumId(new_config) != 0) { rtp_transport_->AccountForAudioPacketsInPacedSender(true); + if (send_side_bwe_with_overhead_) + rtp_transport_->IncludeOverheadInPacedSender(); rtc::Event thread_sync_event; worker_queue_->PostTask([&] { RTC_DCHECK_RUN_ON(worker_queue_); diff --git a/audio/audio_send_stream_unittest.cc b/audio/audio_send_stream_unittest.cc index 04723664ee..3b9fbb7f39 100644 --- a/audio/audio_send_stream_unittest.cc +++ b/audio/audio_send_stream_unittest.cc @@ -490,6 +490,8 @@ TEST(AudioSendStreamTest, SendCodecAppliesAudioNetworkAdaptor) { const std::string kAnaConfigString = "abcde"; const std::string kAnaReconfigString = "12345"; + helper.config().rtp.extensions.push_back(RtpExtension( + RtpExtension::kTransportSequenceNumberUri, kTransportSequenceNumberId)); helper.config().audio_network_adaptor_config = kAnaConfigString; EXPECT_CALL(helper.mock_encoder_factory(), MakeAudioEncoderMock(_, _, _, _)) diff --git a/call/rtp_transport_controller_send.cc b/call/rtp_transport_controller_send.cc index 62b7008396..c2946adbaf 100644 --- a/call/rtp_transport_controller_send.cc +++ b/call/rtp_transport_controller_send.cc @@ -434,6 +434,10 @@ void RtpTransportControllerSend::AccountForAudioPacketsInPacedSender( pacer()->SetAccountForAudioPackets(account_for_audio); } +void RtpTransportControllerSend::IncludeOverheadInPacedSender() { + pacer()->SetIncludeOverhead(); +} + void RtpTransportControllerSend::OnReceivedEstimatedBitrate(uint32_t bitrate) { RemoteBitrateReport msg; msg.receive_time = Timestamp::ms(clock_->TimeInMilliseconds()); diff --git a/call/rtp_transport_controller_send.h b/call/rtp_transport_controller_send.h index f74c4e598f..b07bea73d8 100644 --- a/call/rtp_transport_controller_send.h +++ b/call/rtp_transport_controller_send.h @@ -107,6 +107,7 @@ class RtpTransportControllerSend final size_t transport_overhead_per_packet) override; void AccountForAudioPacketsInPacedSender(bool account_for_audio) override; + void IncludeOverheadInPacedSender() override; // Implements RtcpBandwidthObserver interface void OnReceivedEstimatedBitrate(uint32_t bitrate) override; diff --git a/call/rtp_transport_controller_send_interface.h b/call/rtp_transport_controller_send_interface.h index 1e881dc42c..b40aabdc2c 100644 --- a/call/rtp_transport_controller_send_interface.h +++ b/call/rtp_transport_controller_send_interface.h @@ -150,6 +150,7 @@ class RtpTransportControllerSendInterface { size_t transport_overhead_per_packet) = 0; virtual void AccountForAudioPacketsInPacedSender(bool account_for_audio) = 0; + virtual void IncludeOverheadInPacedSender() = 0; }; } // namespace webrtc diff --git a/call/rtp_video_sender.cc b/call/rtp_video_sender.cc index a926eb514c..413171fa67 100644 --- a/call/rtp_video_sender.cc +++ b/call/rtp_video_sender.cc @@ -279,6 +279,11 @@ absl::optional GetVideoCodecType(const RtpConfig& config) { } return PayloadStringToCodecType(config.payload_name); } +bool TransportSeqNumExtensionConfigured(const RtpConfig& config_config) { + return absl::c_any_of(config_config.extensions, [](const RtpExtension& ext) { + return ext.uri == RtpExtension::kTransportSequenceNumberUri; + }); +} } // namespace RtpVideoSender::RtpVideoSender( @@ -301,6 +306,7 @@ RtpVideoSender::RtpVideoSender( "WebRTC-SubtractPacketizationOverhead")), use_early_loss_detection_( !webrtc::field_trial::IsDisabled("WebRTC-UseEarlyLossDetection")), + has_packet_feedback_(TransportSeqNumExtensionConfigured(rtp_config)), active_(false), module_process_thread_(nullptr), suspended_ssrcs_(std::move(suspended_ssrcs)), @@ -330,6 +336,8 @@ RtpVideoSender::RtpVideoSender( frame_counts_(rtp_config.ssrcs.size()), frame_count_observer_(observers.frame_count_observer) { RTC_DCHECK_EQ(rtp_config_.ssrcs.size(), rtp_streams_.size()); + if (send_side_bwe_with_overhead_ && has_packet_feedback_) + transport_->IncludeOverheadInPacedSender(); module_process_thread_checker_.Detach(); // SSRCs are assumed to be sorted in the same order as |rtp_modules|. for (uint32_t ssrc : rtp_config_.ssrcs) { @@ -700,7 +708,7 @@ void RtpVideoSender::OnBitrateUpdated(BitrateAllocationUpdate update, DataSize max_total_packet_size = DataSize::bytes( rtp_config_.max_packet_size + transport_overhead_bytes_per_packet_); uint32_t payload_bitrate_bps = update.target_bitrate.bps(); - if (send_side_bwe_with_overhead_) { + if (send_side_bwe_with_overhead_ && has_packet_feedback_) { DataRate overhead_rate = CalculateOverheadRate( update.target_bitrate, max_total_packet_size, packet_overhead); // TODO(srte): We probably should not accept 0 payload bitrate here. @@ -736,7 +744,7 @@ void RtpVideoSender::OnBitrateUpdated(BitrateAllocationUpdate update, loss_mask_vector_.clear(); uint32_t encoder_overhead_rate_bps = 0; - if (send_side_bwe_with_overhead_) { + if (send_side_bwe_with_overhead_ && has_packet_feedback_) { // TODO(srte): The packet size should probably be the same as in the // CalculateOverheadRate call above (just max_total_packet_size), it doesn't // make sense to use different packet rates for different overhead diff --git a/call/rtp_video_sender.h b/call/rtp_video_sender.h index fb01f1b263..eb7e4315be 100644 --- a/call/rtp_video_sender.h +++ b/call/rtp_video_sender.h @@ -163,6 +163,7 @@ class RtpVideoSender : public RtpVideoSenderInterface, const bool send_side_bwe_with_overhead_; const bool account_for_packetization_overhead_; const bool use_early_loss_detection_; + const bool has_packet_feedback_; // TODO(holmer): Remove crit_ once RtpVideoSender runs on the // transport task queue. diff --git a/call/test/mock_rtp_transport_controller_send.h b/call/test/mock_rtp_transport_controller_send.h index 04dac29f33..fad27b018f 100644 --- a/call/test/mock_rtp_transport_controller_send.h +++ b/call/test/mock_rtp_transport_controller_send.h @@ -67,6 +67,7 @@ class MockRtpTransportControllerSend MOCK_METHOD1(SetClientBitratePreferences, void(const BitrateSettings&)); MOCK_METHOD1(OnTransportOverheadChanged, void(size_t)); MOCK_METHOD1(AccountForAudioPacketsInPacedSender, void(bool)); + MOCK_METHOD0(IncludeOverheadInPacedSender, void()); MOCK_METHOD1(OnReceivedPacket, void(const ReceivedPacket&)); }; } // namespace webrtc diff --git a/modules/audio_coding/codecs/opus/audio_encoder_opus.cc b/modules/audio_coding/codecs/opus/audio_encoder_opus.cc index 44cfe9e5a2..168bcec241 100644 --- a/modules/audio_coding/codecs/opus/audio_encoder_opus.cc +++ b/modules/audio_coding/codecs/opus/audio_encoder_opus.cc @@ -593,6 +593,11 @@ void AudioEncoderOpusImpl::OnReceivedUplinkPacketLossFraction( ApplyAudioNetworkAdaptor(); } +void AudioEncoderOpusImpl::OnReceivedTargetAudioBitrate( + int target_audio_bitrate_bps) { + SetTargetBitrate(target_audio_bitrate_bps); +} + void AudioEncoderOpusImpl::OnReceivedUplinkBandwidth( int target_audio_bitrate_bps, absl::optional bwe_period_ms, diff --git a/modules/audio_coding/codecs/opus/audio_encoder_opus.h b/modules/audio_coding/codecs/opus/audio_encoder_opus.h index 66c489f79b..40fd167c10 100644 --- a/modules/audio_coding/codecs/opus/audio_encoder_opus.h +++ b/modules/audio_coding/codecs/opus/audio_encoder_opus.h @@ -104,6 +104,7 @@ class AudioEncoderOpusImpl final : public AudioEncoder { void DisableAudioNetworkAdaptor() override; void OnReceivedUplinkPacketLossFraction( float uplink_packet_loss_fraction) override; + void OnReceivedTargetAudioBitrate(int target_audio_bitrate_bps) override; void OnReceivedUplinkBandwidth( int target_audio_bitrate_bps, absl::optional bwe_period_ms) override; diff --git a/modules/pacing/paced_sender.cc b/modules/pacing/paced_sender.cc index f6c85d4ed3..6dc47b6892 100644 --- a/modules/pacing/paced_sender.cc +++ b/modules/pacing/paced_sender.cc @@ -126,6 +126,11 @@ void PacedSender::SetAccountForAudioPackets(bool account_for_audio) { pacing_controller_.SetAccountForAudioPackets(account_for_audio); } +void PacedSender::SetIncludeOverhead() { + rtc::CritScope cs(&critsect_); + pacing_controller_.SetIncludeOverhead(); +} + TimeDelta PacedSender::ExpectedQueueTime() const { rtc::CritScope cs(&critsect_); return pacing_controller_.ExpectedQueueTime(); diff --git a/modules/pacing/paced_sender.h b/modules/pacing/paced_sender.h index 06a6c26e16..36913080e0 100644 --- a/modules/pacing/paced_sender.h +++ b/modules/pacing/paced_sender.h @@ -97,6 +97,8 @@ class PacedSender : public Module, // at high priority. void SetAccountForAudioPackets(bool account_for_audio) override; + void SetIncludeOverhead() override; + // Returns the time since the oldest queued packet was enqueued. TimeDelta OldestPacketWaitTime() const override; diff --git a/modules/pacing/pacing_controller.cc b/modules/pacing/pacing_controller.cc index e6dd7ac93a..09b76301fb 100644 --- a/modules/pacing/pacing_controller.cc +++ b/modules/pacing/pacing_controller.cc @@ -99,8 +99,6 @@ PacingController::PacingController(Clock* clock, pace_audio_(IsEnabled(*field_trials_, "WebRTC-Pacer-BlockAudio")), small_first_probe_packet_( IsEnabled(*field_trials_, "WebRTC-Pacer-SmallFirstProbePacket")), - send_side_bwe_with_overhead_( - IsEnabled(*field_trials_, "WebRTC-SendSideBwe-WithOverhead")), min_packet_limit_(kDefaultMinPacketLimit), last_timestamp_(clock_->CurrentTime()), paused_(false), @@ -120,7 +118,8 @@ PacingController::PacingController(Clock* clock, congestion_window_size_(DataSize::PlusInfinity()), outstanding_data_(DataSize::Zero()), queue_time_limit(kMaxExpectedQueueLength), - account_for_audio_(false) { + account_for_audio_(false), + include_overhead_(false) { if (!drain_large_queues_) { RTC_LOG(LS_WARNING) << "Pacer queues will not be drained," "pushback experiment must be enabled."; @@ -226,6 +225,11 @@ void PacingController::SetAccountForAudioPackets(bool account_for_audio) { account_for_audio_ = account_for_audio; } +void PacingController::SetIncludeOverhead() { + include_overhead_ = true; + packet_queue_.SetIncludeOverhead(); +} + TimeDelta PacingController::ExpectedQueueTime() const { RTC_DCHECK_GT(pacing_bitrate_, DataRate::Zero()); return TimeDelta::ms( @@ -517,10 +521,10 @@ void PacingController::ProcessPackets() { RTC_DCHECK(rtp_packet); RTC_DCHECK(rtp_packet->packet_type().has_value()); const RtpPacketToSend::Type packet_type = *rtp_packet->packet_type(); - const DataSize packet_size = DataSize::bytes( - send_side_bwe_with_overhead_ - ? rtp_packet->size() - : rtp_packet->payload_size() + rtp_packet->padding_size()); + const DataSize packet_size = + DataSize::bytes(include_overhead_ ? rtp_packet->size() + : rtp_packet->payload_size() + + rtp_packet->padding_size()); packet_sender_->SendRtpPacket(std::move(rtp_packet), pacing_info); data_sent += packet_size; diff --git a/modules/pacing/pacing_controller.h b/modules/pacing/pacing_controller.h index 75c0aa3b64..fb4d9d30c7 100644 --- a/modules/pacing/pacing_controller.h +++ b/modules/pacing/pacing_controller.h @@ -107,6 +107,7 @@ class PacingController { // the pacer budget calculation. The audio traffic still will be injected // at high priority. void SetAccountForAudioPackets(bool account_for_audio); + void SetIncludeOverhead(); // Returns the time since the oldest queued packet was enqueued. TimeDelta OldestPacketWaitTime() const; @@ -176,7 +177,6 @@ class PacingController { const bool send_padding_if_silent_; const bool pace_audio_; const bool small_first_probe_packet_; - const bool send_side_bwe_with_overhead_; TimeDelta min_packet_limit_; @@ -219,6 +219,7 @@ class PacingController { TimeDelta queue_time_limit; bool account_for_audio_; + bool include_overhead_; }; } // namespace webrtc diff --git a/modules/pacing/round_robin_packet_queue.cc b/modules/pacing/round_robin_packet_queue.cc index 16542b3a81..754ff5888a 100644 --- a/modules/pacing/round_robin_packet_queue.cc +++ b/modules/pacing/round_robin_packet_queue.cc @@ -93,6 +93,16 @@ void RoundRobinPacketQueue::QueuedPacket::SubtractPauseTime( enqueue_time_ -= pause_time_sum; } +RoundRobinPacketQueue::PriorityPacketQueue::const_iterator +RoundRobinPacketQueue::PriorityPacketQueue::begin() const { + return c.begin(); +} + +RoundRobinPacketQueue::PriorityPacketQueue::const_iterator +RoundRobinPacketQueue::PriorityPacketQueue::end() const { + return c.end(); +} + RoundRobinPacketQueue::Stream::Stream() : size(DataSize::Zero()), ssrc(0) {} RoundRobinPacketQueue::Stream::Stream(const Stream& stream) = default; RoundRobinPacketQueue::Stream::~Stream() = default; @@ -114,8 +124,7 @@ RoundRobinPacketQueue::RoundRobinPacketQueue( max_size_(kMaxLeadingSize), queue_time_sum_(TimeDelta::Zero()), pause_time_sum_(TimeDelta::Zero()), - send_side_bwe_with_overhead_( - IsEnabled(field_trials, "WebRTC-SendSideBwe-WithOverhead")) {} + include_overhead_(false) {} RoundRobinPacketQueue::~RoundRobinPacketQueue() { // Make sure to release any packets owned by raw pointer in QueuedPacket. @@ -158,7 +167,7 @@ std::unique_ptr RoundRobinPacketQueue::Pop() { // case a "budget" will be built up for the stream sending at the lower // rate. To avoid building a too large budget we limit |bytes| to be within // kMaxLeading bytes of the stream that has sent the most amount of bytes. - DataSize packet_size = queued_packet.Size(send_side_bwe_with_overhead_); + DataSize packet_size = queued_packet.Size(include_overhead_); stream->size = std::max(stream->size + packet_size, max_size_ - kMaxLeadingSize); max_size_ = std::max(max_size_, stream->size); @@ -238,6 +247,17 @@ void RoundRobinPacketQueue::SetPauseState(bool paused, Timestamp now) { paused_ = paused; } +void RoundRobinPacketQueue::SetIncludeOverhead() { + include_overhead_ = true; + // We need to update the size to reflect overhead for existing packets. + size_ = DataSize::Zero(); + for (const auto& stream : streams_) { + for (const QueuedPacket& packet : stream.second.packet_queue) { + size_ += packet.Size(include_overhead_); + } + } +} + TimeDelta RoundRobinPacketQueue::AverageQueueTime() const { if (Empty()) return TimeDelta::Zero(); @@ -279,7 +299,7 @@ void RoundRobinPacketQueue::Push(QueuedPacket packet) { packet.SubtractPauseTime(pause_time_sum_); size_packets_ += 1; - size_ += packet.Size(send_side_bwe_with_overhead_); + size_ += packet.Size(include_overhead_); stream->packet_queue.push(packet); } diff --git a/modules/pacing/round_robin_packet_queue.h b/modules/pacing/round_robin_packet_queue.h index 96b458f4c0..d0a2f7cb72 100644 --- a/modules/pacing/round_robin_packet_queue.h +++ b/modules/pacing/round_robin_packet_queue.h @@ -52,6 +52,7 @@ class RoundRobinPacketQueue { TimeDelta AverageQueueTime() const; void UpdateQueueTime(Timestamp now); void SetPauseState(bool paused, Timestamp now); + void SetIncludeOverhead(); private: struct QueuedPacket { @@ -89,6 +90,13 @@ class RoundRobinPacketQueue { RtpPacketToSend* owned_packet_; }; + class PriorityPacketQueue : public std::priority_queue { + public: + using const_iterator = container_type::const_iterator; + const_iterator begin() const; + const_iterator end() const; + }; + struct StreamPrioKey { StreamPrioKey(int priority, DataSize size) : priority(priority), size(size) {} @@ -111,7 +119,8 @@ class RoundRobinPacketQueue { DataSize size; uint32_t ssrc; - std::priority_queue packet_queue; + + PriorityPacketQueue packet_queue; // Whenever a packet is inserted for this stream we check if |priority_it| // points to an element in |stream_priorities_|, and if it does it means @@ -150,7 +159,7 @@ class RoundRobinPacketQueue { // the age of the oldest packet in the queue. std::multiset enqueue_times_; - const bool send_side_bwe_with_overhead_; + bool include_overhead_; }; } // namespace webrtc diff --git a/modules/pacing/rtp_packet_pacer.h b/modules/pacing/rtp_packet_pacer.h index 305be54234..2f11c1f5d6 100644 --- a/modules/pacing/rtp_packet_pacer.h +++ b/modules/pacing/rtp_packet_pacer.h @@ -64,6 +64,7 @@ class RtpPacketPacer { // the pacer budget calculation. The audio traffic still will be injected // at high priority. virtual void SetAccountForAudioPackets(bool account_for_audio) = 0; + virtual void SetIncludeOverhead() = 0; }; } // namespace webrtc diff --git a/modules/pacing/task_queue_paced_sender.cc b/modules/pacing/task_queue_paced_sender.cc index e1745db9d5..54d2d844ca 100644 --- a/modules/pacing/task_queue_paced_sender.cc +++ b/modules/pacing/task_queue_paced_sender.cc @@ -136,6 +136,13 @@ void TaskQueuePacedSender::SetAccountForAudioPackets(bool account_for_audio) { }); } +void TaskQueuePacedSender::SetIncludeOverhead() { + task_queue_.PostTask([this]() { + RTC_DCHECK_RUN_ON(&task_queue_); + pacing_controller_.SetIncludeOverhead(); + }); +} + void TaskQueuePacedSender::SetQueueTimeLimit(TimeDelta limit) { task_queue_.PostTask([this, limit]() { RTC_DCHECK_RUN_ON(&task_queue_); diff --git a/modules/pacing/task_queue_paced_sender.h b/modules/pacing/task_queue_paced_sender.h index 719886a931..a50ffa2784 100644 --- a/modules/pacing/task_queue_paced_sender.h +++ b/modules/pacing/task_queue_paced_sender.h @@ -79,6 +79,7 @@ class TaskQueuePacedSender : public RtpPacketPacer, // at high priority. void SetAccountForAudioPackets(bool account_for_audio) override; + void SetIncludeOverhead() override; // Returns the time since the oldest queued packet was enqueued. TimeDelta OldestPacketWaitTime() const override;