From 179a3923b9e402f427728d52b3024a3de1696a66 Mon Sep 17 00:00:00 2001 From: "Piotr (Peter) Slatala" Date: Fri, 16 Nov 2018 09:57:58 -0800 Subject: [PATCH] Implement TargetBitrate, NetworkRoute and overhead features of media transport interface. So far ANA was not available for media transport interface. With recent changes to media transport, we can now account for packet overhead, network route (ip/tcp/udp/turn overheads) and we can also use bandwidth estimate from the media transport. Bug: webrtc:9719 Change-Id: I98c9a09dd418b763c339ee2ee05592e164cf9199 Reviewed-on: https://webrtc-review.googlesource.com/c/110367 Commit-Queue: Peter Slatala Reviewed-by: Steve Anton Reviewed-by: Fredrik Solenberg Reviewed-by: Anton Sukhanov Cr-Commit-Position: refs/heads/master@{#25677} --- audio/channel_receive.cc | 9 ++++++ audio/channel_send.cc | 68 +++++++++++++++++++++++++++++++++++----- audio/channel_send.h | 24 +++++++++++--- call/BUILD.gn | 1 + call/call_unittest.cc | 23 ++++++++++++++ pc/channel.cc | 31 ++++++++++++++++-- pc/channel.h | 6 +++- 7 files changed, 146 insertions(+), 16 deletions(-) diff --git a/audio/channel_receive.cc b/audio/channel_receive.cc index 9c100fba75..e2ce7537bf 100644 --- a/audio/channel_receive.cc +++ b/audio/channel_receive.cc @@ -1004,6 +1004,15 @@ int ChannelReceive::GetRtpTimestampRateHz() const { } int64_t ChannelReceive::GetRTT() const { + if (media_transport_) { + auto target_rate = media_transport_->GetLatestTargetTransferRate(); + if (target_rate.has_value()) { + return target_rate->network_estimate.round_trip_time.ms(); + } + + return 0; + } + RtcpMode method = _rtpRtcpModule->RTCP(); if (method == RtcpMode::kOff) { return 0; diff --git a/audio/channel_send.cc b/audio/channel_send.cc index a8b93ccd2a..e96ef4afde 100644 --- a/audio/channel_send.cc +++ b/audio/channel_send.cc @@ -479,15 +479,23 @@ ChannelSend::ChannelSend(rtc::TaskQueue* encoder_queue, audio_coding_.reset(AudioCodingModule::Create(AudioCodingModule::Config())); RtpRtcp::Configuration configuration; + + // We gradually remove codepaths that depend on RTP when using media + // transport. All of this logic should be moved to the future + // 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; + configuration.bandwidth_callback = rtcp_observer_.get(); + configuration.transport_feedback_callback = feedback_observer_proxy_.get(); + } + configuration.audio = true; configuration.outgoing_transport = this; - configuration.overhead_observer = this; - configuration.bandwidth_callback = rtcp_observer_.get(); configuration.paced_sender = rtp_packet_sender_proxy_.get(); configuration.transport_sequence_number_allocator = seq_num_allocator_proxy_.get(); - configuration.transport_feedback_callback = feedback_observer_proxy_.get(); configuration.event_log = event_log_; configuration.rtt_stats = rtcp_rtt_stats; @@ -500,6 +508,16 @@ ChannelSend::ChannelSend(rtc::TaskQueue* encoder_queue, _rtpRtcpModule.reset(RtpRtcp::CreateRtpRtcp(configuration)); _rtpRtcpModule->SetSendingMediaStatus(false); + // We want to invoke the 'TargetRateObserver' and |OnOverheadChanged| + // callbacks after the audio_coding_ is fully initialized. + 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."; + } + channel_state_.Reset(); _moduleProcessThreadPtr->RegisterModule(_rtpRtcpModule.get(), RTC_FROM_HERE); @@ -522,6 +540,10 @@ ChannelSend::ChannelSend(rtc::TaskQueue* encoder_queue, ChannelSend::~ChannelSend() { RTC_DCHECK(construction_thread_.CalledOnValidThread()); + if (media_transport_) { + media_transport_->RemoveTargetTransferRateObserver(this); + } + StopSend(); int error = audio_coding_->RegisterTransportCallback(NULL); @@ -693,6 +715,12 @@ void ChannelSend::RegisterTransport(Transport* transport) { // TODO(nisse): Delete always-true return value. bool ChannelSend::ReceivedRTCPPacket(const uint8_t* data, size_t length) { + if (media_transport_) { + // Ignore RTCP packets while media transport is used. + // Those packets should not arrive, but we are seeing occasional packets. + return 0; + } + // Deliver RTCP packet to RTP/RTCP module for parsing _rtpRtcpModule->IncomingRtcpPacket(data, length); @@ -710,11 +738,7 @@ bool ChannelSend::ReceivedRTCPPacket(const uint8_t* data, size_t length) { } retransmission_rate_limiter_->SetWindowSize(nack_window_ms); - // Invoke audio encoders OnReceivedRtt(). - audio_coding_->ModifyEncoder([&](std::unique_ptr* encoder) { - if (*encoder) - (*encoder)->OnReceivedRtt(rtt); - }); + OnReceivedRtt(rtt); return true; } @@ -1010,6 +1034,19 @@ int ChannelSend::GetRtpTimestampRateHz() const { } int64_t ChannelSend::GetRTT() const { + if (media_transport_) { + // GetRTT is generally used in the RTCP codepath, where media transport is + // not present and so it shouldn't be needed. But it's also invoked in + // 'GetStats' method, and for now returning media transport RTT here gives + // us "free" rtt stats for media transport. + auto target_rate = media_transport_->GetLatestTargetTransferRate(); + if (target_rate.has_value()) { + return target_rate.value().network_estimate.round_trip_time.ms(); + } + + return 0; + } + RtcpMode method = _rtpRtcpModule->RTCP(); if (method == RtcpMode::kOff) { return 0; @@ -1046,5 +1083,20 @@ void ChannelSend::SetFrameEncryptor( } } +void ChannelSend::OnTargetTransferRate(TargetTransferRate rate) { + RTC_DCHECK(media_transport_); + OnReceivedRtt(rate.network_estimate.round_trip_time.ms()); +} + +void ChannelSend::OnReceivedRtt(int64_t rtt_ms) { + // Invoke audio encoders OnReceivedRtt(). + audio_coding_->ModifyEncoder( + [rtt_ms](std::unique_ptr* encoder) { + if (*encoder) { + (*encoder)->OnReceivedRtt(rtt_ms); + } + }); +} + } // namespace voe } // namespace webrtc diff --git a/audio/channel_send.h b/audio/channel_send.h index 63e8d04e86..3d194a7ecd 100644 --- a/audio/channel_send.h +++ b/audio/channel_send.h @@ -108,11 +108,11 @@ class ChannelSendState { State state_; }; -class ChannelSend - : public Transport, - public AudioPacketizationCallback, // receive encoded packets from the - // ACM - public OverheadObserver { +class ChannelSend : public Transport, + public AudioPacketizationCallback, // receive encoded + // packets from the ACM + public OverheadObserver, + public TargetTransferRateObserver { public: // TODO(nisse): Make OnUplinkPacketLossRate public, and delete friend // declaration. @@ -202,12 +202,24 @@ class ChannelSend void OnRecoverableUplinkPacketLossRate(float recoverable_packet_loss_rate); + // Returns the RTT in milliseconds. int64_t GetRTT() const; // E2EE Custom Audio Frame Encryption void SetFrameEncryptor( rtc::scoped_refptr frame_encryptor); + // In RTP we currently rely on RTCP packets (|ReceivedRTCPPacket|) to inform + // about RTT. + // In media transport we rely on the TargetTransferRateObserver instead. + // In other words, if you are using RTP, you should expect + // |ReceivedRTCPPacket| to be called, if you are using media transport, + // |OnTargetTransferRate| will be called. + // + // In future, RTP media will move to the media transport implementation and + // these conditions will be removed. + void OnTargetTransferRate(TargetTransferRate rate) override; + private: class ProcessAndEncodeAudioTask; @@ -261,6 +273,8 @@ class ChannelSend // for encoding. void ProcessAndEncodeAudioOnTaskQueue(AudioFrame* audio_input); + void OnReceivedRtt(int64_t rtt_ms); + rtc::CriticalSection _callbackCritSect; rtc::CriticalSection volume_settings_critsect_; diff --git a/call/BUILD.gn b/call/BUILD.gn index 34c16efcc0..50decd1216 100644 --- a/call/BUILD.gn +++ b/call/BUILD.gn @@ -333,6 +333,7 @@ if (rtc_include_tests) { "..:webrtc_common", "../api:array_view", "../api:fake_media_transport", + "../api:fake_media_transport", "../api:libjingle_peerconnection_api", "../api:mock_audio_mixer", "../api/audio_codecs:builtin_audio_decoder_factory", diff --git a/call/call_unittest.cc b/call/call_unittest.cc index 43c3355159..770f255313 100644 --- a/call/call_unittest.cc +++ b/call/call_unittest.cc @@ -290,4 +290,27 @@ TEST(CallTest, RecreatingAudioStreamWithSameSsrcReusesRtpState) { EXPECT_EQ(rtp_state1.media_has_been_sent, rtp_state2.media_has_been_sent); } +TEST(CallTest, RegisterMediaTransportBitrateCallbacksInCreateStream) { + CallHelper call; + MediaTransportSettings settings; + webrtc::FakeMediaTransport fake_media_transport(settings); + + EXPECT_EQ(0, fake_media_transport.target_rate_observers_size()); + AudioSendStream::Config config(/*send_transport=*/nullptr, + /*media_transport=*/&fake_media_transport); + + call->MediaTransportChange(&fake_media_transport); + AudioSendStream* stream = call->CreateAudioSendStream(config); + + // We get 2 subscribers: one subscriber from call.cc, and one from + // ChannelSend. + EXPECT_EQ(2, fake_media_transport.target_rate_observers_size()); + + call->DestroyAudioSendStream(stream); + EXPECT_EQ(1, fake_media_transport.target_rate_observers_size()); + + call->MediaTransportChange(nullptr); + EXPECT_EQ(0, fake_media_transport.target_rate_observers_size()); +} + } // namespace webrtc diff --git a/pc/channel.cc b/pc/channel.cc index 0f857cdd3b..67d698487c 100644 --- a/pc/channel.cc +++ b/pc/channel.cc @@ -118,6 +118,11 @@ BaseChannel::BaseChannel(rtc::Thread* worker_thread, BaseChannel::~BaseChannel() { TRACE_EVENT0("webrtc", "BaseChannel::~BaseChannel"); RTC_DCHECK_RUN_ON(worker_thread_); + + if (media_transport_) { + media_transport_->SetNetworkChangeCallback(nullptr); + } + // Eats any outstanding messages or packets. worker_thread_->Clear(&invoker_); worker_thread_->Clear(this); @@ -137,8 +142,15 @@ bool BaseChannel::ConnectToRtpTransport() { this, &BaseChannel::OnTransportReadyToSend); rtp_transport_->SignalRtcpPacketReceived.connect( this, &BaseChannel::OnRtcpPacketReceived); - rtp_transport_->SignalNetworkRouteChanged.connect( - this, &BaseChannel::OnNetworkRouteChanged); + + // If media transport is used, it's responsible for providing network + // route changed callbacks. + if (!media_transport_) { + rtp_transport_->SignalNetworkRouteChanged.connect( + this, &BaseChannel::OnNetworkRouteChanged); + } + // TODO(bugs.webrtc.org/9719): Media transport should also be used to provide + // 'writable' state here. rtp_transport_->SignalWritableState.connect(this, &BaseChannel::OnWritableState); rtp_transport_->SignalSentPacket.connect(this, @@ -159,12 +171,20 @@ void BaseChannel::DisconnectFromRtpTransport() { void BaseChannel::Init_w(webrtc::RtpTransportInternal* rtp_transport, webrtc::MediaTransportInterface* media_transport) { RTC_DCHECK_RUN_ON(worker_thread_); + media_transport_ = media_transport; + network_thread_->Invoke( RTC_FROM_HERE, [this, rtp_transport] { SetRtpTransport(rtp_transport); }); // Both RTP and RTCP channels should be set, we can call SetInterface on // the media channel and it can set network options. media_channel_->SetInterface(this, media_transport); + + RTC_LOG(LS_INFO) << "BaseChannel::Init_w, media_transport=" + << (media_transport_ != nullptr); + if (media_transport_) { + media_transport_->SetNetworkChangeCallback(this); + } } void BaseChannel::Deinit() { @@ -353,6 +373,8 @@ void BaseChannel::OnWritableState(bool writable) { void BaseChannel::OnNetworkRouteChanged( absl::optional network_route) { + RTC_LOG(LS_INFO) << "Network route was changed."; + RTC_DCHECK(network_thread_->IsCurrent()); rtc::NetworkRoute new_route; if (network_route) { @@ -767,6 +789,11 @@ void BaseChannel::UpdateMediaSendRecvState() { Bind(&BaseChannel::UpdateMediaSendRecvState_w, this)); } +void BaseChannel::OnNetworkRouteChanged( + const rtc::NetworkRoute& network_route) { + OnNetworkRouteChanged(absl::make_optional(network_route)); +} + void VoiceChannel::UpdateMediaSendRecvState_w() { // Render incoming data if we're the active call, and we have the local // content. We receive data on the default channel and multiplexed streams. diff --git a/pc/channel.h b/pc/channel.h index 5644bb457d..6264bb3904 100644 --- a/pc/channel.h +++ b/pc/channel.h @@ -72,7 +72,8 @@ class BaseChannel : public ChannelInterface, public rtc::MessageHandler, public sigslot::has_slots<>, public MediaChannel::NetworkInterface, - public webrtc::RtpPacketSinkInterface { + public webrtc::RtpPacketSinkInterface, + public webrtc::MediaTransportNetworkChangeCallback { public: // If |srtp_required| is true, the channel will not send or receive any // RTP/RTCP packets without using SRTP (either using SDES or DTLS-SRTP). @@ -303,6 +304,9 @@ class BaseChannel : public ChannelInterface, void SignalSentPacket_n(const rtc::SentPacket& sent_packet); void SignalSentPacket_w(const rtc::SentPacket& sent_packet); bool IsReadyToSendMedia_n() const; + + // MediaTransportNetworkChangeCallback override. + void OnNetworkRouteChanged(const rtc::NetworkRoute& network_route) override; rtc::Thread* const worker_thread_; rtc::Thread* const network_thread_; rtc::Thread* const signaling_thread_;