From 3eb29c12358930a60134f185cd849e0d12aa9166 Mon Sep 17 00:00:00 2001 From: Tommi Date: Fri, 4 Feb 2022 16:44:37 +0100 Subject: [PATCH] Use non-proxied source object in VideoTrack. Use the internal representation of the video source object from the track. Before there were implicit thread hops due to use of the proxy. Also, override AudioTrack's enabled methods to enforce thread expectations. Bug: webrtc:13540 Change-Id: I4bc7aca96d6fc24f31ade45e47f52599f1cc2f97 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/250180 Reviewed-by: Harald Alvestrand Commit-Queue: Tomas Gunnarsson Cr-Commit-Position: refs/heads/main@{#35911} --- pc/BUILD.gn | 1 + pc/audio_track.cc | 10 +++++++++ pc/audio_track.h | 5 +++++ pc/media_stream_track_proxy.h | 4 ++-- pc/video_rtp_receiver.cc | 6 +---- pc/video_track.cc | 42 +++++++++++++++++++---------------- pc/video_track.h | 14 ++++++++---- 7 files changed, 52 insertions(+), 30 deletions(-) diff --git a/pc/BUILD.gn b/pc/BUILD.gn index 4b7ad3b3fa..d07d5731b0 100644 --- a/pc/BUILD.gn +++ b/pc/BUILD.gn @@ -868,6 +868,7 @@ rtc_library("video_track") { "../api:sequence_checker", "../api/video:video_frame", "../media:rtc_media_base", + "../pc:rtc_pc_base", "../rtc_base", "../rtc_base:checks", "../rtc_base:refcount", diff --git a/pc/audio_track.cc b/pc/audio_track.cc index be087f693b..25c7dde00b 100644 --- a/pc/audio_track.cc +++ b/pc/audio_track.cc @@ -42,6 +42,16 @@ std::string AudioTrack::kind() const { return kAudioKind; } +bool AudioTrack::set_enabled(bool enable) { + RTC_DCHECK_RUN_ON(&thread_checker_); + return MediaStreamTrack::set_enabled(enable); +} + +bool AudioTrack::enabled() const { + RTC_DCHECK_RUN_ON(&thread_checker_); + return MediaStreamTrack::enabled(); +} + AudioSourceInterface* AudioTrack::GetSource() const { // Callable from any thread. return audio_source_.get(); diff --git a/pc/audio_track.h b/pc/audio_track.h index 8a705cf8fb..e65f27e1a9 100644 --- a/pc/audio_track.h +++ b/pc/audio_track.h @@ -41,6 +41,11 @@ class AudioTrack : public MediaStreamTrack, // MediaStreamTrack implementation. std::string kind() const override; + // NOTE: `set_enabled` and `enabled` are overridden only to ensure threading + // expectations consistent with what's declared in media_stream_track_proxy.h. + bool set_enabled(bool enable) override; + bool enabled() const override; + // AudioTrackInterface implementation. AudioSourceInterface* GetSource() const override; diff --git a/pc/media_stream_track_proxy.h b/pc/media_stream_track_proxy.h index 02784233fd..ca57c0b6bf 100644 --- a/pc/media_stream_track_proxy.h +++ b/pc/media_stream_track_proxy.h @@ -29,12 +29,12 @@ 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) BYPASS_PROXY_CONSTMETHOD0(AudioSourceInterface*, GetSource) PROXY_METHOD1(void, AddSink, AudioTrackSinkInterface*) PROXY_METHOD1(void, RemoveSink, AudioTrackSinkInterface*) PROXY_METHOD1(bool, GetSignalLevel, int*) PROXY_METHOD0(rtc::scoped_refptr, GetAudioProcessor) -PROXY_METHOD1(bool, set_enabled, bool) PROXY_METHOD1(void, RegisterObserver, ObserverInterface*) PROXY_METHOD1(void, UnregisterObserver, ObserverInterface*) END_PROXY_MAP(AudioTrack) @@ -43,7 +43,7 @@ BEGIN_PROXY_MAP(VideoTrack) PROXY_PRIMARY_THREAD_DESTRUCTOR() BYPASS_PROXY_CONSTMETHOD0(std::string, kind) BYPASS_PROXY_CONSTMETHOD0(std::string, id) -PROXY_SECONDARY_CONSTMETHOD0(TrackState, state) +PROXY_CONSTMETHOD0(TrackState, state) PROXY_SECONDARY_CONSTMETHOD0(bool, enabled) PROXY_SECONDARY_METHOD1(bool, set_enabled, bool) PROXY_SECONDARY_CONSTMETHOD0(ContentHint, content_hint) diff --git a/pc/video_rtp_receiver.cc b/pc/video_rtp_receiver.cc index c5ead9e773..d1f8fb0d96 100644 --- a/pc/video_rtp_receiver.cc +++ b/pc/video_rtp_receiver.cc @@ -41,11 +41,7 @@ VideoRtpReceiver::VideoRtpReceiver( track_(VideoTrackProxyWithInternal::Create( rtc::Thread::Current(), worker_thread, - VideoTrack::Create(receiver_id, - CreateVideoTrackSourceProxy(rtc::Thread::Current(), - worker_thread, - source_), - worker_thread))), + VideoTrack::Create(receiver_id, source_, worker_thread))), attachment_id_(GenerateUniqueId()) { RTC_DCHECK(worker_thread_); SetStreams(streams); diff --git a/pc/video_track.cc b/pc/video_track.cc index ad552ea1c9..b96e7d5644 100644 --- a/pc/video_track.cc +++ b/pc/video_track.cc @@ -22,12 +22,14 @@ namespace webrtc { -VideoTrack::VideoTrack(const std::string& label, - VideoTrackSourceInterface* video_source, - rtc::Thread* worker_thread) +VideoTrack::VideoTrack( + const std::string& label, + rtc::scoped_refptr< + VideoTrackSourceProxyWithInternal> source, + rtc::Thread* worker_thread) : MediaStreamTrack(label), worker_thread_(worker_thread), - video_source_(video_source), + video_source_(std::move(source)), content_hint_(ContentHint::kNone) { RTC_DCHECK_RUN_ON(&signaling_thread_); // Detach the thread checker for VideoSourceBaseGuarded since we'll make calls @@ -54,18 +56,18 @@ void VideoTrack::AddOrUpdateSink(rtc::VideoSinkInterface* sink, VideoSourceBaseGuarded::AddOrUpdateSink(sink, wants); rtc::VideoSinkWants modified_wants = wants; modified_wants.black_frames = !enabled(); - video_source_->AddOrUpdateSink(sink, modified_wants); + video_source_->internal()->AddOrUpdateSink(sink, modified_wants); } void VideoTrack::RemoveSink(rtc::VideoSinkInterface* sink) { RTC_DCHECK_RUN_ON(worker_thread_); VideoSourceBaseGuarded::RemoveSink(sink); - video_source_->RemoveSink(sink); + video_source_->internal()->RemoveSink(sink); } void VideoTrack::RequestRefreshFrame() { RTC_DCHECK_RUN_ON(worker_thread_); - video_source_->RequestRefreshFrame(); + video_source_->internal()->RequestRefreshFrame(); } VideoTrackSourceInterface* VideoTrack::GetSource() const { @@ -88,10 +90,11 @@ void VideoTrack::set_content_hint(ContentHint hint) { bool VideoTrack::set_enabled(bool enable) { RTC_DCHECK_RUN_ON(worker_thread_); + auto* source = video_source_->internal(); 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); + source->AddOrUpdateSink(sink_pair.sink, modified_wants); } return MediaStreamTrack::set_enabled(enable); } @@ -102,28 +105,29 @@ bool VideoTrack::enabled() const { } MediaStreamTrackInterface::TrackState VideoTrack::state() const { - RTC_DCHECK_RUN_ON(worker_thread_); + RTC_DCHECK_RUN_ON(&signaling_thread_); return MediaStreamTrack::state(); } void VideoTrack::OnChanged() { 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); - }); + auto* source = video_source_->internal(); + auto state = source->state(); + rtc::Thread::ScopedDisallowBlockingCalls no_blocking_calls; + set_state(state == MediaSourceInterface::kEnded ? kEnded : kLive); } rtc::scoped_refptr VideoTrack::Create( const std::string& id, VideoTrackSourceInterface* source, rtc::Thread* worker_thread) { - return rtc::make_ref_counted(id, source, worker_thread); + rtc::scoped_refptr< + VideoTrackSourceProxyWithInternal> + source_proxy = VideoTrackSourceProxy::Create(rtc::Thread::Current(), + worker_thread, source); + + return rtc::make_ref_counted(id, std::move(source_proxy), + worker_thread); } } // namespace webrtc diff --git a/pc/video_track.h b/pc/video_track.h index 15bfc878f5..d5480c4410 100644 --- a/pc/video_track.h +++ b/pc/video_track.h @@ -21,6 +21,7 @@ #include "api/video/video_sink_interface.h" #include "api/video/video_source_interface.h" #include "media/base/video_source_base.h" +#include "pc/video_track_source_proxy.h" #include "rtc_base/thread.h" #include "rtc_base/thread_annotations.h" @@ -49,9 +50,11 @@ class VideoTrack : public MediaStreamTrack, std::string kind() const override; protected: - VideoTrack(const std::string& id, - VideoTrackSourceInterface* video_source, - rtc::Thread* worker_thread); + VideoTrack( + const std::string& id, + rtc::scoped_refptr< + VideoTrackSourceProxyWithInternal> source, + rtc::Thread* worker_thread); ~VideoTrack(); private: @@ -60,7 +63,10 @@ class VideoTrack : public MediaStreamTrack, RTC_NO_UNIQUE_ADDRESS webrtc::SequenceChecker signaling_thread_; rtc::Thread* const worker_thread_; - const rtc::scoped_refptr video_source_; + const rtc::scoped_refptr< + VideoTrackSourceProxyWithInternal> + video_source_; + ContentHint content_hint_ RTC_GUARDED_BY(worker_thread_); };