From 46536e3da48a166935ef5e3caba3716af6e06ff8 Mon Sep 17 00:00:00 2001 From: Artem Titov Date: Thu, 14 Oct 2021 15:17:48 +0200 Subject: [PATCH] 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 Commit-Queue: Artem Titov Cr-Commit-Position: refs/heads/main@{#35212} --- ...fault_video_quality_analyzer_frames_comparator.cc | 12 ++++-------- ..._video_quality_analyzer_frames_comparator_test.cc | 6 +++--- 2 files changed, 7 insertions(+), 11 deletions(-) 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 adfe027d94..ef9d32fa1a 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 @@ void DefaultVideoQualityAnalyzerFramesComparator::ProcessComparisons() { while (true) { // Try to pick next comparison to perform from the queue. absl::optional comparison = absl::nullopt; + bool more_new_comparisons_expected; { MutexLock lock(&mutex_); if (!comparisons_.empty()) { @@ -336,16 +337,11 @@ void DefaultVideoQualityAnalyzerFramesComparator::ProcessComparisons() { 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; } 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 46cf4c414d..adbd97a876 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 @@ TEST(DefaultVideoQualityAnalyzerFramesComparatorTest, /*captured=*/absl::nullopt, /*rendered=*/absl::nullopt, FrameComparisonType::kRegular, frame_stats); - comparator.Stop({}); + comparator.Stop(/*last_rendered_frame_times=*/{}); std::map stats = comparator.stream_stats(); EXPECT_DOUBLE_EQ(GetFirstOrDie(stats.at(stats_key).transport_time_ms), 20.0); @@ -161,7 +161,7 @@ TEST(DefaultVideoQualityAnalyzerFramesComparatorTest, /*captured=*/absl::nullopt, /*rendered=*/absl::nullopt, FrameComparisonType::kRegular, frame_stats2); - comparator.Stop({}); + comparator.Stop(/*last_rendered_frame_times=*/{}); std::map stats = comparator.stream_stats(); EXPECT_DOUBLE_EQ( @@ -244,7 +244,7 @@ TEST(DefaultVideoQualityAnalyzerFramesComparatorTest, /*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);