From 5d67f82360660b4bb5c36f719055242dbaf1945a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20M=C3=B6ller?= Date: Wed, 23 May 2018 16:28:17 +0200 Subject: Refactor VideoTrackSource, without raw pointer injection. Bug: None Change-Id: If4aa8ba72eb3dbdd7dca8970cd6349f1679bc222 Reviewed-on: https://webrtc-review.googlesource.com/78403 Commit-Queue: Niels Moller Reviewed-by: Karl Wiberg Reviewed-by: Per Kjellander Cr-Commit-Position: refs/heads/master@{#23370} --- pc/rtpreceiver.cc | 6 ++---- pc/rtpreceiver.h | 22 +++++++++++++++++----- pc/test/fakeperiodicvideotracksource.h | 8 ++++---- pc/test/fakevideotracksource.h | 21 ++++++++------------- pc/videocapturertracksource.cc | 2 +- pc/videocapturertracksource.h | 5 +++++ pc/videotrack_unittest.cc | 31 ++++++++++++++++++++----------- pc/videotracksource.cc | 17 +++++------------ pc/videotracksource.h | 15 ++++++++++----- 9 files changed, 72 insertions(+), 55 deletions(-) (limited to 'pc') diff --git a/pc/rtpreceiver.cc b/pc/rtpreceiver.cc index ee45c4e547..341261b331 100644 --- a/pc/rtpreceiver.cc +++ b/pc/rtpreceiver.cc @@ -215,8 +215,7 @@ VideoRtpReceiver::VideoRtpReceiver( const std::vector>& streams) : worker_thread_(worker_thread), id_(receiver_id), - source_(new RefCountedObject(&broadcaster_, - true /* remote */)), + source_(new RefCountedObject()), track_(VideoTrackProxy::Create( rtc::Thread::Current(), worker_thread, @@ -270,7 +269,6 @@ void VideoRtpReceiver::Stop() { return; } source_->SetState(MediaSourceInterface::kEnded); - source_->OnSourceDestroyed(); if (!media_channel_ || !ssrc_) { RTC_LOG(LS_WARNING) << "VideoRtpReceiver::Stop: No video channel exists."; } else { @@ -293,7 +291,7 @@ void VideoRtpReceiver::SetupMediaChannel(uint32_t ssrc) { SetSink(nullptr); } ssrc_ = ssrc; - SetSink(&broadcaster_); + SetSink(source_->sink()); } void VideoRtpReceiver::SetStreams( diff --git a/pc/rtpreceiver.h b/pc/rtpreceiver.h index 55ed29ecf1..4d6457f8e2 100644 --- a/pc/rtpreceiver.h +++ b/pc/rtpreceiver.h @@ -202,19 +202,31 @@ class VideoRtpReceiver : public rtc::RefCountedObject { int AttachmentId() const override { return attachment_id_; } private: + class VideoRtpTrackSource : public VideoTrackSource { + public: + VideoRtpTrackSource() : VideoTrackSource(true /* remote */) {} + + rtc::VideoSourceInterface* source() override { + return &broadcaster_; + } + rtc::VideoSinkInterface* sink() { return &broadcaster_; } + + private: + // |broadcaster_| is needed since the decoder can only handle one sink. + // It might be better if the decoder can handle multiple sinks and consider + // the VideoSinkWants. + rtc::VideoBroadcaster broadcaster_; + }; + bool SetSink(rtc::VideoSinkInterface* sink); rtc::Thread* const worker_thread_; const std::string id_; cricket::VideoMediaChannel* media_channel_ = nullptr; rtc::Optional ssrc_; - // |broadcaster_| is needed since the decoder can only handle one sink. - // It might be better if the decoder can handle multiple sinks and consider - // the VideoSinkWants. - rtc::VideoBroadcaster broadcaster_; // |source_| is held here to be able to change the state of the source when // the VideoRtpReceiver is stopped. - rtc::scoped_refptr source_; + rtc::scoped_refptr source_; rtc::scoped_refptr track_; std::vector> streams_; bool stopped_ = false; diff --git a/pc/test/fakeperiodicvideotracksource.h b/pc/test/fakeperiodicvideotracksource.h index 1be347c084..8cdac0e99c 100644 --- a/pc/test/fakeperiodicvideotracksource.h +++ b/pc/test/fakeperiodicvideotracksource.h @@ -25,13 +25,13 @@ class FakePeriodicVideoTrackSource : public VideoTrackSource { FakePeriodicVideoTrackSource(FakePeriodicVideoSource::Config config, bool remote) - // Note that VideoTrack constructor gets a pointer to an - // uninitialized source object; that works because it only - // stores the pointer for later use. - : VideoTrackSource(&source_, remote), source_(config) {} + : VideoTrackSource(remote), source_(config) {} ~FakePeriodicVideoTrackSource() = default; + protected: + rtc::VideoSourceInterface* source() override { return &source_; } + private: FakePeriodicVideoSource source_; }; diff --git a/pc/test/fakevideotracksource.h b/pc/test/fakevideotracksource.h index 9cc5ef185a..1b5bd87c53 100644 --- a/pc/test/fakevideotracksource.h +++ b/pc/test/fakevideotracksource.h @@ -12,7 +12,6 @@ #define PC_TEST_FAKEVIDEOTRACKSOURCE_H_ #include "api/mediastreaminterface.h" -#include "api/video/video_source_interface.h" #include "pc/videotracksource.h" namespace webrtc { @@ -30,23 +29,19 @@ class FakeVideoTrackSource : public VideoTrackSource { } bool is_screencast() const override { return is_screencast_; } + void AddOrUpdateSink(rtc::VideoSinkInterface* sink, + const rtc::VideoSinkWants& wants) override {} + void RemoveSink(rtc::VideoSinkInterface* sink) {} protected: explicit FakeVideoTrackSource(bool is_screencast) - : VideoTrackSource(&source_, false /* remote */), - is_screencast_(is_screencast) {} - virtual ~FakeVideoTrackSource() {} + : VideoTrackSource(false /* remote */), is_screencast_(is_screencast) {} + ~FakeVideoTrackSource() override = default; - private: - class Source : public rtc::VideoSourceInterface { - public: - void AddOrUpdateSink(rtc::VideoSinkInterface* sink, - const rtc::VideoSinkWants& wants) override {} - - void RemoveSink(rtc::VideoSinkInterface* sink) override {} - }; + // Unused, since we override AddOrUpdateSink and RemoveSink above. + rtc::VideoSourceInterface* source() override { return nullptr; } - Source source_; + private: const bool is_screencast_; }; diff --git a/pc/videocapturertracksource.cc b/pc/videocapturertracksource.cc index bbfb3348be..d90ba51244 100644 --- a/pc/videocapturertracksource.cc +++ b/pc/videocapturertracksource.cc @@ -288,7 +288,7 @@ VideoCapturerTrackSource::VideoCapturerTrackSource( rtc::Thread* worker_thread, std::unique_ptr capturer, bool remote) - : VideoTrackSource(capturer.get(), remote), + : VideoTrackSource(remote), signaling_thread_(rtc::Thread::Current()), worker_thread_(worker_thread), video_capturer_(std::move(capturer)), diff --git a/pc/videocapturertracksource.h b/pc/videocapturertracksource.h index b80323f6b0..5854944ade 100644 --- a/pc/videocapturertracksource.h +++ b/pc/videocapturertracksource.h @@ -57,6 +57,11 @@ class VideoCapturerTrackSource : public VideoTrackSource, std::unique_ptr capturer, bool remote); virtual ~VideoCapturerTrackSource(); + + rtc::VideoSourceInterface* source() override { + return video_capturer_.get(); + } + void Initialize(const webrtc::MediaConstraintsInterface* constraints); private: diff --git a/pc/videotrack_unittest.cc b/pc/videotrack_unittest.cc index 77d16dec78..e833d6ddf0 100644 --- a/pc/videotrack_unittest.cc +++ b/pc/videotrack_unittest.cc @@ -25,22 +25,31 @@ using webrtc::VideoTrackSource; using webrtc::VideoTrack; using webrtc::VideoTrackInterface; +class TestVideoTrackSource : public VideoTrackSource { + public: + TestVideoTrackSource() : VideoTrackSource(true /* remote */) {} + rtc::VideoSourceInterface* source() override { + return &capturer_; + } + cricket::FakeVideoCapturerWithTaskQueue* capturer() { return &capturer_; } + + private: + cricket::FakeVideoCapturerWithTaskQueue capturer_; +}; class VideoTrackTest : public testing::Test { public: VideoTrackTest() { static const char kVideoTrackId[] = "track_id"; - video_track_source_ = new rtc::RefCountedObject( - &capturer_, true /* remote */); + video_track_source_ = new rtc::RefCountedObject(); video_track_ = VideoTrack::Create(kVideoTrackId, video_track_source_, rtc::Thread::Current()); - capturer_.Start( + video_track_source_->capturer()->Start( cricket::VideoFormat(640, 480, cricket::VideoFormat::FpsToInterval(30), cricket::FOURCC_I420)); } protected: - cricket::FakeVideoCapturerWithTaskQueue capturer_; - rtc::scoped_refptr video_track_source_; + rtc::scoped_refptr video_track_source_; rtc::scoped_refptr video_track_; }; @@ -58,18 +67,18 @@ TEST_F(VideoTrackTest, RenderVideo) { std::unique_ptr renderer_1( new FakeVideoTrackRenderer(video_track_.get())); - capturer_.CaptureFrame(); + video_track_source_->capturer()->CaptureFrame(); EXPECT_EQ(1, renderer_1->num_rendered_frames()); // FakeVideoTrackRenderer register itself to |video_track_| std::unique_ptr renderer_2( new FakeVideoTrackRenderer(video_track_.get())); - capturer_.CaptureFrame(); + video_track_source_->capturer()->CaptureFrame(); EXPECT_EQ(2, renderer_1->num_rendered_frames()); EXPECT_EQ(1, renderer_2->num_rendered_frames()); renderer_1.reset(nullptr); - capturer_.CaptureFrame(); + video_track_source_->capturer()->CaptureFrame(); EXPECT_EQ(2, renderer_2->num_rendered_frames()); } @@ -78,17 +87,17 @@ TEST_F(VideoTrackTest, DisableTrackBlackout) { std::unique_ptr renderer( new FakeVideoTrackRenderer(video_track_.get())); - capturer_.CaptureFrame(); + video_track_source_->capturer()->CaptureFrame(); EXPECT_EQ(1, renderer->num_rendered_frames()); EXPECT_FALSE(renderer->black_frame()); video_track_->set_enabled(false); - capturer_.CaptureFrame(); + video_track_source_->capturer()->CaptureFrame(); EXPECT_EQ(2, renderer->num_rendered_frames()); EXPECT_TRUE(renderer->black_frame()); video_track_->set_enabled(true); - capturer_.CaptureFrame(); + video_track_source_->capturer()->CaptureFrame(); EXPECT_EQ(3, renderer->num_rendered_frames()); EXPECT_FALSE(renderer->black_frame()); } diff --git a/pc/videotracksource.cc b/pc/videotracksource.cc index 56f48e7375..de87acb21d 100644 --- a/pc/videotracksource.cc +++ b/pc/videotracksource.cc @@ -14,6 +14,9 @@ namespace webrtc { +VideoTrackSource::VideoTrackSource(bool remote) + : VideoTrackSource(nullptr, remote) {} + VideoTrackSource::VideoTrackSource( rtc::VideoSourceInterface* source, bool remote) @@ -28,26 +31,16 @@ void VideoTrackSource::SetState(SourceState new_state) { } } -void VideoTrackSource::OnSourceDestroyed() { - source_ = nullptr; -} - void VideoTrackSource::AddOrUpdateSink( rtc::VideoSinkInterface* sink, const rtc::VideoSinkWants& wants) { RTC_DCHECK(worker_thread_checker_.CalledOnValidThread()); - if (!source_) { - return; - } - source_->AddOrUpdateSink(sink, wants); + source()->AddOrUpdateSink(sink, wants); } void VideoTrackSource::RemoveSink(rtc::VideoSinkInterface* sink) { RTC_DCHECK(worker_thread_checker_.CalledOnValidThread()); - if (!source_) { - return; - } - source_->RemoveSink(sink); + source()->RemoveSink(sink); } } // namespace webrtc diff --git a/pc/videotracksource.h b/pc/videotracksource.h index 5ead399ff4..f3eef3b675 100644 --- a/pc/videotracksource.h +++ b/pc/videotracksource.h @@ -17,17 +17,16 @@ #include "media/base/mediachannel.h" #include "rtc_base/thread_checker.h" -// VideoTrackSource implements VideoTrackSourceInterface. namespace webrtc { +// VideoTrackSource is a convenience base class for implementations of +// VideoTrackSourceInterface. class VideoTrackSource : public Notifier { public: + explicit VideoTrackSource(bool remote); + // TODO(nisse): Delete, kept only for temporary backwards compatibility. VideoTrackSource(rtc::VideoSourceInterface* source, bool remote); void SetState(SourceState new_state); - // OnSourceDestroyed clears this instance pointer to |source_|. It is useful - // when the underlying rtc::VideoSourceInterface is destroyed before the - // reference counted VideoTrackSource. - void OnSourceDestroyed(); SourceState state() const override { return state_; } bool remote() const override { return remote_; } @@ -41,8 +40,14 @@ class VideoTrackSource : public Notifier { const rtc::VideoSinkWants& wants) override; void RemoveSink(rtc::VideoSinkInterface* sink) override; + protected: + // TODO(nisse): Default implementations for temporary backwards + // compatibility. + virtual rtc::VideoSourceInterface* source() { return source_; } + private: rtc::ThreadChecker worker_thread_checker_; + // TODO(nisse): Delete, kept only for temporary backwards compatibility. rtc::VideoSourceInterface* source_; SourceState state_; const bool remote_; -- cgit v1.2.3