From f68a06c34b79c01137d4f1178627e6af853bbd5f Mon Sep 17 00:00:00 2001 From: Artem Titov Date: Wed, 21 Sep 2022 16:04:29 +0200 Subject: [PATCH] [PCLF] Cleanup old video dumping API Bug: b/240540206 Change-Id: I1184f3f73a6de430e7103783b8959d8ff222e31e Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/270485 Reviewed-by: Mirko Bonadei Commit-Queue: Artem Titov Cr-Commit-Position: refs/heads/main@{#38163} --- .../peerconnection_quality_test_fixture.h | 43 -------- ...video_quality_analyzer_injection_helper.cc | 101 +++++------------- .../video_quality_analyzer_injection_helper.h | 16 +-- test/pc/e2e/peer_configurer.cc | 9 -- 4 files changed, 35 insertions(+), 134 deletions(-) diff --git a/api/test/peerconnection_quality_test_fixture.h b/api/test/peerconnection_quality_test_fixture.h index b60c3b4285..028848b3b8 100644 --- a/api/test/peerconnection_quality_test_fixture.h +++ b/api/test/peerconnection_quality_test_fixture.h @@ -320,54 +320,11 @@ class PeerConnectionE2EQualityTestFixture { // each RtpEncodingParameters of RtpParameters of corresponding // RtpSenderInterface for this video stream. absl::optional temporal_layers_count; - // DEPRECATED: use input_dump_options instead. If specified the input stream - // will be also copied to specified file. It is actually one of the test's - // output file, which contains copy of what was captured during the test for - // this video stream on sender side. It is useful when generator is used as - // input. - absl::optional input_dump_file_name; - // DEPRECATED: use input_dump_options instead. Used only if - // `input_dump_file_name` is set. Specifies the module for the video frames - // to be dumped. Modulo equals X means every Xth frame will be written to - // the dump file. The value must be greater than 0. - int input_dump_sampling_modulo = 1; // If specified defines how input should be dumped. It is actually one of // the test's output file, which contains copy of what was captured during // the test for this video stream on sender side. It is useful when // generator is used as input. absl::optional input_dump_options; - // DEPRECATED: use output_dump_options instead. If specified this file will - // be used as output on the receiver side for this stream. - // - // If multiple output streams will be produced by this stream (e.g. when the - // stream represented by this `VideoConfig` is received by more than one - // peer), output files will be appended with receiver names. If the second - // and other receivers will be added in the middle of the call after the - // first frame for this stream has been already written to the output file, - // then only dumps for newly added peers will be appended with receiver - // name, the dump for the first receiver will have name equal to the - // specified one. For example: - // * If we have peers A and B and A has `VideoConfig` V_a with - // V_a.output_dump_file_name = "/foo/a_output.yuv", then the stream - // related to V_a will be written into "/foo/a_output.yuv". - // * If we have peers A, B and C and A has `VideoConfig` V_a with - // V_a.output_dump_file_name = "/foo/a_output.yuv", then the stream - // related to V_a will be written for peer B into "/foo/a_output.yuv.B" - // and for peer C into "/foo/a_output.yuv.C" - // * If we have peers A and B and A has `VideoConfig` V_a with - // V_a.output_dump_file_name = "/foo/a_output.yuv", then if after B - // received the first frame related to V_a peer C joined the call, then - // the stream related to V_a will be written for peer B into - // "/foo/a_output.yuv" and for peer C into "/foo/a_output.yuv.C" - // - // The produced files contains what was rendered for this video stream on - // receiver side. - absl::optional output_dump_file_name; - // DEPRECATED: use output_dump_options instead. Used only if - // `output_dump_file_name` is set. Specifies the module for the video frames - // to be dumped. Modulo equals X means every Xth frame will be written to - // the dump file. The value must be greater than 0. - int output_dump_sampling_modulo = 1; // If specified defines how output should be dumped on the receiver side for // this stream. The produced files contain what was rendered for this video // stream on receiver side per each receiver. 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 661208e888..8cc64fdce2 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 @@ -28,30 +28,8 @@ namespace webrtc { namespace webrtc_pc_e2e { - namespace { -class VideoWriter final : public rtc::VideoSinkInterface { - public: - VideoWriter(test::VideoFrameWriter* video_writer, int sampling_modulo) - : video_writer_(video_writer), sampling_modulo_(sampling_modulo) {} - ~VideoWriter() override = default; - - void OnFrame(const VideoFrame& frame) override { - if (frames_counter_++ % sampling_modulo_ != 0) { - return; - } - bool result = video_writer_->WriteFrame(frame); - RTC_CHECK(result) << "Failed to write frame"; - } - - private: - test::VideoFrameWriter* const video_writer_; - const int sampling_modulo_; - - int64_t frames_counter_ = 0; -}; - class AnalyzingFramePreprocessor : public test::TestVideoCapturer::FramePreprocessor { public: @@ -109,7 +87,7 @@ void VideoQualityAnalyzerInjectionHelper::VideoFrameIdsWriter::WriteFrameId( << "Failed to write frame id to the output file: " << file_name_; } -VideoQualityAnalyzerInjectionHelper::VideoWriter2::VideoWriter2( +VideoQualityAnalyzerInjectionHelper::VideoWriter::VideoWriter( test::VideoFrameWriter* video_writer, VideoFrameIdsWriter* frame_ids_writer, int sampling_modulo) @@ -117,7 +95,7 @@ VideoQualityAnalyzerInjectionHelper::VideoWriter2::VideoWriter2( frame_ids_writer_(frame_ids_writer), sampling_modulo_(sampling_modulo) {} -void VideoQualityAnalyzerInjectionHelper::VideoWriter2::OnFrame( +void VideoQualityAnalyzerInjectionHelper::VideoWriter::OnFrame( const VideoFrame& frame) { if (frames_counter_++ % sampling_modulo_ != 0) { return; @@ -172,28 +150,15 @@ VideoQualityAnalyzerInjectionHelper::CreateFramePreprocessor( std::vector>> sinks; test::VideoFrameWriter* writer = nullptr; if (config.input_dump_options.has_value()) { - // Using new API for video dumping. - writer = MaybeCreateVideoWriter( + writer = CreateVideoWriter( config.input_dump_options->GetInputDumpFileName(*config.stream_label), config); - RTC_CHECK(writer); - VideoFrameIdsWriter* frame_ids_writer = nullptr; - if (config.input_dump_options->export_frame_ids()) { - frame_ids_writers_.push_back(std::make_unique( - *config.input_dump_options->GetInputFrameIdsDumpFileName( - *config.stream_label))); - frame_ids_writer = frame_ids_writers_.back().get(); - } - sinks.push_back(std::make_unique( + VideoFrameIdsWriter* frame_ids_writer = MaybeCreateVideoFrameIdsWriter( + config.input_dump_options->GetInputFrameIdsDumpFileName( + *config.stream_label)); + sinks.push_back(std::make_unique( writer, frame_ids_writer, config.input_dump_options->sampling_modulo())); - } else { - // Using old API. To be removed. - writer = MaybeCreateVideoWriter(config.input_dump_file_name, config); - if (writer) { - sinks.push_back(std::make_unique( - writer, config.input_dump_sampling_modulo)); - } } if (config.show_on_screen) { sinks.push_back(absl::WrapUnique( @@ -256,19 +221,15 @@ void VideoQualityAnalyzerInjectionHelper::Stop() { frame_ids_writers_.clear(); } -test::VideoFrameWriter* -VideoQualityAnalyzerInjectionHelper::MaybeCreateVideoWriter( - absl::optional file_name, +test::VideoFrameWriter* VideoQualityAnalyzerInjectionHelper::CreateVideoWriter( + absl::string_view 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. std::unique_ptr video_writer = std::make_unique( - *file_name, config.width, config.height, config.fps); + std::string(file_name), config.width, config.height, config.fps); if (config.output_dump_use_fixed_framerate) { video_writer = std::make_unique( config.fps, clock_, std::move(video_writer)); @@ -278,6 +239,17 @@ VideoQualityAnalyzerInjectionHelper::MaybeCreateVideoWriter( return out; } +VideoQualityAnalyzerInjectionHelper::VideoFrameIdsWriter* +VideoQualityAnalyzerInjectionHelper::MaybeCreateVideoFrameIdsWriter( + absl::optional frame_ids_dump_file_name) { + if (!frame_ids_dump_file_name.has_value()) { + return nullptr; + } + frame_ids_writers_.push_back( + std::make_unique(*frame_ids_dump_file_name)); + return frame_ids_writers_.back().get(); +} + void VideoQualityAnalyzerInjectionHelper::OnFrame(absl::string_view peer_name, const VideoFrame& frame) { rtc::scoped_refptr i420_buffer = @@ -321,37 +293,16 @@ VideoQualityAnalyzerInjectionHelper::PopulateSinks( std::vector>> sinks; test::VideoFrameWriter* writer = nullptr; if (config.output_dump_options.has_value()) { - // Using new API with output directory. - writer = MaybeCreateVideoWriter( + writer = CreateVideoWriter( config.output_dump_options->GetOutputDumpFileName( receiver_stream.stream_label, receiver_stream.peer_name), config); - RTC_CHECK(writer); - VideoFrameIdsWriter* frame_ids_writer = nullptr; - if (config.output_dump_options->export_frame_ids()) { - frame_ids_writers_.push_back(std::make_unique( - *config.output_dump_options->GetOutputFrameIdsDumpFileName( - receiver_stream.stream_label, receiver_stream.peer_name))); - frame_ids_writer = frame_ids_writers_.back().get(); - } - sinks.push_back(std::make_unique( + VideoFrameIdsWriter* frame_ids_writer = MaybeCreateVideoFrameIdsWriter( + config.output_dump_options->GetOutputFrameIdsDumpFileName( + receiver_stream.stream_label, receiver_stream.peer_name)); + sinks.push_back(std::make_unique( writer, frame_ids_writer, config.output_dump_options->sampling_modulo())); - } else { - // Using old API. To be removed. - absl::optional output_dump_file_name = - config.output_dump_file_name; - if (output_dump_file_name.has_value() && peers_count_ > 2) { - // TODO(titovartem): make this default behavior for any amount of peers. - rtc::StringBuilder builder(*output_dump_file_name); - builder << "." << receiver_stream.peer_name; - output_dump_file_name = builder.str(); - } - writer = MaybeCreateVideoWriter(output_dump_file_name, config); - if (writer) { - sinks.push_back(std::make_unique( - writer, config.output_dump_sampling_modulo)); - } } if (config.show_on_screen) { sinks.push_back(absl::WrapUnique( 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 76b8b446bc..9108488a71 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 @@ -145,12 +145,12 @@ class VideoQualityAnalyzerInjectionHelper : public StatsObserverInterface { FILE* output_file_; }; - class VideoWriter2 final : public rtc::VideoSinkInterface { + class VideoWriter final : public rtc::VideoSinkInterface { public: - VideoWriter2(test::VideoFrameWriter* video_writer, - VideoFrameIdsWriter* frame_ids_writer, - int sampling_modulo); - ~VideoWriter2() override = default; + VideoWriter(test::VideoFrameWriter* video_writer, + VideoFrameIdsWriter* frame_ids_writer, + int sampling_modulo); + ~VideoWriter() override = default; void OnFrame(const VideoFrame& frame) override; @@ -162,9 +162,11 @@ class VideoQualityAnalyzerInjectionHelper : public StatsObserverInterface { int64_t frames_counter_ = 0; }; - test::VideoFrameWriter* MaybeCreateVideoWriter( - absl::optional file_name, + test::VideoFrameWriter* CreateVideoWriter( + absl::string_view file_name, const PeerConnectionE2EQualityTestFixture::VideoConfig& config); + VideoFrameIdsWriter* MaybeCreateVideoFrameIdsWriter( + absl::optional frame_ids_dump_file_name); // Creates a deep copy of the frame and passes it to the video analyzer, while // passing real frame to the sinks void OnFrame(absl::string_view peer_name, const VideoFrame& frame); diff --git a/test/pc/e2e/peer_configurer.cc b/test/pc/e2e/peer_configurer.cc index 998f789cc1..4571d350ae 100644 --- a/test/pc/e2e/peer_configurer.cc +++ b/test/pc/e2e/peer_configurer.cc @@ -109,15 +109,6 @@ void PeerParamsPreprocessor::ValidateParams(const PeerConfigurerImpl& peer) { RTC_CHECK(inserted) << "Duplicate video_config.stream_label=" << video_config.stream_label.value(); - if (video_config.input_dump_file_name.has_value()) { - RTC_CHECK_GT(video_config.input_dump_sampling_modulo, 0) - << "video_config.input_dump_sampling_modulo must be greater than 0"; - } - if (video_config.output_dump_file_name.has_value()) { - RTC_CHECK_GT(video_config.output_dump_sampling_modulo, 0) - << "video_config.input_dump_sampling_modulo must be greater than 0"; - } - // TODO(bugs.webrtc.org/4762): remove this check after synchronization of // more than two streams is supported. if (video_config.sync_group.has_value()) {