commit 46536e3da48a166935ef5e3caba3716af6e06ff8
author Artem Titov <titovartem@webrtc.org> Thu Oct 14 13:17:48 2021
committer WebRTC LUCI CQ <webrtc-scoped@luci-project-accounts.iam.gserviceaccount.com> Thu Oct 14 18:50:09 2021
tree 9a9c610a03f592e628cc1c5929a145d2f375ebf2
parent 6f8fa5af77808849cb5f2de39c469cbe44bf91c6

Fix: DefaultVideoQualityAnalyzerFramesComparator::Stop() may not block

DefaultVideoQualityAnalyzerFramesComparator::Stop() may not block until
all frames comparisons are processed in case when new comparison was
added after worker thread checked for available comparisons and Stop()
was invoked before worker thread checked the state_.

Bug: webrtc:13277, webrtc:13240
Change-Id: Ic16fdc01e43c04529cd83e5d9ef66d7573973cfd
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/235205
Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
Commit-Queue: Artem Titov <titovartem@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35212}

test/pc/e2e/analyzer/video/default_video_quality_analyzer_frames_comparator.cc

diff --git a/test/pc/e2e/analyzer/video/default_video_quality_analyzer_frames_comparator.cc b/test/pc/e2e/analyzer/video/default_video_quality_analyzer_frames_comparator.cc
index adfe027..ef9d32f 100644
--- a/test/pc/e2e/analyzer/video/default_video_quality_analyzer_frames_comparator.cc
+++ b/test/pc/e2e/analyzer/video/default_video_quality_analyzer_frames_comparator.cc
@@ -327,6 +327,7 @@
   while (true) {
     // Try to pick next comparison to perform from the queue.
     absl::optional<FrameComparison> comparison = absl::nullopt;
+    bool more_new_comparisons_expected;
     {
       MutexLock lock(&mutex_);
       if (!comparisons_.empty()) {
@@ -336,16 +337,11 @@
           comparison_available_event_.Set();
         }
       }
+      // If state is stopped => no new frame comparisons are expected.
+      more_new_comparisons_expected = state_ != State::kStopped;
     }
     if (!comparison) {
-      bool more_frames_expected;
-      {
-        // If there are no comparisons and state is stopped =>
-        // no more frames expected.
-        MutexLock lock(&mutex_);
-        more_frames_expected = state_ != State::kStopped;
-      }
-      if (!more_frames_expected) {
+      if (!more_new_comparisons_expected) {
         comparison_available_event_.Set();
         return;
       }

test/pc/e2e/analyzer/video/default_video_quality_analyzer_frames_comparator_test.cc

diff --git a/test/pc/e2e/analyzer/video/default_video_quality_analyzer_frames_comparator_test.cc b/test/pc/e2e/analyzer/video/default_video_quality_analyzer_frames_comparator_test.cc
index 46cf4c41..adbd97a 100644
--- a/test/pc/e2e/analyzer/video/default_video_quality_analyzer_frames_comparator_test.cc
+++ b/test/pc/e2e/analyzer/video/default_video_quality_analyzer_frames_comparator_test.cc
@@ -117,7 +117,7 @@
                            /*captured=*/absl::nullopt,
                            /*rendered=*/absl::nullopt,
                            FrameComparisonType::kRegular, frame_stats);
-  comparator.Stop({});
+  comparator.Stop(/*last_rendered_frame_times=*/{});
 
   std::map<InternalStatsKey, StreamStats> stats = comparator.stream_stats();
   EXPECT_DOUBLE_EQ(GetFirstOrDie(stats.at(stats_key).transport_time_ms), 20.0);
@@ -161,7 +161,7 @@
                            /*captured=*/absl::nullopt,
                            /*rendered=*/absl::nullopt,
                            FrameComparisonType::kRegular, frame_stats2);
-  comparator.Stop({});
+  comparator.Stop(/*last_rendered_frame_times=*/{});
 
   std::map<InternalStatsKey, StreamStats> stats = comparator.stream_stats();
   EXPECT_DOUBLE_EQ(
@@ -244,7 +244,7 @@
                            /*rendered=*/absl::nullopt,
                            FrameComparisonType::kRegular,
                            stats[stats.size() - 1]);
-  comparator.Stop({});
+  comparator.Stop(/*last_rendered_frame_times=*/{});
 
   EXPECT_EQ(comparator.stream_stats().size(), 1lu);
   StreamStats result_stats = comparator.stream_stats().at(stats_key);