diff --git a/api/media_stream_track_proxy.h b/api/media_stream_track_proxy.h index a0fe676d58..f2d5f214d6 100644 --- a/api/media_stream_track_proxy.h +++ b/api/media_stream_track_proxy.h @@ -30,7 +30,7 @@ BYPASS_PROXY_CONSTMETHOD0(std::string, kind) BYPASS_PROXY_CONSTMETHOD0(std::string, id) PROXY_CONSTMETHOD0(TrackState, state) PROXY_CONSTMETHOD0(bool, enabled) -PROXY_CONSTMETHOD0(AudioSourceInterface*, GetSource) +BYPASS_PROXY_CONSTMETHOD0(AudioSourceInterface*, GetSource) PROXY_METHOD1(void, AddSink, AudioTrackSinkInterface*) PROXY_METHOD1(void, RemoveSink, AudioTrackSinkInterface*) PROXY_METHOD1(bool, GetSignalLevel, int*) @@ -44,17 +44,17 @@ BEGIN_PROXY_MAP(VideoTrack) PROXY_PRIMARY_THREAD_DESTRUCTOR() BYPASS_PROXY_CONSTMETHOD0(std::string, kind) BYPASS_PROXY_CONSTMETHOD0(std::string, id) -PROXY_CONSTMETHOD0(TrackState, state) -PROXY_CONSTMETHOD0(bool, enabled) -PROXY_METHOD1(bool, set_enabled, bool) -PROXY_CONSTMETHOD0(ContentHint, content_hint) -PROXY_METHOD1(void, set_content_hint, ContentHint) +PROXY_SECONDARY_CONSTMETHOD0(TrackState, state) +PROXY_SECONDARY_CONSTMETHOD0(bool, enabled) +PROXY_SECONDARY_METHOD1(bool, set_enabled, bool) +PROXY_SECONDARY_CONSTMETHOD0(ContentHint, content_hint) +PROXY_SECONDARY_METHOD1(void, set_content_hint, ContentHint) PROXY_SECONDARY_METHOD2(void, AddOrUpdateSink, rtc::VideoSinkInterface*, const rtc::VideoSinkWants&) PROXY_SECONDARY_METHOD1(void, RemoveSink, rtc::VideoSinkInterface*) -PROXY_CONSTMETHOD0(VideoTrackSourceInterface*, GetSource) +BYPASS_PROXY_CONSTMETHOD0(VideoTrackSourceInterface*, GetSource) PROXY_METHOD1(void, RegisterObserver, ObserverInterface*) PROXY_METHOD1(void, UnregisterObserver, ObserverInterface*) diff --git a/api/proxy.h b/api/proxy.h index 5e730d9b5f..a530e98a77 100644 --- a/api/proxy.h +++ b/api/proxy.h @@ -435,10 +435,6 @@ class ConstMethodCall : public QueuedTask { #define BYPASS_PROXY_CONSTMETHOD0(r, method) \ r method() const override { \ proxy_internal::TraceApiCall(class_name_, PROXY_STRINGIZE(method)); \ - static_assert( \ - std::is_same::value || !std::is_pointer::value, \ - "Type is a pointer"); \ - static_assert(!std::is_reference::value, "Type is a reference"); \ return c_->method(); \ } diff --git a/media/BUILD.gn b/media/BUILD.gn index c85a037e67..8b0888173d 100644 --- a/media/BUILD.gn +++ b/media/BUILD.gn @@ -106,6 +106,7 @@ rtc_library("rtc_media_base") { "../rtc_base:stringutils", "../rtc_base/synchronization:mutex", "../rtc_base/system:file_wrapper", + "../rtc_base/system:no_unique_address", "../rtc_base/system:rtc_export", "../rtc_base/task_utils:pending_task_safety_flag", "../rtc_base/task_utils:to_queued_task", diff --git a/media/base/video_source_base.cc b/media/base/video_source_base.cc index d057a24ad8..2454902069 100644 --- a/media/base/video_source_base.cc +++ b/media/base/video_source_base.cc @@ -10,6 +10,8 @@ #include "media/base/video_source_base.h" +#include + #include "absl/algorithm/container.h" #include "rtc_base/checks.h" @@ -52,4 +54,51 @@ VideoSourceBase::SinkPair* VideoSourceBase::FindSinkPair( return nullptr; } +VideoSourceBaseGuarded::VideoSourceBaseGuarded() = default; +VideoSourceBaseGuarded::~VideoSourceBaseGuarded() = default; + +void VideoSourceBaseGuarded::AddOrUpdateSink( + VideoSinkInterface* sink, + const VideoSinkWants& wants) { + RTC_DCHECK_RUN_ON(&source_sequence_); + RTC_DCHECK(sink != nullptr); + + SinkPair* sink_pair = FindSinkPair(sink); + if (!sink_pair) { + sinks_.push_back(SinkPair(sink, wants)); + } else { + sink_pair->wants = wants; + } +} + +void VideoSourceBaseGuarded::RemoveSink( + VideoSinkInterface* sink) { + RTC_DCHECK_RUN_ON(&source_sequence_); + RTC_DCHECK(sink != nullptr); + RTC_DCHECK(FindSinkPair(sink)); + sinks_.erase(std::remove_if(sinks_.begin(), sinks_.end(), + [sink](const SinkPair& sink_pair) { + return sink_pair.sink == sink; + }), + sinks_.end()); +} + +VideoSourceBaseGuarded::SinkPair* VideoSourceBaseGuarded::FindSinkPair( + const VideoSinkInterface* sink) { + RTC_DCHECK_RUN_ON(&source_sequence_); + auto sink_pair_it = absl::c_find_if( + sinks_, + [sink](const SinkPair& sink_pair) { return sink_pair.sink == sink; }); + if (sink_pair_it != sinks_.end()) { + return &*sink_pair_it; + } + return nullptr; +} + +const std::vector& +VideoSourceBaseGuarded::sink_pairs() const { + RTC_DCHECK_RUN_ON(&source_sequence_); + return sinks_; +} + } // namespace rtc diff --git a/media/base/video_source_base.h b/media/base/video_source_base.h index 59b7dab164..2644723aa7 100644 --- a/media/base/video_source_base.h +++ b/media/base/video_source_base.h @@ -17,10 +17,14 @@ #include "api/video/video_frame.h" #include "api/video/video_sink_interface.h" #include "api/video/video_source_interface.h" +#include "rtc_base/system/no_unique_address.h" namespace rtc { -// VideoSourceBase is not thread safe. +// VideoSourceBase is not thread safe. Before using this class, consider using +// VideoSourceBaseGuarded below instead, which is an identical implementation +// but applies a sequence checker to help protect internal state. +// TODO(bugs.webrtc.org/12780): Delete this class. class VideoSourceBase : public VideoSourceInterface { public: VideoSourceBase(); @@ -44,6 +48,36 @@ class VideoSourceBase : public VideoSourceInterface { std::vector sinks_; }; +// VideoSourceBaseGuarded assumes that operations related to sinks, occur on the +// same TQ/thread that the object was constructed on. +class VideoSourceBaseGuarded : public VideoSourceInterface { + public: + VideoSourceBaseGuarded(); + ~VideoSourceBaseGuarded() override; + + void AddOrUpdateSink(VideoSinkInterface* sink, + const VideoSinkWants& wants) override; + void RemoveSink(VideoSinkInterface* sink) override; + + protected: + struct SinkPair { + SinkPair(VideoSinkInterface* sink, VideoSinkWants wants) + : sink(sink), wants(wants) {} + VideoSinkInterface* sink; + VideoSinkWants wants; + }; + + SinkPair* FindSinkPair(const VideoSinkInterface* sink); + const std::vector& sink_pairs() const; + + // Keep the `source_sequence_` checker protected to allow sub classes the + // ability to call Detach() if/when appropriate. + RTC_NO_UNIQUE_ADDRESS webrtc::SequenceChecker source_sequence_; + + private: + std::vector sinks_ RTC_GUARDED_BY(&source_sequence_); +}; + } // namespace rtc #endif // MEDIA_BASE_VIDEO_SOURCE_BASE_H_ diff --git a/pc/audio_track.cc b/pc/audio_track.cc index 191d4efbc4..be087f693b 100644 --- a/pc/audio_track.cc +++ b/pc/audio_track.cc @@ -32,7 +32,7 @@ AudioTrack::AudioTrack(const std::string& label, } 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 @@ std::string AudioTrack::kind() const { } 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 07511a5c94..8a705cf8fb 100644 --- a/pc/audio_track.h +++ b/pc/audio_track.h @@ -41,13 +41,13 @@ class AudioTrack : public MediaStreamTrack, // 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 2a4889a576..1d5caacddb 100644 --- a/pc/track_media_info_map_unittest.cc +++ b/pc/track_media_info_map_unittest.cc @@ -31,6 +31,45 @@ namespace webrtc { 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 * 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 * 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 ssrcs) { RtpParameters params; @@ -79,23 +118,35 @@ rtc::scoped_refptr CreateMockRtpReceiver( return receiver; } +rtc::scoped_refptr CreateVideoTrack( + const std::string& id) { + return VideoTrack::Create(id, FakeVideoTrackSource::Create(false), + rtc::Thread::Current()); +} + +rtc::scoped_refptr CreateMockVideoTrack( + const std::string& id) { + auto track = rtc::make_ref_counted(); + 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 @@ class TrackMediaInfoMapTest : public ::testing::Test { std::unique_ptr map_; rtc::scoped_refptr local_audio_track_; rtc::scoped_refptr remote_audio_track_; - rtc::scoped_refptr local_video_track_; - rtc::scoped_refptr remote_video_track_; + rtc::scoped_refptr local_video_track_; + rtc::scoped_refptr remote_video_track_; }; } // namespace diff --git a/pc/video_rtp_receiver.cc b/pc/video_rtp_receiver.cc index 99a200db31..8db4d9f02f 100644 --- a/pc/video_rtp_receiver.cc +++ b/pc/video_rtp_receiver.cc @@ -49,7 +49,7 @@ VideoRtpReceiver::VideoRtpReceiver( 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 b4f511b5fb..d0246faa87 100644 --- a/pc/video_track.cc +++ b/pc/video_track.cc @@ -11,6 +11,7 @@ #include "pc/video_track.h" #include +#include #include #include "api/notifier.h" @@ -28,10 +29,16 @@ VideoTrack::VideoTrack(const std::string& label, 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 @@ std::string VideoTrack::kind() const { // thread. void VideoTrack::AddOrUpdateSink(rtc::VideoSinkInterface* 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* 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 @@ void VideoTrack::set_content_hint(ContentHint hint) { } bool VideoTrack::set_enabled(bool enable) { - RTC_DCHECK(signaling_thread_checker_.IsCurrent()); - worker_thread_->Invoke(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::set_enabled(enable); } +bool VideoTrack::enabled() const { + RTC_DCHECK_RUN_ON(worker_thread_); + return MediaStreamTrack::enabled(); +} + +MediaStreamTrackInterface::TrackState VideoTrack::state() const { + RTC_DCHECK_RUN_ON(worker_thread_); + return MediaStreamTrack::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( + 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::Create( diff --git a/pc/video_track.h b/pc/video_track.h index bff63fcb96..e840c8097f 100644 --- a/pc/video_track.h +++ b/pc/video_track.h @@ -27,7 +27,7 @@ namespace webrtc { class VideoTrack : public MediaStreamTrack, - public rtc::VideoSourceBase, + public rtc::VideoSourceBaseGuarded, public ObserverInterface { public: static rtc::scoped_refptr Create( @@ -38,13 +38,13 @@ class VideoTrack : public MediaStreamTrack, void AddOrUpdateSink(rtc::VideoSinkInterface* sink, const rtc::VideoSinkWants& wants) override; void RemoveSink(rtc::VideoSinkInterface* 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 @@ class VideoTrack : public MediaStreamTrack, // 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 video_source_; - ContentHint content_hint_ RTC_GUARDED_BY(signaling_thread_checker_); + const rtc::scoped_refptr 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 f45d44aa32..d15eaaf43c 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(); }