From c335b0e63bff56ca0fbfa617dee6a644c85df164 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Bostr=C3=B6m?= Date: Thu, 8 Apr 2021 07:25:38 +0200 Subject: [PATCH] [Unified Plan] Don't end audio tracks when SSRC changes. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The RemoteAudioSource has an AudioDataProxy that acts as a sink, passing along data from AudioRecvStreams to the RemoteAudioSource. If an SSRC is changed (or other reconfiguration happens) with SDP, the recv stream and proxy get recreated. In Plan B, because remote tracks maps 1:1 with SSRCs, it made sense to end remote track/audio source in response to this. In Plan B, a new receiver, with a new track and a new proxy would be created for the new SSRC. In Unified Plan however, remote tracks correspond to m= sections. The remote track should only end on port:0 (or RTCP BYE or timeout, etc), not because the recv stream of an m= section is recreated. The code already supports changing SSRC and this is working correctly, but because ~AudioDataProxy() would end the source this would cause the MediaStreamTrack of the receiver to end (even though the media engine is still processing the remote audio stream correctly under the hood). This issue only happened on audio tracks, and because of timing of PostTasks the track would kEnd in Chromium *after* promise.then(). This CL fixes that issue by not ending the source when the proxy is destroyed. Destroying a recv stream is a temporary action in Unified Plan, unless stopped. Tests are added ensuring tracks are kLive. I have manually verified that this CL fixes the issue and that both audio and video is flowing through the entire pipeline: https://jsfiddle.net/henbos/h21xec97/122/ Bug: chromium:1121454 Change-Id: Ic21ac8ea263ccf021b96a14d3e4e3b24eb756c86 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/214136 Commit-Queue: Henrik Boström Reviewed-by: Harald Alvestrand Cr-Commit-Position: refs/heads/master@{#33645} --- pc/audio_rtp_receiver.cc | 16 +++++++--- pc/audio_rtp_receiver.h | 6 ++-- pc/peer_connection_rtp_unittest.cc | 50 ++++++++++++++++++++++++++++++ pc/remote_audio_source.cc | 18 +++++++++-- pc/remote_audio_source.h | 18 ++++++++++- pc/rtp_sender_receiver_unittest.cc | 3 +- pc/rtp_transmission_manager.cc | 7 +++-- 7 files changed, 104 insertions(+), 14 deletions(-) diff --git a/pc/audio_rtp_receiver.cc b/pc/audio_rtp_receiver.cc index e8fad28d10..48553ba9f5 100644 --- a/pc/audio_rtp_receiver.cc +++ b/pc/audio_rtp_receiver.cc @@ -28,18 +28,25 @@ namespace webrtc { AudioRtpReceiver::AudioRtpReceiver(rtc::Thread* worker_thread, std::string receiver_id, - std::vector stream_ids) + std::vector stream_ids, + bool is_unified_plan) : AudioRtpReceiver(worker_thread, receiver_id, - CreateStreamsFromIds(std::move(stream_ids))) {} + CreateStreamsFromIds(std::move(stream_ids)), + is_unified_plan) {} AudioRtpReceiver::AudioRtpReceiver( rtc::Thread* worker_thread, const std::string& receiver_id, - const std::vector>& streams) + const std::vector>& streams, + bool is_unified_plan) : worker_thread_(worker_thread), id_(receiver_id), - source_(new rtc::RefCountedObject(worker_thread)), + source_(new rtc::RefCountedObject( + worker_thread, + is_unified_plan + ? RemoteAudioSource::OnAudioChannelGoneAction::kSurvive + : RemoteAudioSource::OnAudioChannelGoneAction::kEnd)), track_(AudioTrackProxyWithInternal::Create( rtc::Thread::Current(), AudioTrack::Create(receiver_id, source_))), @@ -137,6 +144,7 @@ void AudioRtpReceiver::Stop() { if (stopped_) { return; } + source_->SetState(MediaSourceInterface::kEnded); if (media_channel_) { // Allow that SetOutputVolume fail. This is the normal case when the // underlying media channel has already been deleted. diff --git a/pc/audio_rtp_receiver.h b/pc/audio_rtp_receiver.h index ec77bbc486..789d4a0f52 100644 --- a/pc/audio_rtp_receiver.h +++ b/pc/audio_rtp_receiver.h @@ -43,12 +43,14 @@ class AudioRtpReceiver : public ObserverInterface, public: AudioRtpReceiver(rtc::Thread* worker_thread, std::string receiver_id, - std::vector stream_ids); + std::vector stream_ids, + bool is_unified_plan); // TODO(https://crbug.com/webrtc/9480): Remove this when streams() is removed. AudioRtpReceiver( rtc::Thread* worker_thread, const std::string& receiver_id, - const std::vector>& streams); + const std::vector>& streams, + bool is_unified_plan); virtual ~AudioRtpReceiver(); // ObserverInterface implementation diff --git a/pc/peer_connection_rtp_unittest.cc b/pc/peer_connection_rtp_unittest.cc index 3614f0c749..54287e7b3b 100644 --- a/pc/peer_connection_rtp_unittest.cc +++ b/pc/peer_connection_rtp_unittest.cc @@ -779,6 +779,56 @@ TEST_F(PeerConnectionRtpTestUnifiedPlan, UnsignaledSsrcCreatesReceiverStreams) { EXPECT_EQ(receivers[0]->streams()[0]->id(), kStreamId1); EXPECT_EQ(receivers[0]->streams()[1]->id(), kStreamId2); } +TEST_F(PeerConnectionRtpTestUnifiedPlan, TracksDoNotEndWhenSsrcChanges) { + constexpr uint32_t kFirstMungedSsrc = 1337u; + + auto caller = CreatePeerConnection(); + auto callee = CreatePeerConnection(); + + // Caller offers to receive audio and video. + RtpTransceiverInit init; + init.direction = RtpTransceiverDirection::kRecvOnly; + caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO, init); + caller->AddTransceiver(cricket::MEDIA_TYPE_VIDEO, init); + + // Callee wants to send audio and video tracks. + callee->AddTrack(callee->CreateAudioTrack("audio_track"), {}); + callee->AddTrack(callee->CreateVideoTrack("video_track"), {}); + + // Do inittial offer/answer exchange. + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + ASSERT_TRUE( + caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal())); + ASSERT_EQ(caller->observer()->add_track_events_.size(), 2u); + ASSERT_EQ(caller->pc()->GetReceivers().size(), 2u); + + // Do a follow-up offer/answer exchange where the SSRCs are modified. + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + auto answer = callee->CreateAnswer(); + auto& contents = answer->description()->contents(); + ASSERT_TRUE(!contents.empty()); + for (size_t i = 0; i < contents.size(); ++i) { + auto& mutable_streams = contents[i].media_description()->mutable_streams(); + ASSERT_EQ(mutable_streams.size(), 1u); + mutable_streams[0].ssrcs = {kFirstMungedSsrc + static_cast(i)}; + } + ASSERT_TRUE( + callee->SetLocalDescription(CloneSessionDescription(answer.get()))); + ASSERT_TRUE( + caller->SetRemoteDescription(CloneSessionDescription(answer.get()))); + + // No furher track events should fire because we never changed direction, only + // SSRCs. + ASSERT_EQ(caller->observer()->add_track_events_.size(), 2u); + // We should have the same number of receivers as before. + auto receivers = caller->pc()->GetReceivers(); + ASSERT_EQ(receivers.size(), 2u); + // The tracks are still alive. + EXPECT_EQ(receivers[0]->track()->state(), + MediaStreamTrackInterface::TrackState::kLive); + EXPECT_EQ(receivers[1]->track()->state(), + MediaStreamTrackInterface::TrackState::kLive); +} // Tests that with Unified Plan if the the stream id changes for a track when // when setting a new remote description, that the media stream is updated diff --git a/pc/remote_audio_source.cc b/pc/remote_audio_source.cc index 3f53bb8148..848fba372b 100644 --- a/pc/remote_audio_source.cc +++ b/pc/remote_audio_source.cc @@ -49,9 +49,12 @@ class RemoteAudioSource::AudioDataProxy : public AudioSinkInterface { const rtc::scoped_refptr source_; }; -RemoteAudioSource::RemoteAudioSource(rtc::Thread* worker_thread) +RemoteAudioSource::RemoteAudioSource( + rtc::Thread* worker_thread, + OnAudioChannelGoneAction on_audio_channel_gone_action) : main_thread_(rtc::Thread::Current()), worker_thread_(worker_thread), + on_audio_channel_gone_action_(on_audio_channel_gone_action), state_(MediaSourceInterface::kLive) { RTC_DCHECK(main_thread_); RTC_DCHECK(worker_thread_); @@ -90,6 +93,13 @@ void RemoteAudioSource::Stop(cricket::VoiceMediaChannel* media_channel, }); } +void RemoteAudioSource::SetState(SourceState new_state) { + if (state_ != new_state) { + state_ = new_state; + FireOnChanged(); + } +} + MediaSourceInterface::SourceState RemoteAudioSource::state() const { RTC_DCHECK(main_thread_->IsCurrent()); return state_; @@ -156,6 +166,9 @@ void RemoteAudioSource::OnData(const AudioSinkInterface::Data& audio) { } void RemoteAudioSource::OnAudioChannelGone() { + if (on_audio_channel_gone_action_ != OnAudioChannelGoneAction::kEnd) { + return; + } // Called when the audio channel is deleted. It may be the worker thread // in libjingle or may be a different worker thread. // This object needs to live long enough for the cleanup logic in OnMessage to @@ -170,8 +183,7 @@ void RemoteAudioSource::OnAudioChannelGone() { void RemoteAudioSource::OnMessage(rtc::Message* msg) { RTC_DCHECK(main_thread_->IsCurrent()); sinks_.clear(); - state_ = MediaSourceInterface::kEnded; - FireOnChanged(); + SetState(MediaSourceInterface::kEnded); // Will possibly delete this RemoteAudioSource since it is reference counted // in the message. delete msg->pdata; diff --git a/pc/remote_audio_source.h b/pc/remote_audio_source.h index 276a103549..2eae073272 100644 --- a/pc/remote_audio_source.h +++ b/pc/remote_audio_source.h @@ -40,7 +40,21 @@ namespace webrtc { class RemoteAudioSource : public Notifier, rtc::MessageHandler { public: - explicit RemoteAudioSource(rtc::Thread* worker_thread); + // In Unified Plan, receivers map to m= sections and their tracks and sources + // survive SSRCs being reconfigured. The life cycle of the remote audio source + // is associated with the life cycle of the m= section, and thus even if an + // audio channel is destroyed the RemoteAudioSource should kSurvive. + // + // In Plan B however, remote audio sources map 1:1 with an SSRCs and if an + // audio channel is destroyed, the RemoteAudioSource should kEnd. + enum class OnAudioChannelGoneAction { + kSurvive, + kEnd, + }; + + explicit RemoteAudioSource( + rtc::Thread* worker_thread, + OnAudioChannelGoneAction on_audio_channel_gone_action); // Register and unregister remote audio source with the underlying media // engine. @@ -48,6 +62,7 @@ class RemoteAudioSource : public Notifier, absl::optional ssrc); void Stop(cricket::VoiceMediaChannel* media_channel, absl::optional ssrc); + void SetState(SourceState new_state); // MediaSourceInterface implementation. MediaSourceInterface::SourceState state() const override; @@ -75,6 +90,7 @@ class RemoteAudioSource : public Notifier, rtc::Thread* const main_thread_; rtc::Thread* const worker_thread_; + const OnAudioChannelGoneAction on_audio_channel_gone_action_; std::list audio_observers_; Mutex sink_lock_; std::list sinks_; diff --git a/pc/rtp_sender_receiver_unittest.cc b/pc/rtp_sender_receiver_unittest.cc index 4d6d58d8f6..97093e82be 100644 --- a/pc/rtp_sender_receiver_unittest.cc +++ b/pc/rtp_sender_receiver_unittest.cc @@ -302,7 +302,8 @@ class RtpSenderReceiverTest void CreateAudioRtpReceiver( std::vector> streams = {}) { audio_rtp_receiver_ = - new AudioRtpReceiver(rtc::Thread::Current(), kAudioTrackId, streams); + new AudioRtpReceiver(rtc::Thread::Current(), kAudioTrackId, streams, + /*is_unified_plan=*/true); audio_rtp_receiver_->SetMediaChannel(voice_media_channel_); audio_rtp_receiver_->SetupMediaChannel(kAudioSsrc); audio_track_ = audio_rtp_receiver_->audio_track(); diff --git a/pc/rtp_transmission_manager.cc b/pc/rtp_transmission_manager.cc index e796f9b1b1..eaf29b889f 100644 --- a/pc/rtp_transmission_manager.cc +++ b/pc/rtp_transmission_manager.cc @@ -240,8 +240,9 @@ RtpTransmissionManager::CreateReceiver(cricket::MediaType media_type, receiver; if (media_type == cricket::MEDIA_TYPE_AUDIO) { receiver = RtpReceiverProxyWithInternal::Create( - signaling_thread(), new AudioRtpReceiver(worker_thread(), receiver_id, - std::vector({}))); + signaling_thread(), + new AudioRtpReceiver(worker_thread(), receiver_id, + std::vector({}), IsUnifiedPlan())); NoteUsageEvent(UsageEvent::AUDIO_ADDED); } else { RTC_DCHECK_EQ(media_type, cricket::MEDIA_TYPE_VIDEO); @@ -453,7 +454,7 @@ void RtpTransmissionManager::CreateAudioReceiver( // TODO(https://crbug.com/webrtc/9480): When we remove remote_streams(), use // the constructor taking stream IDs instead. auto* audio_receiver = new AudioRtpReceiver( - worker_thread(), remote_sender_info.sender_id, streams); + worker_thread(), remote_sender_info.sender_id, streams, IsUnifiedPlan()); audio_receiver->SetMediaChannel(voice_media_channel()); if (remote_sender_info.sender_id == kDefaultAudioSenderId) { audio_receiver->SetupUnsignaledMediaChannel();