From 2af96059a314edcbffa07c147ce386759c0f5813 Mon Sep 17 00:00:00 2001 From: Artem Titov Date: Sun, 6 Nov 2022 20:19:57 +0100 Subject: [PATCH] [PCLF] Add infra metrics to the AnalyzingVideoSink Bug: b/240540204 Change-Id: If3f5436d701336b0bc122477c61b97b5dc28f422 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/282001 Reviewed-by: Mirko Bonadei Commit-Queue: Artem Titov Cr-Commit-Position: refs/heads/main@{#38561} --- test/pc/e2e/BUILD.gn | 2 + .../analyzer/video/analyzing_video_sink.cc | 28 ++++++- .../e2e/analyzer/video/analyzing_video_sink.h | 25 +++++- .../video/analyzing_video_sink_test.cc | 76 ++++++++++++++++--- ...video_quality_analyzer_injection_helper.cc | 7 +- .../video_quality_analyzer_injection_helper.h | 5 +- test/pc/e2e/peer_connection_quality_test.cc | 3 +- 7 files changed, 126 insertions(+), 20 deletions(-) diff --git a/test/pc/e2e/BUILD.gn b/test/pc/e2e/BUILD.gn index 8bad901980..2a2ef58dbf 100644 --- a/test/pc/e2e/BUILD.gn +++ b/test/pc/e2e/BUILD.gn @@ -233,7 +233,9 @@ if (!build_with_chromium) { "../..:test_renderer", "../../../api:peer_connection_quality_test_fixture_api", "../../../api:video_quality_analyzer_api", + "../../../api/numerics", "../../../api/test/video:video_frame_writer", + "../../../api/units:timestamp", "../../../api/video:video_frame", "../../../rtc_base:checks", "../../../rtc_base:logging", diff --git a/test/pc/e2e/analyzer/video/analyzing_video_sink.cc b/test/pc/e2e/analyzer/video/analyzing_video_sink.cc index 23af426c72..acf273c893 100644 --- a/test/pc/e2e/analyzer/video/analyzing_video_sink.cc +++ b/test/pc/e2e/analyzer/video/analyzing_video_sink.cc @@ -18,6 +18,7 @@ #include "absl/types/optional.h" #include "api/test/peerconnection_quality_test_fixture.h" #include "api/test/video/video_frame_writer.h" +#include "api/units/timestamp.h" #include "api/video/i420_buffer.h" #include "api/video/video_frame.h" #include "rtc_base/checks.h" @@ -42,8 +43,10 @@ AnalyzingVideoSink::AnalyzingVideoSink(absl::string_view peer_name, Clock* clock, VideoQualityAnalyzerInterface& analyzer, AnalyzingVideoSinksHelper& sinks_helper, - const VideoSubscription& subscription) + const VideoSubscription& subscription, + bool report_infra_stats) : peer_name_(peer_name), + report_infra_stats_(report_infra_stats), clock_(clock), analyzer_(&analyzer), sinks_helper_(&sinks_helper), @@ -92,6 +95,8 @@ void AnalyzingVideoSink::OnFrame(const VideoFrame& frame) { AnalyzeFrame(frame); } else { std::string stream_label = analyzer_->GetStreamLabel(frame.id()); + MutexLock lock(&mutex_); + Timestamp processing_started = clock_->CurrentTime(); SinksDescriptor* sinks_descriptor = PopulateSinks(stream_label); RTC_CHECK(sinks_descriptor != nullptr); @@ -101,14 +106,30 @@ void AnalyzingVideoSink::OnFrame(const VideoFrame& frame) { for (auto& sink : sinks_descriptor->sinks) { sink->OnFrame(scaled_frame); } + Timestamp processing_finished = clock_->CurrentTime(); + + if (report_infra_stats_) { + stats_.analyzing_sink_processing_time_ms.AddSample( + (processing_finished - processing_started).ms()); + } } } +AnalyzingVideoSink::Stats AnalyzingVideoSink::stats() const { + MutexLock lock(&mutex_); + return stats_; +} + VideoFrame AnalyzingVideoSink::ScaleVideoFrame( const VideoFrame& frame, const VideoResolution& required_resolution) { + Timestamp processing_started = clock_->CurrentTime(); if (required_resolution.width() == static_cast(frame.width()) && required_resolution.height() == static_cast(frame.height())) { + if (report_infra_stats_) { + stats_.scaling_tims_ms.AddSample( + (clock_->CurrentTime() - processing_started).ms()); + } return frame; } @@ -131,6 +152,10 @@ VideoFrame AnalyzingVideoSink::ScaleVideoFrame( VideoFrame scaled_frame = frame; scaled_frame.set_video_frame_buffer(scaled_buffer); + if (report_infra_stats_) { + stats_.scaling_tims_ms.AddSample( + (clock_->CurrentTime() - processing_started).ms()); + } return scaled_frame; } @@ -144,7 +169,6 @@ void AnalyzingVideoSink::AnalyzeFrame(const VideoFrame& frame) { AnalyzingVideoSink::SinksDescriptor* AnalyzingVideoSink::PopulateSinks( absl::string_view stream_label) { // Fast pass: sinks already exists. - MutexLock lock(&mutex_); auto sinks_it = stream_sinks_.find(std::string(stream_label)); if (sinks_it != stream_sinks_.end()) { return &sinks_it->second; diff --git a/test/pc/e2e/analyzer/video/analyzing_video_sink.h b/test/pc/e2e/analyzer/video/analyzing_video_sink.h index 4654d10a26..af75e0c873 100644 --- a/test/pc/e2e/analyzer/video/analyzing_video_sink.h +++ b/test/pc/e2e/analyzer/video/analyzing_video_sink.h @@ -17,6 +17,7 @@ #include #include "absl/strings/string_view.h" +#include "api/numerics/samples_stats_counter.h" #include "api/test/peerconnection_quality_test_fixture.h" #include "api/test/video/video_frame_writer.h" #include "api/test/video_quality_analyzer_interface.h" @@ -33,13 +34,24 @@ namespace webrtc_pc_e2e { // A sink to inject video quality analyzer as a sink into WebRTC. class AnalyzingVideoSink : public rtc::VideoSinkInterface { public: + struct Stats { + // Time required to scale video frame to the requested rendered resolution. + // Collected only for frames with ID set and iff `report_infra_stats` is + // true. + SamplesStatsCounter scaling_tims_ms; + // Time required to process single video frame. Collected only for frames + // with ID set and iff `report_infra_stats` is true. + SamplesStatsCounter analyzing_sink_processing_time_ms; + }; + AnalyzingVideoSink( absl::string_view peer_name, Clock* clock, VideoQualityAnalyzerInterface& analyzer, AnalyzingVideoSinksHelper& sinks_helper, const PeerConnectionE2EQualityTestFixture::VideoSubscription& - subscription); + subscription, + bool report_infra_stats); // Updates subscription used by this peer to render received video. void UpdateSubscription( @@ -48,6 +60,8 @@ class AnalyzingVideoSink : public rtc::VideoSinkInterface { void OnFrame(const VideoFrame& frame) override; + Stats stats() const; + private: struct SinksDescriptor { SinksDescriptor( @@ -71,23 +85,26 @@ class AnalyzingVideoSink : public rtc::VideoSinkInterface { VideoFrame ScaleVideoFrame( const VideoFrame& frame, const PeerConnectionE2EQualityTestFixture::VideoResolution& - required_resolution); + required_resolution) RTC_EXCLUSIVE_LOCKS_REQUIRED(mutex_); // Creates full copy of the frame to free any frame owned internal buffers // and passes created copy to analyzer. Uses `I420Buffer` to represent // frame content. void AnalyzeFrame(const VideoFrame& frame); // Populates sink for specified stream and caches them in `stream_sinks_`. - SinksDescriptor* PopulateSinks(absl::string_view stream_label); + SinksDescriptor* PopulateSinks(absl::string_view stream_label) + RTC_EXCLUSIVE_LOCKS_REQUIRED(mutex_); const std::string peer_name_; + const bool report_infra_stats_; Clock* const clock_; VideoQualityAnalyzerInterface* const analyzer_; AnalyzingVideoSinksHelper* const sinks_helper_; - Mutex mutex_; + mutable Mutex mutex_; PeerConnectionE2EQualityTestFixture::VideoSubscription subscription_ RTC_GUARDED_BY(mutex_); std::map stream_sinks_ RTC_GUARDED_BY(mutex_); + Stats stats_ RTC_GUARDED_BY(mutex_); }; } // namespace webrtc_pc_e2e diff --git a/test/pc/e2e/analyzer/video/analyzing_video_sink_test.cc b/test/pc/e2e/analyzer/video/analyzing_video_sink_test.cc index 16fc05d718..bb17a86e8c 100644 --- a/test/pc/e2e/analyzer/video/analyzing_video_sink_test.cc +++ b/test/pc/e2e/analyzer/video/analyzing_video_sink_test.cc @@ -40,6 +40,8 @@ namespace { using ::testing::ElementsAreArray; using ::testing::Eq; +using ::testing::Ge; +using ::testing::Gt; using ::testing::Test; using VideoConfig = @@ -56,7 +58,8 @@ void CleanDir(absl::string_view dir, size_t expected_output_files_count) { absl::optional> dir_content = test::ReadDirectory(dir); if (expected_output_files_count == 0) { - ASSERT_FALSE(dir_content.has_value()) << "Empty directory is expected"; + ASSERT_TRUE(!dir_content.has_value() || dir_content->empty()) + << "Empty directory is expected"; } else { ASSERT_TRUE(dir_content.has_value()) << "Test directory is empty!"; EXPECT_EQ(dir_content->size(), expected_output_files_count); @@ -158,7 +161,7 @@ TEST_F(AnalyzingVideoSinkTest, VideoFramesAreDumpedCorrectly) { AnalyzingVideoSinksHelper helper; helper.AddConfig("alice", video_config); AnalyzingVideoSink sink("bob", Clock::GetRealTimeClock(), analyzer, helper, - subscription); + subscription, /*report_infra_stats=*/false); sink.OnFrame(frame); } @@ -201,7 +204,7 @@ TEST_F(AnalyzingVideoSinkTest, AnalyzingVideoSinksHelper helper; helper.AddConfig("alice", video_config); AnalyzingVideoSink sink("bob", Clock::GetRealTimeClock(), analyzer, helper, - subscription); + subscription, /*report_infra_stats=*/false); sink.OnFrame(frame); } @@ -246,7 +249,7 @@ TEST_F(AnalyzingVideoSinkTest, AnalyzingVideoSinksHelper helper; helper.AddConfig("alice", video_config); AnalyzingVideoSink sink("bob", Clock::GetRealTimeClock(), analyzer, helper, - subscription); + subscription, /*report_infra_stats=*/false); sink.OnFrame(frame); } @@ -298,7 +301,7 @@ TEST_F(AnalyzingVideoSinkTest, AnalyzingVideoSinksHelper helper; helper.AddConfig("alice", video_config); AnalyzingVideoSink sink("bob", Clock::GetRealTimeClock(), analyzer, helper, - subscription_before); + subscription_before, /*report_infra_stats=*/false); sink.OnFrame(frame_before); sink.UpdateSubscription(subscription_after); @@ -371,7 +374,7 @@ TEST_F(AnalyzingVideoSinkTest, AnalyzingVideoSinksHelper helper; helper.AddConfig("alice", video_config); AnalyzingVideoSink sink("bob", Clock::GetRealTimeClock(), analyzer, helper, - subscription_before); + subscription_before, /*report_infra_stats=*/false); sink.OnFrame(frame_before); sink.UpdateSubscription(subscription_after); @@ -426,7 +429,7 @@ TEST_F(AnalyzingVideoSinkTest, SmallDiviationsInAspectRationAreAllowed) { AnalyzingVideoSinksHelper helper; helper.AddConfig("alice", video_config); AnalyzingVideoSink sink("bob", Clock::GetRealTimeClock(), analyzer, helper, - subscription); + subscription, /*report_infra_stats=*/false); sink.OnFrame(frame); } @@ -473,7 +476,7 @@ TEST_F(AnalyzingVideoSinkTest, VideoFramesIdsAreDumpedWhenRequested) { AnalyzingVideoSinksHelper helper; helper.AddConfig("alice", video_config); AnalyzingVideoSink sink("bob", Clock::GetRealTimeClock(), analyzer, helper, - subscription); + subscription, /*report_infra_stats=*/false); for (int i = 0; i < 10; ++i) { VideoFrame frame = CreateFrame(*frame_generator); frame.set_id(analyzer.OnFrameCaptured("alice", "alice_video", frame)); @@ -520,7 +523,7 @@ TEST_F(AnalyzingVideoSinkTest, AnalyzingVideoSinksHelper helper; helper.AddConfig("alice", video_config); AnalyzingVideoSink sink("bob", simulated_time.GetClock(), analyzer, helper, - subscription); + subscription, /*report_infra_stats=*/false); sink.OnFrame(frame1); // Advance almost 1 second, so the first frame has to be repeated 9 time // more. @@ -569,6 +572,61 @@ TEST_F(AnalyzingVideoSinkTest, ExpectOutputFilesCount(2); } +TEST_F(AnalyzingVideoSinkTest, InfraMetricsCollectedWhenRequested) { + VideoSubscription subscription; + subscription.SubscribeToPeer( + "alice", VideoResolution(/*width=*/1280, /*height=*/720, /*fps=*/30)); + VideoConfig video_config("alice_video", /*width=*/640, /*height=*/360, + /*fps=*/30); + + ExampleVideoQualityAnalyzer analyzer; + std::unique_ptr frame_generator = + CreateFrameGenerator(/*width=*/640, /*height=*/360); + VideoFrame frame = CreateFrame(*frame_generator); + frame.set_id(analyzer.OnFrameCaptured("alice", "alice_video", frame)); + + AnalyzingVideoSinksHelper helper; + helper.AddConfig("alice", video_config); + AnalyzingVideoSink sink("bob", Clock::GetRealTimeClock(), analyzer, helper, + subscription, /*report_infra_stats=*/true); + sink.OnFrame(frame); + + AnalyzingVideoSink::Stats stats = sink.stats(); + EXPECT_THAT(stats.scaling_tims_ms.NumSamples(), Eq(1)); + EXPECT_THAT(stats.scaling_tims_ms.GetAverage(), Gt(0)); + EXPECT_THAT(stats.analyzing_sink_processing_time_ms.NumSamples(), Eq(1)); + EXPECT_THAT(stats.analyzing_sink_processing_time_ms.GetAverage(), + Ge(stats.scaling_tims_ms.GetAverage())); + + ExpectOutputFilesCount(0); +} + +TEST_F(AnalyzingVideoSinkTest, InfraMetricsNotCollectedWhenNotRequested) { + VideoSubscription subscription; + subscription.SubscribeToPeer( + "alice", VideoResolution(/*width=*/1280, /*height=*/720, /*fps=*/30)); + VideoConfig video_config("alice_video", /*width=*/640, /*height=*/360, + /*fps=*/30); + + ExampleVideoQualityAnalyzer analyzer; + std::unique_ptr frame_generator = + CreateFrameGenerator(/*width=*/640, /*height=*/360); + VideoFrame frame = CreateFrame(*frame_generator); + frame.set_id(analyzer.OnFrameCaptured("alice", "alice_video", frame)); + + AnalyzingVideoSinksHelper helper; + helper.AddConfig("alice", video_config); + AnalyzingVideoSink sink("bob", Clock::GetRealTimeClock(), analyzer, helper, + subscription, /*report_infra_stats=*/false); + sink.OnFrame(frame); + + AnalyzingVideoSink::Stats stats = sink.stats(); + EXPECT_THAT(stats.scaling_tims_ms.NumSamples(), Eq(0)); + EXPECT_THAT(stats.analyzing_sink_processing_time_ms.NumSamples(), Eq(0)); + + ExpectOutputFilesCount(0); +} + } // namespace } // namespace webrtc_pc_e2e } // namespace webrtc 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 dfe9ff8491..7ce7932658 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 @@ -150,10 +150,11 @@ VideoQualityAnalyzerInjectionHelper::CreateVideoSink( std::unique_ptr VideoQualityAnalyzerInjectionHelper::CreateVideoSink( absl::string_view peer_name, - const PeerConnectionE2EQualityTestFixture::VideoSubscription& - subscription) { + const PeerConnectionE2EQualityTestFixture::VideoSubscription& subscription, + bool report_infra_metrics) { return std::make_unique(peer_name, clock_, *analyzer_, - sinks_helper_, subscription); + sinks_helper_, subscription, + report_infra_metrics); } void VideoQualityAnalyzerInjectionHelper::Start( 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 5b800c6aa9..db49e05c90 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 @@ -77,12 +77,15 @@ class VideoQualityAnalyzerInjectionHelper : public StatsObserverInterface { // `output_dump_file_name` in its VideoConfig, which was used for // CreateFramePreprocessor(...), then video also will be written // into that file. + // TODO(titovartem): Remove method with `peer_name` only parameter. std::unique_ptr> CreateVideoSink( absl::string_view peer_name); + // TODO(titovartem): Remove default value for `report_infra_metrics`. std::unique_ptr CreateVideoSink( absl::string_view peer_name, const PeerConnectionE2EQualityTestFixture::VideoSubscription& - subscription); + subscription, + bool report_infra_metrics = false); void Start(std::string test_case_name, rtc::ArrayView peer_names, diff --git a/test/pc/e2e/peer_connection_quality_test.cc b/test/pc/e2e/peer_connection_quality_test.cc index dd685dbf5a..e0b0ce9a99 100644 --- a/test/pc/e2e/peer_connection_quality_test.cc +++ b/test/pc/e2e/peer_connection_quality_test.cc @@ -473,7 +473,8 @@ void PeerConnectionE2EQualityTest::OnTrackCallback( // track->kind() is kVideoKind. auto* video_track = static_cast(track.get()); std::unique_ptr> video_sink = - video_quality_analyzer_injection_helper_->CreateVideoSink(peer_name); + video_quality_analyzer_injection_helper_->CreateVideoSink( + peer_name, peer_subscription, /*report_infra_stats=*/false); video_track->AddOrUpdateSink(video_sink.get(), rtc::VideoSinkWants()); output_video_sinks_.push_back(std::move(video_sink)); }