From 2da85916abdb0c21b0608ca0e51a6250a69cf8a5 Mon Sep 17 00:00:00 2001 From: Mirko Bonadei Date: Wed, 9 Feb 2022 10:54:24 +0000 Subject: [PATCH] Revert "Reland "Remove `stopped_` from AudioRtpReceiver and VideoRtpReceiver."" This reverts commit 3ed36c0521546881656c73984456485dcab16205. Reason for revert: Breaks downstream project. Original change's description: > Reland "Remove `stopped_` from AudioRtpReceiver and VideoRtpReceiver." > > This is a reland of bb57e2d7aa9b36843233d1394422f03d12d9c31f > > The difference from the original CL is that a check for > `state_ == kLive` inside of RemoteAudioSource::AddSink has been removed. > This caused a side effect that registering the sink while the source > was in an "initializing" state, failed. The last remaining state > however, is `kEnded` - but since there's no logic in the class around > the expected value of the states, the check inside of AddSink() > doesn't provide an additional value - it's rather a surprise for > developers if it doesn't succeed. So, now removed. > > Original change's description: > > Remove `stopped_` from AudioRtpReceiver and VideoRtpReceiver. > > > > This simplifies the logic in these classes a bit, which makes upcoming > > change easier. The `stopped_` flag in these classes was essentially > > the same thing as `media_channel_ == nullptr`, which is what's > > consistently used now for the same checks. > > > > Bug: webrtc:13540 > > Change-Id: Ib60bfad9f28d5ddee8a8d5170c3f2a7ef017a5ca > > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/250163 > > Reviewed-by: Harald Alvestrand > > Commit-Queue: Tomas Gunnarsson > > Cr-Commit-Position: refs/heads/main@{#35907} > > Bug: webrtc:13540 > Change-Id: I3e5b3046fae11cb56b50c38c5f08972a6f283dd5 > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/251326 > Auto-Submit: Tomas Gunnarsson > Commit-Queue: Tomas Gunnarsson > Reviewed-by: Harald Alvestrand > Commit-Queue: Harald Alvestrand > Cr-Commit-Position: refs/heads/main@{#35958} TBR=ilnik@webrtc.org,tommi@webrtc.org,hta@webrtc.org,webrtc-scoped@luci-project-accounts.iam.gserviceaccount.com Change-Id: Ieb7235d88c808c78ad0847403be991d4dce1ace6 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: webrtc:13540 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/251383 Owners-Override: Mirko Bonadei Bot-Commit: rubber-stamper@appspot.gserviceaccount.com Commit-Queue: Mirko Bonadei Cr-Commit-Position: refs/heads/main@{#35963} --- pc/BUILD.gn | 1 - pc/audio_rtp_receiver.cc | 48 ++++++++++++++++++++----------- pc/audio_rtp_receiver.h | 1 + pc/audio_rtp_receiver_unittest.cc | 1 - pc/remote_audio_source.cc | 7 ++++- pc/video_rtp_receiver.cc | 47 ++++++++++++++++++++---------- pc/video_rtp_receiver.h | 11 +++++++ pc/video_rtp_receiver_unittest.cc | 1 + pc/video_track_source.cc | 3 +- pc/video_track_source.h | 11 ++----- 10 files changed, 87 insertions(+), 44 deletions(-) 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_; };