Reland "Fix race between enabled() and set_enabled() in VideoTrack."
This reverts commit 096ad02c02b4bc6c046282b8793ef84d041dd0d8.
Reason for revert: Including a fix for the test issue.
Original change's description:
> Revert "Fix race between enabled() and set_enabled() in VideoTrack."
>
> This reverts commit 5ffefe9d2d743c66f8a8bcbc5ad9662a3138840a.
>
> Reason for revert: Breaks Chromium Android browser tests on fyi bots.
>
> Original change's description:
> > Fix race between enabled() and set_enabled() in VideoTrack.
> >
> > Along the way I introduced VideoSourceBaseGuarded, which is equivalent
> > to VideoSourceBase except that it applies thread checks. I found that
> > it's easy to use VideoSourceBase incorrectly and in fact there appear
> > to be tests that do this.
> >
> > I made the source object const in VideoTrack, as it already was in
> > AudioTrack, and that allowed for making the GetSource() accessors
> > bypass the proxy thread hop and give the caller direct access.
> >
> > Bug: webrtc:12773, b/188139639, webrtc:12780
> > Change-Id: I022175c4239a1306ef54059c131d81411d5124fe
> > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/219160
> > Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
> > Reviewed-by: Andrey Logvin <landrey@webrtc.org>
> > Commit-Queue: Tommi <tommi@webrtc.org>
> > Cr-Commit-Position: refs/heads/master@{#34096}
>
> TBR=mbonadei@webrtc.org,tommi@webrtc.org,landrey@webrtc.org,webrtc-scoped@luci-project-accounts.iam.gserviceaccount.com
>
> Change-Id: I16323d459c76eb6a87cc602a0048f6ee01c81626
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: webrtc:12773
> Bug: b/188139639
> Bug: webrtc:12780
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/219637
> Reviewed-by: Evan Shrubsole <eshr@google.com>
> Commit-Queue: Evan Shrubsole <eshr@google.com>
> Cr-Commit-Position: refs/heads/master@{#34101}
# Not skipping CQ checks because this is a reland.
Bug: webrtc:12773
Bug: b/188139639
Bug: webrtc:12780
Change-Id: Ib35fe15a6c43de8f286d60aff02b19df1ab76925
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/219639
Reviewed-by: Tommi <tommi@webrtc.org>
Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
Reviewed-by: Andrey Logvin <landrey@webrtc.org>
Reviewed-by: Evan Shrubsole <eshr@google.com>
Commit-Queue: Tommi <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34104}
diff --git a/pc/audio_track.cc b/pc/audio_track.cc
index 191d4ef..be087f6 100644
--- a/pc/audio_track.cc
+++ b/pc/audio_track.cc
@@ -32,7 +32,7 @@
}
AudioTrack::~AudioTrack() {
- RTC_DCHECK(thread_checker_.IsCurrent());
+ RTC_DCHECK_RUN_ON(&thread_checker_);
set_state(MediaStreamTrackInterface::kEnded);
if (audio_source_)
audio_source_->UnregisterObserver(this);
@@ -43,24 +43,24 @@
}
AudioSourceInterface* AudioTrack::GetSource() const {
- RTC_DCHECK(thread_checker_.IsCurrent());
+ // Callable from any thread.
return audio_source_.get();
}
void AudioTrack::AddSink(AudioTrackSinkInterface* sink) {
- RTC_DCHECK(thread_checker_.IsCurrent());
+ RTC_DCHECK_RUN_ON(&thread_checker_);
if (audio_source_)
audio_source_->AddSink(sink);
}
void AudioTrack::RemoveSink(AudioTrackSinkInterface* sink) {
- RTC_DCHECK(thread_checker_.IsCurrent());
+ RTC_DCHECK_RUN_ON(&thread_checker_);
if (audio_source_)
audio_source_->RemoveSink(sink);
}
void AudioTrack::OnChanged() {
- RTC_DCHECK(thread_checker_.IsCurrent());
+ RTC_DCHECK_RUN_ON(&thread_checker_);
if (audio_source_->state() == MediaSourceInterface::kEnded) {
set_state(kEnded);
} else {
diff --git a/pc/audio_track.h b/pc/audio_track.h
index 07511a5..8a705cf 100644
--- a/pc/audio_track.h
+++ b/pc/audio_track.h
@@ -41,13 +41,13 @@
// MediaStreamTrack implementation.
std::string kind() const override;
- private:
// AudioTrackInterface implementation.
AudioSourceInterface* GetSource() const override;
void AddSink(AudioTrackSinkInterface* sink) override;
void RemoveSink(AudioTrackSinkInterface* sink) override;
+ private:
// ObserverInterface implementation.
void OnChanged() override;
diff --git a/pc/track_media_info_map_unittest.cc b/pc/track_media_info_map_unittest.cc
index 2a4889a..1d5caac 100644
--- a/pc/track_media_info_map_unittest.cc
+++ b/pc/track_media_info_map_unittest.cc
@@ -31,6 +31,45 @@
namespace {
+class MockVideoTrack : public VideoTrackInterface {
+ public:
+ // NotifierInterface
+ MOCK_METHOD(void,
+ RegisterObserver,
+ (ObserverInterface * observer),
+ (override));
+ MOCK_METHOD(void,
+ UnregisterObserver,
+ (ObserverInterface * observer),
+ (override));
+
+ // MediaStreamTrackInterface
+ MOCK_METHOD(std::string, kind, (), (const, override));
+ MOCK_METHOD(std::string, id, (), (const, override));
+ MOCK_METHOD(bool, enabled, (), (const, override));
+ MOCK_METHOD(bool, set_enabled, (bool enable), (override));
+ MOCK_METHOD(TrackState, state, (), (const, override));
+
+ // VideoSourceInterface
+ MOCK_METHOD(void,
+ AddOrUpdateSink,
+ (rtc::VideoSinkInterface<VideoFrame> * sink,
+ const rtc::VideoSinkWants& wants),
+ (override));
+ // RemoveSink must guarantee that at the time the method returns,
+ // there is no current and no future calls to VideoSinkInterface::OnFrame.
+ MOCK_METHOD(void,
+ RemoveSink,
+ (rtc::VideoSinkInterface<VideoFrame> * sink),
+ (override));
+
+ // VideoTrackInterface
+ MOCK_METHOD(VideoTrackSourceInterface*, GetSource, (), (const, override));
+
+ MOCK_METHOD(ContentHint, content_hint, (), (const, override));
+ MOCK_METHOD(void, set_content_hint, (ContentHint hint), (override));
+};
+
RtpParameters CreateRtpParametersWithSsrcs(
std::initializer_list<uint32_t> ssrcs) {
RtpParameters params;
@@ -79,23 +118,35 @@
return receiver;
}
+rtc::scoped_refptr<VideoTrackInterface> CreateVideoTrack(
+ const std::string& id) {
+ return VideoTrack::Create(id, FakeVideoTrackSource::Create(false),
+ rtc::Thread::Current());
+}
+
+rtc::scoped_refptr<VideoTrackInterface> CreateMockVideoTrack(
+ const std::string& id) {
+ auto track = rtc::make_ref_counted<MockVideoTrack>();
+ EXPECT_CALL(*track, kind())
+ .WillRepeatedly(::testing::Return(VideoTrack::kVideoKind));
+ return track;
+}
+
class TrackMediaInfoMapTest : public ::testing::Test {
public:
TrackMediaInfoMapTest() : TrackMediaInfoMapTest(true) {}
- explicit TrackMediaInfoMapTest(bool use_current_thread)
+ explicit TrackMediaInfoMapTest(bool use_real_video_track)
: voice_media_info_(new cricket::VoiceMediaInfo()),
video_media_info_(new cricket::VideoMediaInfo()),
local_audio_track_(AudioTrack::Create("LocalAudioTrack", nullptr)),
remote_audio_track_(AudioTrack::Create("RemoteAudioTrack", nullptr)),
- local_video_track_(VideoTrack::Create(
- "LocalVideoTrack",
- FakeVideoTrackSource::Create(false),
- use_current_thread ? rtc::Thread::Current() : nullptr)),
- remote_video_track_(VideoTrack::Create(
- "RemoteVideoTrack",
- FakeVideoTrackSource::Create(false),
- use_current_thread ? rtc::Thread::Current() : nullptr)) {}
+ local_video_track_(use_real_video_track
+ ? CreateVideoTrack("LocalVideoTrack")
+ : CreateMockVideoTrack("LocalVideoTrack")),
+ remote_video_track_(use_real_video_track
+ ? CreateVideoTrack("RemoteVideoTrack")
+ : CreateMockVideoTrack("LocalVideoTrack")) {}
~TrackMediaInfoMapTest() {
// If we have a map the ownership has been passed to the map, only delete if
@@ -179,8 +230,8 @@
std::unique_ptr<TrackMediaInfoMap> map_;
rtc::scoped_refptr<AudioTrack> local_audio_track_;
rtc::scoped_refptr<AudioTrack> remote_audio_track_;
- rtc::scoped_refptr<VideoTrack> local_video_track_;
- rtc::scoped_refptr<VideoTrack> remote_video_track_;
+ rtc::scoped_refptr<VideoTrackInterface> local_video_track_;
+ rtc::scoped_refptr<VideoTrackInterface> remote_video_track_;
};
} // namespace
diff --git a/pc/video_rtp_receiver.cc b/pc/video_rtp_receiver.cc
index 99a200d..8db4d9f 100644
--- a/pc/video_rtp_receiver.cc
+++ b/pc/video_rtp_receiver.cc
@@ -49,7 +49,7 @@
attachment_id_(GenerateUniqueId()) {
RTC_DCHECK(worker_thread_);
SetStreams(streams);
- source_->SetState(MediaSourceInterface::kLive);
+ RTC_DCHECK_EQ(source_->state(), MediaSourceInterface::kLive);
}
VideoRtpReceiver::~VideoRtpReceiver() {
diff --git a/pc/video_track.cc b/pc/video_track.cc
index b4f511b..d0246fa 100644
--- a/pc/video_track.cc
+++ b/pc/video_track.cc
@@ -11,6 +11,7 @@
#include "pc/video_track.h"
#include <string>
+#include <utility>
#include <vector>
#include "api/notifier.h"
@@ -28,10 +29,16 @@
worker_thread_(worker_thread),
video_source_(video_source),
content_hint_(ContentHint::kNone) {
+ RTC_DCHECK_RUN_ON(&signaling_thread_);
+ // Detach the thread checker for VideoSourceBaseGuarded since we'll make calls
+ // to VideoSourceBaseGuarded on the worker thread, but we're currently on the
+ // signaling thread.
+ source_sequence_.Detach();
video_source_->RegisterObserver(this);
}
VideoTrack::~VideoTrack() {
+ RTC_DCHECK_RUN_ON(&signaling_thread_);
video_source_->UnregisterObserver(this);
}
@@ -43,26 +50,31 @@
// thread.
void VideoTrack::AddOrUpdateSink(rtc::VideoSinkInterface<VideoFrame>* sink,
const rtc::VideoSinkWants& wants) {
- RTC_DCHECK(worker_thread_->IsCurrent());
- VideoSourceBase::AddOrUpdateSink(sink, wants);
+ RTC_DCHECK_RUN_ON(worker_thread_);
+ VideoSourceBaseGuarded::AddOrUpdateSink(sink, wants);
rtc::VideoSinkWants modified_wants = wants;
modified_wants.black_frames = !enabled();
video_source_->AddOrUpdateSink(sink, modified_wants);
}
void VideoTrack::RemoveSink(rtc::VideoSinkInterface<VideoFrame>* sink) {
- RTC_DCHECK(worker_thread_->IsCurrent());
- VideoSourceBase::RemoveSink(sink);
+ RTC_DCHECK_RUN_ON(worker_thread_);
+ VideoSourceBaseGuarded::RemoveSink(sink);
video_source_->RemoveSink(sink);
}
+VideoTrackSourceInterface* VideoTrack::GetSource() const {
+ // Callable from any thread.
+ return video_source_.get();
+}
+
VideoTrackInterface::ContentHint VideoTrack::content_hint() const {
- RTC_DCHECK_RUN_ON(&signaling_thread_checker_);
+ RTC_DCHECK_RUN_ON(worker_thread_);
return content_hint_;
}
void VideoTrack::set_content_hint(ContentHint hint) {
- RTC_DCHECK_RUN_ON(&signaling_thread_checker_);
+ RTC_DCHECK_RUN_ON(worker_thread_);
if (content_hint_ == hint)
return;
content_hint_ = hint;
@@ -70,25 +82,36 @@
}
bool VideoTrack::set_enabled(bool enable) {
- RTC_DCHECK(signaling_thread_checker_.IsCurrent());
- worker_thread_->Invoke<void>(RTC_FROM_HERE, [enable, this] {
- RTC_DCHECK(worker_thread_->IsCurrent());
- for (auto& sink_pair : sink_pairs()) {
- rtc::VideoSinkWants modified_wants = sink_pair.wants;
- modified_wants.black_frames = !enable;
- video_source_->AddOrUpdateSink(sink_pair.sink, modified_wants);
- }
- });
+ RTC_DCHECK_RUN_ON(worker_thread_);
+ for (auto& sink_pair : sink_pairs()) {
+ rtc::VideoSinkWants modified_wants = sink_pair.wants;
+ modified_wants.black_frames = !enable;
+ video_source_->AddOrUpdateSink(sink_pair.sink, modified_wants);
+ }
return MediaStreamTrack<VideoTrackInterface>::set_enabled(enable);
}
+bool VideoTrack::enabled() const {
+ RTC_DCHECK_RUN_ON(worker_thread_);
+ return MediaStreamTrack<VideoTrackInterface>::enabled();
+}
+
+MediaStreamTrackInterface::TrackState VideoTrack::state() const {
+ RTC_DCHECK_RUN_ON(worker_thread_);
+ return MediaStreamTrack<VideoTrackInterface>::state();
+}
+
void VideoTrack::OnChanged() {
- RTC_DCHECK(signaling_thread_checker_.IsCurrent());
- if (video_source_->state() == MediaSourceInterface::kEnded) {
- set_state(kEnded);
- } else {
- set_state(kLive);
- }
+ RTC_DCHECK_RUN_ON(&signaling_thread_);
+ worker_thread_->Invoke<void>(
+ RTC_FROM_HERE, [this, state = video_source_->state()]() {
+ // TODO(tommi): Calling set_state() this way isn't ideal since we're
+ // currently blocking the signaling thread and set_state() may
+ // internally fire notifications via `FireOnChanged()` which may further
+ // amplify the blocking effect on the signaling thread.
+ rtc::Thread::ScopedDisallowBlockingCalls no_blocking_calls;
+ set_state(state == MediaSourceInterface::kEnded ? kEnded : kLive);
+ });
}
rtc::scoped_refptr<VideoTrack> VideoTrack::Create(
diff --git a/pc/video_track.h b/pc/video_track.h
index bff63fc..e840c80 100644
--- a/pc/video_track.h
+++ b/pc/video_track.h
@@ -27,7 +27,7 @@
namespace webrtc {
class VideoTrack : public MediaStreamTrack<VideoTrackInterface>,
- public rtc::VideoSourceBase,
+ public rtc::VideoSourceBaseGuarded,
public ObserverInterface {
public:
static rtc::scoped_refptr<VideoTrack> Create(
@@ -38,13 +38,13 @@
void AddOrUpdateSink(rtc::VideoSinkInterface<VideoFrame>* sink,
const rtc::VideoSinkWants& wants) override;
void RemoveSink(rtc::VideoSinkInterface<VideoFrame>* sink) override;
+ VideoTrackSourceInterface* GetSource() const override;
- VideoTrackSourceInterface* GetSource() const override {
- return video_source_.get();
- }
ContentHint content_hint() const override;
void set_content_hint(ContentHint hint) override;
bool set_enabled(bool enable) override;
+ bool enabled() const override;
+ MediaStreamTrackInterface::TrackState state() const override;
std::string kind() const override;
protected:
@@ -57,10 +57,10 @@
// Implements ObserverInterface. Observes |video_source_| state.
void OnChanged() override;
+ RTC_NO_UNIQUE_ADDRESS webrtc::SequenceChecker signaling_thread_;
rtc::Thread* const worker_thread_;
- SequenceChecker signaling_thread_checker_;
- rtc::scoped_refptr<VideoTrackSourceInterface> video_source_;
- ContentHint content_hint_ RTC_GUARDED_BY(signaling_thread_checker_);
+ const rtc::scoped_refptr<VideoTrackSourceInterface> video_source_;
+ ContentHint content_hint_ RTC_GUARDED_BY(worker_thread_);
};
} // namespace webrtc
diff --git a/pc/video_track_source.cc b/pc/video_track_source.cc
index f45d44a..d15eaaf 100644
--- a/pc/video_track_source.cc
+++ b/pc/video_track_source.cc
@@ -15,7 +15,7 @@
namespace webrtc {
VideoTrackSource::VideoTrackSource(bool remote)
- : state_(kInitializing), remote_(remote) {
+ : state_(kLive), remote_(remote) {
worker_thread_checker_.Detach();
}
