From 6589def397b2685e617260ee0004fbc969a266aa Mon Sep 17 00:00:00 2001 From: Tommi Date: Thu, 17 Feb 2022 23:36:47 +0100 Subject: [PATCH] Align sender/receiver teardown in RtpTransceiver. This makes SetChannel() consistently make 2 invokes instead of a multiple of senders+receivers (previous minimum was 4 but could be larger). * Stop() doesn't hop to the worker thread. * SetMediaChannel(), an already-required step on the worker thread for senders and *sometimes* for receivers[1], is now consistently required for both. This simplifies transceiver teardown and enables the next bullet. * Transceiver stops all senders and receivers in one go rather than ping ponging between threads. [1] When not required, it was done implicitly inside of Stop(). See changes in `RtpTransceiver::SetChannel` Bug: webrtc:13540 Change-Id: Ied61636c8ef09d782bf519524fff2a31e15219a8 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/249797 Reviewed-by: Harald Alvestrand Reviewed-by: Tomas Gunnarsson Commit-Queue: Tomas Gunnarsson Cr-Commit-Position: refs/heads/main@{#36057} --- pc/BUILD.gn | 1 + pc/audio_rtp_receiver.cc | 150 +++++++++++++-------------- pc/audio_rtp_receiver.h | 26 +++-- pc/audio_rtp_receiver_unittest.cc | 3 +- pc/rtc_stats_collector_unittest.cc | 24 ++++- pc/rtp_receiver.h | 21 +++- pc/rtp_sender.h | 5 + pc/rtp_sender_receiver_unittest.cc | 7 +- pc/rtp_transceiver.cc | 59 +++++++---- pc/rtp_transceiver_unittest.cc | 35 ++++--- pc/rtp_transmission_manager.cc | 18 ++-- pc/stats_collector_unittest.cc | 5 +- pc/test/mock_rtp_receiver_internal.h | 2 +- pc/video_rtp_receiver.cc | 131 ++++++++++++----------- pc/video_rtp_receiver.h | 13 ++- pc/video_rtp_receiver_unittest.cc | 34 +++--- 16 files changed, 314 insertions(+), 220 deletions(-) diff --git a/pc/BUILD.gn b/pc/BUILD.gn index e7b7cc0102..1a296c1658 100644 --- a/pc/BUILD.gn +++ b/pc/BUILD.gn @@ -1278,6 +1278,7 @@ rtc_library("video_track") { "../api:sequence_checker", "../api/video:video_frame", "../media:rtc_media_base", + "../pc:rtc_pc_base", "../rtc_base", "../rtc_base:checks", "../rtc_base:refcount", diff --git a/pc/audio_rtp_receiver.cc b/pc/audio_rtp_receiver.cc index a49b7ce48e..43294c7e93 100644 --- a/pc/audio_rtp_receiver.cc +++ b/pc/audio_rtp_receiver.cc @@ -25,20 +25,24 @@ namespace webrtc { -AudioRtpReceiver::AudioRtpReceiver(rtc::Thread* worker_thread, - std::string receiver_id, - std::vector stream_ids, - bool is_unified_plan) +AudioRtpReceiver::AudioRtpReceiver( + rtc::Thread* worker_thread, + std::string receiver_id, + std::vector stream_ids, + bool is_unified_plan, + cricket::VoiceMediaChannel* voice_channel /*= nullptr*/) : AudioRtpReceiver(worker_thread, receiver_id, CreateStreamsFromIds(std::move(stream_ids)), - is_unified_plan) {} + is_unified_plan, + voice_channel) {} AudioRtpReceiver::AudioRtpReceiver( rtc::Thread* worker_thread, const std::string& receiver_id, const std::vector>& streams, - bool is_unified_plan) + bool is_unified_plan, + cricket::VoiceMediaChannel* voice_channel /*= nullptr*/) : worker_thread_(worker_thread), id_(receiver_id), source_(rtc::make_ref_counted( @@ -49,7 +53,8 @@ AudioRtpReceiver::AudioRtpReceiver( track_(AudioTrackProxyWithInternal::Create( rtc::Thread::Current(), AudioTrack::Create(receiver_id, source_))), - cached_track_enabled_(track_->enabled()), + media_channel_(voice_channel), + cached_track_enabled_(track_->internal()->enabled()), attachment_id_(GenerateUniqueId()), worker_thread_safety_(PendingTaskSafetyFlag::CreateDetachedInactive()) { RTC_DCHECK(worker_thread_); @@ -69,15 +74,15 @@ AudioRtpReceiver::~AudioRtpReceiver() { void AudioRtpReceiver::OnChanged() { RTC_DCHECK_RUN_ON(&signaling_thread_checker_); - if (cached_track_enabled_ != track_->enabled()) { - cached_track_enabled_ = track_->enabled(); - worker_thread_->PostTask(ToQueuedTask( - worker_thread_safety_, - [this, enabled = cached_track_enabled_, volume = cached_volume_]() { - RTC_DCHECK_RUN_ON(worker_thread_); - Reconfigure(enabled, volume); - })); - } + const bool enabled = track_->internal()->enabled(); + if (cached_track_enabled_ == enabled) + return; + cached_track_enabled_ = enabled; + worker_thread_->PostTask( + ToQueuedTask(worker_thread_safety_, [this, enabled]() { + RTC_DCHECK_RUN_ON(worker_thread_); + Reconfigure(enabled); + })); } // RTC_RUN_ON(worker_thread_) @@ -97,20 +102,18 @@ void AudioRtpReceiver::OnSetVolume(double volume) { RTC_DCHECK_GE(volume, 0); RTC_DCHECK_LE(volume, 10); - // Update the cached_volume_ even when stopped, to allow clients to set the - // volume before starting/restarting, eg see crbug.com/1272566. - cached_volume_ = volume; - - // When the track is disabled, the volume of the source, which is the - // corresponding WebRtc Voice Engine channel will be 0. So we do not allow - // setting the volume to the source when the track is disabled. - if (track_->enabled()) { - worker_thread_->PostTask( - ToQueuedTask(worker_thread_safety_, [this, volume = cached_volume_]() { - RTC_DCHECK_RUN_ON(worker_thread_); - SetOutputVolume_w(volume); - })); - } + bool track_enabled = track_->internal()->enabled(); + worker_thread_->Invoke(RTC_FROM_HERE, [&]() { + RTC_DCHECK_RUN_ON(worker_thread_); + // Update the cached_volume_ even when stopped, to allow clients to set + // the volume before starting/restarting, eg see crbug.com/1272566. + cached_volume_ = volume; + // When the track is disabled, the volume of the source, which is the + // corresponding WebRtc Voice Engine channel will be 0. So we do not + // allow setting the volume to the source when the track is disabled. + if (track_enabled) + SetOutputVolume_w(volume); + }); } rtc::scoped_refptr AudioRtpReceiver::dtls_transport() @@ -159,52 +162,49 @@ AudioRtpReceiver::GetFrameDecryptor() const { void AudioRtpReceiver::Stop() { RTC_DCHECK_RUN_ON(&signaling_thread_checker_); - // TODO(deadbeef): Need to do more here to fully stop receiving packets. source_->SetState(MediaSourceInterface::kEnded); - - worker_thread_->Invoke(RTC_FROM_HERE, [&]() { - RTC_DCHECK_RUN_ON(worker_thread_); - - if (media_channel_) - SetOutputVolume_w(0.0); - - SetMediaChannel_w(nullptr); - }); -} - -void AudioRtpReceiver::StopAndEndTrack() { - RTC_DCHECK_RUN_ON(&signaling_thread_checker_); - Stop(); track_->internal()->set_ended(); } -void AudioRtpReceiver::RestartMediaChannel(absl::optional ssrc) { +void AudioRtpReceiver::SetSourceEnded() { RTC_DCHECK_RUN_ON(&signaling_thread_checker_); + source_->SetState(MediaSourceInterface::kEnded); +} + +// RTC_RUN_ON(&signaling_thread_checker_) +void AudioRtpReceiver::RestartMediaChannel(absl::optional ssrc) { + bool enabled = track_->internal()->enabled(); MediaSourceInterface::SourceState state = source_->state(); - worker_thread_->Invoke( - RTC_FROM_HERE, - [&, enabled = cached_track_enabled_, volume = cached_volume_]() { - RTC_DCHECK_RUN_ON(worker_thread_); - if (!media_channel_) - return; // Can't restart. - - if (state != MediaSourceInterface::kInitializing) { - if (ssrc_ == ssrc) - return; - source_->Stop(media_channel_, ssrc_); - } - - ssrc_ = std::move(ssrc); - source_->Start(media_channel_, ssrc_); - if (ssrc_) { - media_channel_->SetBaseMinimumPlayoutDelayMs(*ssrc_, delay_.GetMs()); - } - - Reconfigure(enabled, volume); - }); + worker_thread_->Invoke(RTC_FROM_HERE, [&]() { + RTC_DCHECK_RUN_ON(worker_thread_); + RestartMediaChannel_w(std::move(ssrc), enabled, state); + }); source_->SetState(MediaSourceInterface::kLive); } +// RTC_RUN_ON(worker_thread_) +void AudioRtpReceiver::RestartMediaChannel_w( + absl::optional ssrc, + bool track_enabled, + MediaSourceInterface::SourceState state) { + if (!media_channel_) + return; // Can't restart. + + if (state != MediaSourceInterface::kInitializing) { + if (ssrc_ == ssrc) + return; + source_->Stop(media_channel_, ssrc_); + } + + ssrc_ = std::move(ssrc); + source_->Start(media_channel_, ssrc_); + if (ssrc_) { + media_channel_->SetBaseMinimumPlayoutDelayMs(*ssrc_, delay_.GetMs()); + } + + Reconfigure(track_enabled); +} + void AudioRtpReceiver::SetupMediaChannel(uint32_t ssrc) { RTC_DCHECK_RUN_ON(&signaling_thread_checker_); RestartMediaChannel(ssrc); @@ -284,10 +284,10 @@ void AudioRtpReceiver::SetDepacketizerToDecoderFrameTransformer( } // RTC_RUN_ON(worker_thread_) -void AudioRtpReceiver::Reconfigure(bool track_enabled, double volume) { +void AudioRtpReceiver::Reconfigure(bool track_enabled) { RTC_DCHECK(media_channel_); - SetOutputVolume_w(track_enabled ? volume : 0); + SetOutputVolume_w(track_enabled ? cached_volume_ : 0); if (ssrc_ && frame_decryptor_) { // Reattach the frame decryptor if we were reconfigured. @@ -318,18 +318,12 @@ void AudioRtpReceiver::SetJitterBufferMinimumDelay( } void AudioRtpReceiver::SetMediaChannel(cricket::MediaChannel* media_channel) { - RTC_DCHECK_RUN_ON(&signaling_thread_checker_); + RTC_DCHECK_RUN_ON(worker_thread_); RTC_DCHECK(media_channel == nullptr || media_channel->media_type() == media_type()); + if (!media_channel && media_channel_) + SetOutputVolume_w(0.0); - worker_thread_->Invoke(RTC_FROM_HERE, [&] { - RTC_DCHECK_RUN_ON(worker_thread_); - SetMediaChannel_w(media_channel); - }); -} - -// RTC_RUN_ON(worker_thread_) -void AudioRtpReceiver::SetMediaChannel_w(cricket::MediaChannel* media_channel) { media_channel ? worker_thread_safety_->SetAlive() : worker_thread_safety_->SetNotAlive(); media_channel_ = static_cast(media_channel); diff --git a/pc/audio_rtp_receiver.h b/pc/audio_rtp_receiver.h index 978c550dfe..6f70243c0f 100644 --- a/pc/audio_rtp_receiver.h +++ b/pc/audio_rtp_receiver.h @@ -45,16 +45,24 @@ class AudioRtpReceiver : public ObserverInterface, public AudioSourceInterface::AudioObserver, public RtpReceiverInternal { public: + // The constructor supports optionally passing the voice channel to the + // instance at construction time without having to call `SetMediaChannel()` + // on the worker thread straight after construction. + // However, when using that, the assumption is that right after construction, + // a call to either `SetupUnsignaledMediaChannel` or `SetupMediaChannel` + // will be made, which will internally start the source on the worker thread. AudioRtpReceiver(rtc::Thread* worker_thread, std::string receiver_id, std::vector stream_ids, - bool is_unified_plan); + bool is_unified_plan, + cricket::VoiceMediaChannel* voice_channel = nullptr); // 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, - bool is_unified_plan); + bool is_unified_plan, + cricket::VoiceMediaChannel* media_channel = nullptr); virtual ~AudioRtpReceiver(); // ObserverInterface implementation @@ -90,7 +98,7 @@ class AudioRtpReceiver : public ObserverInterface, // RtpReceiverInternal implementation. void Stop() override; - void StopAndEndTrack() override; + void SetSourceEnded() override; void SetupMediaChannel(uint32_t ssrc) override; void SetupUnsignaledMediaChannel() override; uint32_t ssrc() const override; @@ -114,12 +122,14 @@ class AudioRtpReceiver : public ObserverInterface, override; private: - void RestartMediaChannel(absl::optional ssrc); - void Reconfigure(bool track_enabled, double volume) + void RestartMediaChannel(absl::optional ssrc) + RTC_RUN_ON(&signaling_thread_checker_); + void RestartMediaChannel_w(absl::optional ssrc, + bool track_enabled, + MediaSourceInterface::SourceState state) RTC_RUN_ON(worker_thread_); + void Reconfigure(bool track_enabled) RTC_RUN_ON(worker_thread_); void SetOutputVolume_w(double volume) RTC_RUN_ON(worker_thread_); - void SetMediaChannel_w(cricket::MediaChannel* media_channel) - RTC_RUN_ON(worker_thread_); RTC_NO_UNIQUE_ADDRESS SequenceChecker signaling_thread_checker_; rtc::Thread* const worker_thread_; @@ -132,7 +142,7 @@ class AudioRtpReceiver : public ObserverInterface, std::vector> streams_ RTC_GUARDED_BY(&signaling_thread_checker_); bool cached_track_enabled_ RTC_GUARDED_BY(&signaling_thread_checker_); - double cached_volume_ RTC_GUARDED_BY(&signaling_thread_checker_) = 1.0; + double cached_volume_ RTC_GUARDED_BY(worker_thread_) = 1.0; RtpReceiverObserverInterface* observer_ RTC_GUARDED_BY(&signaling_thread_checker_) = nullptr; bool received_first_packet_ RTC_GUARDED_BY(&signaling_thread_checker_) = diff --git a/pc/audio_rtp_receiver_unittest.cc b/pc/audio_rtp_receiver_unittest.cc index 763677b046..294e580525 100644 --- a/pc/audio_rtp_receiver_unittest.cc +++ b/pc/audio_rtp_receiver_unittest.cc @@ -24,6 +24,7 @@ using ::testing::Mock; static const int kTimeOut = 100; static const double kDefaultVolume = 1; static const double kVolume = 3.7; +static const double kVolumeMuted = 0.0; static const uint32_t kSsrc = 3; namespace webrtc { @@ -42,8 +43,8 @@ class AudioRtpReceiverTest : public ::testing::Test { } ~AudioRtpReceiverTest() { + EXPECT_CALL(media_channel_, SetOutputVolume(kSsrc, kVolumeMuted)); receiver_->SetMediaChannel(nullptr); - receiver_->Stop(); } rtc::Thread* worker_; diff --git a/pc/rtc_stats_collector_unittest.cc b/pc/rtc_stats_collector_unittest.cc index 8f0936c26c..b39621d862 100644 --- a/pc/rtc_stats_collector_unittest.cc +++ b/pc/rtc_stats_collector_unittest.cc @@ -363,7 +363,6 @@ rtc::scoped_refptr CreateMockSender( EXPECT_CALL(*sender, AttachmentId()).WillRepeatedly(Return(attachment_id)); EXPECT_CALL(*sender, stream_ids()).WillRepeatedly(Return(local_stream_ids)); EXPECT_CALL(*sender, SetTransceiverAsStopped()); - EXPECT_CALL(*sender, Stop()); return sender; } @@ -389,7 +388,7 @@ rtc::scoped_refptr CreateMockReceiver( return params; })); EXPECT_CALL(*receiver, AttachmentId()).WillRepeatedly(Return(attachment_id)); - EXPECT_CALL(*receiver, StopAndEndTrack()); + EXPECT_CALL(*receiver, Stop()).WillRepeatedly(Return()); return receiver; } @@ -460,6 +459,8 @@ class RTCStatsCollectorWrapper { rtc::scoped_refptr sender = CreateMockSender(media_type, track, ssrc, attachment_id, {}); + EXPECT_CALL(*sender, Stop()); + EXPECT_CALL(*sender, SetMediaChannel(_)); pc_->AddSender(sender); return sender; } @@ -490,6 +491,7 @@ class RTCStatsCollectorWrapper { .WillRepeatedly( Return(std::vector>( {remote_stream}))); + EXPECT_CALL(*receiver, SetMediaChannel(_)).WillRepeatedly(Return()); pc_->AddReceiver(receiver); return receiver; } @@ -532,6 +534,7 @@ class RTCStatsCollectorWrapper { voice_sender_info.local_stats[0].ssrc, voice_sender_info.local_stats[0].ssrc + 10, local_stream_ids); EXPECT_CALL(*rtp_sender, SetMediaChannel(_)).WillRepeatedly(Return()); + EXPECT_CALL(*rtp_sender, Stop()); pc_->AddSender(rtp_sender); } @@ -550,7 +553,7 @@ class RTCStatsCollectorWrapper { voice_receiver_info.local_stats[0].ssrc + 10); EXPECT_CALL(*rtp_receiver, streams()) .WillRepeatedly(Return(remote_streams)); - EXPECT_CALL(*rtp_receiver, SetMediaChannel(_)); + EXPECT_CALL(*rtp_receiver, SetMediaChannel(_)).WillRepeatedly(Return()); pc_->AddReceiver(rtp_receiver); } @@ -569,6 +572,7 @@ class RTCStatsCollectorWrapper { video_sender_info.local_stats[0].ssrc, video_sender_info.local_stats[0].ssrc + 10, local_stream_ids); EXPECT_CALL(*rtp_sender, SetMediaChannel(_)).WillRepeatedly(Return()); + EXPECT_CALL(*rtp_sender, Stop()); pc_->AddSender(rtp_sender); } @@ -587,7 +591,7 @@ class RTCStatsCollectorWrapper { video_receiver_info.local_stats[0].ssrc + 10); EXPECT_CALL(*rtp_receiver, streams()) .WillRepeatedly(Return(remote_streams)); - EXPECT_CALL(*rtp_receiver, SetMediaChannel(_)); + EXPECT_CALL(*rtp_receiver, SetMediaChannel(_)).WillRepeatedly(Return()); pc_->AddReceiver(rtp_receiver); } @@ -2691,6 +2695,8 @@ TEST_F(RTCStatsCollectorTest, RTCVideoSourceStatsCollectedForSenderWithTrack) { "LocalVideoTrackID", MediaStreamTrackInterface::kLive, video_source); rtc::scoped_refptr sender = CreateMockSender( cricket::MEDIA_TYPE_VIDEO, video_track, kSsrc, kAttachmentId, {}); + EXPECT_CALL(*sender, Stop()); + EXPECT_CALL(*sender, SetMediaChannel(_)); pc_->AddSender(sender); rtc::scoped_refptr report = stats_->GetStatsReport(); @@ -2734,6 +2740,8 @@ TEST_F(RTCStatsCollectorTest, "LocalVideoTrackID", MediaStreamTrackInterface::kLive, video_source); rtc::scoped_refptr sender = CreateMockSender( cricket::MEDIA_TYPE_VIDEO, video_track, kNoSsrc, kAttachmentId, {}); + EXPECT_CALL(*sender, Stop()); + EXPECT_CALL(*sender, SetMediaChannel(_)); pc_->AddSender(sender); rtc::scoped_refptr report = stats_->GetStatsReport(); @@ -2763,6 +2771,8 @@ TEST_F(RTCStatsCollectorTest, /*source=*/nullptr); rtc::scoped_refptr sender = CreateMockSender( cricket::MEDIA_TYPE_VIDEO, video_track, kSsrc, kAttachmentId, {}); + EXPECT_CALL(*sender, Stop()); + EXPECT_CALL(*sender, SetMediaChannel(_)); pc_->AddSender(sender); rtc::scoped_refptr report = stats_->GetStatsReport(); @@ -2785,6 +2795,8 @@ TEST_F(RTCStatsCollectorTest, pc_->AddVoiceChannel("AudioMid", "TransportName", voice_media_info); rtc::scoped_refptr sender = CreateMockSender( cricket::MEDIA_TYPE_AUDIO, /*track=*/nullptr, kSsrc, kAttachmentId, {}); + EXPECT_CALL(*sender, Stop()); + EXPECT_CALL(*sender, SetMediaChannel(_)); pc_->AddSender(sender); rtc::scoped_refptr report = stats_->GetStatsReport(); @@ -3108,6 +3120,8 @@ TEST_F(RTCStatsCollectorTest, rtc::scoped_refptr sender = CreateMockSender( cricket::MEDIA_TYPE_VIDEO, /*track=*/nullptr, kSsrc, kAttachmentId, {}); + EXPECT_CALL(*sender, Stop()); + EXPECT_CALL(*sender, SetMediaChannel(_)); pc_->AddSender(sender); rtc::scoped_refptr report = stats_->GetStatsReport(); @@ -3257,6 +3271,7 @@ TEST_F(RTCStatsCollectorTest, StatsReportedOnZeroSsrc) { MediaStreamTrackInterface::kLive); rtc::scoped_refptr sender = CreateMockSender(cricket::MEDIA_TYPE_AUDIO, track, 0, 49, {}); + EXPECT_CALL(*sender, Stop()); pc_->AddSender(sender); rtc::scoped_refptr report = stats_->GetStatsReport(); @@ -3276,6 +3291,7 @@ TEST_F(RTCStatsCollectorTest, DoNotCrashOnSsrcChange) { MediaStreamTrackInterface::kLive); rtc::scoped_refptr sender = CreateMockSender(cricket::MEDIA_TYPE_AUDIO, track, 4711, 49, {}); + EXPECT_CALL(*sender, Stop()); pc_->AddSender(sender); // We do not generate any matching voice_sender_info stats. diff --git a/pc/rtp_receiver.h b/pc/rtp_receiver.h index 73fc5b9858..7d124dfd69 100644 --- a/pc/rtp_receiver.h +++ b/pc/rtp_receiver.h @@ -42,16 +42,27 @@ namespace webrtc { // Internal class used by PeerConnection. class RtpReceiverInternal : public RtpReceiverInterface { public: - // Stops receiving. The track may be reactivated. + // Call on the signaling thread, to let the receiver know that the the + // embedded source object should enter a stopped/ended state and the track's + // state set to `kEnded`, a final state that cannot be reversed. virtual void Stop() = 0; - // Stops the receiver permanently. - // Causes the associated track to enter kEnded state. Cannot be reversed. - virtual void StopAndEndTrack() = 0; + + // Call on the signaling thread to set the source's state to `ended` before + // clearing the media channel (`SetMediaChannel(nullptr)`) on the worker + // thread. + // The difference between `Stop()` and `SetSourceEnded()` is that the latter + // does not change the state of the associated track. + // NOTE: Calling this function should be followed with a call to + // `SetMediaChannel(nullptr)` on the worker thread, to complete the operation. + virtual void SetSourceEnded() = 0; // Sets the underlying MediaEngine channel associated with this RtpSender. // A VoiceMediaChannel should be used for audio RtpSenders and // a VideoMediaChannel should be used for video RtpSenders. - // Must call SetMediaChannel(nullptr) before the media channel is destroyed. + // NOTE: + // * SetMediaChannel(nullptr) must be called before the media channel is + // destroyed. + // * This method must be invoked on the worker thread. virtual void SetMediaChannel(cricket::MediaChannel* media_channel) = 0; // Configures the RtpReceiver with the underlying media channel, with the diff --git a/pc/rtp_sender.h b/pc/rtp_sender.h index b87f8d4813..ca2d1385ce 100644 --- a/pc/rtp_sender.h +++ b/pc/rtp_sender.h @@ -222,6 +222,11 @@ class RtpSenderBase : public RtpSenderInternal, public ObserverInterface { std::vector stream_ids_; RtpParameters init_parameters_; + // TODO(tommi): `media_channel_` and several other member variables in this + // class (ssrc_, stopped_, etc) are accessed from more than one thread without + // a guard or lock. Internally there are also several Invoke()s that we could + // remove since the upstream code may already be performing several operations + // on the worker thread. cricket::MediaChannel* media_channel_ = nullptr; rtc::scoped_refptr track_; diff --git a/pc/rtp_sender_receiver_unittest.cc b/pc/rtp_sender_receiver_unittest.cc index 2c6bc7b71a..d947b8b3e9 100644 --- a/pc/rtp_sender_receiver_unittest.cc +++ b/pc/rtp_sender_receiver_unittest.cc @@ -347,7 +347,7 @@ class RtpSenderReceiverTest void DestroyAudioRtpReceiver() { if (!audio_rtp_receiver_) return; - audio_rtp_receiver_->Stop(); + audio_rtp_receiver_->SetMediaChannel(nullptr); audio_rtp_receiver_ = nullptr; VerifyVoiceChannelNoOutput(); } @@ -356,6 +356,7 @@ class RtpSenderReceiverTest if (!video_rtp_receiver_) return; video_rtp_receiver_->Stop(); + video_rtp_receiver_->SetMediaChannel(nullptr); video_rtp_receiver_ = nullptr; VerifyVideoChannelNoOutput(); } @@ -1640,7 +1641,7 @@ TEST_F(RtpSenderReceiverTest, AudioReceiverCannotSetFrameDecryptorAfterStop) { rtc::scoped_refptr fake_frame_decryptor( new FakeFrameDecryptor()); EXPECT_EQ(nullptr, audio_rtp_receiver_->GetFrameDecryptor()); - audio_rtp_receiver_->Stop(); + audio_rtp_receiver_->SetMediaChannel(nullptr); audio_rtp_receiver_->SetFrameDecryptor(fake_frame_decryptor); // TODO(webrtc:9926) - Validate media channel not set once fakes updated. DestroyAudioRtpReceiver(); @@ -1687,7 +1688,7 @@ TEST_F(RtpSenderReceiverTest, VideoReceiverCannotSetFrameDecryptorAfterStop) { rtc::scoped_refptr fake_frame_decryptor( new FakeFrameDecryptor()); EXPECT_EQ(nullptr, video_rtp_receiver_->GetFrameDecryptor()); - video_rtp_receiver_->Stop(); + video_rtp_receiver_->SetMediaChannel(nullptr); video_rtp_receiver_->SetFrameDecryptor(fake_frame_decryptor); // TODO(webrtc:9926) - Validate media channel not set once fakes updated. DestroyVideoRtpReceiver(); diff --git a/pc/rtp_transceiver.cc b/pc/rtp_transceiver.cc index c8afec899a..013277fa53 100644 --- a/pc/rtp_transceiver.cc +++ b/pc/rtp_transceiver.cc @@ -212,26 +212,34 @@ void RtpTransceiver::SetChannel( } }); - for (const auto& sender : senders_) { - sender->internal()->SetMediaChannel(channel_ ? channel_->media_channel() - : nullptr); - } - RTC_DCHECK_BLOCK_COUNT_NO_MORE_THAN(1); - for (const auto& receiver : receivers_) { - if (!channel_) { - receiver->internal()->Stop(); - } else { - receiver->internal()->SetMediaChannel(channel_->media_channel()); - } + if (!channel_) { + for (const auto& receiver : receivers_) + receiver->internal()->SetSourceEnded(); + RTC_DCHECK_BLOCK_COUNT_NO_MORE_THAN(1); // There should not be an invoke. } - // Destroy the channel, if we had one, now _after_ updating the receivers who - // might have had references to the previous channel. - if (channel_to_delete) { - channel_manager_->DestroyChannel(channel_to_delete); + if (channel_to_delete || !senders_.empty() || !receivers_.empty()) { + channel_manager_->worker_thread()->Invoke(RTC_FROM_HERE, [&]() { + auto* media_channel = channel_ ? channel_->media_channel() : nullptr; + for (const auto& sender : senders_) { + sender->internal()->SetMediaChannel(media_channel); + } + + for (const auto& receiver : receivers_) { + receiver->internal()->SetMediaChannel(media_channel); + } + + // Destroy the channel, if we had one, now _after_ updating the receivers + // who might have had references to the previous channel. + if (channel_to_delete) { + channel_manager_->DestroyChannel(channel_to_delete); + } + }); } + + RTC_DCHECK_BLOCK_COUNT_NO_MORE_THAN(2); } void RtpTransceiver::AddSender( @@ -272,6 +280,7 @@ void RtpTransceiver::AddReceiver( } bool RtpTransceiver::RemoveReceiver(RtpReceiverInterface* receiver) { + RTC_DCHECK_RUN_ON(thread_); RTC_DCHECK(!unified_plan_); if (receiver) { RTC_DCHECK_EQ(media_type(), receiver->media_type()); @@ -280,8 +289,13 @@ bool RtpTransceiver::RemoveReceiver(RtpReceiverInterface* receiver) { if (it == receivers_.end()) { return false; } - // `Stop()` will clear the internally cached pointer to the media channel. + (*it)->internal()->Stop(); + channel_manager_->worker_thread()->Invoke(RTC_FROM_HERE, [&]() { + // `Stop()` will clear the receiver's pointer to the media channel. + (*it)->internal()->SetMediaChannel(nullptr); + }); + receivers_.erase(it); return true; } @@ -399,15 +413,22 @@ void RtpTransceiver::StopSendingAndReceiving() { // // 3. Stop sending media with sender. // + RTC_DCHECK_RUN_ON(thread_); + // 4. Send an RTCP BYE for each RTP stream that was being sent by sender, as // specified in [RFC3550]. - RTC_DCHECK_RUN_ON(thread_); for (const auto& sender : senders_) sender->internal()->Stop(); - // 5. Stop receiving media with receiver. + // Signal to receiver sources that we're stopping. for (const auto& receiver : receivers_) - receiver->internal()->StopAndEndTrack(); + receiver->internal()->Stop(); + + channel_manager_->worker_thread()->Invoke(RTC_FROM_HERE, [&]() { + // 5 Stop receiving media with receiver. + for (const auto& receiver : receivers_) + receiver->internal()->SetMediaChannel(nullptr); + }); stopping_ = true; direction_ = webrtc::RtpTransceiverDirection::kInactive; diff --git a/pc/rtp_transceiver_unittest.cc b/pc/rtp_transceiver_unittest.cc index d63af92169..df6dd29f31 100644 --- a/pc/rtp_transceiver_unittest.cc +++ b/pc/rtp_transceiver_unittest.cc @@ -146,7 +146,8 @@ class RtpTransceiverUnifiedPlanTest : public ::testing::Test { // Basic tests for Stop() TEST_F(RtpTransceiverUnifiedPlanTest, StopSetsDirection) { - EXPECT_CALL(*receiver_.get(), StopAndEndTrack()); + EXPECT_CALL(*receiver_.get(), Stop()); + EXPECT_CALL(*receiver_.get(), SetMediaChannel(_)); EXPECT_CALL(*sender_.get(), SetTransceiverAsStopped()); EXPECT_CALL(*sender_.get(), Stop()); @@ -204,8 +205,7 @@ class RtpTransceiverTestForHeaderExtensions : public ::testing::Test { } void ClearChannel(cricket::MockChannelInterface& mock_channel) { - EXPECT_CALL(*sender_.get(), SetMediaChannel(nullptr)); - EXPECT_CALL(*receiver_.get(), Stop()); + EXPECT_CALL(*sender_.get(), SetMediaChannel(_)); EXPECT_CALL(mock_channel, SetFirstPacketReceivedCallback(_)); EXPECT_CALL(channel_manager_, DestroyChannel(&mock_channel)) .WillRepeatedly(testing::Return()); @@ -221,7 +221,8 @@ class RtpTransceiverTestForHeaderExtensions : public ::testing::Test { }; TEST_F(RtpTransceiverTestForHeaderExtensions, OffersChannelManagerList) { - EXPECT_CALL(*receiver_.get(), StopAndEndTrack()); + EXPECT_CALL(*receiver_.get(), Stop()); + EXPECT_CALL(*receiver_.get(), SetMediaChannel(_)); EXPECT_CALL(*sender_.get(), SetTransceiverAsStopped()); EXPECT_CALL(*sender_.get(), Stop()); @@ -229,7 +230,8 @@ TEST_F(RtpTransceiverTestForHeaderExtensions, OffersChannelManagerList) { } TEST_F(RtpTransceiverTestForHeaderExtensions, ModifiesDirection) { - EXPECT_CALL(*receiver_.get(), StopAndEndTrack()); + EXPECT_CALL(*receiver_.get(), Stop()); + EXPECT_CALL(*receiver_.get(), SetMediaChannel(_)); EXPECT_CALL(*sender_.get(), SetTransceiverAsStopped()); EXPECT_CALL(*sender_.get(), Stop()); @@ -253,7 +255,8 @@ TEST_F(RtpTransceiverTestForHeaderExtensions, ModifiesDirection) { } TEST_F(RtpTransceiverTestForHeaderExtensions, AcceptsStoppedExtension) { - EXPECT_CALL(*receiver_.get(), StopAndEndTrack()); + EXPECT_CALL(*receiver_.get(), Stop()); + EXPECT_CALL(*receiver_.get(), SetMediaChannel(_)); EXPECT_CALL(*sender_.get(), SetTransceiverAsStopped()); EXPECT_CALL(*sender_.get(), Stop()); @@ -265,7 +268,8 @@ TEST_F(RtpTransceiverTestForHeaderExtensions, AcceptsStoppedExtension) { } TEST_F(RtpTransceiverTestForHeaderExtensions, RejectsUnsupportedExtension) { - EXPECT_CALL(*receiver_.get(), StopAndEndTrack()); + EXPECT_CALL(*receiver_.get(), Stop()); + EXPECT_CALL(*receiver_.get(), SetMediaChannel(_)); EXPECT_CALL(*sender_.get(), SetTransceiverAsStopped()); EXPECT_CALL(*sender_.get(), Stop()); @@ -279,7 +283,8 @@ TEST_F(RtpTransceiverTestForHeaderExtensions, RejectsUnsupportedExtension) { TEST_F(RtpTransceiverTestForHeaderExtensions, RejectsStoppedMandatoryExtensions) { - EXPECT_CALL(*receiver_.get(), StopAndEndTrack()); + EXPECT_CALL(*receiver_.get(), Stop()); + EXPECT_CALL(*receiver_.get(), SetMediaChannel(_)); EXPECT_CALL(*sender_.get(), SetTransceiverAsStopped()); EXPECT_CALL(*sender_.get(), Stop()); @@ -299,7 +304,8 @@ TEST_F(RtpTransceiverTestForHeaderExtensions, TEST_F(RtpTransceiverTestForHeaderExtensions, NoNegotiatedHdrExtsWithoutChannel) { - EXPECT_CALL(*receiver_.get(), StopAndEndTrack()); + EXPECT_CALL(*receiver_.get(), Stop()); + EXPECT_CALL(*receiver_.get(), SetMediaChannel(_)); EXPECT_CALL(*sender_.get(), SetTransceiverAsStopped()); EXPECT_CALL(*sender_.get(), Stop()); EXPECT_THAT(transceiver_.HeaderExtensionsNegotiated(), ElementsAre()); @@ -308,8 +314,8 @@ TEST_F(RtpTransceiverTestForHeaderExtensions, TEST_F(RtpTransceiverTestForHeaderExtensions, NoNegotiatedHdrExtsWithChannelWithoutNegotiation) { const std::string content_name("my_mid"); - EXPECT_CALL(*receiver_.get(), SetMediaChannel(_)); - EXPECT_CALL(*receiver_.get(), StopAndEndTrack()); + EXPECT_CALL(*receiver_.get(), SetMediaChannel(_)).WillRepeatedly(Return()); + EXPECT_CALL(*receiver_.get(), Stop()).WillRepeatedly(Return()); EXPECT_CALL(*sender_.get(), SetMediaChannel(_)); EXPECT_CALL(*sender_.get(), SetTransceiverAsStopped()); EXPECT_CALL(*sender_.get(), Stop()); @@ -329,8 +335,8 @@ TEST_F(RtpTransceiverTestForHeaderExtensions, TEST_F(RtpTransceiverTestForHeaderExtensions, ReturnsNegotiatedHdrExts) { const std::string content_name("my_mid"); - EXPECT_CALL(*receiver_.get(), SetMediaChannel(_)); - EXPECT_CALL(*receiver_.get(), StopAndEndTrack()); + EXPECT_CALL(*receiver_.get(), SetMediaChannel(_)).WillRepeatedly(Return()); + EXPECT_CALL(*receiver_.get(), Stop()).WillRepeatedly(Return()); EXPECT_CALL(*sender_.get(), SetMediaChannel(_)); EXPECT_CALL(*sender_.get(), SetTransceiverAsStopped()); EXPECT_CALL(*sender_.get(), Stop()); @@ -362,7 +368,8 @@ TEST_F(RtpTransceiverTestForHeaderExtensions, ReturnsNegotiatedHdrExts) { TEST_F(RtpTransceiverTestForHeaderExtensions, ReturnsNegotiatedHdrExtsSecondTime) { - EXPECT_CALL(*receiver_.get(), StopAndEndTrack()); + EXPECT_CALL(*receiver_.get(), Stop()); + EXPECT_CALL(*receiver_.get(), SetMediaChannel(_)); EXPECT_CALL(*sender_.get(), SetTransceiverAsStopped()); EXPECT_CALL(*sender_.get(), Stop()); diff --git a/pc/rtp_transmission_manager.cc b/pc/rtp_transmission_manager.cc index 951d1c38e5..130dc311a4 100644 --- a/pc/rtp_transmission_manager.cc +++ b/pc/rtp_transmission_manager.cc @@ -459,13 +459,14 @@ void RtpTransmissionManager::CreateAudioReceiver( // TODO(https://crbug.com/webrtc/9480): When we remove remote_streams(), use // the constructor taking stream IDs instead. auto audio_receiver = rtc::make_ref_counted( - worker_thread(), remote_sender_info.sender_id, streams, IsUnifiedPlan()); - audio_receiver->SetMediaChannel(voice_media_channel()); + worker_thread(), remote_sender_info.sender_id, streams, IsUnifiedPlan(), + voice_media_channel()); if (remote_sender_info.sender_id == kDefaultAudioSenderId) { audio_receiver->SetupUnsignaledMediaChannel(); } else { audio_receiver->SetupMediaChannel(remote_sender_info.first_ssrc); } + auto receiver = RtpReceiverProxyWithInternal::Create( signaling_thread(), worker_thread(), std::move(audio_receiver)); GetAudioTransceiver()->internal()->AddReceiver(receiver); @@ -483,12 +484,13 @@ void RtpTransmissionManager::CreateVideoReceiver( // the constructor taking stream IDs instead. auto video_receiver = rtc::make_ref_counted( worker_thread(), remote_sender_info.sender_id, streams); - video_receiver->SetMediaChannel(video_media_channel()); - if (remote_sender_info.sender_id == kDefaultVideoSenderId) { - video_receiver->SetupUnsignaledMediaChannel(); - } else { - video_receiver->SetupMediaChannel(remote_sender_info.first_ssrc); - } + + video_receiver->SetupMediaChannel( + remote_sender_info.sender_id == kDefaultVideoSenderId + ? absl::nullopt + : absl::optional(remote_sender_info.first_ssrc), + video_media_channel()); + auto receiver = RtpReceiverProxyWithInternal::Create( signaling_thread(), worker_thread(), std::move(video_receiver)); GetVideoTransceiver()->internal()->AddReceiver(receiver); diff --git a/pc/stats_collector_unittest.cc b/pc/stats_collector_unittest.cc index c9740460db..7688ffe727 100644 --- a/pc/stats_collector_unittest.cc +++ b/pc/stats_collector_unittest.cc @@ -764,9 +764,8 @@ static rtc::scoped_refptr CreateMockReceiver( Return(track->kind() == MediaStreamTrackInterface::kAudioKind ? cricket::MEDIA_TYPE_AUDIO : cricket::MEDIA_TYPE_VIDEO)); - EXPECT_CALL(*receiver, SetMediaChannel(_)).Times(AtMost(1)); - EXPECT_CALL(*receiver, Stop()); - EXPECT_CALL(*receiver, StopAndEndTrack()); + EXPECT_CALL(*receiver, SetMediaChannel(_)).WillRepeatedly(Return()); + EXPECT_CALL(*receiver, Stop()).WillRepeatedly(Return()); return receiver; } diff --git a/pc/test/mock_rtp_receiver_internal.h b/pc/test/mock_rtp_receiver_internal.h index ba244039af..c222a04e2f 100644 --- a/pc/test/mock_rtp_receiver_internal.h +++ b/pc/test/mock_rtp_receiver_internal.h @@ -57,7 +57,7 @@ class MockRtpReceiverInternal : public RtpReceiverInternal { // RtpReceiverInternal methods. MOCK_METHOD(void, Stop, (), (override)); - MOCK_METHOD(void, StopAndEndTrack, (), (override)); + MOCK_METHOD(void, SetSourceEnded, (), (override)); MOCK_METHOD(void, SetMediaChannel, (cricket::MediaChannel*), (override)); MOCK_METHOD(void, SetupMediaChannel, (uint32_t), (override)); MOCK_METHOD(void, SetupUnsignaledMediaChannel, (), (override)); diff --git a/pc/video_rtp_receiver.cc b/pc/video_rtp_receiver.cc index d1f8fb0d96..a428603745 100644 --- a/pc/video_rtp_receiver.cc +++ b/pc/video_rtp_receiver.cc @@ -109,76 +109,68 @@ void VideoRtpReceiver::SetDepacketizerToDecoderFrameTransformer( void VideoRtpReceiver::Stop() { RTC_DCHECK_RUN_ON(&signaling_thread_checker_); - // TODO(deadbeef): Need to do more here to fully stop receiving packets. - source_->SetState(MediaSourceInterface::kEnded); - - worker_thread_->Invoke(RTC_FROM_HERE, [&] { - RTC_DCHECK_RUN_ON(worker_thread_); - if (media_channel_) { - SetSink(nullptr); - SetMediaChannel_w(nullptr); - } - source_->ClearCallback(); - }); -} - -void VideoRtpReceiver::StopAndEndTrack() { - RTC_DCHECK_RUN_ON(&signaling_thread_checker_); - Stop(); track_->internal()->set_ended(); } -void VideoRtpReceiver::RestartMediaChannel(absl::optional ssrc) { +void VideoRtpReceiver::SetSourceEnded() { RTC_DCHECK_RUN_ON(&signaling_thread_checker_); + source_->SetState(MediaSourceInterface::kEnded); +} +// RTC_RUN_ON(&signaling_thread_checker_) +void VideoRtpReceiver::RestartMediaChannel(absl::optional ssrc) { MediaSourceInterface::SourceState state = source_->state(); - // TODO(tommi): Can we restart the media channel without blocking? worker_thread_->Invoke(RTC_FROM_HERE, [&] { RTC_DCHECK_RUN_ON(worker_thread_); - if (!media_channel_) { - // Ignore further negotiations if we've already been stopped and don't - // have an associated media channel. - return; // Can't restart. - } - - const bool encoded_sink_enabled = saved_encoded_sink_enabled_; - - if (state != MediaSourceInterface::kInitializing) { - if (ssrc == ssrc_) - return; - - // Disconnect from a previous ssrc. - SetSink(nullptr); - - if (encoded_sink_enabled) - SetEncodedSinkEnabled(false); - } - - // Set up the new ssrc. - ssrc_ = std::move(ssrc); - SetSink(source_->sink()); - if (encoded_sink_enabled) { - SetEncodedSinkEnabled(true); - } - - if (frame_transformer_ && media_channel_) { - media_channel_->SetDepacketizerToDecoderFrameTransformer( - ssrc_.value_or(0), frame_transformer_); - } - - if (media_channel_ && ssrc_) { - if (frame_decryptor_) { - media_channel_->SetFrameDecryptor(*ssrc_, frame_decryptor_); - } - - media_channel_->SetBaseMinimumPlayoutDelayMs(*ssrc_, delay_.GetMs()); - } + RestartMediaChannel_w(std::move(ssrc), state); }); source_->SetState(MediaSourceInterface::kLive); } +// RTC_RUN_ON(worker_thread_) +void VideoRtpReceiver::RestartMediaChannel_w( + absl::optional ssrc, + MediaSourceInterface::SourceState state) { + if (!media_channel_) { + return; // Can't restart. + } + + const bool encoded_sink_enabled = saved_encoded_sink_enabled_; + + if (state != MediaSourceInterface::kInitializing) { + if (ssrc == ssrc_) + return; + + // Disconnect from a previous ssrc. + SetSink(nullptr); + + if (encoded_sink_enabled) + SetEncodedSinkEnabled(false); + } + + // Set up the new ssrc. + ssrc_ = std::move(ssrc); + SetSink(source_->sink()); + if (encoded_sink_enabled) { + SetEncodedSinkEnabled(true); + } + + if (frame_transformer_ && media_channel_) { + media_channel_->SetDepacketizerToDecoderFrameTransformer( + ssrc_.value_or(0), frame_transformer_); + } + + if (media_channel_ && ssrc_) { + if (frame_decryptor_) { + media_channel_->SetFrameDecryptor(*ssrc_, frame_decryptor_); + } + + media_channel_->SetBaseMinimumPlayoutDelayMs(*ssrc_, delay_.GetMs()); + } +} + // RTC_RUN_ON(worker_thread_) void VideoRtpReceiver::SetSink(rtc::VideoSinkInterface* sink) { if (ssrc_) { @@ -266,14 +258,11 @@ void VideoRtpReceiver::SetJitterBufferMinimumDelay( } void VideoRtpReceiver::SetMediaChannel(cricket::MediaChannel* media_channel) { - RTC_DCHECK_RUN_ON(&signaling_thread_checker_); + RTC_DCHECK_RUN_ON(worker_thread_); RTC_DCHECK(media_channel == nullptr || media_channel->media_type() == media_type()); - worker_thread_->Invoke(RTC_FROM_HERE, [&] { - RTC_DCHECK_RUN_ON(worker_thread_); - SetMediaChannel_w(media_channel); - }); + SetMediaChannel_w(media_channel); } // RTC_RUN_ON(worker_thread_) @@ -281,6 +270,10 @@ void VideoRtpReceiver::SetMediaChannel_w(cricket::MediaChannel* media_channel) { if (media_channel == media_channel_) return; + if (!media_channel) { + SetSink(nullptr); + } + bool encoded_sink_enabled = saved_encoded_sink_enabled_; if (encoded_sink_enabled && media_channel_) { // Turn off the old sink, if any. @@ -303,6 +296,9 @@ void VideoRtpReceiver::SetMediaChannel_w(cricket::MediaChannel* media_channel) { ssrc_.value_or(0), frame_transformer_); } } + + if (!media_channel) + source_->ClearCallback(); } void VideoRtpReceiver::NotifyFirstPacketReceived() { @@ -320,6 +316,19 @@ std::vector VideoRtpReceiver::GetSources() const { return media_channel_->GetSources(*ssrc_); } +void VideoRtpReceiver::SetupMediaChannel(absl::optional ssrc, + cricket::MediaChannel* media_channel) { + RTC_DCHECK_RUN_ON(&signaling_thread_checker_); + RTC_DCHECK(media_channel); + MediaSourceInterface::SourceState state = source_->state(); + worker_thread_->Invoke(RTC_FROM_HERE, [&] { + RTC_DCHECK_RUN_ON(worker_thread_); + SetMediaChannel_w(media_channel); + RestartMediaChannel_w(std::move(ssrc), state); + }); + source_->SetState(MediaSourceInterface::kLive); +} + void VideoRtpReceiver::OnGenerateKeyFrame() { RTC_DCHECK_RUN_ON(worker_thread_); if (!media_channel_) { diff --git a/pc/video_rtp_receiver.h b/pc/video_rtp_receiver.h index 681f423e29..4261e417d2 100644 --- a/pc/video_rtp_receiver.h +++ b/pc/video_rtp_receiver.h @@ -88,7 +88,7 @@ class VideoRtpReceiver : public RtpReceiverInternal { // RtpReceiverInternal implementation. void Stop() override; - void StopAndEndTrack() override; + void SetSourceEnded() override; void SetupMediaChannel(uint32_t ssrc) override; void SetupUnsignaledMediaChannel() override; uint32_t ssrc() const override; @@ -110,8 +110,17 @@ class VideoRtpReceiver : public RtpReceiverInternal { std::vector GetSources() const override; + // Combines SetMediaChannel, SetupMediaChannel and + // SetupUnsignaledMediaChannel. + void SetupMediaChannel(absl::optional ssrc, + cricket::MediaChannel* media_channel); + private: - void RestartMediaChannel(absl::optional ssrc); + void RestartMediaChannel(absl::optional ssrc) + RTC_RUN_ON(&signaling_thread_checker_); + void RestartMediaChannel_w(absl::optional ssrc, + MediaSourceInterface::SourceState state) + RTC_RUN_ON(worker_thread_); void SetSink(rtc::VideoSinkInterface* sink) RTC_RUN_ON(worker_thread_); void SetMediaChannel_w(cricket::MediaChannel* media_channel) diff --git a/pc/video_rtp_receiver_unittest.cc b/pc/video_rtp_receiver_unittest.cc index 42ff2611ef..56aa3688a8 100644 --- a/pc/video_rtp_receiver_unittest.cc +++ b/pc/video_rtp_receiver_unittest.cc @@ -60,13 +60,20 @@ class VideoRtpReceiverTest : public testing::Test { std::string("receiver"), std::vector({"stream"}))) { worker_thread_->Start(); - receiver_->SetMediaChannel(&channel_); + SetMediaChannel(&channel_); } ~VideoRtpReceiverTest() override { - // Clear expectations that tests may have set up before calling Stop(). + // Clear expectations that tests may have set up before calling + // SetMediaChannel(nullptr). Mock::VerifyAndClearExpectations(&channel_); receiver_->Stop(); + SetMediaChannel(nullptr); + } + + void SetMediaChannel(cricket::MediaChannel* media_channel) { + worker_thread_->Invoke( + RTC_FROM_HERE, [&]() { receiver_->SetMediaChannel(media_channel); }); } webrtc::VideoTrackSourceInterface* Source() { @@ -94,23 +101,24 @@ TEST_F(VideoRtpReceiverTest, MockVideoMediaChannel channel2(nullptr, cricket::VideoOptions()); EXPECT_CALL(channel_, GenerateKeyFrame).Times(0); EXPECT_CALL(channel2, GenerateKeyFrame).Times(0); - receiver_->SetMediaChannel(&channel2); + SetMediaChannel(&channel2); Mock::VerifyAndClearExpectations(&channel2); // Generate a key frame. When we switch channel next time, we will have to // re-generate it as we don't know if it was eventually received + EXPECT_CALL(channel2, GenerateKeyFrame).Times(1); Source()->GenerateKeyFrame(); MockVideoMediaChannel channel3(nullptr, cricket::VideoOptions()); EXPECT_CALL(channel3, GenerateKeyFrame); - receiver_->SetMediaChannel(&channel3); + SetMediaChannel(&channel3); // Switching to a new channel should now not cause calls to GenerateKeyFrame. StrictMock channel4(nullptr, cricket::VideoOptions()); - receiver_->SetMediaChannel(&channel4); + SetMediaChannel(&channel4); - // We must call Stop() here since the mock media channels live on the stack - // and `receiver_` still has a pointer to those objects. - receiver_->Stop(); + // We must call SetMediaChannel(nullptr) here since the mock media channels + // live on the stack and `receiver_` still has a pointer to those objects. + SetMediaChannel(nullptr); } TEST_F(VideoRtpReceiverTest, EnablesEncodedOutput) { @@ -135,7 +143,7 @@ TEST_F(VideoRtpReceiverTest, DisablesEnablesEncodedOutputOnChannelSwitch) { Source()->AddEncodedSink(&sink); MockVideoMediaChannel channel2(nullptr, cricket::VideoOptions()); EXPECT_CALL(channel2, SetRecordableEncodedFrameCallback); - receiver_->SetMediaChannel(&channel2); + SetMediaChannel(&channel2); Mock::VerifyAndClearExpectations(&channel2); // When clearing encoded frame buffer function, we need channel switches @@ -143,11 +151,11 @@ TEST_F(VideoRtpReceiverTest, DisablesEnablesEncodedOutputOnChannelSwitch) { EXPECT_CALL(channel2, ClearRecordableEncodedFrameCallback); Source()->RemoveEncodedSink(&sink); StrictMock channel3(nullptr, cricket::VideoOptions()); - receiver_->SetMediaChannel(&channel3); + SetMediaChannel(&channel3); - // We must call Stop() here since the mock media channels live on the stack - // and `receiver_` still has a pointer to those objects. - receiver_->Stop(); + // We must call SetMediaChannel(nullptr) here since the mock media channels + // live on the stack and `receiver_` still has a pointer to those objects. + SetMediaChannel(nullptr); } TEST_F(VideoRtpReceiverTest, BroadcastsEncodedFramesWhenEnabled) {