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

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",

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<VideoFrame> {
- public:
-  AnalyzingVideoSink(
-      VideoQualityAnalyzerInterface* analyzer,
-      std::vector<std::unique_ptr<rtc::VideoSinkInterface<VideoFrame>>> 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<std::unique_ptr<rtc::VideoSinkInterface<VideoFrame>>>
-      sinks_;
-};
-
 }  // namespace
 
 VideoQualityAnalyzerInjectionHelper::VideoQualityAnalyzerInjectionHelper(
@@ -137,9 +108,10 @@ VideoQualityAnalyzerInjectionHelper::WrapVideoDecoderFactory(
 
 std::unique_ptr<test::TestVideoCapturer::FramePreprocessor>
 VideoQualityAnalyzerInjectionHelper::CreateFramePreprocessor(
-    const VideoConfig& config,
-    test::VideoFrameWriter* writer) const {
+    const VideoConfig& config) {
   std::vector<std::unique_ptr<rtc::VideoSinkInterface<VideoFrame>>> sinks;
+  test::VideoFrameWriter* writer =
+      MaybeCreateVideoWriter(config.input_dump_file_name, config);
   if (writer) {
     sinks.push_back(std::make_unique<VideoWriter>(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<AnalyzingFramePreprocessor>(
       std::move(*config.stream_label), analyzer_.get(), std::move(sinks));
 }
 
 std::unique_ptr<rtc::VideoSinkInterface<VideoFrame>>
-VideoQualityAnalyzerInjectionHelper::CreateVideoSink(
-    const VideoConfig& config,
-    test::VideoFrameWriter* writer) const {
-  std::vector<std::unique_ptr<rtc::VideoSinkInterface<VideoFrame>>> sinks;
-  if (writer) {
-    sinks.push_back(std::make_unique<VideoWriter>(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<AnalyzingVideoSink>(analyzer_.get(),
-                                              std::move(sinks));
+VideoQualityAnalyzerInjectionHelper::CreateVideoSink() {
+  return std::make_unique<AnalyzingVideoSink>(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<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.
+  auto video_writer = std::make_unique<test::Y4mVideoFrameWriterImpl>(
+      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<std::unique_ptr<rtc::VideoSinkInterface<VideoFrame>>>* sinks =
+      PopulateSinks(stream_label);
+  if (sinks == nullptr) {
+    return;
+  }
+  for (auto& sink : *sinks) {
+    sink->OnFrame(frame);
+  }
+}
+
+std::vector<std::unique_ptr<rtc::VideoSinkInterface<VideoFrame>>>*
+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<std::unique_ptr<rtc::VideoSinkInterface<VideoFrame>>> sinks;
+  test::VideoFrameWriter* writer =
+      MaybeCreateVideoWriter(config.output_dump_file_name, config);
+  if (writer) {
+    sinks.push_back(std::make_unique<VideoWriter>(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

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<VideoDecoderFactory> 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<test::TestVideoCapturer::FramePreprocessor>
-  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<rtc::VideoSinkInterface<VideoFrame>> 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<rtc::VideoSinkInterface<VideoFrame>> 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<VideoFrame> {
+   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<std::string> file_name,
+      const PeerConnectionE2EQualityTestFixture::VideoConfig& config);
+  void OnFrame(const VideoFrame& frame);
+  std::vector<std::unique_ptr<rtc::VideoSinkInterface<VideoFrame>>>*
+  PopulateSinks(const std::string& stream_label);
+
   std::unique_ptr<VideoQualityAnalyzerInterface> analyzer_;
   EncodedImageDataInjector* injector_;
   EncodedImageDataExtractor* extractor_;
 
+  std::vector<std::unique_ptr<test::VideoFrameWriter>> video_writers_;
+
+  rtc::CriticalSection lock_;
+  std::map<std::string, VideoConfig> known_video_configs_ RTC_GUARDED_BY(lock_);
+  std::map<std::string,
+           std::vector<std::unique_ptr<rtc::VideoSinkInterface<VideoFrame>>>>
+      sinks_ RTC_GUARDED_BY(lock_);
+
   std::unique_ptr<IdGenerator<int>> encoding_entities_id_generator_;
 };
 

test/pc/e2e/media/media_helper.cc

@@ -9,6 +9,7 @@
  */
 #include "test/pc/e2e/media/media_helper.h"
 
+#include <string>
 #include <utility>
 
 #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<test::TestVideoCapturer> capturer = CreateVideoCapturer(
         video_config, peer->ReleaseVideoGenerator(i),
         video_quality_analyzer_injection_helper_->CreateFramePreprocessor(
-            video_config, writer));
+            video_config));
     rtc::scoped_refptr<TestVideoCapturerVideoTrackSource> source =
         new rtc::RefCountedObject<TestVideoCapturerVideoTrackSource>(
             std::move(capturer),
@@ -99,22 +91,6 @@ MediaHelper::MaybeAddVideo(TestPeer* peer) {
   return out;
 }
 
-test::VideoFrameWriter* MediaHelper::MaybeCreateVideoWriter(
-    absl::optional<std::string> 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<test::Y4mVideoFrameWriterImpl>(
-      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<test::TestVideoCapturer> MediaHelper::CreateVideoCapturer(
     const VideoConfig& video_config,
     std::unique_ptr<test::FrameGeneratorInterface> generator,

test/pc/e2e/media/media_helper.h

@@ -12,7 +12,6 @@
 #define TEST_PC_E2E_MEDIA_MEDIA_HELPER_H_
 
 #include <memory>
-#include <string>
 #include <vector>
 
 #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<rtc::scoped_refptr<TestVideoCapturerVideoTrackSource>>
   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<std::string> file_name,
-      const PeerConnectionE2EQualityTestFixture::VideoConfig& config);
-
  private:
   std::unique_ptr<test::TestVideoCapturer> 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<std::unique_ptr<test::VideoFrameWriter>> video_writers_;
 };
 
 }  // namespace webrtc_pc_e2e

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<VideoTrackInterface*>(track.get());
   std::unique_ptr<rtc::VideoSinkInterface<VideoFrame>> 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));
 }