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();