Refactor VideoTrackSource, without raw pointer injection.

Bug: None
Change-Id: If4aa8ba72eb3dbdd7dca8970cd6349f1679bc222
Reviewed-on: https://webrtc-review.googlesource.com/78403
Commit-Queue: Niels Moller <nisse@webrtc.org>
Reviewed-by: Karl Wiberg <kwiberg@webrtc.org>
Reviewed-by: Per Kjellander <perkj@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#23370}

pc/rtpreceiver.cc

@@ -215,8 +215,7 @@ VideoRtpReceiver::VideoRtpReceiver(
     const std::vector<rtc::scoped_refptr<MediaStreamInterface>>& streams)
     : worker_thread_(worker_thread),
       id_(receiver_id),
-      source_(new RefCountedObject<VideoTrackSource>(&broadcaster_,
-                                                     true /* remote */)),
+      source_(new RefCountedObject<VideoRtpTrackSource>()),
       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(

pc/rtpreceiver.h

@@ -202,19 +202,31 @@ class VideoRtpReceiver : public rtc::RefCountedObject<RtpReceiverInternal> {
   int AttachmentId() const override { return attachment_id_; }
 
  private:
+  class VideoRtpTrackSource : public VideoTrackSource {
+   public:
+    VideoRtpTrackSource() : VideoTrackSource(true /* remote */) {}
+
+    rtc::VideoSourceInterface<VideoFrame>* source() override {
+      return &broadcaster_;
+    }
+    rtc::VideoSinkInterface<VideoFrame>* 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<VideoFrame>* sink);
 
   rtc::Thread* const worker_thread_;
   const std::string id_;
   cricket::VideoMediaChannel* media_channel_ = nullptr;
   rtc::Optional<uint32_t> 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<VideoTrackSource> source_;
+  rtc::scoped_refptr<VideoRtpTrackSource> source_;
   rtc::scoped_refptr<VideoTrackInterface> track_;
   std::vector<rtc::scoped_refptr<MediaStreamInterface>> streams_;
   bool stopped_ = false;

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<VideoFrame>* source() override { return &source_; }
+
  private:
   FakePeriodicVideoSource source_;
 };

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<VideoFrame>* sink,
+                       const rtc::VideoSinkWants& wants) override {}
+  void RemoveSink(rtc::VideoSinkInterface<VideoFrame>* 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;
+
+  // Unused, since we override AddOrUpdateSink and RemoveSink above.
+  rtc::VideoSourceInterface<VideoFrame>* source() override { return nullptr; }
 
  private:
-  class Source : public rtc::VideoSourceInterface<VideoFrame> {
-   public:
-    void AddOrUpdateSink(rtc::VideoSinkInterface<VideoFrame>* sink,
-                         const rtc::VideoSinkWants& wants) override {}
-
-    void RemoveSink(rtc::VideoSinkInterface<VideoFrame>* sink) override {}
-  };
-
-  Source source_;
   const bool is_screencast_;
 };
 

pc/videocapturertracksource.cc

@@ -288,7 +288,7 @@ VideoCapturerTrackSource::VideoCapturerTrackSource(
     rtc::Thread* worker_thread,
     std::unique_ptr<cricket::VideoCapturer> capturer,
     bool remote)
-    : VideoTrackSource(capturer.get(), remote),
+    : VideoTrackSource(remote),
       signaling_thread_(rtc::Thread::Current()),
       worker_thread_(worker_thread),
       video_capturer_(std::move(capturer)),

pc/videocapturertracksource.h

@@ -57,6 +57,11 @@ class VideoCapturerTrackSource : public VideoTrackSource,
                            std::unique_ptr<cricket::VideoCapturer> capturer,
                            bool remote);
   virtual ~VideoCapturerTrackSource();
+
+  rtc::VideoSourceInterface<VideoFrame>* source() override {
+    return video_capturer_.get();
+  }
+
   void Initialize(const webrtc::MediaConstraintsInterface* constraints);
 
  private:

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<webrtc::VideoFrame>* 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<VideoTrackSource>(
-        &capturer_, true /* remote */);
+    video_track_source_ = new rtc::RefCountedObject<TestVideoTrackSource>();
     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<VideoTrackSource> video_track_source_;
+  rtc::scoped_refptr<TestVideoTrackSource> video_track_source_;
   rtc::scoped_refptr<VideoTrackInterface> video_track_;
 };
 
@@ -58,18 +67,18 @@ TEST_F(VideoTrackTest, RenderVideo) {
   std::unique_ptr<FakeVideoTrackRenderer> 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<FakeVideoTrackRenderer> 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<FakeVideoTrackRenderer> 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());
 }

pc/videotracksource.cc

@@ -14,6 +14,9 @@
 
 namespace webrtc {
 
+VideoTrackSource::VideoTrackSource(bool remote)
+    : VideoTrackSource(nullptr, remote) {}
+
 VideoTrackSource::VideoTrackSource(
     rtc::VideoSourceInterface<VideoFrame>* source,
     bool remote)
@@ -28,26 +31,16 @@ void VideoTrackSource::SetState(SourceState new_state) {
   }
 }
 
-void VideoTrackSource::OnSourceDestroyed() {
-  source_ = nullptr;
-}
-
 void VideoTrackSource::AddOrUpdateSink(
     rtc::VideoSinkInterface<VideoFrame>* 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<VideoFrame>* sink) {
   RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
-  if (!source_) {
-    return;
-  }
-  source_->RemoveSink(sink);
+  source()->RemoveSink(sink);
 }
 
 }  //  namespace webrtc

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<VideoTrackSourceInterface> {
  public:
+  explicit VideoTrackSource(bool remote);
+  // TODO(nisse): Delete, kept only for temporary backwards compatibility.
   VideoTrackSource(rtc::VideoSourceInterface<VideoFrame>* 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<VideoTrackSourceInterface> {
                        const rtc::VideoSinkWants& wants) override;
   void RemoveSink(rtc::VideoSinkInterface<VideoFrame>* sink) override;
 
+ protected:
+  // TODO(nisse): Default implementations for temporary backwards
+  // compatibility.
+  virtual rtc::VideoSourceInterface<VideoFrame>* source() { return source_; }
+
  private:
   rtc::ThreadChecker worker_thread_checker_;
+  // TODO(nisse): Delete, kept only for temporary backwards compatibility.
   rtc::VideoSourceInterface<VideoFrame>* source_;
   SourceState state_;
   const bool remote_;