From a2cf8ee85419b6c252d252b663f1f31db365ae02 Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Tue, 16 May 2023 13:26:33 +0200 Subject: [PATCH] Simplify handling rtcp messages in audio send channel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Delete VoERtcpObserver proxy: pass BWE related message directly to transport controller pass ReportBlock directly to ChannelSend, assuming there will be single report block per source ssrc Bug: None Change-Id: I8378326bff1dc3c2736960166fc782ee822a9c12 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/305224 Commit-Queue: Danil Chapovalov Reviewed-by: Jakob Ivarsson‎ Cr-Commit-Position: refs/heads/main@{#40081} --- audio/audio_send_stream.cc | 51 ++++++------- audio/audio_send_stream.h | 1 - audio/audio_send_stream_unittest.cc | 14 +--- audio/channel_send.cc | 113 +++++----------------------- audio/channel_send.h | 5 +- audio/channel_send_unittest.cc | 5 +- audio/mock_voe_channel_proxy.h | 2 +- 7 files changed, 49 insertions(+), 142 deletions(-) diff --git a/audio/audio_send_stream.cc b/audio/audio_send_stream.cc index 41e0b5179d..0caf59a20e 100644 --- a/audio/audio_send_stream.cc +++ b/audio/audio_send_stream.cc @@ -110,29 +110,28 @@ AudioSendStream::AudioSendStream( RtcpRttStats* rtcp_rtt_stats, const absl::optional& suspended_rtp_state, const FieldTrialsView& field_trials) - : AudioSendStream( - clock, - config, - audio_state, - task_queue_factory, - rtp_transport, - bitrate_allocator, - event_log, - suspended_rtp_state, - voe::CreateChannelSend(clock, - task_queue_factory, - config.send_transport, - rtcp_rtt_stats, - event_log, - config.frame_encryptor.get(), - config.crypto_options, - config.rtp.extmap_allow_mixed, - config.rtcp_report_interval_ms, - config.rtp.ssrc, - config.frame_transformer, - rtp_transport->transport_feedback_observer(), - field_trials), - field_trials) {} + : AudioSendStream(clock, + config, + audio_state, + task_queue_factory, + rtp_transport, + bitrate_allocator, + event_log, + suspended_rtp_state, + voe::CreateChannelSend(clock, + task_queue_factory, + config.send_transport, + rtcp_rtt_stats, + event_log, + config.frame_encryptor.get(), + config.crypto_options, + config.rtp.extmap_allow_mixed, + config.rtcp_report_interval_ms, + config.rtp.ssrc, + config.frame_transformer, + rtp_transport, + field_trials), + field_trials) {} AudioSendStream::AudioSendStream( Clock* clock, @@ -284,8 +283,6 @@ void AudioSendStream::ConfigureStream( channel_send_->ResetSenderCongestionControlObjects(); } - RtcpBandwidthObserver* bandwidth_observer = nullptr; - if (!allocate_audio_without_feedback_ && new_ids.transport_sequence_number != 0) { rtp_rtcp_module_->RegisterRtpHeaderExtension( @@ -298,10 +295,8 @@ void AudioSendStream::ConfigureStream( if (enable_audio_alr_probing_) { rtp_transport_->EnablePeriodicAlrProbing(true); } - bandwidth_observer = rtp_transport_->GetBandwidthObserver(); } - channel_send_->RegisterSenderCongestionControlObjects(rtp_transport_, - bandwidth_observer); + channel_send_->RegisterSenderCongestionControlObjects(rtp_transport_); } // MID RTP header extension. if ((first_time || new_ids.mid != old_ids.mid || diff --git a/audio/audio_send_stream.h b/audio/audio_send_stream.h index 6cda9c3d52..62ccd524cb 100644 --- a/audio/audio_send_stream.h +++ b/audio/audio_send_stream.h @@ -32,7 +32,6 @@ namespace webrtc { class RtcEventLog; -class RtcpBandwidthObserver; class RtcpRttStats; class RtpTransportControllerSendInterface; diff --git a/audio/audio_send_stream_unittest.cc b/audio/audio_send_stream_unittest.cc index 1826793164..9afcfda0a6 100644 --- a/audio/audio_send_stream_unittest.cc +++ b/audio/audio_send_stream_unittest.cc @@ -232,15 +232,10 @@ struct ConfigHelper { RegisterRtpHeaderExtension(TransportSequenceNumber::Uri(), kTransportSequenceNumberId)) .Times(1); - EXPECT_CALL(*channel_send_, - RegisterSenderCongestionControlObjects( - &rtp_transport_, Eq(&bandwidth_observer_))) - .Times(1); - } else { - EXPECT_CALL(*channel_send_, RegisterSenderCongestionControlObjects( - &rtp_transport_, Eq(nullptr))) - .Times(1); } + EXPECT_CALL(*channel_send_, + RegisterSenderCongestionControlObjects(&rtp_transport_)) + .Times(1); EXPECT_CALL(*channel_send_, ResetSenderCongestionControlObjects()).Times(1); } @@ -798,8 +793,7 @@ TEST(AudioSendStreamTest, ReconfigureTransportCcResetsFirst) { EXPECT_CALL(*helper.channel_send(), ResetSenderCongestionControlObjects()) .Times(1); EXPECT_CALL(*helper.channel_send(), - RegisterSenderCongestionControlObjects(helper.transport(), - Ne(nullptr))) + RegisterSenderCongestionControlObjects(helper.transport())) .Times(1); } diff --git a/audio/channel_send.cc b/audio/channel_send.cc index 229b379f51..c893f9a61c 100644 --- a/audio/channel_send.cc +++ b/audio/channel_send.cc @@ -54,12 +54,12 @@ constexpr int64_t kMinRetransmissionWindowMs = 30; class RtpPacketSenderProxy; class TransportSequenceNumberProxy; -class VoERtcpObserver; class ChannelSend : public ChannelSendInterface, public AudioPacketizationCallback, // receive encoded // packets from the ACM - public RtcpPacketTypeCounterObserver { + public RtcpPacketTypeCounterObserver, + public ReportBlockDataObserver { public: ChannelSend(Clock* clock, TaskQueueFactory* task_queue_factory, @@ -72,7 +72,7 @@ class ChannelSend : public ChannelSendInterface, int rtcp_report_interval_ms, uint32_t ssrc, rtc::scoped_refptr frame_transformer, - TransportFeedbackObserver* feedback_observer, + RtpTransportControllerSendInterface* transport_controller, const FieldTrialsView& field_trials); ~ChannelSend() override; @@ -115,8 +115,7 @@ class ChannelSend : public ChannelSendInterface, void SetSendAudioLevelIndicationStatus(bool enable, int id) override; void RegisterSenderCongestionControlObjects( - RtpTransportControllerSendInterface* transport, - RtcpBandwidthObserver* bandwidth_observer) override; + RtpTransportControllerSendInterface* transport) override; void ResetSenderCongestionControlObjects() override; void SetRTCP_CNAME(absl::string_view c_name) override; std::vector GetRemoteRTCPReportBlocks() const override; @@ -150,7 +149,8 @@ class ChannelSend : public ChannelSendInterface, uint32_t ssrc, const RtcpPacketTypeCounter& packet_counter) override; - void OnUplinkPacketLossRate(float packet_loss_rate); + // ReportBlockDataObserver. + void OnReportBlockDataUpdated(ReportBlockData report_block) override; private: // From AudioPacketizationCallback in the ACM @@ -207,12 +207,8 @@ class ChannelSend : public ChannelSendInterface, bool input_mute_ RTC_GUARDED_BY(volume_settings_mutex_) = false; bool previous_frame_muted_ RTC_GUARDED_BY(encoder_queue_) = false; - // RtcpBandwidthObserver - const std::unique_ptr rtcp_observer_; - PacketRouter* packet_router_ RTC_GUARDED_BY(&worker_thread_checker_) = nullptr; - TransportFeedbackObserver* const feedback_observer_; const std::unique_ptr rtp_packet_pacer_proxy_; const std::unique_ptr retransmission_rate_limiter_; @@ -272,80 +268,6 @@ class RtpPacketSenderProxy : public RtpPacketSender { RtpPacketSender* rtp_packet_pacer_ RTC_GUARDED_BY(&mutex_); }; -class VoERtcpObserver : public RtcpBandwidthObserver { - public: - explicit VoERtcpObserver(ChannelSend* owner) - : owner_(owner), bandwidth_observer_(nullptr) {} - ~VoERtcpObserver() override {} - - void SetBandwidthObserver(RtcpBandwidthObserver* bandwidth_observer) { - MutexLock lock(&mutex_); - bandwidth_observer_ = bandwidth_observer; - } - - void OnReceivedEstimatedBitrate(uint32_t bitrate) override { - MutexLock lock(&mutex_); - if (bandwidth_observer_) { - bandwidth_observer_->OnReceivedEstimatedBitrate(bitrate); - } - } - - void OnReceivedRtcpReceiverReport(const ReportBlockList& report_blocks, - int64_t rtt, - int64_t now_ms) override { - { - MutexLock lock(&mutex_); - if (bandwidth_observer_) { - bandwidth_observer_->OnReceivedRtcpReceiverReport(report_blocks, rtt, - now_ms); - } - } - // TODO(mflodman): Do we need to aggregate reports here or can we jut send - // what we get? I.e. do we ever get multiple reports bundled into one RTCP - // report for VoiceEngine? - if (report_blocks.empty()) - return; - - int fraction_lost_aggregate = 0; - int total_number_of_packets = 0; - - // If receiving multiple report blocks, calculate the weighted average based - // on the number of packets a report refers to. - for (ReportBlockList::const_iterator block_it = report_blocks.begin(); - block_it != report_blocks.end(); ++block_it) { - // Find the previous extended high sequence number for this remote SSRC, - // to calculate the number of RTP packets this report refers to. Ignore if - // we haven't seen this SSRC before. - std::map::iterator seq_num_it = - extended_max_sequence_number_.find(block_it->source_ssrc); - int number_of_packets = 0; - if (seq_num_it != extended_max_sequence_number_.end()) { - number_of_packets = - block_it->extended_highest_sequence_number - seq_num_it->second; - } - fraction_lost_aggregate += number_of_packets * block_it->fraction_lost; - total_number_of_packets += number_of_packets; - - extended_max_sequence_number_[block_it->source_ssrc] = - block_it->extended_highest_sequence_number; - } - int weighted_fraction_lost = 0; - if (total_number_of_packets > 0) { - weighted_fraction_lost = - (fraction_lost_aggregate + total_number_of_packets / 2) / - total_number_of_packets; - } - owner_->OnUplinkPacketLossRate(weighted_fraction_lost / 255.0f); - } - - private: - ChannelSend* owner_; - // Maps remote side ssrc to extended highest sequence number received. - std::map extended_max_sequence_number_; - Mutex mutex_; - RtcpBandwidthObserver* bandwidth_observer_ RTC_GUARDED_BY(mutex_); -}; - int32_t ChannelSend::SendData(AudioFrameType frameType, uint8_t payloadType, uint32_t rtp_timestamp, @@ -458,12 +380,10 @@ ChannelSend::ChannelSend( int rtcp_report_interval_ms, uint32_t ssrc, rtc::scoped_refptr frame_transformer, - TransportFeedbackObserver* feedback_observer, + RtpTransportControllerSendInterface* transport_controller, const FieldTrialsView& field_trials) : ssrc_(ssrc), event_log_(rtc_event_log), - rtcp_observer_(new VoERtcpObserver(this)), - feedback_observer_(feedback_observer), rtp_packet_pacer_proxy_(new RtpPacketSenderProxy()), retransmission_rate_limiter_( new RateLimiter(clock, kMaxRetransmissionWindowMs)), @@ -475,8 +395,11 @@ ChannelSend::ChannelSend( audio_coding_ = AudioCodingModule::Create(); RtpRtcpInterface::Configuration configuration; - configuration.bandwidth_callback = rtcp_observer_.get(); - configuration.transport_feedback_callback = feedback_observer_; + configuration.report_block_data_observer = this; + configuration.bandwidth_callback = + transport_controller->GetBandwidthObserver(); + configuration.transport_feedback_callback = + transport_controller->transport_feedback_observer(); configuration.clock = (clock ? clock : Clock::GetRealTimeClock()); configuration.audio = true; configuration.outgoing_transport = rtp_transport; @@ -618,7 +541,8 @@ int ChannelSend::GetTargetBitrate() const { return audio_coding_->GetTargetBitrate(); } -void ChannelSend::OnUplinkPacketLossRate(float packet_loss_rate) { +void ChannelSend::OnReportBlockDataUpdated(ReportBlockData report_block) { + float packet_loss_rate = report_block.fraction_lost(); CallEncoder([&](AudioEncoder* encoder) { encoder->OnReceivedUplinkPacketLossFraction(packet_loss_rate); }); @@ -703,8 +627,7 @@ void ChannelSend::SetSendAudioLevelIndicationStatus(bool enable, int id) { } void ChannelSend::RegisterSenderCongestionControlObjects( - RtpTransportControllerSendInterface* transport, - RtcpBandwidthObserver* bandwidth_observer) { + RtpTransportControllerSendInterface* transport) { RTC_DCHECK_RUN_ON(&worker_thread_checker_); RtpPacketSender* rtp_packet_pacer = transport->packet_sender(); PacketRouter* packet_router = transport->packet_router(); @@ -712,7 +635,6 @@ void ChannelSend::RegisterSenderCongestionControlObjects( RTC_DCHECK(rtp_packet_pacer); RTC_DCHECK(packet_router); RTC_DCHECK(!packet_router_); - rtcp_observer_->SetBandwidthObserver(bandwidth_observer); rtp_packet_pacer_proxy_->SetPacketPacer(rtp_packet_pacer); rtp_rtcp_->SetStorePacketsStatus(true, 600); packet_router_ = packet_router; @@ -722,7 +644,6 @@ void ChannelSend::ResetSenderCongestionControlObjects() { RTC_DCHECK_RUN_ON(&worker_thread_checker_); RTC_DCHECK(packet_router_); rtp_rtcp_->SetStorePacketsStatus(false, 600); - rtcp_observer_->SetBandwidthObserver(nullptr); packet_router_ = nullptr; rtp_packet_pacer_proxy_->SetPacketPacer(nullptr); } @@ -947,13 +868,13 @@ std::unique_ptr CreateChannelSend( int rtcp_report_interval_ms, uint32_t ssrc, rtc::scoped_refptr frame_transformer, - TransportFeedbackObserver* feedback_observer, + RtpTransportControllerSendInterface* transport_controller, const FieldTrialsView& field_trials) { return std::make_unique( clock, task_queue_factory, rtp_transport, rtcp_rtt_stats, rtc_event_log, frame_encryptor, crypto_options, extmap_allow_mixed, rtcp_report_interval_ms, ssrc, std::move(frame_transformer), - feedback_observer, field_trials); + transport_controller, field_trials); } } // namespace voe diff --git a/audio/channel_send.h b/audio/channel_send.h index a0aa42a706..00d954c952 100644 --- a/audio/channel_send.h +++ b/audio/channel_send.h @@ -71,8 +71,7 @@ class ChannelSendInterface { virtual void SetRTCP_CNAME(absl::string_view c_name) = 0; virtual void SetSendAudioLevelIndicationStatus(bool enable, int id) = 0; virtual void RegisterSenderCongestionControlObjects( - RtpTransportControllerSendInterface* transport, - RtcpBandwidthObserver* bandwidth_observer) = 0; + RtpTransportControllerSendInterface* transport) = 0; virtual void ResetSenderCongestionControlObjects() = 0; virtual std::vector GetRemoteRTCPReportBlocks() const = 0; virtual ANAStats GetANAStatistics() const = 0; @@ -126,7 +125,7 @@ std::unique_ptr CreateChannelSend( int rtcp_report_interval_ms, uint32_t ssrc, rtc::scoped_refptr frame_transformer, - TransportFeedbackObserver* feedback_observer, + RtpTransportControllerSendInterface* transport_controller, const FieldTrialsView& field_trials); } // namespace voe diff --git a/audio/channel_send_unittest.cc b/audio/channel_send_unittest.cc index 97882b93b7..9d51af6da4 100644 --- a/audio/channel_send_unittest.cc +++ b/audio/channel_send_unittest.cc @@ -61,14 +61,13 @@ class ChannelSendTest : public ::testing::Test { channel_ = voe::CreateChannelSend( time_controller_.GetClock(), time_controller_.GetTaskQueueFactory(), &transport_, nullptr, &event_log_, nullptr, crypto_options_, false, - kRtcpIntervalMs, kSsrc, nullptr, nullptr, field_trials_); + kRtcpIntervalMs, kSsrc, nullptr, &transport_controller_, field_trials_); encoder_factory_ = CreateBuiltinAudioEncoderFactory(); std::unique_ptr encoder = encoder_factory_->MakeAudioEncoder( kPayloadType, SdpAudioFormat("opus", kRtpRateHz, 2), {}); channel_->SetEncoder(kPayloadType, std::move(encoder)); transport_controller_.EnsureStarted(); - channel_->RegisterSenderCongestionControlObjects(&transport_controller_, - nullptr); + channel_->RegisterSenderCongestionControlObjects(&transport_controller_); ON_CALL(transport_, SendRtcp).WillByDefault(Return(true)); ON_CALL(transport_, SendRtp).WillByDefault(Return(true)); } diff --git a/audio/mock_voe_channel_proxy.h b/audio/mock_voe_channel_proxy.h index 671de08bd4..29005173df 100644 --- a/audio/mock_voe_channel_proxy.h +++ b/audio/mock_voe_channel_proxy.h @@ -131,7 +131,7 @@ class MockChannelSend : public voe::ChannelSendInterface { (override)); MOCK_METHOD(void, RegisterSenderCongestionControlObjects, - (RtpTransportControllerSendInterface*, RtcpBandwidthObserver*), + (RtpTransportControllerSendInterface*), (override)); MOCK_METHOD(void, ResetSenderCongestionControlObjects, (), (override)); MOCK_METHOD(CallSendStatistics, GetRTCPStatistics, (), (const, override));