From 10bd1716fbb215b9e6835f14b371c7ba19e393a1 Mon Sep 17 00:00:00 2001 From: Tomas Gunnarsson Date: Mon, 7 Feb 2022 12:19:37 +0000 Subject: [PATCH] Revert "Reland "Use non-proxied source object in VideoTrack."" This reverts commit 2c47235b0ef1c7bc1aeadc7d0e3aed18e131b3c7. Reason for revert: Downstream issue still doesn't seem to be resolved. Original change's description: > Reland "Use non-proxied source object in VideoTrack." > > This is a reland of 3eb29c12358930a60134f185cd849e0d12aa9166 > > This reland doesn't contain the AudioTrack changes (see original > description) that got triggered in some cases and needs to be > addressed separately. > > Original change's description: > > 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} > > Bug: webrtc:13540 > Change-Id: I59997be174cc9278e3e5910d493efd5352e6de68 > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/250940 > Auto-Submit: Tomas Gunnarsson > Reviewed-by: Harald Alvestrand > Commit-Queue: Harald Alvestrand > Cr-Commit-Position: refs/heads/main@{#35924} TBR=tommi@webrtc.org,hta@webrtc.org,webrtc-scoped@luci-project-accounts.iam.gserviceaccount.com Change-Id: Ib9a76db660c5d18203c13b4feaf5b47f56d7e930 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: webrtc:13540 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/251082 Reviewed-by: Tomas Gunnarsson Reviewed-by: Mirko Bonadei Commit-Queue: Mirko Bonadei Cr-Commit-Position: refs/heads/main@{#35930} --- pc/BUILD.gn | 1 - pc/media_stream_track_proxy.h | 4 ++-- pc/video_rtp_receiver.cc | 6 ++++- pc/video_track.cc | 42 ++++++++++++++++------------------- pc/video_track.h | 14 ++++-------- 5 files changed, 30 insertions(+), 37 deletions(-) diff --git a/pc/BUILD.gn b/pc/BUILD.gn index 7716e5977e..070515057f 100644 --- a/pc/BUILD.gn +++ b/pc/BUILD.gn @@ -923,7 +923,6 @@ 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/media_stream_track_proxy.h b/pc/media_stream_track_proxy.h index ca57c0b6bf..02784233fd 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_CONSTMETHOD0(TrackState, state) +PROXY_SECONDARY_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 d1f8fb0d96..c5ead9e773 100644 --- a/pc/video_rtp_receiver.cc +++ b/pc/video_rtp_receiver.cc @@ -41,7 +41,11 @@ VideoRtpReceiver::VideoRtpReceiver( track_(VideoTrackProxyWithInternal::Create( rtc::Thread::Current(), worker_thread, - VideoTrack::Create(receiver_id, source_, worker_thread))), + VideoTrack::Create(receiver_id, + CreateVideoTrackSourceProxy(rtc::Thread::Current(), + worker_thread, + 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 b96e7d5644..ad552ea1c9 100644 --- a/pc/video_track.cc +++ b/pc/video_track.cc @@ -22,14 +22,12 @@ namespace webrtc { -VideoTrack::VideoTrack( - const std::string& label, - rtc::scoped_refptr< - VideoTrackSourceProxyWithInternal> source, - rtc::Thread* worker_thread) +VideoTrack::VideoTrack(const std::string& label, + VideoTrackSourceInterface* video_source, + rtc::Thread* worker_thread) : MediaStreamTrack(label), worker_thread_(worker_thread), - video_source_(std::move(source)), + 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 @@ -56,18 +54,18 @@ void VideoTrack::AddOrUpdateSink(rtc::VideoSinkInterface* sink, VideoSourceBaseGuarded::AddOrUpdateSink(sink, wants); rtc::VideoSinkWants modified_wants = wants; modified_wants.black_frames = !enabled(); - video_source_->internal()->AddOrUpdateSink(sink, modified_wants); + video_source_->AddOrUpdateSink(sink, modified_wants); } void VideoTrack::RemoveSink(rtc::VideoSinkInterface* sink) { RTC_DCHECK_RUN_ON(worker_thread_); VideoSourceBaseGuarded::RemoveSink(sink); - video_source_->internal()->RemoveSink(sink); + video_source_->RemoveSink(sink); } void VideoTrack::RequestRefreshFrame() { RTC_DCHECK_RUN_ON(worker_thread_); - video_source_->internal()->RequestRefreshFrame(); + video_source_->RequestRefreshFrame(); } VideoTrackSourceInterface* VideoTrack::GetSource() const { @@ -90,11 +88,10 @@ 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; - source->AddOrUpdateSink(sink_pair.sink, modified_wants); + video_source_->AddOrUpdateSink(sink_pair.sink, modified_wants); } return MediaStreamTrack::set_enabled(enable); } @@ -105,29 +102,28 @@ bool VideoTrack::enabled() const { } MediaStreamTrackInterface::TrackState VideoTrack::state() const { - RTC_DCHECK_RUN_ON(&signaling_thread_); + RTC_DCHECK_RUN_ON(worker_thread_); return MediaStreamTrack::state(); } void VideoTrack::OnChanged() { RTC_DCHECK_RUN_ON(&signaling_thread_); - auto* source = video_source_->internal(); - auto state = source->state(); - rtc::Thread::ScopedDisallowBlockingCalls no_blocking_calls; - set_state(state == MediaSourceInterface::kEnded ? kEnded : kLive); + 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( const std::string& id, VideoTrackSourceInterface* source, rtc::Thread* 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); + return rtc::make_ref_counted(id, source, worker_thread); } } // namespace webrtc diff --git a/pc/video_track.h b/pc/video_track.h index d5480c4410..15bfc878f5 100644 --- a/pc/video_track.h +++ b/pc/video_track.h @@ -21,7 +21,6 @@ #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" @@ -50,11 +49,9 @@ class VideoTrack : public MediaStreamTrack, std::string kind() const override; protected: - VideoTrack( - const std::string& id, - rtc::scoped_refptr< - VideoTrackSourceProxyWithInternal> source, - rtc::Thread* worker_thread); + VideoTrack(const std::string& id, + VideoTrackSourceInterface* video_source, + rtc::Thread* worker_thread); ~VideoTrack(); private: @@ -63,10 +60,7 @@ class VideoTrack : public MediaStreamTrack, RTC_NO_UNIQUE_ADDRESS webrtc::SequenceChecker signaling_thread_; rtc::Thread* const worker_thread_; - const rtc::scoped_refptr< - VideoTrackSourceProxyWithInternal> - video_source_; - + const rtc::scoped_refptr video_source_; ContentHint content_hint_ RTC_GUARDED_BY(worker_thread_); };