Reland "Remove `stopped_` from AudioRtpReceiver and VideoRtpReceiver."
This is a reland of 3ed36c0521546881656c73984456485dcab16205
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 <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}
Bug: webrtc:13540
Change-Id: I6d7d67fddb1ddfc69a302f0f69a9b815f2fd82f7
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/251386
Reviewed-by: Tomas Gunnarsson <tommi@webrtc.org>
Commit-Queue: Mirko Bonadei <mbonadei@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35967}
diff --git a/pc/BUILD.gn b/pc/BUILD.gn
index 7fcb046..ba05867 100644
--- a/pc/BUILD.gn
+++ b/pc/BUILD.gn
@@ -1416,6 +1416,7 @@
"../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 7890d9b..a49b7ce 100644
--- a/pc/audio_rtp_receiver.cc
+++ b/pc/audio_rtp_receiver.cc
@@ -61,7 +61,6 @@
AudioRtpReceiver::~AudioRtpReceiver() {
RTC_DCHECK_RUN_ON(&signaling_thread_checker_);
- RTC_DCHECK(stopped_);
RTC_DCHECK(!media_channel_);
track_->GetSource()->UnregisterAudioObserver(this);
@@ -85,6 +84,10 @@
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);
}
@@ -94,13 +97,10 @@
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,10 +160,7 @@
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;
- }
+ source_->SetState(MediaSourceInterface::kEnded);
worker_thread_->Invoke<void>(RTC_FROM_HERE, [&]() {
RTC_DCHECK_RUN_ON(worker_thread_);
@@ -183,22 +180,17 @@
void AudioRtpReceiver::RestartMediaChannel(absl::optional<uint32_t> ssrc) {
RTC_DCHECK_RUN_ON(&signaling_thread_checker_);
- bool ok = worker_thread_->Invoke<bool>(
- RTC_FROM_HERE, [&, enabled = cached_track_enabled_,
- volume = cached_volume_, was_stopped = stopped_]() {
+ MediaSourceInterface::SourceState state = source_->state();
+ worker_thread_->Invoke<void>(
+ RTC_FROM_HERE,
+ [&, enabled = cached_track_enabled_, volume = cached_volume_]() {
RTC_DCHECK_RUN_ON(worker_thread_);
- if (!media_channel_) {
- RTC_DCHECK(was_stopped);
- return false; // Can't restart.
- }
+ if (!media_channel_)
+ return; // Can't restart.
- if (!was_stopped && ssrc_ == ssrc) {
- // Already running with that ssrc.
- RTC_DCHECK(worker_thread_safety_->alive());
- return true;
- }
-
- if (!was_stopped) {
+ if (state != MediaSourceInterface::kInitializing) {
+ if (ssrc_ == ssrc)
+ return;
source_->Stop(media_channel_, ssrc_);
}
@@ -209,13 +201,8 @@
}
Reconfigure(enabled, volume);
- return true;
});
-
- if (!ok)
- return;
-
- stopped_ = false;
+ source_->SetState(MediaSourceInterface::kLive);
}
void AudioRtpReceiver::SetupMediaChannel(uint32_t ssrc) {
@@ -335,9 +322,6 @@
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);
diff --git a/pc/audio_rtp_receiver.h b/pc/audio_rtp_receiver.h
index aef497d..978c550 100644
--- a/pc/audio_rtp_receiver.h
+++ b/pc/audio_rtp_receiver.h
@@ -133,7 +133,6 @@
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 1865115..763677b 100644
--- a/pc/audio_rtp_receiver_unittest.cc
+++ b/pc/audio_rtp_receiver_unittest.cc
@@ -63,6 +63,7 @@
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 dc890e7..78a35f3 100644
--- a/pc/remote_audio_source.cc
+++ b/pc/remote_audio_source.cc
@@ -55,7 +55,7 @@
: main_thread_(rtc::Thread::Current()),
worker_thread_(worker_thread),
on_audio_channel_gone_action_(on_audio_channel_gone_action),
- state_(MediaSourceInterface::kLive) {
+ state_(MediaSourceInterface::kInitializing) {
RTC_DCHECK(main_thread_);
RTC_DCHECK(worker_thread_);
}
@@ -134,11 +134,6 @@
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 8db4d9f..c5ead9e 100644
--- a/pc/video_rtp_receiver.cc
+++ b/pc/video_rtp_receiver.cc
@@ -49,12 +49,11 @@
attachment_id_(GenerateUniqueId()) {
RTC_DCHECK(worker_thread_);
SetStreams(streams);
- RTC_DCHECK_EQ(source_->state(), MediaSourceInterface::kLive);
+ RTC_DCHECK_EQ(source_->state(), MediaSourceInterface::kInitializing);
}
VideoRtpReceiver::~VideoRtpReceiver() {
RTC_DCHECK_RUN_ON(&signaling_thread_checker_);
- RTC_DCHECK(stopped_);
RTC_DCHECK(!media_channel_);
}
@@ -116,10 +115,7 @@
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;
- }
+ source_->SetState(MediaSourceInterface::kEnded);
worker_thread_->Invoke<void>(RTC_FROM_HERE, [&] {
RTC_DCHECK_RUN_ON(worker_thread_);
@@ -140,33 +136,29 @@
void VideoRtpReceiver::RestartMediaChannel(absl::optional<uint32_t> ssrc) {
RTC_DCHECK_RUN_ON(&signaling_thread_checker_);
- // `stopped_` will be `true` on construction. RestartMediaChannel
- // can in this case function like "ensure started" and flip `stopped_`
- // to false.
+ MediaSourceInterface::SourceState state = source_->state();
// TODO(tommi): Can we restart the media channel without blocking?
- bool ok = worker_thread_->Invoke<bool>(RTC_FROM_HERE, [&, was_stopped =
- stopped_] {
+ worker_thread_->Invoke<void>(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.
- RTC_DCHECK(was_stopped);
- return false; // Can't restart.
+ return; // Can't restart.
}
- if (!was_stopped && ssrc_ == ssrc) {
- // Already running with that ssrc.
- return true;
- }
+ const bool encoded_sink_enabled = saved_encoded_sink_enabled_;
- // Disconnect from the previous ssrc.
- if (!was_stopped) {
+ if (state != MediaSourceInterface::kInitializing) {
+ if (ssrc == ssrc_)
+ return;
+
+ // Disconnect from a previous ssrc.
SetSink(nullptr);
- }
- bool encoded_sink_enabled = saved_encoded_sink_enabled_;
- SetEncodedSinkEnabled(false);
+ if (encoded_sink_enabled)
+ SetEncodedSinkEnabled(false);
+ }
// Set up the new ssrc.
ssrc_ = std::move(ssrc);
@@ -187,14 +179,8 @@
media_channel_->SetBaseMinimumPlayoutDelayMs(*ssrc_, delay_.GetMs());
}
-
- return true;
});
-
- if (!ok)
- return;
-
- stopped_ = false;
+ source_->SetState(MediaSourceInterface::kLive);
}
// RTC_RUN_ON(worker_thread_)
@@ -288,9 +274,6 @@
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);
diff --git a/pc/video_rtp_receiver.h b/pc/video_rtp_receiver.h
index b538186..681f423 100644
--- a/pc/video_rtp_receiver.h
+++ b/pc/video_rtp_receiver.h
@@ -141,8 +141,6 @@
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_);
@@ -152,15 +150,6 @@
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_) =
diff --git a/pc/video_rtp_receiver_unittest.cc b/pc/video_rtp_receiver_unittest.cc
index 3a8099d..42ff261 100644
--- a/pc/video_rtp_receiver_unittest.cc
+++ b/pc/video_rtp_receiver_unittest.cc
@@ -169,7 +169,6 @@
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 d15eaaf..64e99cc 100644
--- a/pc/video_track_source.cc
+++ b/pc/video_track_source.cc
@@ -15,11 +15,12 @@
namespace webrtc {
VideoTrackSource::VideoTrackSource(bool remote)
- : state_(kLive), remote_(remote) {
+ : state_(kInitializing), 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 4a29381..3f568f6 100644
--- a/pc/video_track_source.h
+++ b/pc/video_track_source.h
@@ -20,6 +20,7 @@
#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 {
@@ -31,7 +32,10 @@
explicit VideoTrackSource(bool remote);
void SetState(SourceState new_state);
- SourceState state() const override { return state_; }
+ SourceState state() const override {
+ RTC_DCHECK_RUN_ON(&signaling_thread_checker_);
+ return state_;
+ }
bool remote() const override { return remote_; }
bool is_screencast() const override { return false; }
@@ -56,8 +60,9 @@
virtual rtc::VideoSourceInterface<VideoFrame>* source() = 0;
private:
- SequenceChecker worker_thread_checker_;
- SourceState state_;
+ RTC_NO_UNIQUE_ADDRESS SequenceChecker worker_thread_checker_;
+ RTC_NO_UNIQUE_ADDRESS SequenceChecker signaling_thread_checker_;
+ SourceState state_ RTC_GUARDED_BY(&signaling_thread_checker_);
const bool remote_;
};
