[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 <mbonadei@webrtc.org>
Commit-Queue: Artem Titov <titovartem@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#38163}

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<int> 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<std::string> 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<VideoDumpOptions> 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<std::string> 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.

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<VideoFrame> {
- 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<std::unique_ptr<rtc::VideoSinkInterface<VideoFrame>>> sinks;
   test::VideoFrameWriter* writer = nullptr;
   if (config.input_dump_options.has_value()) {
-    // Using new API for video dumping.
+    writer = CreateVideoWriter(
-    writer = MaybeCreateVideoWriter(
         config.input_dump_options->GetInputDumpFileName(*config.stream_label),
         config);
-    RTC_CHECK(writer);
+    VideoFrameIdsWriter* frame_ids_writer = MaybeCreateVideoFrameIdsWriter(
-    VideoFrameIdsWriter* frame_ids_writer = nullptr;
+        config.input_dump_options->GetInputFrameIdsDumpFileName(
-    if (config.input_dump_options->export_frame_ids()) {
+            *config.stream_label));
-      frame_ids_writers_.push_back(std::make_unique<VideoFrameIdsWriter>(
+    sinks.push_back(std::make_unique<VideoWriter>(
-          *config.input_dump_options->GetInputFrameIdsDumpFileName(
-              *config.stream_label)));
-      frame_ids_writer = frame_ids_writers_.back().get();
-    }
-    sinks.push_back(std::make_unique<VideoWriter2>(
         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<VideoWriter>(
-          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*
+test::VideoFrameWriter* VideoQualityAnalyzerInjectionHelper::CreateVideoWriter(
-VideoQualityAnalyzerInjectionHelper::MaybeCreateVideoWriter(
+    absl::string_view file_name,
-    absl::optional<std::string> 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<test::VideoFrameWriter> video_writer =
       std::make_unique<test::Y4mVideoFrameWriterImpl>(
-          *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<test::FixedFpsVideoFrameWriterAdapter>(
         config.fps, clock_, std::move(video_writer));
@@ -278,6 +239,17 @@ VideoQualityAnalyzerInjectionHelper::MaybeCreateVideoWriter(
   return out;
 }
 
+VideoQualityAnalyzerInjectionHelper::VideoFrameIdsWriter*
+VideoQualityAnalyzerInjectionHelper::MaybeCreateVideoFrameIdsWriter(
+    absl::optional<std::string> frame_ids_dump_file_name) {
+  if (!frame_ids_dump_file_name.has_value()) {
+    return nullptr;
+  }
+  frame_ids_writers_.push_back(
+      std::make_unique<VideoFrameIdsWriter>(*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<I420BufferInterface> i420_buffer =
@@ -321,37 +293,16 @@ VideoQualityAnalyzerInjectionHelper::PopulateSinks(
   std::vector<std::unique_ptr<rtc::VideoSinkInterface<VideoFrame>>> sinks;
   test::VideoFrameWriter* writer = nullptr;
   if (config.output_dump_options.has_value()) {
-    // Using new API with output directory.
+    writer = CreateVideoWriter(
-    writer = MaybeCreateVideoWriter(
         config.output_dump_options->GetOutputDumpFileName(
             receiver_stream.stream_label, receiver_stream.peer_name),
         config);
-    RTC_CHECK(writer);
+    VideoFrameIdsWriter* frame_ids_writer = MaybeCreateVideoFrameIdsWriter(
-    VideoFrameIdsWriter* frame_ids_writer = nullptr;
+        config.output_dump_options->GetOutputFrameIdsDumpFileName(
-    if (config.output_dump_options->export_frame_ids()) {
+            receiver_stream.stream_label, receiver_stream.peer_name));
-      frame_ids_writers_.push_back(std::make_unique<VideoFrameIdsWriter>(
+    sinks.push_back(std::make_unique<VideoWriter>(
-          *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<VideoWriter2>(
         writer, frame_ids_writer,
         config.output_dump_options->sampling_modulo()));
-  } else {
-    // Using old API. To be removed.
-    absl::optional<std::string> 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<VideoWriter>(
-          writer, config.output_dump_sampling_modulo));
-    }
   }
   if (config.show_on_screen) {
     sinks.push_back(absl::WrapUnique(

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<VideoFrame> {
+  class VideoWriter final : public rtc::VideoSinkInterface<VideoFrame> {
    public:
-    VideoWriter2(test::VideoFrameWriter* video_writer,
+    VideoWriter(test::VideoFrameWriter* video_writer,
-                 VideoFrameIdsWriter* frame_ids_writer,
+                VideoFrameIdsWriter* frame_ids_writer,
-                 int sampling_modulo);
+                int sampling_modulo);
-    ~VideoWriter2() override = default;
+    ~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(
+  test::VideoFrameWriter* CreateVideoWriter(
-      absl::optional<std::string> file_name,
+      absl::string_view file_name,
       const PeerConnectionE2EQualityTestFixture::VideoConfig& config);
+  VideoFrameIdsWriter* MaybeCreateVideoFrameIdsWriter(
+      absl::optional<std::string> 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);

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()) {