From 3793bb447ae5d502bd4f763d688c65eb47c05ede Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20M=C3=B6ller?= Date: Thu, 20 Dec 2018 13:46:06 +0100 Subject: [PATCH] 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 Commit-Queue: Niels Moller Cr-Commit-Position: refs/heads/master@{#26073} --- media/base/videobroadcaster.cc | 8 +------ media/base/videobroadcaster.h | 12 ++++------ media/base/videosourcebase.cc | 7 +----- media/base/videosourcebase.h | 1 - test/frame_generator_capturer.cc | 39 +++++++++++++------------------- test/frame_generator_capturer.h | 2 +- test/test_video_capturer.cc | 36 +++++++++++++++++++---------- test/test_video_capturer.h | 17 ++++++-------- test/vcm_capturer.cc | 25 ++------------------ test/vcm_capturer.h | 9 +------- 10 files changed, 58 insertions(+), 98 deletions(-) diff --git a/media/base/videobroadcaster.cc b/media/base/videobroadcaster.cc index f953bab422..125cf17c97 100644 --- a/media/base/videobroadcaster.cc +++ b/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* 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* 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()) { diff --git a/media/base/videobroadcaster.h b/media/base/videobroadcaster.h index 5f02a35dc8..18bfc9d06f 100644 --- a/media/base/videobroadcaster.h +++ b/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 { 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_); diff --git a/media/base/videosourcebase.cc b/media/base/videosourcebase.cc index 64b49cc351..47dfaabdc8 100644 --- a/media/base/videosourcebase.cc +++ b/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* 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* 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* sink) { VideoSourceBase::SinkPair* VideoSourceBase::FindSinkPair( const VideoSinkInterface* 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; }); diff --git a/media/base/videosourcebase.h b/media/base/videosourcebase.h index 6c7d5a30e7..aaae61c353 100644 --- a/media/base/videosourcebase.h +++ b/media/base/videosourcebase.h @@ -39,7 +39,6 @@ class VideoSourceBase : public VideoSourceInterface { SinkPair* FindSinkPair(const VideoSinkInterface* sink); const std::vector& sink_pairs() const { return sinks_; } - ThreadChecker thread_checker_; private: std::vector sinks_; diff --git a/test/frame_generator_capturer.cc b/test/frame_generator_capturer.cc index 858f895f64..768bab5cb1 100644 --- a/test/frame_generator_capturer.cc +++ b/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 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* 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::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* 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() { diff --git a/test/frame_generator_capturer.h b/test/frame_generator_capturer.h index cc938b03d1..cb76806fbe 100644 --- a/test/frame_generator_capturer.h +++ b/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* sink_ RTC_GUARDED_BY(&lock_); SinkWantsObserver* sink_wants_observer_ RTC_GUARDED_BY(&lock_); rtc::CriticalSection lock_; diff --git a/test/test_video_capturer.cc b/test/test_video_capturer.cc index bc0cedab88..0d57715a2b 100644 --- a/test/test_video_capturer.cc +++ b/test/test_video_capturer.cc @@ -10,6 +10,8 @@ #include "test/test_video_capturer.h" +#include + #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 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 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 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* sink, const rtc::VideoSinkWants& wants) { - video_adapter_->OnResolutionFramerateRequest( + broadcaster_.AddOrUpdateSink(sink, wants); + UpdateVideoAdapter(); +} + +void TestVideoCapturer::RemoveSink(rtc::VideoSinkInterface* 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); } diff --git a/test/test_video_capturer.h b/test/test_video_capturer.h index 93b3e8f2d8..250736e604 100644 --- a/test/test_video_capturer.h +++ b/test/test_video_capturer.h @@ -14,19 +14,12 @@ #include -#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 { @@ -36,13 +29,17 @@ class TestVideoCapturer : public rtc::VideoSourceInterface { void AddOrUpdateSink(rtc::VideoSinkInterface* sink, const rtc::VideoSinkWants& wants) override; + void RemoveSink(rtc::VideoSinkInterface* sink) override; protected: - absl::optional AdaptFrame(const VideoFrame& frame); + void OnFrame(const VideoFrame& frame); rtc::VideoSinkWants GetSinkWants(); private: - const std::unique_ptr video_adapter_; + void UpdateVideoAdapter(); + + rtc::VideoBroadcaster broadcaster_; + cricket::VideoAdapter video_adapter_; }; } // namespace test } // namespace webrtc diff --git a/test/vcm_capturer.cc b/test/vcm_capturer.cc index 40402f88cb..353be0a317 100644 --- a/test/vcm_capturer.cc +++ b/test/vcm_capturer.cc @@ -13,8 +13,6 @@ #include #include -#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* 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* 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 out_frame = AdaptFrame(frame); - if (out_frame) - sink_->OnFrame(*out_frame); - } + TestVideoCapturer::OnFrame(frame); } } // namespace test diff --git a/test/vcm_capturer.h b/test/vcm_capturer.h index 208a77158a..59000ae4ee 100644 --- a/test/vcm_capturer.h +++ b/test/vcm_capturer.h @@ -11,10 +11,9 @@ #define TEST_VCM_CAPTURER_H_ #include +#include -#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* sink, - const rtc::VideoSinkWants& wants) override; - void RemoveSink(rtc::VideoSinkInterface* 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* sink_ RTC_GUARDED_BY(crit_); rtc::scoped_refptr vcm_; VideoCaptureCapability capability_; };