From 3e1ac5440799686a0bdb5a9e547544b0ae0961bd Mon Sep 17 00:00:00 2001 From: Artem Titov Date: Fri, 24 Apr 2020 10:44:45 +0200 Subject: [PATCH] Refactor video dumping and rendering in PC level test. Move creation of video sinks for dumping and showing rendered video on screen into video quality analyzer injection helper to eliminate need to search for video config in on track callback, which makes this more reliable and reusable. Bug: webrtc:11479 Change-Id: I6bb5409688fd39268f9f97bde4c9b0833a64396b Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/173820 Reviewed-by: Mirko Bonadei Commit-Queue: Artem Titov Cr-Commit-Position: refs/heads/master@{#31128} --- test/pc/e2e/BUILD.gn | 2 +- ...video_quality_analyzer_injection_helper.cc | 120 +++++++++++------- .../video_quality_analyzer_injection_helper.h | 45 +++++-- test/pc/e2e/media/media_helper.cc | 28 +--- test/pc/e2e/media/media_helper.h | 11 -- test/pc/e2e/peer_connection_quality_test.cc | 13 +- 6 files changed, 115 insertions(+), 104 deletions(-) diff --git a/test/pc/e2e/BUILD.gn b/test/pc/e2e/BUILD.gn index 537b6a5d5b..ca26c02e21 100644 --- a/test/pc/e2e/BUILD.gn +++ b/test/pc/e2e/BUILD.gn @@ -193,6 +193,7 @@ if (rtc_include_tests) { "../../../api/video:video_frame", "../../../api/video:video_rtp_headers", "../../../api/video_codecs:video_codecs_api", + "../../../rtc_base:criticalsection", "../../../test:video_test_common", "../../../test:video_test_support", "//third_party/abseil-cpp/absl/memory", @@ -272,7 +273,6 @@ if (rtc_include_tests) { "../..:fileutils", "../..:platform_video_capturer", "../..:video_test_common", - "../..:video_test_support", "../../../api:create_frame_generator", "../../../api:frame_generator_api", "../../../api:peer_connection_quality_test_fixture_api", diff --git a/test/pc/e2e/analyzer/video/video_quality_analyzer_injection_helper.cc b/test/pc/e2e/analyzer/video/video_quality_analyzer_injection_helper.cc index 70dbcd265e..074188439b 100644 --- a/test/pc/e2e/analyzer/video/video_quality_analyzer_injection_helper.cc +++ b/test/pc/e2e/analyzer/video/video_quality_analyzer_injection_helper.cc @@ -70,35 +70,6 @@ class AnalyzingFramePreprocessor sinks_; }; -// Implements the video sink, that forwards rendered frames to the video quality -// analyzer and provided sinks. -class AnalyzingVideoSink final : public rtc::VideoSinkInterface { - public: - AnalyzingVideoSink( - VideoQualityAnalyzerInterface* analyzer, - std::vector>> sinks) - : analyzer_(analyzer), sinks_(std::move(sinks)) { - RTC_DCHECK(analyzer_); - } - ~AnalyzingVideoSink() override = default; - - void OnFrame(const VideoFrame& frame) override { - if (IsDummyFrameBuffer(frame.video_frame_buffer()->ToI420())) { - // This is dummy frame, so we don't need to process it further. - return; - } - analyzer_->OnFrameRendered(frame); - for (auto& sink : sinks_) { - sink->OnFrame(frame); - } - } - - private: - VideoQualityAnalyzerInterface* const analyzer_; - const std::vector>> - sinks_; -}; - } // namespace VideoQualityAnalyzerInjectionHelper::VideoQualityAnalyzerInjectionHelper( @@ -137,9 +108,10 @@ VideoQualityAnalyzerInjectionHelper::WrapVideoDecoderFactory( std::unique_ptr VideoQualityAnalyzerInjectionHelper::CreateFramePreprocessor( - const VideoConfig& config, - test::VideoFrameWriter* writer) const { + const VideoConfig& config) { std::vector>> sinks; + test::VideoFrameWriter* writer = + MaybeCreateVideoWriter(config.input_dump_file_name, config); if (writer) { sinks.push_back(std::make_unique(writer)); } @@ -148,25 +120,17 @@ VideoQualityAnalyzerInjectionHelper::CreateFramePreprocessor( test::VideoRenderer::Create((*config.stream_label + "-capture").c_str(), config.width, config.height))); } + { + rtc::CritScope crit(&lock_); + known_video_configs_.insert({*config.stream_label, config}); + } return std::make_unique( std::move(*config.stream_label), analyzer_.get(), std::move(sinks)); } std::unique_ptr> -VideoQualityAnalyzerInjectionHelper::CreateVideoSink( - const VideoConfig& config, - test::VideoFrameWriter* writer) const { - std::vector>> sinks; - if (writer) { - sinks.push_back(std::make_unique(writer)); - } - if (config.show_on_screen) { - sinks.push_back(absl::WrapUnique( - test::VideoRenderer::Create((*config.stream_label + "-render").c_str(), - config.width, config.height))); - } - return std::make_unique(analyzer_.get(), - std::move(sinks)); +VideoQualityAnalyzerInjectionHelper::CreateVideoSink() { + return std::make_unique(this); } void VideoQualityAnalyzerInjectionHelper::Start(std::string test_case_name, @@ -182,6 +146,72 @@ void VideoQualityAnalyzerInjectionHelper::OnStatsReports( void VideoQualityAnalyzerInjectionHelper::Stop() { analyzer_->Stop(); + for (const auto& video_writer : video_writers_) { + video_writer->Close(); + } + video_writers_.clear(); +} + +test::VideoFrameWriter* +VideoQualityAnalyzerInjectionHelper::MaybeCreateVideoWriter( + absl::optional file_name, + const PeerConnectionE2EQualityTestFixture::VideoConfig& config) { + if (!file_name.has_value()) { + return nullptr; + } + // TODO(titovartem) create only one file writer for simulcast video track. + // For now this code will be invoked for each simulcast stream separately, but + // only one file will be used. + auto video_writer = std::make_unique( + file_name.value(), config.width, config.height, config.fps); + test::VideoFrameWriter* out = video_writer.get(); + video_writers_.push_back(std::move(video_writer)); + return out; +} + +void VideoQualityAnalyzerInjectionHelper::OnFrame(const VideoFrame& frame) { + if (IsDummyFrameBuffer(frame.video_frame_buffer()->ToI420())) { + // This is dummy frame, so we don't need to process it further. + return; + } + analyzer_->OnFrameRendered(frame); + std::string stream_label = analyzer_->GetStreamLabel(frame.id()); + std::vector>>* sinks = + PopulateSinks(stream_label); + if (sinks == nullptr) { + return; + } + for (auto& sink : *sinks) { + sink->OnFrame(frame); + } +} + +std::vector>>* +VideoQualityAnalyzerInjectionHelper::PopulateSinks( + const std::string& stream_label) { + rtc::CritScope crit(&lock_); + auto sinks_it = sinks_.find(stream_label); + if (sinks_it != sinks_.end()) { + return &sinks_it->second; + } + auto it = known_video_configs_.find(stream_label); + RTC_DCHECK(it != known_video_configs_.end()) + << "No video config for stream " << stream_label; + const VideoConfig& config = it->second; + + std::vector>> sinks; + test::VideoFrameWriter* writer = + MaybeCreateVideoWriter(config.output_dump_file_name, config); + if (writer) { + sinks.push_back(std::make_unique(writer)); + } + if (config.show_on_screen) { + sinks.push_back(absl::WrapUnique( + test::VideoRenderer::Create((*config.stream_label + "-render").c_str(), + config.width, config.height))); + } + sinks_.insert({stream_label, std::move(sinks)}); + return &(sinks_.find(stream_label)->second); } } // namespace webrtc_pc_e2e diff --git a/test/pc/e2e/analyzer/video/video_quality_analyzer_injection_helper.h b/test/pc/e2e/analyzer/video/video_quality_analyzer_injection_helper.h index ccda57baaf..a0daa9ff18 100644 --- a/test/pc/e2e/analyzer/video/video_quality_analyzer_injection_helper.h +++ b/test/pc/e2e/analyzer/video/video_quality_analyzer_injection_helper.h @@ -22,6 +22,7 @@ #include "api/video/video_sink_interface.h" #include "api/video_codecs/video_decoder_factory.h" #include "api/video_codecs/video_encoder_factory.h" +#include "rtc_base/critical_section.h" #include "test/pc/e2e/analyzer/video/encoded_image_data_injector.h" #include "test/pc/e2e/analyzer/video/id_generator.h" #include "test/test_video_capturer.h" @@ -55,17 +56,15 @@ class VideoQualityAnalyzerInjectionHelper : public StatsObserverInterface { std::unique_ptr delegate) const; // Creates VideoFrame preprocessor, that will allow video quality analyzer to - // get access to the captured frames. If |writer| in not nullptr, will dump - // captured frames with provided writer. + // get access to the captured frames. If provided config also specifies + // |input_dump_file_name|, video will be written into that file. std::unique_ptr - CreateFramePreprocessor(const VideoConfig& config, - test::VideoFrameWriter* writer) const; + CreateFramePreprocessor(const VideoConfig& config); // Creates sink, that will allow video quality analyzer to get access to - // the rendered frames. If |writer| in not nullptr, will dump rendered - // frames with provided writer. - std::unique_ptr> CreateVideoSink( - const VideoConfig& config, - test::VideoFrameWriter* writer) const; + // the rendered frames. If corresponding video track has + // |output_dump_file_name| in its VideoConfig, then video also will be written + // into that file. + std::unique_ptr> CreateVideoSink(); void Start(std::string test_case_name, int max_threads_count); @@ -75,13 +74,41 @@ class VideoQualityAnalyzerInjectionHelper : public StatsObserverInterface { const StatsReports& stats_reports) override; // Stops VideoQualityAnalyzerInterface to populate final data and metrics. + // Should be invoked after analyzed video tracks are disposed. void Stop(); private: + class AnalyzingVideoSink final : public rtc::VideoSinkInterface { + public: + explicit AnalyzingVideoSink(VideoQualityAnalyzerInjectionHelper* helper) + : helper_(helper) {} + ~AnalyzingVideoSink() override = default; + + void OnFrame(const VideoFrame& frame) override { helper_->OnFrame(frame); } + + private: + VideoQualityAnalyzerInjectionHelper* const helper_; + }; + + test::VideoFrameWriter* MaybeCreateVideoWriter( + absl::optional file_name, + const PeerConnectionE2EQualityTestFixture::VideoConfig& config); + void OnFrame(const VideoFrame& frame); + std::vector>>* + PopulateSinks(const std::string& stream_label); + std::unique_ptr analyzer_; EncodedImageDataInjector* injector_; EncodedImageDataExtractor* extractor_; + std::vector> video_writers_; + + rtc::CriticalSection lock_; + std::map known_video_configs_ RTC_GUARDED_BY(lock_); + std::map>>> + sinks_ RTC_GUARDED_BY(lock_); + std::unique_ptr> encoding_entities_id_generator_; }; diff --git a/test/pc/e2e/media/media_helper.cc b/test/pc/e2e/media/media_helper.cc index e584795a3a..38179d2b40 100644 --- a/test/pc/e2e/media/media_helper.cc +++ b/test/pc/e2e/media/media_helper.cc @@ -9,6 +9,7 @@ */ #include "test/pc/e2e/media/media_helper.h" +#include #include #include "api/test/create_frame_generator.h" @@ -29,13 +30,6 @@ using VideoGeneratorType = ::webrtc::webrtc_pc_e2e:: } // namespace -MediaHelper::~MediaHelper() { - for (const auto& video_writer : video_writers_) { - video_writer->Close(); - } - video_writers_.clear(); -} - void MediaHelper::MaybeAddAudio(TestPeer* peer) { if (!peer->params()->audio_config) { return; @@ -59,12 +53,10 @@ MediaHelper::MaybeAddVideo(TestPeer* peer) { for (size_t i = 0; i < params->video_configs.size(); ++i) { auto video_config = params->video_configs[i]; // Setup input video source into peer connection. - test::VideoFrameWriter* writer = - MaybeCreateVideoWriter(video_config.input_dump_file_name, video_config); std::unique_ptr capturer = CreateVideoCapturer( video_config, peer->ReleaseVideoGenerator(i), video_quality_analyzer_injection_helper_->CreateFramePreprocessor( - video_config, writer)); + video_config)); rtc::scoped_refptr source = new rtc::RefCountedObject( std::move(capturer), @@ -99,22 +91,6 @@ MediaHelper::MaybeAddVideo(TestPeer* peer) { return out; } -test::VideoFrameWriter* MediaHelper::MaybeCreateVideoWriter( - absl::optional file_name, - const VideoConfig& config) { - if (!file_name) { - return nullptr; - } - // TODO(titovartem) create only one file writer for simulcast video track. - // For now this code will be invoked for each simulcast stream separately, but - // only one file will be used. - auto video_writer = std::make_unique( - file_name.value(), config.width, config.height, config.fps); - test::VideoFrameWriter* out = video_writer.get(); - video_writers_.push_back(std::move(video_writer)); - return out; -} - std::unique_ptr MediaHelper::CreateVideoCapturer( const VideoConfig& video_config, std::unique_ptr generator, diff --git a/test/pc/e2e/media/media_helper.h b/test/pc/e2e/media/media_helper.h index 740cd2c00a..e10a9fabd1 100644 --- a/test/pc/e2e/media/media_helper.h +++ b/test/pc/e2e/media/media_helper.h @@ -12,7 +12,6 @@ #define TEST_PC_E2E_MEDIA_MEDIA_HELPER_H_ #include -#include #include #include "api/test/frame_generator_interface.h" @@ -20,7 +19,6 @@ #include "test/pc/e2e/analyzer/video/video_quality_analyzer_injection_helper.h" #include "test/pc/e2e/media/test_video_capturer_video_track_source.h" #include "test/pc/e2e/test_peer.h" -#include "test/testsupport/video_frame_writer.h" namespace webrtc { namespace webrtc_pc_e2e { @@ -34,20 +32,12 @@ class MediaHelper { task_queue_factory_(task_queue_factory), video_quality_analyzer_injection_helper_( video_quality_analyzer_injection_helper) {} - ~MediaHelper(); void MaybeAddAudio(TestPeer* peer); std::vector> MaybeAddVideo(TestPeer* peer); - // Creates a video file writer if |file_name| is not empty. Created writer - // will be owned by MediaHelper and will be closed on MediaHelper destruction. - // If |file_name| is empty will return nullptr. - test::VideoFrameWriter* MaybeCreateVideoWriter( - absl::optional file_name, - const PeerConnectionE2EQualityTestFixture::VideoConfig& config); - private: std::unique_ptr CreateVideoCapturer( const PeerConnectionE2EQualityTestFixture::VideoConfig& video_config, @@ -61,7 +51,6 @@ class MediaHelper { Clock* const clock_; TaskQueueFactory* const task_queue_factory_; VideoQualityAnalyzerInjectionHelper* video_quality_analyzer_injection_helper_; - std::vector> video_writers_; }; } // namespace webrtc_pc_e2e diff --git a/test/pc/e2e/peer_connection_quality_test.cc b/test/pc/e2e/peer_connection_quality_test.cc index d60856a0a9..c5c9388f15 100644 --- a/test/pc/e2e/peer_connection_quality_test.cc +++ b/test/pc/e2e/peer_connection_quality_test.cc @@ -386,22 +386,11 @@ void PeerConnectionE2EQualityTest::OnTrackCallback( return; } - VideoConfig* video_config = nullptr; - for (auto& config : remote_video_configs) { - if (config.stream_label == stream_label) { - video_config = &config; - break; - } - } - RTC_CHECK(video_config); - test::VideoFrameWriter* writer = media_helper_->MaybeCreateVideoWriter( - video_config->output_dump_file_name, *video_config); // It is safe to cast here, because it is checked above that // track->kind() is kVideoKind. auto* video_track = static_cast(track.get()); std::unique_ptr> video_sink = - video_quality_analyzer_injection_helper_->CreateVideoSink(*video_config, - writer); + video_quality_analyzer_injection_helper_->CreateVideoSink(); video_track->AddOrUpdateSink(video_sink.get(), rtc::VideoSinkWants()); output_video_sinks_.push_back(std::move(video_sink)); }