diff --git a/audio/audio_send_stream.cc b/audio/audio_send_stream.cc index 544c936c8a..baf51b2bb0 100644 --- a/audio/audio_send_stream.cc +++ b/audio/audio_send_stream.cc @@ -111,6 +111,7 @@ AudioSendStream::AudioSendStream( voe::CreateChannelSend(worker_queue, module_process_thread, config.media_transport, + /*overhead_observer=*/this, config.send_transport, rtcp_rtt_stats, event_log, @@ -152,7 +153,15 @@ AudioSendStream::AudioSendStream( // should be no rtp_transport, and below check should be strengthened to XOR // (either rtp_transport or media_transport but not both). RTC_DCHECK(rtp_transport || config.media_transport); - + if (config.media_transport) { + // TODO(sukhanov): Currently media transport audio overhead is considered + // constant, we will not get overhead_observer calls when using + // media_transport. In the future when we introduce RTP media transport we + // should make audio overhead interface consistent and work for both RTP and + // non-RTP implementations. + audio_overhead_per_packet_bytes_ = + config.media_transport->GetAudioPacketOverhead(); + } rtp_rtcp_module_ = channel_send_->GetRtpRtcp(); RTC_DCHECK(rtp_rtcp_module_); @@ -480,9 +489,38 @@ void AudioSendStream::OnPacketFeedbackVector( } } -void AudioSendStream::SetTransportOverhead(int transport_overhead_per_packet) { +void AudioSendStream::SetTransportOverhead( + int transport_overhead_per_packet_bytes) { RTC_DCHECK(worker_thread_checker_.CalledOnValidThread()); - channel_send_->SetTransportOverhead(transport_overhead_per_packet); + rtc::CritScope cs(&overhead_per_packet_lock_); + transport_overhead_per_packet_bytes_ = transport_overhead_per_packet_bytes; + UpdateOverheadForEncoder(); +} + +void AudioSendStream::OnOverheadChanged( + size_t overhead_bytes_per_packet_bytes) { + rtc::CritScope cs(&overhead_per_packet_lock_); + audio_overhead_per_packet_bytes_ = overhead_bytes_per_packet_bytes; + UpdateOverheadForEncoder(); +} + +void AudioSendStream::UpdateOverheadForEncoder() { + const size_t overhead_per_packet_bytes = GetPerPacketOverheadBytes(); + CallEncoder(channel_send_, [&](AudioEncoder* encoder) { + if (encoder) { + encoder->OnReceivedOverhead(overhead_per_packet_bytes); + } + }); +} + +size_t AudioSendStream::TestOnlyGetPerPacketOverheadBytes() const { + rtc::CritScope cs(&overhead_per_packet_lock_); + return GetPerPacketOverheadBytes(); +} + +size_t AudioSendStream::GetPerPacketOverheadBytes() const { + return transport_overhead_per_packet_bytes_ + + audio_overhead_per_packet_bytes_; } RtpState AudioSendStream::GetRtpState() const { @@ -568,10 +606,18 @@ bool AudioSendStream::SetupSendCodec(AudioSendStream* stream, new_config.send_codec_spec->format.clockrate_hz); } + // Set currently known overhead (used in ANA, opus only). + // If overhead changes later, it will be updated in UpdateOverheadForEncoder. + { + rtc::CritScope cs(&stream->overhead_per_packet_lock_); + encoder->OnReceivedOverhead(stream->GetPerPacketOverheadBytes()); + } + stream->StoreEncoderProperties(encoder->SampleRateHz(), encoder->NumChannels()); stream->channel_send_->SetEncoder(new_config.send_codec_spec->payload_type, std::move(encoder)); + return true; } @@ -619,6 +665,12 @@ bool AudioSendStream::ReconfigureSendCodec(AudioSendStream* stream, ReconfigureANA(stream, new_config); ReconfigureCNG(stream, new_config); + // Set currently known overhead (used in ANA, opus only). + { + rtc::CritScope cs(&stream->overhead_per_packet_lock_); + stream->UpdateOverheadForEncoder(); + } + return true; } diff --git a/audio/audio_send_stream.h b/audio/audio_send_stream.h index eb8c952f7f..a1dab792dd 100644 --- a/audio/audio_send_stream.h +++ b/audio/audio_send_stream.h @@ -36,7 +36,8 @@ class AudioState; class AudioSendStream final : public webrtc::AudioSendStream, public webrtc::BitrateAllocatorObserver, - public webrtc::PacketFeedbackObserver { + public webrtc::PacketFeedbackObserver, + public webrtc::OverheadObserver { public: AudioSendStream(const webrtc::AudioSendStream::Config& config, const rtc::scoped_refptr& audio_state, @@ -85,11 +86,19 @@ class AudioSendStream final : public webrtc::AudioSendStream, void OnPacketFeedbackVector( const std::vector& packet_feedback_vector) override; - void SetTransportOverhead(int transport_overhead_per_packet); + void SetTransportOverhead(int transport_overhead_per_packet_bytes); + + // OverheadObserver override reports audio packetization overhead from + // RTP/RTCP module or Media Transport. + void OnOverheadChanged(size_t overhead_bytes_per_packet_bytes) override; RtpState GetRtpState() const; const voe::ChannelSendInterface* GetChannel() const; + // Returns combined per-packet overhead. + size_t TestOnlyGetPerPacketOverheadBytes() const + RTC_LOCKS_EXCLUDED(overhead_per_packet_lock_); + private: class TimedTransport; @@ -116,6 +125,15 @@ class AudioSendStream final : public webrtc::AudioSendStream, double bitrate_priority); void RemoveBitrateObserver(); + // Sets per-packet overhead on encoded (for ANA) based on current known values + // of transport and packetization overheads. + void UpdateOverheadForEncoder() + RTC_EXCLUSIVE_LOCKS_REQUIRED(overhead_per_packet_lock_); + + // Returns combined per-packet overhead. + size_t GetPerPacketOverheadBytes() const + RTC_EXCLUSIVE_LOCKS_REQUIRED(overhead_per_packet_lock_); + void RegisterCngPayloadType(int payload_type, int clockrate_hz); rtc::ThreadChecker worker_thread_checker_; @@ -156,6 +174,16 @@ class AudioSendStream final : public webrtc::AudioSendStream, const std::vector& extensions); static int TransportSeqNumId(const Config& config); + rtc::CriticalSection overhead_per_packet_lock_; + + // Current transport overhead (ICE, TURN, etc.) + size_t transport_overhead_per_packet_bytes_ + RTC_GUARDED_BY(overhead_per_packet_lock_) = 0; + + // Current audio packetization overhead (RTP or Media Transport). + size_t audio_overhead_per_packet_bytes_ + RTC_GUARDED_BY(overhead_per_packet_lock_) = 0; + RTC_DISALLOW_IMPLICIT_CONSTRUCTORS(AudioSendStream); }; } // namespace internal diff --git a/audio/audio_send_stream_unittest.cc b/audio/audio_send_stream_unittest.cc index 5aaa07ece5..0f5edd7c9c 100644 --- a/audio/audio_send_stream_unittest.cc +++ b/audio/audio_send_stream_unittest.cc @@ -520,9 +520,61 @@ TEST(AudioSendStreamTest, ReconfigureTransportCcResetsFirst) { helper.transport(), Ne(nullptr))) .Times(1); } + + // ModifyEncoder will be called to re-set overhead. + EXPECT_CALL(*helper.channel_send(), ModifyEncoder(testing::_)).Times(1); + send_stream->Reconfigure(new_config); } +TEST(AudioSendStreamTest, OnTransportOverheadChanged) { + ConfigHelper helper(false, true); + auto send_stream = helper.CreateAudioSendStream(); + auto new_config = helper.config(); + + // ModifyEncoder will be called on overhead change. + EXPECT_CALL(*helper.channel_send(), ModifyEncoder(testing::_)).Times(1); + + const size_t transport_overhead_per_packet_bytes = 333; + send_stream->SetTransportOverhead(transport_overhead_per_packet_bytes); + + EXPECT_EQ(transport_overhead_per_packet_bytes, + send_stream->TestOnlyGetPerPacketOverheadBytes()); +} + +TEST(AudioSendStreamTest, OnAudioOverheadChanged) { + ConfigHelper helper(false, true); + auto send_stream = helper.CreateAudioSendStream(); + auto new_config = helper.config(); + + // ModifyEncoder will be called on overhead change. + EXPECT_CALL(*helper.channel_send(), ModifyEncoder(testing::_)).Times(1); + + const size_t audio_overhead_per_packet_bytes = 555; + send_stream->OnOverheadChanged(audio_overhead_per_packet_bytes); + EXPECT_EQ(audio_overhead_per_packet_bytes, + send_stream->TestOnlyGetPerPacketOverheadBytes()); +} + +TEST(AudioSendStreamTest, OnAudioAndTransportOverheadChanged) { + ConfigHelper helper(false, true); + auto send_stream = helper.CreateAudioSendStream(); + auto new_config = helper.config(); + + // ModifyEncoder will be called when each of overhead changes. + EXPECT_CALL(*helper.channel_send(), ModifyEncoder(testing::_)).Times(2); + + const size_t transport_overhead_per_packet_bytes = 333; + send_stream->SetTransportOverhead(transport_overhead_per_packet_bytes); + + const size_t audio_overhead_per_packet_bytes = 555; + send_stream->OnOverheadChanged(audio_overhead_per_packet_bytes); + + EXPECT_EQ( + transport_overhead_per_packet_bytes + audio_overhead_per_packet_bytes, + send_stream->TestOnlyGetPerPacketOverheadBytes()); +} + // Validates that reconfiguring the AudioSendStream with a Frame encryptor // correctly reconfigures on the object without crashing. TEST(AudioSendStreamTest, ReconfigureWithFrameEncryptor) { diff --git a/audio/channel_send.cc b/audio/channel_send.cc index 00e48a6b69..dfbbb5799f 100644 --- a/audio/channel_send.cc +++ b/audio/channel_send.cc @@ -78,7 +78,6 @@ class VoERtcpObserver; class ChannelSend : public ChannelSendInterface, - public OverheadObserver, public AudioPacketizationCallback, // receive encoded packets from the // ACM public TargetTransferRateObserver { @@ -90,6 +89,7 @@ class ChannelSend ChannelSend(rtc::TaskQueue* encoder_queue, ProcessThread* module_process_thread, MediaTransportInterface* media_transport, + OverheadObserver* overhead_observer, Transport* rtp_transport, RtcpRttStats* rtcp_rtt_stats, RtcEventLog* rtc_event_log, @@ -160,8 +160,6 @@ class ChannelSend // packet. void ProcessAndEncodeAudio(std::unique_ptr audio_frame) override; - void SetTransportOverhead(size_t transport_overhead_per_packet) override; - // The existence of this function alongside OnUplinkPacketLossRate is // a compromise. We want the encoder to be agnostic of the PLR source, but // we also don't want it to receive conflicting information from TWCC and @@ -188,17 +186,11 @@ class ChannelSend size_t payloadSize, const RTPFragmentationHeader* fragmentation) override; - // From OverheadObserver in the RTP/RTCP module - void OnOverheadChanged(size_t overhead_bytes_per_packet) override; - void OnUplinkPacketLossRate(float packet_loss_rate); bool InputMute() const; int SetSendRtpHeaderExtension(bool enable, RTPExtensionType type, int id); - void UpdateOverheadForEncoder() - RTC_EXCLUSIVE_LOCKS_REQUIRED(overhead_per_packet_lock_); - int32_t SendRtpAudio(FrameType frameType, uint8_t payloadType, uint32_t timeStamp, @@ -254,10 +246,7 @@ class ChannelSend // TODO(henrika): can today be accessed on the main thread and on the // task queue; hence potential race. bool _includeAudioLevelIndication; - size_t transport_overhead_per_packet_ - RTC_GUARDED_BY(overhead_per_packet_lock_); - size_t rtp_overhead_per_packet_ RTC_GUARDED_BY(overhead_per_packet_lock_); - rtc::CriticalSection overhead_per_packet_lock_; + // RtcpBandwidthObserver const std::unique_ptr rtcp_observer_; @@ -636,6 +625,7 @@ int32_t ChannelSend::SendMediaTransportAudio( ChannelSend::ChannelSend(rtc::TaskQueue* encoder_queue, ProcessThread* module_process_thread, MediaTransportInterface* media_transport, + OverheadObserver* overhead_observer, Transport* rtp_transport, RtcpRttStats* rtcp_rtt_stats, RtcEventLog* rtc_event_log, @@ -650,8 +640,6 @@ ChannelSend::ChannelSend(rtc::TaskQueue* encoder_queue, input_mute_(false), previous_frame_muted_(false), _includeAudioLevelIndication(false), - transport_overhead_per_packet_(0), - rtp_overhead_per_packet_(0), rtcp_observer_(new VoERtcpObserver(this)), feedback_observer_proxy_(new TransportFeedbackProxy()), seq_num_allocator_proxy_(new TransportSequenceNumberProxy()), @@ -677,7 +665,12 @@ ChannelSend::ChannelSend(rtc::TaskQueue* encoder_queue, // RTPMediaTransport. In this case it means that overhead and bandwidth // observers should not be called when using media transport. if (!media_transport_) { - configuration.overhead_observer = this; + // TODO(sukhanov): Overhead observer is only needed for RTP path, because in + // media transport audio overhead is currently considered constant (see + // getter MediaTransportInterface::GetAudioPacketOverhead). In the future + // when we introduce RTP media transport we should make audio overhead + // interface consistent and work for both RTP and non-RTP implementations. + configuration.overhead_observer = overhead_observer; configuration.bandwidth_callback = rtcp_observer_.get(); configuration.transport_feedback_callback = feedback_observer_proxy_.get(); } @@ -704,7 +697,6 @@ ChannelSend::ChannelSend(rtc::TaskQueue* encoder_queue, if (media_transport_) { RTC_DLOG(LS_INFO) << "Setting media_transport_ rate observers."; media_transport_->AddTargetTransferRateObserver(this); - OnOverheadChanged(media_transport_->GetAudioPacketOverhead()); } else { RTC_DLOG(LS_INFO) << "Not setting media_transport_ rate observers."; } @@ -731,7 +723,6 @@ ChannelSend::~ChannelSend() { } StopSend(); - int error = audio_coding_->RegisterTransportCallback(NULL); RTC_DCHECK_EQ(0, error); @@ -814,7 +805,9 @@ bool ChannelSend::SetEncoder(int payload_type, void ChannelSend::ModifyEncoder( rtc::FunctionView*)> modifier) { - RTC_DCHECK_RUN_ON(&worker_thread_checker_); + // This method can be called on the worker thread, module process thread + // or network thread. Audio coding is thread safe, so we do not need to + // enforce the calling thread. audio_coding_->ModifyEncoder(modifier); } @@ -1145,30 +1138,6 @@ void ChannelSend::ProcessAndEncodeAudioOnTaskQueue(AudioFrame* audio_input) { _timeStamp += static_cast(audio_input->samples_per_channel_); } -void ChannelSend::UpdateOverheadForEncoder() { - size_t overhead_per_packet = - transport_overhead_per_packet_ + rtp_overhead_per_packet_; - audio_coding_->ModifyEncoder([&](std::unique_ptr* encoder) { - if (*encoder) { - (*encoder)->OnReceivedOverhead(overhead_per_packet); - } - }); -} - -void ChannelSend::SetTransportOverhead(size_t transport_overhead_per_packet) { - RTC_DCHECK_RUN_ON(&worker_thread_checker_); - rtc::CritScope cs(&overhead_per_packet_lock_); - transport_overhead_per_packet_ = transport_overhead_per_packet; - UpdateOverheadForEncoder(); -} - -// TODO(solenberg): Make AudioSendStream an OverheadObserver instead. -void ChannelSend::OnOverheadChanged(size_t overhead_bytes_per_packet) { - rtc::CritScope cs(&overhead_per_packet_lock_); - rtp_overhead_per_packet_ = overhead_bytes_per_packet; - UpdateOverheadForEncoder(); -} - ANAStats ChannelSend::GetANAStatistics() const { RTC_DCHECK_RUN_ON(&worker_thread_checker_); return audio_coding_->GetANAStats(); @@ -1243,6 +1212,9 @@ void ChannelSend::SetFrameEncryptor( } } +// TODO(sukhanov): Consider moving TargetTransferRate observer to +// AudioSendStream. Since AudioSendStream owns encoder and configures ANA, it +// makes sense to consolidate all rate (and overhead) calculation there. void ChannelSend::OnTargetTransferRate(TargetTransferRate rate) { RTC_DCHECK(media_transport_); OnReceivedRtt(rate.network_estimate.round_trip_time.ms()); @@ -1264,6 +1236,7 @@ std::unique_ptr CreateChannelSend( rtc::TaskQueue* encoder_queue, ProcessThread* module_process_thread, MediaTransportInterface* media_transport, + OverheadObserver* overhead_observer, Transport* rtp_transport, RtcpRttStats* rtcp_rtt_stats, RtcEventLog* rtc_event_log, @@ -1272,9 +1245,9 @@ std::unique_ptr CreateChannelSend( bool extmap_allow_mixed, int rtcp_report_interval_ms) { return absl::make_unique( - encoder_queue, module_process_thread, media_transport, rtp_transport, - rtcp_rtt_stats, rtc_event_log, frame_encryptor, crypto_options, - extmap_allow_mixed, rtcp_report_interval_ms); + encoder_queue, module_process_thread, media_transport, overhead_observer, + rtp_transport, rtcp_rtt_stats, rtc_event_log, frame_encryptor, + crypto_options, extmap_allow_mixed, rtcp_report_interval_ms); } } // namespace voe diff --git a/audio/channel_send.h b/audio/channel_send.h index ccd17df67d..148b1ff14b 100644 --- a/audio/channel_send.h +++ b/audio/channel_send.h @@ -89,7 +89,6 @@ class ChannelSendInterface { virtual void ProcessAndEncodeAudio( std::unique_ptr audio_frame) = 0; - virtual void SetTransportOverhead(size_t transport_overhead_per_packet) = 0; virtual RtpRtcp* GetRtpRtcp() const = 0; virtual void OnTwccBasedUplinkPacketLossRate(float packet_loss_rate) = 0; @@ -118,6 +117,7 @@ std::unique_ptr CreateChannelSend( rtc::TaskQueue* encoder_queue, ProcessThread* module_process_thread, MediaTransportInterface* media_transport, + OverheadObserver* overhead_observer, Transport* rtp_transport, RtcpRttStats* rtcp_rtt_stats, RtcEventLog* rtc_event_log,