Ensure TestPeers are destroyed at the end of Run.

In order to correctly close audio dump files, TestPeers have to be
destroyed after the call is finished.

Bug: webrtc:10138
Change-Id: I948e4e1844dfbffd1eef7926a4dd4d7631dbe632
Reviewed-on: https://webrtc-review.googlesource.com/c/122301
Reviewed-by: Artem Titov <titovartem@webrtc.org>
Commit-Queue: Mirko Bonadei <mbonadei@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#26661}

test/pc/e2e/api/create_peerconnection_quality_test_fixture.cc

@@ -19,16 +19,9 @@ namespace webrtc {
 
 std::unique_ptr<PeerConnectionE2EQualityTestFixture>
 CreatePeerConnectionE2EQualityTestFixture(
-    std::unique_ptr<PeerConnectionE2EQualityTestFixture::InjectableComponents>
-        alice_components,
-    std::unique_ptr<PeerConnectionE2EQualityTestFixture::Params> alice_params,
-    std::unique_ptr<PeerConnectionE2EQualityTestFixture::InjectableComponents>
-        bob_components,
-    std::unique_ptr<PeerConnectionE2EQualityTestFixture::Params> bob_params,
     std::unique_ptr<PeerConnectionE2EQualityTestFixture::Analyzers> analyzers) {
   return absl::make_unique<webrtc::test::PeerConnectionE2EQualityTest>(
-      std::move(alice_components), std::move(alice_params),
+      std::move(analyzers));
-      std::move(bob_components), std::move(bob_params), std::move(analyzers));
 }
 
 }  // namespace webrtc

test/pc/e2e/api/create_peerconnection_quality_test_fixture.h

@@ -21,12 +21,6 @@ namespace webrtc {
 // During the test Alice will be caller and Bob will answer the call.
 std::unique_ptr<PeerConnectionE2EQualityTestFixture>
 CreatePeerConnectionE2EQualityTestFixture(
-    std::unique_ptr<PeerConnectionE2EQualityTestFixture::InjectableComponents>
-        alice_components,
-    std::unique_ptr<PeerConnectionE2EQualityTestFixture::Params> alice_params,
-    std::unique_ptr<PeerConnectionE2EQualityTestFixture::InjectableComponents>
-        bob_components,
-    std::unique_ptr<PeerConnectionE2EQualityTestFixture::Params> bob_params,
     std::unique_ptr<PeerConnectionE2EQualityTestFixture::Analyzers> analyzers);
 
 }  // namespace webrtc

test/pc/e2e/api/peerconnection_quality_test_fixture.h

@@ -182,7 +182,11 @@ class PeerConnectionE2EQualityTestFixture {
     TimeDelta run_duration;
   };
 
-  virtual void Run(RunParams run_params) = 0;
+  virtual void Run(std::unique_ptr<InjectableComponents> alice_components,
+                   std::unique_ptr<Params> alice_params,
+                   std::unique_ptr<InjectableComponents> bob_components,
+                   std::unique_ptr<Params> bob_params,
+                   RunParams run_params) = 0;
   virtual ~PeerConnectionE2EQualityTestFixture() = default;
 };
 

test/pc/e2e/peer_connection_e2e_smoke_test.cc

@@ -105,11 +105,11 @@ TEST(PeerConnectionE2EQualityTestSmokeTest, RunWithEmulatedNetwork) {
   auto* video_analyzer = static_cast<ExampleVideoQualityAnalyzer*>(
       analyzers->video_quality_analyzer.get());
 
-  auto fixture = CreatePeerConnectionE2EQualityTestFixture(
+  auto fixture =
-      std::move(alice_components), std::move(alice_params),
+      CreatePeerConnectionE2EQualityTestFixture(std::move(analyzers));
+  fixture->Run(std::move(alice_components), std::move(alice_params),
                std::move(bob_components), absl::make_unique<Params>(),
-      std::move(analyzers));
+               RunParams{TimeDelta::seconds(5)});
-  fixture->Run(RunParams{TimeDelta::seconds(5)});
 
   RTC_LOG(INFO) << "Captured: " << video_analyzer->frames_captured();
   RTC_LOG(INFO) << "Sent    : " << video_analyzer->frames_sent();

test/pc/e2e/peer_connection_quality_test.cc

@@ -52,37 +52,10 @@ std::string VideoConfigSourcePresenceToString(const VideoConfig& video_config) {
 }  // namespace
 
 PeerConnectionE2EQualityTest::PeerConnectionE2EQualityTest(
-    std::unique_ptr<InjectableComponents> alice_components,
-    std::unique_ptr<Params> alice_params,
-    std::unique_ptr<InjectableComponents> bob_components,
-    std::unique_ptr<Params> bob_params,
     std::unique_ptr<Analyzers> analyzers)
-    : clock_(Clock::GetRealTimeClock()),
+    : clock_(Clock::GetRealTimeClock()) {
-      signaling_thread_(rtc::Thread::Create()) {
-  RTC_CHECK(alice_components);
-  RTC_CHECK(alice_params);
-  RTC_CHECK(bob_components);
-  RTC_CHECK(bob_params);
   RTC_CHECK(analyzers);
 
-  // Print test summary
-  RTC_LOG(INFO)
-      << "Media quality test: Alice will make a call to Bob with media video="
-      << !alice_params->video_configs.empty()
-      << "; audio=" << alice_params->audio_config.has_value()
-      << ". Bob will respond with media video="
-      << !bob_params->video_configs.empty()
-      << "; audio=" << bob_params->audio_config.has_value();
-
-  // Check that at least Alice or Bob has at least one media stream.
-  RTC_CHECK(!alice_params->video_configs.empty() ||
-            alice_params->audio_config || !bob_params->video_configs.empty() ||
-            bob_params->audio_config)
-      << "No media in the call";
-
-  signaling_thread_->SetName(kSignalThreadName, nullptr);
-  signaling_thread_->Start();
-
   // Create default video quality analyzer. We will always create an analyzer,
   // even if there are no video streams, because it will be installed into video
   // encoder/decoder factories.
@@ -97,6 +70,34 @@ PeerConnectionE2EQualityTest::PeerConnectionE2EQualityTest(
           std::move(analyzers->video_quality_analyzer),
           encoded_image_id_controller_.get(),
           encoded_image_id_controller_.get());
+}
+
+void PeerConnectionE2EQualityTest::Run(
+    std::unique_ptr<InjectableComponents> alice_components,
+    std::unique_ptr<Params> alice_params,
+    std::unique_ptr<InjectableComponents> bob_components,
+    std::unique_ptr<Params> bob_params,
+    RunParams run_params) {
+  RTC_CHECK(alice_components);
+  RTC_CHECK(alice_params);
+  RTC_CHECK(bob_components);
+  RTC_CHECK(bob_params);
+
+  SetMissedVideoStreamLabels({alice_params.get(), bob_params.get()});
+  ValidateParams({alice_params.get(), bob_params.get()});
+
+  // Print test summary
+  RTC_LOG(INFO)
+      << "Media quality test: Alice will make a call to Bob with media video="
+      << !alice_params->video_configs.empty()
+      << "; audio=" << alice_params->audio_config.has_value()
+      << ". Bob will respond with media video="
+      << !bob_params->video_configs.empty()
+      << "; audio=" << bob_params->audio_config.has_value();
+
+  const std::unique_ptr<rtc::Thread> signaling_thread = rtc::Thread::Create();
+  signaling_thread->SetName(kSignalThreadName, nullptr);
+  signaling_thread->Start();
 
   // Create call participants: Alice and Bob.
   // Audio streams are intercepted in AudioDeviceModule, so if it is required to
@@ -111,17 +112,12 @@ PeerConnectionE2EQualityTest::PeerConnectionE2EQualityTest(
           : absl::nullopt;
   alice_ = TestPeer::CreateTestPeer(
       std::move(alice_components), std::move(alice_params),
-      video_quality_analyzer_injection_helper_.get(), signaling_thread_.get(),
+      video_quality_analyzer_injection_helper_.get(), signaling_thread.get(),
       alice_audio_output_dump_file_name);
   bob_ = TestPeer::CreateTestPeer(
       std::move(bob_components), std::move(bob_params),
-      video_quality_analyzer_injection_helper_.get(), signaling_thread_.get(),
+      video_quality_analyzer_injection_helper_.get(), signaling_thread.get(),
       bob_audio_output_dump_file_name);
-}
-
-void PeerConnectionE2EQualityTest::Run(RunParams run_params) {
-  SetMissedVideoStreamLabels({alice_->params(), bob_->params()});
-  ValidateParams({alice_->params(), bob_->params()});
 
   int num_cores = CpuInfo::DetectNumberOfCores();
   RTC_DCHECK_GE(num_cores, 1);
@@ -136,11 +132,20 @@ void PeerConnectionE2EQualityTest::Run(RunParams run_params) {
   RTC_LOG(INFO) << "video_analyzer_threads=" << video_analyzer_threads;
 
   video_quality_analyzer_injection_helper_->Start(video_analyzer_threads);
-  signaling_thread_->Invoke<void>(
+  signaling_thread->Invoke<void>(
       RTC_FROM_HERE,
       rtc::Bind(&PeerConnectionE2EQualityTest::RunOnSignalingThread, this,
                 run_params));
   video_quality_analyzer_injection_helper_->Stop();
+
+  // Ensuring that TestPeers have been destroyed in order to correctly close
+  // Audio dumps.
+  RTC_CHECK(!alice_);
+  RTC_CHECK(!bob_);
+  // Ensuring that FrameGeneratorCapturerVideoTrackSource and VideoFrameWriter
+  // are destroyed on the right thread.
+  RTC_CHECK(video_sources_.empty());
+  RTC_CHECK(video_writers_.empty());
 }
 
 void PeerConnectionE2EQualityTest::SetMissedVideoStreamLabels(
@@ -163,7 +168,14 @@ void PeerConnectionE2EQualityTest::SetMissedVideoStreamLabels(
 
 void PeerConnectionE2EQualityTest::ValidateParams(std::vector<Params*> params) {
   std::set<std::string> video_labels;
+  int media_streams_count = 0;
+
   for (Params* p : params) {
+    if (p->audio_config) {
+      media_streams_count++;
+    }
+    media_streams_count += p->video_configs.size();
+
     // Validate that each video config has exactly one of |generator|,
     // |input_file_name| or |screen_share_config| set. Also validate that all
     // video stream labels are unique.
@@ -195,13 +207,15 @@ void PeerConnectionE2EQualityTest::ValidateParams(std::vector<Params*> params) {
       }
     }
   }
+
+  RTC_CHECK_GT(media_streams_count, 0) << "No media in the call.";
 }
 
 void PeerConnectionE2EQualityTest::RunOnSignalingThread(RunParams run_params) {
   AddMedia(alice_.get());
   AddMedia(bob_.get());
 
-  SetupCall(alice_.get(), bob_.get());
+  SetupCall();
 
   WaitForTransceiversSetup(alice_->params(), bob_.get());
   WaitForTransceiversSetup(bob_->params(), alice_.get());
@@ -301,21 +315,21 @@ void PeerConnectionE2EQualityTest::AddAudio(TestPeer* peer) {
   peer->AddTransceiver(track);
 }
 
-void PeerConnectionE2EQualityTest::SetupCall(TestPeer* alice, TestPeer* bob) {
+void PeerConnectionE2EQualityTest::SetupCall() {
   // Connect peers.
-  ASSERT_TRUE(alice->ExchangeOfferAnswerWith(bob));
+  ASSERT_TRUE(alice_->ExchangeOfferAnswerWith(bob_.get()));
   // Do the SDP negotiation, and also exchange ice candidates.
-  ASSERT_EQ_WAIT(alice->signaling_state(), PeerConnectionInterface::kStable,
+  ASSERT_EQ_WAIT(alice_->signaling_state(), PeerConnectionInterface::kStable,
                  kDefaultTimeoutMs);
-  ASSERT_TRUE_WAIT(alice->IsIceGatheringDone(), kDefaultTimeoutMs);
+  ASSERT_TRUE_WAIT(alice_->IsIceGatheringDone(), kDefaultTimeoutMs);
-  ASSERT_TRUE_WAIT(bob->IsIceGatheringDone(), kDefaultTimeoutMs);
+  ASSERT_TRUE_WAIT(bob_->IsIceGatheringDone(), kDefaultTimeoutMs);
 
   // Connect an ICE candidate pairs.
-  ASSERT_TRUE(bob->AddIceCandidates(alice->observer()->GetAllCandidates()));
+  ASSERT_TRUE(bob_->AddIceCandidates(alice_->observer()->GetAllCandidates()));
-  ASSERT_TRUE(alice->AddIceCandidates(bob->observer()->GetAllCandidates()));
+  ASSERT_TRUE(alice_->AddIceCandidates(bob_->observer()->GetAllCandidates()));
   // This means that ICE and DTLS are connected.
-  ASSERT_TRUE_WAIT(bob->IsIceConnected(), kDefaultTimeoutMs);
+  ASSERT_TRUE_WAIT(bob_->IsIceConnected(), kDefaultTimeoutMs);
-  ASSERT_TRUE_WAIT(alice->IsIceConnected(), kDefaultTimeoutMs);
+  ASSERT_TRUE_WAIT(alice_->IsIceConnected(), kDefaultTimeoutMs);
 }
 
 void PeerConnectionE2EQualityTest::WaitForTransceiversSetup(
@@ -379,6 +393,11 @@ void PeerConnectionE2EQualityTest::TearDownCall() {
   for (const auto& video_writer : video_writers_) {
     video_writer->Close();
   }
+
+  video_sources_.clear();
+  video_writers_.clear();
+  alice_.reset();
+  bob_.reset();
 }
 
 VideoFrameWriter* PeerConnectionE2EQualityTest::MaybeCreateVideoWriter(

test/pc/e2e/peer_connection_quality_test.h

@@ -38,16 +38,15 @@ class PeerConnectionE2EQualityTest
   using RunParams = PeerConnectionE2EQualityTestFixture::RunParams;
   using VideoConfig = PeerConnectionE2EQualityTestFixture::VideoConfig;
 
-  PeerConnectionE2EQualityTest(
+  PeerConnectionE2EQualityTest(std::unique_ptr<Analyzers> analyzers);
-      std::unique_ptr<InjectableComponents> alice_components,
-      std::unique_ptr<Params> alice_params,
-      std::unique_ptr<InjectableComponents> bob_components,
-      std::unique_ptr<Params> bob_params,
-      std::unique_ptr<Analyzers> analyzers);
 
   ~PeerConnectionE2EQualityTest() override = default;
 
-  void Run(RunParams run_params) override;
+  void Run(std::unique_ptr<InjectableComponents> alice_components,
+           std::unique_ptr<Params> alice_params,
+           std::unique_ptr<InjectableComponents> bob_components,
+           std::unique_ptr<Params> bob_params,
+           RunParams run_params) override;
 
  private:
   // Sets video stream labels that are not specified in VideoConfigs to unique
@@ -63,7 +62,7 @@ class PeerConnectionE2EQualityTest
   std::unique_ptr<FrameGenerator> CreateFrameGenerator(
       const VideoConfig& video_config);
   void AddAudio(TestPeer* peer);
-  void SetupCall(TestPeer* alice, TestPeer* bob);
+  void SetupCall();
   void WaitForTransceiversSetup(Params* params, TestPeer* remote_peer);
   void SetupVideoSink(Params* params, TestPeer* remote_peer);
   void StartVideo();
@@ -77,7 +76,6 @@ class PeerConnectionE2EQualityTest
       video_quality_analyzer_injection_helper_;
   std::unique_ptr<SingleProcessEncodedImageIdInjector>
       encoded_image_id_controller_;
-  const std::unique_ptr<rtc::Thread> signaling_thread_;
 
   std::unique_ptr<TestPeer> alice_;
   std::unique_ptr<TestPeer> bob_;