From ae271ff05aa46c8fd3b6fe73f3a48518dcc7872a Mon Sep 17 00:00:00 2001 From: Artem Titov Date: Tue, 21 Jun 2022 09:37:44 +0200 Subject: [PATCH] [DVQA] Add support for removing peer from analyzer instrumentation Add support for removing peer from EncodedImageDataExtractor and from VideoQualityAnalyzerInjectionHelper. Bug: b/231397778 Change-Id: Ic33da18b7a68149ef68e5e4ba0ee7eabfb634973 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/266364 Reviewed-by: Mirko Bonadei Commit-Queue: Artem Titov Cr-Commit-Position: refs/heads/main@{#37285} --- .../video/encoded_image_data_injector.h | 4 + ...gle_process_encoded_image_data_injector.cc | 19 +++ ...ngle_process_encoded_image_data_injector.h | 1 + ...ss_encoded_image_data_injector_unittest.cc | 154 ++++++++++++++---- .../video/video_frame_tracking_id_injector.h | 1 + ...video_quality_analyzer_injection_helper.cc | 9 + .../video_quality_analyzer_injection_helper.h | 5 + 7 files changed, 158 insertions(+), 35 deletions(-) diff --git a/test/pc/e2e/analyzer/video/encoded_image_data_injector.h b/test/pc/e2e/analyzer/video/encoded_image_data_injector.h index 20f7a4368a..384e901462 100644 --- a/test/pc/e2e/analyzer/video/encoded_image_data_injector.h +++ b/test/pc/e2e/analyzer/video/encoded_image_data_injector.h @@ -56,6 +56,10 @@ class EncodedImageDataExtractor { // frames. Will be invoked before that receiver will start receive data. virtual void AddParticipantInCall() = 0; + // Invoked by framework when it is required to remove receiver for frames. + // Will be invoked after that receiver will stop receiving data. + virtual void RemoveParticipantInCall() = 0; + // Returns encoded image id, extracted from payload and also encoded image // with its original payload. For concatenated spatial layers it should be the // same id. diff --git a/test/pc/e2e/analyzer/video/single_process_encoded_image_data_injector.cc b/test/pc/e2e/analyzer/video/single_process_encoded_image_data_injector.cc index d7ee0f41b9..ccd2f03537 100644 --- a/test/pc/e2e/analyzer/video/single_process_encoded_image_data_injector.cc +++ b/test/pc/e2e/analyzer/video/single_process_encoded_image_data_injector.cc @@ -59,6 +59,25 @@ void SingleProcessEncodedImageDataInjector::AddParticipantInCall() { expected_receivers_count_++; } +void SingleProcessEncodedImageDataInjector::RemoveParticipantInCall() { + MutexLock crit(&lock_); + expected_receivers_count_--; + // Now we need go over `extraction_cache_` and removed frames which have been + // received by `expected_receivers_count_`. + for (auto& [frame_id, extraction_infos] : extraction_cache_) { + for (auto it = extraction_infos.infos.begin(); + it != extraction_infos.infos.end();) { + // Frame is received if `received_count` equals to + // `expected_receivers_count_`. + if (it->second.received_count == expected_receivers_count_) { + it = extraction_infos.infos.erase(it); + } else { + ++it; + } + } + } +} + EncodedImageExtractionResult SingleProcessEncodedImageDataInjector::ExtractData( const EncodedImage& source) { size_t size = source.size(); diff --git a/test/pc/e2e/analyzer/video/single_process_encoded_image_data_injector.h b/test/pc/e2e/analyzer/video/single_process_encoded_image_data_injector.h index 81fc433719..1082440e2f 100644 --- a/test/pc/e2e/analyzer/video/single_process_encoded_image_data_injector.h +++ b/test/pc/e2e/analyzer/video/single_process_encoded_image_data_injector.h @@ -57,6 +57,7 @@ class SingleProcessEncodedImageDataInjector expected_receivers_count_ = expected_receivers_count; } void AddParticipantInCall() override; + void RemoveParticipantInCall() override; EncodedImageExtractionResult ExtractData(const EncodedImage& source) override; private: diff --git a/test/pc/e2e/analyzer/video/single_process_encoded_image_data_injector_unittest.cc b/test/pc/e2e/analyzer/video/single_process_encoded_image_data_injector_unittest.cc index cfeab23562..f6fa40455a 100644 --- a/test/pc/e2e/analyzer/video/single_process_encoded_image_data_injector_unittest.cc +++ b/test/pc/e2e/analyzer/video/single_process_encoded_image_data_injector_unittest.cc @@ -37,11 +37,18 @@ EncodedImage CreateEncodedImageOfSizeNFilledWithValuesFromX(size_t n, return image; } +EncodedImage DeepCopyEncodedImage(const EncodedImage& source) { + EncodedImage copy = source; + copy.SetEncodedData(EncodedImageBuffer::Create(source.data(), source.size())); + return copy; +} + TEST(SingleProcessEncodedImageDataInjectorTest, InjectExtractDiscardFalse) { SingleProcessEncodedImageDataInjector injector; - injector.Start(1); + injector.Start(/*expected_receivers_count=*/1); - EncodedImage source = CreateEncodedImageOfSizeNFilledWithValuesFromX(10, 1); + EncodedImage source = + CreateEncodedImageOfSizeNFilledWithValuesFromX(/*n=*/10, /*x=*/1); source.SetTimestamp(123456789); EncodedImageExtractionResult out = @@ -57,9 +64,10 @@ TEST(SingleProcessEncodedImageDataInjectorTest, InjectExtractDiscardFalse) { TEST(SingleProcessEncodedImageDataInjectorTest, InjectExtractDiscardTrue) { SingleProcessEncodedImageDataInjector injector; - injector.Start(1); + injector.Start(/*expected_receivers_count=*/1); - EncodedImage source = CreateEncodedImageOfSizeNFilledWithValuesFromX(10, 1); + EncodedImage source = + CreateEncodedImageOfSizeNFilledWithValuesFromX(/*n=*/10, /*x=*/1); source.SetTimestamp(123456789); EncodedImageExtractionResult out = @@ -73,9 +81,10 @@ TEST(SingleProcessEncodedImageDataInjectorTest, InjectExtractDiscardTrue) { TEST(SingleProcessEncodedImageDataInjectorTest, InjectWithUnsetSpatialLayerSizes) { SingleProcessEncodedImageDataInjector injector; - injector.Start(1); + injector.Start(/*expected_receivers_count=*/1); - EncodedImage source = CreateEncodedImageOfSizeNFilledWithValuesFromX(10, 1); + EncodedImage source = + CreateEncodedImageOfSizeNFilledWithValuesFromX(/*n=*/10, /*x=*/1); source.SetTimestamp(123456789); EncodedImage intermediate = injector.InjectData(512, false, source); @@ -97,9 +106,10 @@ TEST(SingleProcessEncodedImageDataInjectorTest, TEST(SingleProcessEncodedImageDataInjectorTest, InjectWithZeroSpatialLayerSizes) { SingleProcessEncodedImageDataInjector injector; - injector.Start(1); + injector.Start(/*expected_receivers_count=*/1); - EncodedImage source = CreateEncodedImageOfSizeNFilledWithValuesFromX(10, 1); + EncodedImage source = + CreateEncodedImageOfSizeNFilledWithValuesFromX(/*n=*/10, /*x=*/1); source.SetTimestamp(123456789); EncodedImage intermediate = injector.InjectData(512, false, source); @@ -123,16 +133,19 @@ TEST(SingleProcessEncodedImageDataInjectorTest, TEST(SingleProcessEncodedImageDataInjectorTest, Inject3Extract3) { SingleProcessEncodedImageDataInjector injector; - injector.Start(1); + injector.Start(/*expected_receivers_count=*/1); // 1st frame - EncodedImage source1 = CreateEncodedImageOfSizeNFilledWithValuesFromX(10, 1); + EncodedImage source1 = + CreateEncodedImageOfSizeNFilledWithValuesFromX(/*n=*/10, /*x=*/1); source1.SetTimestamp(123456710); // 2nd frame 1st spatial layer - EncodedImage source2 = CreateEncodedImageOfSizeNFilledWithValuesFromX(10, 11); + EncodedImage source2 = + CreateEncodedImageOfSizeNFilledWithValuesFromX(/*n=*/10, /*x=*/11); source2.SetTimestamp(123456720); // 2nd frame 2nd spatial layer - EncodedImage source3 = CreateEncodedImageOfSizeNFilledWithValuesFromX(10, 21); + EncodedImage source3 = + CreateEncodedImageOfSizeNFilledWithValuesFromX(/*n=*/10, /*x=*/21); source3.SetTimestamp(123456720); EncodedImage intermediate1 = injector.InjectData(510, false, source1); @@ -166,13 +179,16 @@ TEST(SingleProcessEncodedImageDataInjectorTest, Inject3Extract3) { TEST(SingleProcessEncodedImageDataInjectorTest, InjectExtractFromConcatenated) { SingleProcessEncodedImageDataInjector injector; - injector.Start(1); + injector.Start(/*expected_receivers_count=*/1); - EncodedImage source1 = CreateEncodedImageOfSizeNFilledWithValuesFromX(10, 1); + EncodedImage source1 = + CreateEncodedImageOfSizeNFilledWithValuesFromX(/*n=*/10, /*x=*/1); source1.SetTimestamp(123456710); - EncodedImage source2 = CreateEncodedImageOfSizeNFilledWithValuesFromX(10, 11); + EncodedImage source2 = + CreateEncodedImageOfSizeNFilledWithValuesFromX(/*n=*/10, /*x=*/11); source2.SetTimestamp(123456710); - EncodedImage source3 = CreateEncodedImageOfSizeNFilledWithValuesFromX(10, 21); + EncodedImage source3 = + CreateEncodedImageOfSizeNFilledWithValuesFromX(/*n=*/10, /*x=*/21); source3.SetTimestamp(123456710); // Inject id into 3 images with same frame id. @@ -215,13 +231,16 @@ TEST(SingleProcessEncodedImageDataInjectorTest, InjectExtractFromConcatenated) { TEST(SingleProcessEncodedImageDataInjector, InjectExtractFromConcatenatedAllDiscarded) { SingleProcessEncodedImageDataInjector injector; - injector.Start(1); + injector.Start(/*expected_receivers_count=*/1); - EncodedImage source1 = CreateEncodedImageOfSizeNFilledWithValuesFromX(10, 1); + EncodedImage source1 = + CreateEncodedImageOfSizeNFilledWithValuesFromX(/*n=*/10, /*x=*/1); source1.SetTimestamp(123456710); - EncodedImage source2 = CreateEncodedImageOfSizeNFilledWithValuesFromX(10, 11); + EncodedImage source2 = + CreateEncodedImageOfSizeNFilledWithValuesFromX(/*n=*/10, /*x=*/11); source2.SetTimestamp(123456710); - EncodedImage source3 = CreateEncodedImageOfSizeNFilledWithValuesFromX(10, 21); + EncodedImage source3 = + CreateEncodedImageOfSizeNFilledWithValuesFromX(/*n=*/10, /*x=*/21); source3.SetTimestamp(123456710); // Inject id into 3 images with same frame id. @@ -259,9 +278,10 @@ TEST(SingleProcessEncodedImageDataInjector, TEST(SingleProcessEncodedImageDataInjectorTest, InjectOnceExtractTwice) { SingleProcessEncodedImageDataInjector injector; - injector.Start(2); + injector.Start(/*expected_receivers_count=*/2); - EncodedImage source = CreateEncodedImageOfSizeNFilledWithValuesFromX(10, 1); + EncodedImage source = + CreateEncodedImageOfSizeNFilledWithValuesFromX(/*n=*/10, /*x=*/1); source.SetTimestamp(123456789); EncodedImageExtractionResult out = injector.ExtractData( @@ -286,9 +306,10 @@ TEST(SingleProcessEncodedImageDataInjectorTest, InjectOnceExtractTwice) { TEST(SingleProcessEncodedImageDataInjectorTest, Add1stReceiverAfterStart) { SingleProcessEncodedImageDataInjector injector; - injector.Start(0); + injector.Start(/*expected_receivers_count=*/0); - EncodedImage source = CreateEncodedImageOfSizeNFilledWithValuesFromX(10, 1); + EncodedImage source = + CreateEncodedImageOfSizeNFilledWithValuesFromX(/*n=*/10, /*x=*/1); source.SetTimestamp(123456789); EncodedImage modified_image = injector.InjectData( /*id=*/512, /*discard=*/false, source); @@ -307,9 +328,10 @@ TEST(SingleProcessEncodedImageDataInjectorTest, Add1stReceiverAfterStart) { TEST(SingleProcessEncodedImageDataInjectorTest, Add3rdReceiverAfterStart) { SingleProcessEncodedImageDataInjector injector; - injector.Start(2); + injector.Start(/*expected_receivers_count=*/2); - EncodedImage source = CreateEncodedImageOfSizeNFilledWithValuesFromX(10, 1); + EncodedImage source = + CreateEncodedImageOfSizeNFilledWithValuesFromX(/*n=*/10, /*x=*/1); source.SetTimestamp(123456789); EncodedImage modified_image = injector.InjectData( /*id=*/512, /*discard=*/false, source); @@ -328,22 +350,55 @@ TEST(SingleProcessEncodedImageDataInjectorTest, Add3rdReceiverAfterStart) { } } +TEST(SingleProcessEncodedImageDataInjectorTest, + RemoveReceiverRemovesOnlyFullyReceivedFrames) { + SingleProcessEncodedImageDataInjector injector; + injector.Start(/*expected_receivers_count=*/3); + + EncodedImage source1 = + CreateEncodedImageOfSizeNFilledWithValuesFromX(/*n=*/10, /*x=*/1); + source1.SetTimestamp(10); + EncodedImage source2 = + CreateEncodedImageOfSizeNFilledWithValuesFromX(/*n=*/10, /*x=*/1); + source2.SetTimestamp(20); + + EncodedImage modified_image1 = injector.InjectData( + /*id=*/512, /*discard=*/false, source1); + EncodedImage modified_image2 = injector.InjectData( + /*id=*/513, /*discard=*/false, source2); + + // Out of 3 receivers 1st image received by 2 and 2nd image by 1 + injector.ExtractData(DeepCopyEncodedImage(modified_image1)); + injector.ExtractData(DeepCopyEncodedImage(modified_image1)); + injector.ExtractData(DeepCopyEncodedImage(modified_image2)); + + // When we removed one receiver, 2nd image should still be available for + // extraction. + injector.RemoveParticipantInCall(); + + EncodedImageExtractionResult out = + injector.ExtractData(DeepCopyEncodedImage(modified_image2)); + + EXPECT_EQ(out.id, 513); + EXPECT_FALSE(out.discard); + EXPECT_EQ(out.image.size(), 10ul); + EXPECT_EQ(out.image.SpatialLayerFrameSize(0).value_or(0), 0ul); + for (int i = 0; i < 10; ++i) { + EXPECT_EQ(out.image.data()[i], i + 1); + } +} + // Death tests. // Disabled on Android because death tests misbehave on Android, see // base/test/gtest_util.h. #if RTC_DCHECK_IS_ON && GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID) -EncodedImage DeepCopyEncodedImage(const EncodedImage& source) { - EncodedImage copy = source; - copy.SetEncodedData(EncodedImageBuffer::Create(source.data(), source.size())); - return copy; -} - -TEST(SingleProcessEncodedImageDataInjectorTest, +TEST(SingleProcessEncodedImageDataInjectorTestDeathTest, InjectOnceExtractMoreThenExpected) { SingleProcessEncodedImageDataInjector injector; - injector.Start(2); + injector.Start(/*expected_receivers_count=*/2); - EncodedImage source = CreateEncodedImageOfSizeNFilledWithValuesFromX(10, 1); + EncodedImage source = + CreateEncodedImageOfSizeNFilledWithValuesFromX(/*n=*/10, /*x=*/1); source.SetTimestamp(123456789); EncodedImage modified = @@ -354,6 +409,35 @@ TEST(SingleProcessEncodedImageDataInjectorTest, EXPECT_DEATH(injector.ExtractData(DeepCopyEncodedImage(modified)), "Unknown sub_id=0 for frame_id=512"); } + +TEST(SingleProcessEncodedImageDataInjectorTestDeathTest, + RemoveReceiverRemovesOnlyFullyReceivedFramesVerifyFrameIsRemoved) { + SingleProcessEncodedImageDataInjector injector; + injector.Start(/*expected_receivers_count=*/3); + + EncodedImage source1 = + CreateEncodedImageOfSizeNFilledWithValuesFromX(/*n=*/10, /*x=*/1); + source1.SetTimestamp(10); + EncodedImage source2 = + CreateEncodedImageOfSizeNFilledWithValuesFromX(/*n=*/10, /*x=*/1); + source2.SetTimestamp(20); + + EncodedImage modified_image1 = injector.InjectData( + /*id=*/512, /*discard=*/false, source1); + EncodedImage modified_image2 = injector.InjectData( + /*id=*/513, /*discard=*/false, source2); + + // Out of 3 receivers 1st image received by 2 and 2nd image by 1 + injector.ExtractData(DeepCopyEncodedImage(modified_image1)); + injector.ExtractData(DeepCopyEncodedImage(modified_image1)); + injector.ExtractData(DeepCopyEncodedImage(modified_image2)); + + // When we removed one receiver 1st image should be removed. + injector.RemoveParticipantInCall(); + + EXPECT_DEATH(injector.ExtractData(DeepCopyEncodedImage(modified_image1)), + "Unknown sub_id=0 for frame_id=512"); +} #endif // RTC_DCHECK_IS_ON && GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID) } // namespace diff --git a/test/pc/e2e/analyzer/video/video_frame_tracking_id_injector.h b/test/pc/e2e/analyzer/video/video_frame_tracking_id_injector.h index d5ad613f2b..ecc3cd3f51 100644 --- a/test/pc/e2e/analyzer/video/video_frame_tracking_id_injector.h +++ b/test/pc/e2e/analyzer/video/video_frame_tracking_id_injector.h @@ -37,6 +37,7 @@ class VideoFrameTrackingIdInjector : public EncodedImageDataPropagator { void Start(int) override {} void AddParticipantInCall() override {} + void RemoveParticipantInCall() override {} }; } // namespace webrtc_pc_e2e diff --git a/test/pc/e2e/analyzer/video/video_quality_analyzer_injection_helper.cc b/test/pc/e2e/analyzer/video/video_quality_analyzer_injection_helper.cc index dcdf32b3ef..81caeb3a5c 100644 --- a/test/pc/e2e/analyzer/video/video_quality_analyzer_injection_helper.cc +++ b/test/pc/e2e/analyzer/video/video_quality_analyzer_injection_helper.cc @@ -167,6 +167,14 @@ void VideoQualityAnalyzerInjectionHelper::RegisterParticipantInCall( peers_count_++; } +void VideoQualityAnalyzerInjectionHelper::UnregisterParticipantInCall( + absl::string_view peer_name) { + analyzer_->UnregisterParticipantInCall(peer_name); + extractor_->RemoveParticipantInCall(); + MutexLock lock(&mutex_); + peers_count_--; +} + void VideoQualityAnalyzerInjectionHelper::OnStatsReports( absl::string_view pc_label, const rtc::scoped_refptr& report) { @@ -242,6 +250,7 @@ VideoQualityAnalyzerInjectionHelper::PopulateSinks( absl::optional 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(); diff --git a/test/pc/e2e/analyzer/video/video_quality_analyzer_injection_helper.h b/test/pc/e2e/analyzer/video/video_quality_analyzer_injection_helper.h index f130efc193..102fa5df0c 100644 --- a/test/pc/e2e/analyzer/video/video_quality_analyzer_injection_helper.h +++ b/test/pc/e2e/analyzer/video/video_quality_analyzer_injection_helper.h @@ -76,10 +76,15 @@ class VideoQualityAnalyzerInjectionHelper : public StatsObserverInterface { void Start(std::string test_case_name, rtc::ArrayView peer_names, int max_threads_count = 1); + // Registers new call participant to the underlying video quality analyzer. // The method should be called before the participant is actually added. void RegisterParticipantInCall(absl::string_view peer_name); + // Will be called after test removed existing participant in the middle of the + // call. + void UnregisterParticipantInCall(absl::string_view peer_name); + // Forwards `stats_reports` for Peer Connection `pc_label` to // `analyzer_`. void OnStatsReports(