diff --git a/pc/BUILD.gn b/pc/BUILD.gn index ba0586755e..7fcb046904 100644 --- a/pc/BUILD.gn +++ b/pc/BUILD.gn @@ -1416,7 +1416,6 @@ rtc_library("video_track_source") { "../media:rtc_media_base", "../rtc_base:checks", "../rtc_base:rtc_base_approved", - "../rtc_base/system:no_unique_address", "../rtc_base/system:rtc_export", ] absl_deps = [ "//third_party/abseil-cpp/absl/types:optional" ] diff --git a/pc/audio_rtp_receiver.cc b/pc/audio_rtp_receiver.cc index a49b7ce48e..7890d9b1e0 100644 --- a/pc/audio_rtp_receiver.cc +++ b/pc/audio_rtp_receiver.cc @@ -61,6 +61,7 @@ AudioRtpReceiver::AudioRtpReceiver( AudioRtpReceiver::~AudioRtpReceiver() { RTC_DCHECK_RUN_ON(&signaling_thread_checker_); + RTC_DCHECK(stopped_); RTC_DCHECK(!media_channel_); track_->GetSource()->UnregisterAudioObserver(this); @@ -84,10 +85,6 @@ void AudioRtpReceiver::OnChanged() { void AudioRtpReceiver::SetOutputVolume_w(double volume) { RTC_DCHECK_GE(volume, 0.0); RTC_DCHECK_LE(volume, 10.0); - - if (!media_channel_) - return; - ssrc_ ? media_channel_->SetOutputVolume(*ssrc_, volume) : media_channel_->SetDefaultOutputVolume(volume); } @@ -97,10 +94,13 @@ 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 + // 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; + if (stopped_) + return; + // 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. @@ -160,7 +160,10 @@ 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); + if (!stopped_) { + source_->SetState(MediaSourceInterface::kEnded); + stopped_ = true; + } worker_thread_->Invoke(RTC_FROM_HERE, [&]() { RTC_DCHECK_RUN_ON(worker_thread_); @@ -180,17 +183,22 @@ void AudioRtpReceiver::StopAndEndTrack() { void AudioRtpReceiver::RestartMediaChannel(absl::optional ssrc) { RTC_DCHECK_RUN_ON(&signaling_thread_checker_); - MediaSourceInterface::SourceState state = source_->state(); - worker_thread_->Invoke( - RTC_FROM_HERE, - [&, enabled = cached_track_enabled_, volume = cached_volume_]() { + bool ok = worker_thread_->Invoke( + RTC_FROM_HERE, [&, enabled = cached_track_enabled_, + volume = cached_volume_, was_stopped = stopped_]() { RTC_DCHECK_RUN_ON(worker_thread_); - if (!media_channel_) - return; // Can't restart. + if (!media_channel_) { + RTC_DCHECK(was_stopped); + return false; // Can't restart. + } - if (state != MediaSourceInterface::kInitializing) { - if (ssrc_ == ssrc) - return; + if (!was_stopped && ssrc_ == ssrc) { + // Already running with that ssrc. + RTC_DCHECK(worker_thread_safety_->alive()); + return true; + } + + if (!was_stopped) { source_->Stop(media_channel_, ssrc_); } @@ -201,8 +209,13 @@ void AudioRtpReceiver::RestartMediaChannel(absl::optional ssrc) { } Reconfigure(enabled, volume); + return true; }); - source_->SetState(MediaSourceInterface::kLive); + + if (!ok) + return; + + stopped_ = false; } void AudioRtpReceiver::SetupMediaChannel(uint32_t ssrc) { @@ -322,6 +335,9 @@ void AudioRtpReceiver::SetMediaChannel(cricket::MediaChannel* media_channel) { RTC_DCHECK(media_channel == nullptr || media_channel->media_type() == media_type()); + if (stopped_ && !media_channel) + return; + worker_thread_->Invoke(RTC_FROM_HERE, [&] { RTC_DCHECK_RUN_ON(worker_thread_); SetMediaChannel_w(media_channel); diff --git a/pc/audio_rtp_receiver.h b/pc/audio_rtp_receiver.h index 978c550dfe..aef497db76 100644 --- a/pc/audio_rtp_receiver.h +++ b/pc/audio_rtp_receiver.h @@ -133,6 +133,7 @@ class AudioRtpReceiver : public ObserverInterface, 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; + bool stopped_ RTC_GUARDED_BY(&signaling_thread_checker_) = true; 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..1865115b2e 100644 --- a/pc/audio_rtp_receiver_unittest.cc +++ b/pc/audio_rtp_receiver_unittest.cc @@ -63,7 +63,6 @@ TEST_F(AudioRtpReceiverTest, SetOutputVolumeIsCalled) { receiver_->track(); receiver_->track()->set_enabled(true); receiver_->SetMediaChannel(&media_channel_); - EXPECT_CALL(media_channel_, SetDefaultRawAudioSink(_)).Times(0); receiver_->SetupMediaChannel(kSsrc); EXPECT_CALL(media_channel_, SetOutputVolume(kSsrc, kVolume)) diff --git a/pc/remote_audio_source.cc b/pc/remote_audio_source.cc index 78a35f32a8..dc890e737c 100644 --- a/pc/remote_audio_source.cc +++ b/pc/remote_audio_source.cc @@ -55,7 +55,7 @@ RemoteAudioSource::RemoteAudioSource( : main_thread_(rtc::Thread::Current()), worker_thread_(worker_thread), on_audio_channel_gone_action_(on_audio_channel_gone_action), - state_(MediaSourceInterface::kInitializing) { + state_(MediaSourceInterface::kLive) { RTC_DCHECK(main_thread_); RTC_DCHECK(worker_thread_); } @@ -134,6 +134,11 @@ void RemoteAudioSource::AddSink(AudioTrackSinkInterface* sink) { RTC_DCHECK_RUN_ON(main_thread_); RTC_DCHECK(sink); + if (state_ != MediaSourceInterface::kLive) { + RTC_LOG(LS_ERROR) << "Can't register sink as the source isn't live."; + return; + } + MutexLock lock(&sink_lock_); RTC_DCHECK(!absl::c_linear_search(sinks_, sink)); sinks_.push_back(sink); diff --git a/pc/video_rtp_receiver.cc b/pc/video_rtp_receiver.cc index c5ead9e773..8db4d9f02f 100644 --- a/pc/video_rtp_receiver.cc +++ b/pc/video_rtp_receiver.cc @@ -49,11 +49,12 @@ VideoRtpReceiver::VideoRtpReceiver( attachment_id_(GenerateUniqueId()) { RTC_DCHECK(worker_thread_); SetStreams(streams); - RTC_DCHECK_EQ(source_->state(), MediaSourceInterface::kInitializing); + RTC_DCHECK_EQ(source_->state(), MediaSourceInterface::kLive); } VideoRtpReceiver::~VideoRtpReceiver() { RTC_DCHECK_RUN_ON(&signaling_thread_checker_); + RTC_DCHECK(stopped_); RTC_DCHECK(!media_channel_); } @@ -115,7 +116,10 @@ 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); + if (!stopped_) { + source_->SetState(MediaSourceInterface::kEnded); + stopped_ = true; + } worker_thread_->Invoke(RTC_FROM_HERE, [&] { RTC_DCHECK_RUN_ON(worker_thread_); @@ -136,30 +140,34 @@ void VideoRtpReceiver::StopAndEndTrack() { void VideoRtpReceiver::RestartMediaChannel(absl::optional ssrc) { RTC_DCHECK_RUN_ON(&signaling_thread_checker_); - MediaSourceInterface::SourceState state = source_->state(); + // `stopped_` will be `true` on construction. RestartMediaChannel + // can in this case function like "ensure started" and flip `stopped_` + // to false. // TODO(tommi): Can we restart the media channel without blocking? - worker_thread_->Invoke(RTC_FROM_HERE, [&] { + bool ok = worker_thread_->Invoke(RTC_FROM_HERE, [&, was_stopped = + stopped_] { 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. + RTC_DCHECK(was_stopped); + return false; // Can't restart. } - const bool encoded_sink_enabled = saved_encoded_sink_enabled_; + if (!was_stopped && ssrc_ == ssrc) { + // Already running with that ssrc. + return true; + } - if (state != MediaSourceInterface::kInitializing) { - if (ssrc == ssrc_) - return; - - // Disconnect from a previous ssrc. + // Disconnect from the previous ssrc. + if (!was_stopped) { SetSink(nullptr); - - if (encoded_sink_enabled) - SetEncodedSinkEnabled(false); } + bool encoded_sink_enabled = saved_encoded_sink_enabled_; + SetEncodedSinkEnabled(false); + // Set up the new ssrc. ssrc_ = std::move(ssrc); SetSink(source_->sink()); @@ -179,8 +187,14 @@ void VideoRtpReceiver::RestartMediaChannel(absl::optional ssrc) { media_channel_->SetBaseMinimumPlayoutDelayMs(*ssrc_, delay_.GetMs()); } + + return true; }); - source_->SetState(MediaSourceInterface::kLive); + + if (!ok) + return; + + stopped_ = false; } // RTC_RUN_ON(worker_thread_) @@ -274,6 +288,9 @@ void VideoRtpReceiver::SetMediaChannel(cricket::MediaChannel* media_channel) { RTC_DCHECK(media_channel == nullptr || media_channel->media_type() == media_type()); + if (stopped_ && !media_channel) + return; + worker_thread_->Invoke(RTC_FROM_HERE, [&] { RTC_DCHECK_RUN_ON(worker_thread_); SetMediaChannel_w(media_channel); diff --git a/pc/video_rtp_receiver.h b/pc/video_rtp_receiver.h index 681f423e29..b5381860b3 100644 --- a/pc/video_rtp_receiver.h +++ b/pc/video_rtp_receiver.h @@ -141,6 +141,8 @@ class VideoRtpReceiver : public RtpReceiverInternal { rtc::Thread* const worker_thread_; const std::string id_; + // See documentation for `stopped_` below for when a valid media channel + // has been assigned and when this pointer will be null. cricket::VideoMediaChannel* media_channel_ RTC_GUARDED_BY(worker_thread_) = nullptr; absl::optional ssrc_ RTC_GUARDED_BY(worker_thread_); @@ -150,6 +152,15 @@ class VideoRtpReceiver : public RtpReceiverInternal { const rtc::scoped_refptr> track_; std::vector> streams_ RTC_GUARDED_BY(&signaling_thread_checker_); + // `stopped` is state that's used on the signaling thread to indicate whether + // a valid `media_channel_` has been assigned and configured. When an instance + // of VideoRtpReceiver is initially created, `stopped_` is true and will + // remain true until either `SetupMediaChannel` or + // `SetupUnsignaledMediaChannel` is called after assigning a media channel. + // After that, `stopped_` will remain false until `Stop()` is called. + // Note, for checking the state of the class on the worker thread, + // check `media_channel_` instead, as that's the main worker thread state. + bool stopped_ RTC_GUARDED_BY(&signaling_thread_checker_) = true; RtpReceiverObserverInterface* observer_ RTC_GUARDED_BY(&signaling_thread_checker_) = nullptr; bool received_first_packet_ RTC_GUARDED_BY(&signaling_thread_checker_) = diff --git a/pc/video_rtp_receiver_unittest.cc b/pc/video_rtp_receiver_unittest.cc index 42ff2611ef..3a8099d30f 100644 --- a/pc/video_rtp_receiver_unittest.cc +++ b/pc/video_rtp_receiver_unittest.cc @@ -169,6 +169,7 @@ TEST_F(VideoRtpReceiverTest, BroadcastsEncodedFramesWhenEnabled) { TEST_F(VideoRtpReceiverTest, EnablesEncodedOutputOnChannelRestart) { InSequence s; + EXPECT_CALL(channel_, ClearRecordableEncodedFrameCallback(0)); MockVideoSink sink; Source()->AddEncodedSink(&sink); EXPECT_CALL(channel_, SetRecordableEncodedFrameCallback(4711, _)); diff --git a/pc/video_track_source.cc b/pc/video_track_source.cc index 64e99cc064..d15eaaf43c 100644 --- a/pc/video_track_source.cc +++ b/pc/video_track_source.cc @@ -15,12 +15,11 @@ namespace webrtc { VideoTrackSource::VideoTrackSource(bool remote) - : state_(kInitializing), remote_(remote) { + : state_(kLive), remote_(remote) { worker_thread_checker_.Detach(); } void VideoTrackSource::SetState(SourceState new_state) { - RTC_DCHECK_RUN_ON(&signaling_thread_checker_); if (state_ != new_state) { state_ = new_state; FireOnChanged(); diff --git a/pc/video_track_source.h b/pc/video_track_source.h index 3f568f642b..4a29381c4c 100644 --- a/pc/video_track_source.h +++ b/pc/video_track_source.h @@ -20,7 +20,6 @@ #include "api/video/video_sink_interface.h" #include "api/video/video_source_interface.h" #include "media/base/media_channel.h" -#include "rtc_base/system/no_unique_address.h" #include "rtc_base/system/rtc_export.h" namespace webrtc { @@ -32,10 +31,7 @@ class RTC_EXPORT VideoTrackSource : public Notifier { explicit VideoTrackSource(bool remote); void SetState(SourceState new_state); - SourceState state() const override { - RTC_DCHECK_RUN_ON(&signaling_thread_checker_); - return state_; - } + SourceState state() const override { return state_; } bool remote() const override { return remote_; } bool is_screencast() const override { return false; } @@ -60,9 +56,8 @@ class RTC_EXPORT VideoTrackSource : public Notifier { virtual rtc::VideoSourceInterface* source() = 0; private: - RTC_NO_UNIQUE_ADDRESS SequenceChecker worker_thread_checker_; - RTC_NO_UNIQUE_ADDRESS SequenceChecker signaling_thread_checker_; - SourceState state_ RTC_GUARDED_BY(&signaling_thread_checker_); + SequenceChecker worker_thread_checker_; + SourceState state_; const bool remote_; };