Refactor TestVideoCapturer to support multiple sinks.

To be able to reuse VideoBroadcaster, that class needs to be
officially threadsafe. It already had the needed locks, but thread
checkers have to be deleted to allow calls to AddOrUpdateSink on
multiple threads (worker thread + encoder thread).

Bug: webrtc:6353, webrtc:10147
Change-Id: I16128ac205c566f09402b6f22587a340d9a983c1
Reviewed-on: https://webrtc-review.googlesource.com/c/115201
Reviewed-by: Sebastian Jansson <srte@webrtc.org>
Commit-Queue: Niels Moller <nisse@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#26073}

media/base/videobroadcaster.cc

@@ -20,15 +20,12 @@
 
 namespace rtc {
 
-VideoBroadcaster::VideoBroadcaster() {
-  thread_checker_.DetachFromThread();
-}
+VideoBroadcaster::VideoBroadcaster() = default;
 VideoBroadcaster::~VideoBroadcaster() = default;
 
 void VideoBroadcaster::AddOrUpdateSink(
     VideoSinkInterface<webrtc::VideoFrame>* sink,
     const VideoSinkWants& wants) {
-  RTC_DCHECK(thread_checker_.CalledOnValidThread());
   RTC_DCHECK(sink != nullptr);
   rtc::CritScope cs(&sinks_and_wants_lock_);
   VideoSourceBase::AddOrUpdateSink(sink, wants);
@@ -37,7 +34,6 @@ void VideoBroadcaster::AddOrUpdateSink(
 
 void VideoBroadcaster::RemoveSink(
     VideoSinkInterface<webrtc::VideoFrame>* sink) {
-  RTC_DCHECK(thread_checker_.CalledOnValidThread());
   RTC_DCHECK(sink != nullptr);
   rtc::CritScope cs(&sinks_and_wants_lock_);
   VideoSourceBase::RemoveSink(sink);
@@ -83,8 +79,6 @@ void VideoBroadcaster::OnDiscardedFrame() {
 }
 
 void VideoBroadcaster::UpdateWants() {
-  RTC_DCHECK(thread_checker_.CalledOnValidThread());
-
   VideoSinkWants wants;
   wants.rotation_applied = false;
   for (auto& sink : sink_pairs()) {

media/base/videobroadcaster.h

@@ -21,12 +21,11 @@
 
 namespace rtc {
 
-// VideoBroadcaster broadcast video frames to sinks and combines
-// VideoSinkWants from its sinks. It does that by implementing
-// rtc::VideoSourceInterface and rtc::VideoSinkInterface.
-// Sinks must be added and removed on one and only one thread.
-// Video frames can be broadcasted on any thread. I.e VideoBroadcaster::OnFrame
-// can be called on any thread.
+// VideoBroadcaster broadcast video frames to sinks and combines VideoSinkWants
+// from its sinks. It does that by implementing rtc::VideoSourceInterface and
+// rtc::VideoSinkInterface. The class is threadsafe; methods may be called on
+// any thread. This is needed because VideoStreamEncoder calls AddOrUpdateSink
+// both on the worker thread and on the encoder task queue.
 class VideoBroadcaster : public VideoSourceBase,
                          public VideoSinkInterface<webrtc::VideoFrame> {
  public:
@@ -57,7 +56,6 @@ class VideoBroadcaster : public VideoSourceBase,
       int width,
       int height) RTC_EXCLUSIVE_LOCKS_REQUIRED(sinks_and_wants_lock_);
 
-  ThreadChecker thread_checker_;
   rtc::CriticalSection sinks_and_wants_lock_;
 
   VideoSinkWants current_wants_ RTC_GUARDED_BY(sinks_and_wants_lock_);

media/base/videosourcebase.cc

@@ -16,15 +16,12 @@
 
 namespace rtc {
 
-VideoSourceBase::VideoSourceBase() {
-  thread_checker_.DetachFromThread();
-}
+VideoSourceBase::VideoSourceBase() = default;
 VideoSourceBase::~VideoSourceBase() = default;
 
 void VideoSourceBase::AddOrUpdateSink(
     VideoSinkInterface<webrtc::VideoFrame>* sink,
     const VideoSinkWants& wants) {
-  RTC_DCHECK(thread_checker_.CalledOnValidThread());
   RTC_DCHECK(sink != nullptr);
 
   SinkPair* sink_pair = FindSinkPair(sink);
@@ -36,7 +33,6 @@ void VideoSourceBase::AddOrUpdateSink(
 }
 
 void VideoSourceBase::RemoveSink(VideoSinkInterface<webrtc::VideoFrame>* sink) {
-  RTC_DCHECK(thread_checker_.CalledOnValidThread());
   RTC_DCHECK(sink != nullptr);
   RTC_DCHECK(FindSinkPair(sink));
   sinks_.erase(std::remove_if(sinks_.begin(), sinks_.end(),
@@ -48,7 +44,6 @@ void VideoSourceBase::RemoveSink(VideoSinkInterface<webrtc::VideoFrame>* sink) {
 
 VideoSourceBase::SinkPair* VideoSourceBase::FindSinkPair(
     const VideoSinkInterface<webrtc::VideoFrame>* sink) {
-  RTC_DCHECK(thread_checker_.CalledOnValidThread());
   auto sink_pair_it = std::find_if(
       sinks_.begin(), sinks_.end(),
       [sink](const SinkPair& sink_pair) { return sink_pair.sink == sink; });

media/base/videosourcebase.h

@@ -39,7 +39,6 @@ class VideoSourceBase : public VideoSourceInterface<webrtc::VideoFrame> {
   SinkPair* FindSinkPair(const VideoSinkInterface<webrtc::VideoFrame>* sink);
 
   const std::vector<SinkPair>& sink_pairs() const { return sinks_; }
-  ThreadChecker thread_checker_;
 
  private:
   std::vector<SinkPair> sinks_;

test/frame_generator_capturer.cc

@@ -127,7 +127,6 @@ FrameGeneratorCapturer::FrameGeneratorCapturer(
     int target_fps)
     : clock_(clock),
       sending_(true),
-      sink_(nullptr),
       sink_wants_observer_(nullptr),
       frame_generator_(std::move(frame_generator)),
       source_fps_(target_fps),
@@ -186,11 +185,7 @@ void FrameGeneratorCapturer::InsertFrame() {
       first_frame_capture_time_ = frame->ntp_time_ms();
     }
 
-    if (sink_) {
-      absl::optional<VideoFrame> out_frame = AdaptFrame(*frame);
-      if (out_frame)
-        sink_->OnFrame(*out_frame);
-    }
+    TestVideoCapturer::OnFrame(*frame);
   }
 }
 
@@ -236,31 +231,29 @@ void FrameGeneratorCapturer::SetSinkWantsObserver(SinkWantsObserver* observer) {
 void FrameGeneratorCapturer::AddOrUpdateSink(
     rtc::VideoSinkInterface<VideoFrame>* sink,
     const rtc::VideoSinkWants& wants) {
+  TestVideoCapturer::AddOrUpdateSink(sink, wants);
   rtc::CritScope cs(&lock_);
-  RTC_CHECK(!sink_ || sink_ == sink);
-  sink_ = sink;
-  if (sink_wants_observer_)
+  if (sink_wants_observer_) {
+    // Tests need to observe unmodified sink wants.
     sink_wants_observer_->OnSinkWantsChanged(sink, wants);
-
-  // Handle framerate within this class, just pass on resolution for possible
-  // adaptation.
-  rtc::VideoSinkWants resolution_wants = wants;
-  resolution_wants.max_framerate_fps = std::numeric_limits<int>::max();
-  TestVideoCapturer::AddOrUpdateSink(sink, resolution_wants);
-
-  // Ignore any requests for framerate higher than initially configured.
-  if (wants.max_framerate_fps < target_capture_fps_) {
-    wanted_fps_.emplace(wants.max_framerate_fps);
-  } else {
-    wanted_fps_.reset();
   }
+  UpdateFps(GetSinkWants().max_framerate_fps);
 }
 
 void FrameGeneratorCapturer::RemoveSink(
     rtc::VideoSinkInterface<VideoFrame>* sink) {
+  TestVideoCapturer::RemoveSink(sink);
+
   rtc::CritScope cs(&lock_);
-  RTC_CHECK(sink_ == sink);
-  sink_ = nullptr;
+  UpdateFps(GetSinkWants().max_framerate_fps);
+}
+
+void FrameGeneratorCapturer::UpdateFps(int max_fps) {
+  if (max_fps < target_capture_fps_) {
+    wanted_fps_.emplace(max_fps);
+  } else {
+    wanted_fps_.reset();
+  }
 }
 
 void FrameGeneratorCapturer::ForceFrame() {

test/frame_generator_capturer.h

@@ -89,10 +89,10 @@ class FrameGeneratorCapturer : public TestVideoCapturer {
   void InsertFrame();
   static bool Run(void* obj);
   int GetCurrentConfiguredFramerate();
+  void UpdateFps(int max_fps) RTC_EXCLUSIVE_LOCKS_REQUIRED(&lock_);
 
   Clock* const clock_;
   bool sending_;
-  rtc::VideoSinkInterface<VideoFrame>* sink_ RTC_GUARDED_BY(&lock_);
   SinkWantsObserver* sink_wants_observer_ RTC_GUARDED_BY(&lock_);
 
   rtc::CriticalSection lock_;

test/test_video_capturer.cc

@@ -10,6 +10,8 @@
 
 #include "test/test_video_capturer.h"
 
+#include <algorithm>
+
 #include "api/video/i420_buffer.h"
 #include "api/video/video_frame_buffer.h"
 #include "api/video/video_rotation.h"
@@ -17,45 +19,55 @@
 
 namespace webrtc {
 namespace test {
-TestVideoCapturer::TestVideoCapturer()
-    : video_adapter_(new cricket::VideoAdapter()) {}
-TestVideoCapturer::~TestVideoCapturer() {}
+TestVideoCapturer::TestVideoCapturer() = default;
+TestVideoCapturer::~TestVideoCapturer() = default;
 
-absl::optional<VideoFrame> TestVideoCapturer::AdaptFrame(
-    const VideoFrame& frame) {
+void TestVideoCapturer::OnFrame(const VideoFrame& frame) {
   int cropped_width = 0;
   int cropped_height = 0;
   int out_width = 0;
   int out_height = 0;
 
-  if (!video_adapter_->AdaptFrameResolution(
+  if (!video_adapter_.AdaptFrameResolution(
           frame.width(), frame.height(), frame.timestamp_us() * 1000,
           &cropped_width, &cropped_height, &out_width, &out_height)) {
     // Drop frame in order to respect frame rate constraint.
-    return absl::nullopt;
+    return;
   }
 
-  absl::optional<VideoFrame> out_frame;
   if (out_height != frame.height() || out_width != frame.width()) {
     // Video adapter has requested a down-scale. Allocate a new buffer and
     // return scaled version.
     rtc::scoped_refptr<I420Buffer> scaled_buffer =
         I420Buffer::Create(out_width, out_height);
     scaled_buffer->ScaleFrom(*frame.video_frame_buffer()->ToI420());
-    out_frame.emplace(
+    broadcaster_.OnFrame(
         VideoFrame(scaled_buffer, kVideoRotation_0, frame.timestamp_us()));
   } else {
     // No adaptations needed, just return the frame as is.
-    out_frame.emplace(frame);
+    broadcaster_.OnFrame(frame);
   }
+}
 
-  return out_frame;
+rtc::VideoSinkWants TestVideoCapturer::GetSinkWants() {
+  return broadcaster_.wants();
 }
 
 void TestVideoCapturer::AddOrUpdateSink(
     rtc::VideoSinkInterface<VideoFrame>* sink,
     const rtc::VideoSinkWants& wants) {
-  video_adapter_->OnResolutionFramerateRequest(
+  broadcaster_.AddOrUpdateSink(sink, wants);
+  UpdateVideoAdapter();
+}
+
+void TestVideoCapturer::RemoveSink(rtc::VideoSinkInterface<VideoFrame>* sink) {
+  broadcaster_.RemoveSink(sink);
+  UpdateVideoAdapter();
+}
+
+void TestVideoCapturer::UpdateVideoAdapter() {
+  rtc::VideoSinkWants wants = broadcaster_.wants();
+  video_adapter_.OnResolutionFramerateRequest(
       wants.target_pixel_count, wants.max_pixel_count, wants.max_framerate_fps);
 }
 

test/test_video_capturer.h

@@ -14,19 +14,12 @@
 
 #include <memory>
 
-#include "absl/types/optional.h"
-#include "api/video/i420_buffer.h"
 #include "api/video/video_frame.h"
 #include "api/video/video_source_interface.h"
 #include "media/base/videoadapter.h"
-#include "rtc_base/criticalsection.h"
-
-namespace cricket {
-class VideoAdapter;
-}  // namespace cricket
+#include "media/base/videobroadcaster.h"
 
 namespace webrtc {
-class Clock;
 namespace test {
 
 class TestVideoCapturer : public rtc::VideoSourceInterface<VideoFrame> {
@@ -36,13 +29,17 @@ class TestVideoCapturer : public rtc::VideoSourceInterface<VideoFrame> {
 
   void AddOrUpdateSink(rtc::VideoSinkInterface<VideoFrame>* sink,
                        const rtc::VideoSinkWants& wants) override;
+  void RemoveSink(rtc::VideoSinkInterface<VideoFrame>* sink) override;
 
  protected:
-  absl::optional<VideoFrame> AdaptFrame(const VideoFrame& frame);
+  void OnFrame(const VideoFrame& frame);
   rtc::VideoSinkWants GetSinkWants();
 
  private:
-  const std::unique_ptr<cricket::VideoAdapter> video_adapter_;
+  void UpdateVideoAdapter();
+
+  rtc::VideoBroadcaster broadcaster_;
+  cricket::VideoAdapter video_adapter_;
 };
 }  // namespace test
 }  // namespace webrtc

test/vcm_capturer.cc

@@ -13,8 +13,6 @@
 #include <stdint.h>
 #include <memory>
 
-#include "absl/types/optional.h"
-#include "common_types.h"  // NOLINT(build/include)
 #include "modules/video_capture/video_capture_factory.h"
 #include "rtc_base/checks.h"
 #include "rtc_base/logging.h"
@@ -22,7 +20,7 @@
 namespace webrtc {
 namespace test {
 
-VcmCapturer::VcmCapturer() : sink_(nullptr), vcm_(nullptr) {}
+VcmCapturer::VcmCapturer() : vcm_(nullptr) {}
 
 bool VcmCapturer::Init(size_t width,
                        size_t height,
@@ -74,20 +72,6 @@ VcmCapturer* VcmCapturer::Create(size_t width,
   return vcm_capturer.release();
 }
 
-void VcmCapturer::AddOrUpdateSink(rtc::VideoSinkInterface<VideoFrame>* sink,
-                                  const rtc::VideoSinkWants& wants) {
-  rtc::CritScope lock(&crit_);
-  RTC_CHECK(!sink_ || sink_ == sink);
-  sink_ = sink;
-  TestVideoCapturer::AddOrUpdateSink(sink, wants);
-}
-
-void VcmCapturer::RemoveSink(rtc::VideoSinkInterface<VideoFrame>* sink) {
-  rtc::CritScope lock(&crit_);
-  RTC_CHECK(sink_ == sink);
-  sink_ = nullptr;
-}
-
 void VcmCapturer::Destroy() {
   if (!vcm_)
     return;
@@ -103,12 +87,7 @@ VcmCapturer::~VcmCapturer() {
 }
 
 void VcmCapturer::OnFrame(const VideoFrame& frame) {
-  rtc::CritScope lock(&crit_);
-  if (sink_) {
-    absl::optional<VideoFrame> out_frame = AdaptFrame(frame);
-    if (out_frame)
-      sink_->OnFrame(*out_frame);
-  }
+  TestVideoCapturer::OnFrame(frame);
 }
 
 }  // namespace test

test/vcm_capturer.h

@@ -11,10 +11,9 @@
 #define TEST_VCM_CAPTURER_H_
 
 #include <memory>
+#include <vector>
 
-#include "common_video/libyuv/include/webrtc_libyuv.h"
 #include "modules/video_capture/video_capture.h"
-#include "rtc_base/criticalsection.h"
 #include "rtc_base/scoped_ref_ptr.h"
 #include "test/test_video_capturer.h"
 
@@ -30,10 +29,6 @@ class VcmCapturer : public TestVideoCapturer,
                              size_t capture_device_index);
   virtual ~VcmCapturer();
 
-  void AddOrUpdateSink(rtc::VideoSinkInterface<VideoFrame>* sink,
-                       const rtc::VideoSinkWants& wants) override;
-  void RemoveSink(rtc::VideoSinkInterface<VideoFrame>* sink) override;
-
   void OnFrame(const VideoFrame& frame) override;
 
  private:
@@ -44,8 +39,6 @@ class VcmCapturer : public TestVideoCapturer,
             size_t capture_device_index);
   void Destroy();
 
-  rtc::CriticalSection crit_;
-  rtc::VideoSinkInterface<VideoFrame>* sink_ RTC_GUARDED_BY(crit_);
   rtc::scoped_refptr<VideoCaptureModule> vcm_;
   VideoCaptureCapability capability_;
 };