Revert "Reland "Remove stopped_ from AudioRtpReceiver and VideoRtpReceiver.""

This reverts commit 3ed36c0521.

Reason for revert: Breaks downstream project.

Original change's description:
> Reland "Remove `stopped_` from AudioRtpReceiver and VideoRtpReceiver."
>
> This is a reland of bb57e2d7aa
>
> 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 <hta@webrtc.org>
> > Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
> > 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 <tommi@webrtc.org>
> Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
> Reviewed-by: Harald Alvestrand <hta@webrtc.org>
> Commit-Queue: Harald Alvestrand <hta@webrtc.org>
> 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 <mbonadei@webrtc.org>
Bot-Commit: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Mirko Bonadei <mbonadei@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35963}

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" ]

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.
+  if (!stopped_) {
     source_->SetState(MediaSourceInterface::kEnded);
+    stopped_ = true;
+  }
 
   worker_thread_->Invoke<void>(RTC_FROM_HERE, [&]() {
     RTC_DCHECK_RUN_ON(worker_thread_);
@@ -180,17 +183,22 @@ void AudioRtpReceiver::StopAndEndTrack() {
 
 void AudioRtpReceiver::RestartMediaChannel(absl::optional<uint32_t> ssrc) {
   RTC_DCHECK_RUN_ON(&signaling_thread_checker_);
-  MediaSourceInterface::SourceState state = source_->state();
+  bool ok = worker_thread_->Invoke<bool>(
-  worker_thread_->Invoke<void>(
+      RTC_FROM_HERE, [&, enabled = cached_track_enabled_,
-      RTC_FROM_HERE,
+                      volume = cached_volume_, was_stopped = stopped_]() {
-      [&, enabled = cached_track_enabled_, volume = cached_volume_]() {
         RTC_DCHECK_RUN_ON(worker_thread_);
-        if (!media_channel_)
+        if (!media_channel_) {
-          return;  // Can't restart.
+          RTC_DCHECK(was_stopped);
+          return false;  // Can't restart.
+        }
 
-        if (state != MediaSourceInterface::kInitializing) {
+        if (!was_stopped && ssrc_ == ssrc) {
-          if (ssrc_ == ssrc)
+          // Already running with that ssrc.
-            return;
+          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<uint32_t> 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<void>(RTC_FROM_HERE, [&] {
     RTC_DCHECK_RUN_ON(worker_thread_);
     SetMediaChannel_w(media_channel);

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_) =

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

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

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.
 
+  if (!stopped_) {
     source_->SetState(MediaSourceInterface::kEnded);
+    stopped_ = true;
+  }
 
   worker_thread_->Invoke<void>(RTC_FROM_HERE, [&] {
     RTC_DCHECK_RUN_ON(worker_thread_);
@@ -136,30 +140,34 @@ void VideoRtpReceiver::StopAndEndTrack() {
 void VideoRtpReceiver::RestartMediaChannel(absl::optional<uint32_t> 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<void>(RTC_FROM_HERE, [&] {
+  bool ok = worker_thread_->Invoke<bool>(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) {
+    // Disconnect from the previous ssrc.
-      if (ssrc == ssrc_)
+    if (!was_stopped) {
-        return;
-
-      // Disconnect from a previous ssrc.
       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<uint32_t> 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<void>(RTC_FROM_HERE, [&] {
     RTC_DCHECK_RUN_ON(worker_thread_);
     SetMediaChannel_w(media_channel);

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<uint32_t> ssrc_ RTC_GUARDED_BY(worker_thread_);
@@ -150,6 +152,15 @@ class VideoRtpReceiver : public RtpReceiverInternal {
   const rtc::scoped_refptr<VideoTrackProxyWithInternal<VideoTrack>> track_;
   std::vector<rtc::scoped_refptr<MediaStreamInterface>> 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_) =

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, _));

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

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<VideoTrackSourceInterface> {
   explicit VideoTrackSource(bool remote);
   void SetState(SourceState new_state);
 
-  SourceState state() const override {
+  SourceState state() const override { return state_; }
-    RTC_DCHECK_RUN_ON(&signaling_thread_checker_);
-    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<VideoTrackSourceInterface> {
   virtual rtc::VideoSourceInterface<VideoFrame>* source() = 0;
 
  private:
-  RTC_NO_UNIQUE_ADDRESS SequenceChecker worker_thread_checker_;
+  SequenceChecker worker_thread_checker_;
-  RTC_NO_UNIQUE_ADDRESS SequenceChecker signaling_thread_checker_;
+  SourceState state_;
-  SourceState state_ RTC_GUARDED_BY(&signaling_thread_checker_);
   const bool remote_;
 };