Synchronize final stats collection in E2E quality tests The PeerConnectionE2EQualityTest framework previously lost final metrics, such as payload_bytes_received, because it did not wait for asynchronous RTCStatsCollector::GetStats() calls to complete during test teardown. This change ensures all stats are captured by: - Tracking pending asynchronous requests in InternalStatsObserver using a counter and a new IsPolling() method. - Implementing a synchronization point in the test runner that uses the time controller to wait until StatsPoller reports no active polling. - Adding an optional polling delay to StatsPoller to help simulate and verify fixes for race conditions in asynchronous stat delivery. Bug: webrtc:481443652 Change-Id: Ie689bdab4843780c765e8ffd2668d63863328026 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/461000 Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org> Reviewed-by: Åsa Persson <asapersson@webrtc.org> Cr-Commit-Position: refs/heads/main@{#47295}
diff --git a/test/pc/e2e/BUILD.gn b/test/pc/e2e/BUILD.gn index 20b8524..d52cae5 100644 --- a/test/pc/e2e/BUILD.gn +++ b/test/pc/e2e/BUILD.gn
@@ -431,6 +431,9 @@ "../../../api:create_network_emulation_manager", "../../../api:network_emulation_manager_api", "../../../api:peer_connection_quality_test_fixture_api", + "../../../api:rtc_stats_api", + "../../../api:scoped_refptr", + "../../../api:track_id_stream_info_map", "../../../api/test/metrics:global_metrics_logger_and_exporter", "../../../api/test/network_emulation", "../../../api/test/pclf:media_configuration", @@ -461,6 +464,8 @@ "../../../api:rtc_stats_api", "../../../api:scoped_refptr", "../../../api:stats_observer_interface", + "../../../api/task_queue", + "../../../api/units:time_delta", "../../../rtc_base:logging", "../../../rtc_base:macromagic", "../../../rtc_base/synchronization:mutex",
diff --git a/test/pc/e2e/peer_connection_quality_test.cc b/test/pc/e2e/peer_connection_quality_test.cc index 05251f6..ae2ea68 100644 --- a/test/pc/e2e/peer_connection_quality_test.cc +++ b/test/pc/e2e/peer_connection_quality_test.cc
@@ -371,7 +371,8 @@ StatsPoller stats_poller(observers, std::map<std::string, StatsProvider*>{ {*alice_->params().name, alice_.get()}, - {*bob_->params().name, bob_.get()}}); + {*bob_->params().name, bob_.get()}}, + stats_polling_delay_); executor_->ScheduleActivity(TimeDelta::Zero(), kStatsUpdateInterval, [&stats_poller](TimeDelta) { stats_poller.PollStatsAndNotifyObservers(); @@ -415,6 +416,11 @@ // Get final end-of-call stats. stats_poller.PollStatsAndNotifyObservers(); }); + + // Wait until the final stats collection is fully resolved across all peers. + ASSERT_TRUE(time_controller_.Wait( + [&stats_poller]() { return !stats_poller.IsPolling(); }, + kDefaultTimeout)); // We need to detach AEC dumping from peers, because dump uses `task_queue_` // inside. alice_->DetachAecDump();
diff --git a/test/pc/e2e/peer_connection_quality_test.h b/test/pc/e2e/peer_connection_quality_test.h index 77bf735..1888f66 100644 --- a/test/pc/e2e/peer_connection_quality_test.h +++ b/test/pc/e2e/peer_connection_quality_test.h
@@ -86,6 +86,8 @@ return real_test_duration_; } + void SetStatsPollingDelay(TimeDelta delay) { stats_polling_delay_ = delay; } + private: class PeerHandleImpl : public PeerHandle { public: @@ -114,6 +116,7 @@ Timestamp Now() const; TimeController& time_controller_; + TimeDelta stats_polling_delay_ = TimeDelta::Zero(); const std::unique_ptr<TaskQueueFactory> task_queue_factory_; std::string test_case_name_; std::unique_ptr<VideoQualityAnalyzerInjectionHelper>
diff --git a/test/pc/e2e/peer_connection_quality_test_test.cc b/test/pc/e2e/peer_connection_quality_test_test.cc index 94e2d58..8168b53 100644 --- a/test/pc/e2e/peer_connection_quality_test_test.cc +++ b/test/pc/e2e/peer_connection_quality_test_test.cc
@@ -18,6 +18,8 @@ #include <vector> #include "absl/strings/string_view.h" +#include "api/scoped_refptr.h" +#include "api/stats/rtc_stats_report.h" #include "api/test/create_network_emulation_manager.h" #include "api/test/metrics/global_metrics_logger_and_exporter.h" #include "api/test/network_emulation/network_emulation_interfaces.h" @@ -25,6 +27,8 @@ #include "api/test/pclf/media_configuration.h" #include "api/test/pclf/media_quality_test_params.h" #include "api/test/pclf/peer_configurer.h" +#include "api/test/peerconnection_quality_test_fixture.h" +#include "api/test/track_id_stream_info_map.h" #include "api/units/time_delta.h" #include "test/gmock.h" #include "test/gtest.h" @@ -45,7 +49,8 @@ std::optional<std::vector<std::string>> 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); @@ -136,6 +141,66 @@ ExpectOutputFilesCount(1); } +class FinalStatsWaitReporter + : public PeerConnectionE2EQualityTestFixture::QualityMetricsReporter { + public: + void Start(absl::string_view test_case_name, + const TrackIdStreamInfoMap* reporter_helper) override {} + + void OnStatsReports( + absl::string_view pc_label, + const scoped_refptr<const RTCStatsReport>& report) override { + ++calls_count_; + } + + void StopAndReportResults() override { EXPECT_EQ(calls_count_, 4u); } + + private: + size_t calls_count_ = 0; +}; + +TEST_F(PeerConnectionE2EQualityTestTest, StatsAreCollectedAtTheEndOfTheTest) { + std::unique_ptr<NetworkEmulationManager> network_emulation = + CreateNetworkEmulationManager({.time_mode = TimeMode::kSimulated}); + PeerConnectionE2EQualityTest fixture( + "test_case", *network_emulation->time_controller(), + /*audio_quality_analyzer=*/nullptr, /*video_quality_analyzer=*/nullptr, + test::GetGlobalMetricsLogger()); + // Inject a delay that simulates stats gathering taking some time. + fixture.SetStatsPollingDelay(TimeDelta::Millis(100)); + + EmulatedEndpoint* alice_endpoint = + network_emulation->CreateEndpoint(EmulatedEndpointConfig()); + EmulatedEndpoint* bob_endpoint = + network_emulation->CreateEndpoint(EmulatedEndpointConfig()); + + network_emulation->CreateRoute( + alice_endpoint, {network_emulation->CreateUnconstrainedEmulatedNode()}, + bob_endpoint); + network_emulation->CreateRoute( + bob_endpoint, {network_emulation->CreateUnconstrainedEmulatedNode()}, + alice_endpoint); + + EmulatedNetworkManagerInterface* alice_network = + network_emulation->CreateEmulatedNetworkManagerInterface( + {alice_endpoint}); + EmulatedNetworkManagerInterface* bob_network = + network_emulation->CreateEmulatedNetworkManagerInterface({bob_endpoint}); + + PeerConfigurer alice(*alice_network); + alice.SetName("alice"); + alice.AddVideoConfig(VideoConfig("alice_video", 320, 180, 30)); + fixture.AddPeer(std::make_unique<PeerConfigurer>(std::move(alice))); + + PeerConfigurer bob(*bob_network); + bob.SetName("bob"); + fixture.AddPeer(std::make_unique<PeerConfigurer>(std::move(bob))); + + fixture.AddQualityMetricsReporter(std::make_unique<FinalStatsWaitReporter>()); + + fixture.Run(RunParams(TimeDelta::Millis(1))); +} + } // namespace } // namespace webrtc_pc_e2e } // namespace webrtc
diff --git a/test/pc/e2e/stats_poller.cc b/test/pc/e2e/stats_poller.cc index 29533ac..d2ca30e 100644 --- a/test/pc/e2e/stats_poller.cc +++ b/test/pc/e2e/stats_poller.cc
@@ -19,7 +19,9 @@ #include "api/make_ref_counted.h" #include "api/scoped_refptr.h" #include "api/stats/rtc_stats_report.h" +#include "api/task_queue/task_queue_base.h" #include "api/test/stats_observer_interface.h" +#include "api/units/time_delta.h" #include "rtc_base/synchronization/mutex.h" #include "test/pc/e2e/stats_provider.h" #include "test/pc/e2e/test_peer.h" @@ -28,7 +30,19 @@ namespace webrtc_pc_e2e { void InternalStatsObserver::PollStats() { - peer_->GetStats(this); + { + MutexLock lock(&mutex_); + ++pending_requests_; + } + if (stats_delay_.IsZero()) { + peer_->GetStats(this); + } else { + // Artificial delay for testing race conditions. + TaskQueueBase::Current()->PostDelayedTask( + [this, captured_observer = scoped_refptr<InternalStatsObserver>( + this)]() { peer_->GetStats(captured_observer.get()); }, + stats_delay_); + } } void InternalStatsObserver::OnStatsDelivered( @@ -36,25 +50,36 @@ for (auto* observer : observers_) { observer->OnStatsReports(pc_label_, report); } + { + MutexLock lock(&mutex_); + --pending_requests_; + } +} + +bool InternalStatsObserver::IsPolling() const { + MutexLock lock(&mutex_); + return pending_requests_ > 0; } StatsPoller::StatsPoller(std::vector<StatsObserverInterface*> observers, - std::map<std::string, StatsProvider*> peers) + std::map<std::string, StatsProvider*> peers, + TimeDelta stats_delay) : observers_(std::move(observers)) { MutexLock lock(&mutex_); for (auto& peer : peers) { pollers_.push_back(make_ref_counted<InternalStatsObserver>( - peer.first, peer.second, observers_)); + peer.first, peer.second, observers_, stats_delay)); } } StatsPoller::StatsPoller(std::vector<StatsObserverInterface*> observers, - std::map<std::string, TestPeer*> peers) + std::map<std::string, TestPeer*> peers, + TimeDelta stats_delay) : observers_(std::move(observers)) { MutexLock lock(&mutex_); for (auto& peer : peers) { pollers_.push_back(make_ref_counted<InternalStatsObserver>( - peer.first, peer.second, observers_)); + peer.first, peer.second, observers_, stats_delay)); } } @@ -83,5 +108,15 @@ return false; } +bool StatsPoller::IsPolling() const { + MutexLock lock(&mutex_); + for (const auto& poller : pollers_) { + if (poller->IsPolling()) { + return true; + } + } + return false; +} + } // namespace webrtc_pc_e2e } // namespace webrtc
diff --git a/test/pc/e2e/stats_poller.h b/test/pc/e2e/stats_poller.h index f9b8890..541e943 100644 --- a/test/pc/e2e/stats_poller.h +++ b/test/pc/e2e/stats_poller.h
@@ -21,6 +21,7 @@ #include "api/stats/rtc_stats_collector_callback.h" #include "api/stats/rtc_stats_report.h" #include "api/test/stats_observer_interface.h" +#include "api/units/time_delta.h" #include "rtc_base/synchronization/mutex.h" #include "rtc_base/thread_annotations.h" #include "test/pc/e2e/stats_provider.h" @@ -29,14 +30,18 @@ namespace webrtc { namespace webrtc_pc_e2e { -// Helper class that will notify all the webrtc::test::StatsObserverInterface +// Helper class that will notify all the test::StatsObserverInterface // objects subscribed. class InternalStatsObserver : public RTCStatsCollectorCallback { public: InternalStatsObserver(absl::string_view pc_label, StatsProvider* peer, - std::vector<StatsObserverInterface*> observers) - : pc_label_(pc_label), peer_(peer), observers_(std::move(observers)) {} + std::vector<StatsObserverInterface*> observers, + TimeDelta stats_delay = TimeDelta::Zero()) + : pc_label_(pc_label), + peer_(peer), + observers_(std::move(observers)), + stats_delay_(stats_delay) {} std::string pc_label() const { return pc_label_; } @@ -45,24 +50,33 @@ void OnStatsDelivered( const scoped_refptr<const RTCStatsReport>& report) override; + bool IsPolling() const; + private: std::string pc_label_; - StatsProvider* peer_; + StatsProvider* const peer_; std::vector<StatsObserverInterface*> observers_; + TimeDelta stats_delay_; + mutable Mutex mutex_; + int pending_requests_ RTC_GUARDED_BY(mutex_) = 0; }; // Helper class to invoke GetStats on a PeerConnection by passing a -// webrtc::StatsObserver that will notify all the -// webrtc::test::StatsObserverInterface subscribed. +// StatsObserver that will notify all the test::StatsObserverInterface +// subscribed. class StatsPoller { public: StatsPoller(std::vector<StatsObserverInterface*> observers, - std::map<std::string, StatsProvider*> peers_to_observe); + std::map<std::string, StatsProvider*> peers_to_observe, + TimeDelta stats_delay = TimeDelta::Zero()); StatsPoller(std::vector<StatsObserverInterface*> observers, - std::map<std::string, TestPeer*> peers_to_observe); + std::map<std::string, TestPeer*> peers_to_observe, + TimeDelta stats_delay = TimeDelta::Zero()); void PollStatsAndNotifyObservers(); + bool IsPolling() const; + void RegisterParticipantInCall(absl::string_view peer_name, StatsProvider* peer); // Unregister participant from stats poller. Returns true if participant was @@ -71,7 +85,7 @@ private: const std::vector<StatsObserverInterface*> observers_; - webrtc::Mutex mutex_; + mutable Mutex mutex_; std::vector<scoped_refptr<InternalStatsObserver>> pollers_ RTC_GUARDED_BY(mutex_); };