From 1df1bf8551c8e01ee72e2ef336d27a3073060487 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Bostr=C3=B6m?= Date: Tue, 20 Mar 2018 13:24:20 +0100 Subject: [PATCH] PeerConnectionInterface::GetStats() with selector argument added. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This exposes the stats selection algorithm[1] on the PeerConnection. Per-spec, there are four flavors of getStats(): 1. RTCPeerConnection.getStats(). 2. RTCPeerConnection.getStats(MediaStreamTrack selector). 3. RTCRtpSender.getStats(). 4. RTCRtpReceiver.getStats(). 1) is the parameterless getStats() which is already shipped. 2) is the same as 3) and 4) except the track is used to look up the corresponding sender/receiver to use as the selector. 3) and 4) perform stats collection with a filter, which is implemented in RTCStatsCollector.GetStatsReport(selector). For technical reasons, it is easier to place GetStats() on the PeerConnection where the RTCStatsCollector lives than to place it on the sender/receiver. Passing the selector as an argument or as a "this" makes little difference other than style. Wiring Chrome up such that the JavaScript APIs is like the spec is trivial after GetStats() is added to PeerConnectionInterface. This CL also adds comments documenting our intent to deprecate and remove the legacy GetStats() APIs some time in the future. [1] https://w3c.github.io/webrtc-pc/#dfn-stats-selection-algorithm Bug: chromium:680172 Change-Id: I09316ba6f20b25d4f9c11785d0a1a1262d6062a1 Reviewed-on: https://webrtc-review.googlesource.com/62900 Reviewed-by: Taylor Brandstetter Commit-Queue: Henrik Boström Cr-Commit-Position: refs/heads/master@{#22602} --- api/peerconnectioninterface.h | 43 ++++++++++++-- api/peerconnectionproxy.h | 8 +++ api/stats/rtcstats_objects.h | 4 ++ pc/peerconnection.cc | 58 +++++++++++++++++++ pc/peerconnection.h | 8 +++ pc/rtcstats_integrationtest.cc | 98 ++++++++++++++++++++++++++++++-- pc/test/fakepeerconnectionbase.h | 6 ++ stats/rtcstats_objects.cc | 10 +++- 8 files changed, 222 insertions(+), 13 deletions(-) diff --git a/api/peerconnectioninterface.h b/api/peerconnectioninterface.h index e58713ff12..cf9a1f564c 100644 --- a/api/peerconnectioninterface.h +++ b/api/peerconnectioninterface.h @@ -805,15 +805,46 @@ class PeerConnectionInterface : public rtc::RefCountInterface { return {}; } + // The legacy non-compliant GetStats() API. This correspond to the + // callback-based version of getStats() in JavaScript. The returned metrics + // are UNDOCUMENTED and many of them rely on implementation-specific details. + // The goal is to DELETE THIS VERSION but we can't today because it is heavily + // relied upon by third parties. See https://crbug.com/822696. + // + // This version is wired up into Chrome. Any stats implemented are + // automatically exposed to the Web Platform. This has BYPASSED the Chrome + // release processes for years and lead to cross-browser incompatibility + // issues and web application reliance on Chrome-only behavior. + // + // This API is in "maintenance mode", serious regressions should be fixed but + // adding new stats is highly discouraged. + // + // TODO(hbos): Deprecate and remove this when third parties have migrated to + // the spec-compliant GetStats() API. https://crbug.com/822696 virtual bool GetStats(StatsObserver* observer, - MediaStreamTrackInterface* track, + MediaStreamTrackInterface* track, // Optional StatsOutputLevel level) = 0; - // Gets stats using the new stats collection API, see webrtc/api/stats/. These - // will replace old stats collection API when the new API has matured enough. - // TODO(hbos): Default implementation that does nothing only exists as to not - // break third party projects. As soon as they have been updated this should - // be changed to "= 0;". + // The spec-compliant GetStats() API. This correspond to the promise-based + // version of getStats() in JavaScript. Implementation status is described in + // api/stats/rtcstats_objects.h. For more details on stats, see spec: + // https://w3c.github.io/webrtc-pc/#dom-rtcpeerconnection-getstats + // TODO(hbos): Takes shared ownership, use rtc::scoped_refptr<> instead. This + // requires stop overriding the current version in third party or making third + // party calls explicit to avoid ambiguity during switch. Make the future + // version abstract as soon as third party projects implement it. virtual void GetStats(RTCStatsCollectorCallback* callback) {} + // Spec-compliant getStats() performing the stats selection algorithm with the + // sender. https://w3c.github.io/webrtc-pc/#dom-rtcrtpsender-getstats + // TODO(hbos): Make abstract as soon as third party projects implement it. + virtual void GetStats( + rtc::scoped_refptr selector, + rtc::scoped_refptr callback) {} + // Spec-compliant getStats() performing the stats selection algorithm with the + // receiver. https://w3c.github.io/webrtc-pc/#dom-rtcrtpreceiver-getstats + // TODO(hbos): Make abstract as soon as third party projects implement it. + virtual void GetStats( + rtc::scoped_refptr selector, + rtc::scoped_refptr callback) {} // Clear cached stats in the RTCStatsCollector. // Exposed for testing while waiting for automatic cache clear to work. // https://bugs.webrtc.org/8693 diff --git a/api/peerconnectionproxy.h b/api/peerconnectionproxy.h index 7235f5b4b2..9325adcec3 100644 --- a/api/peerconnectionproxy.h +++ b/api/peerconnectionproxy.h @@ -70,6 +70,14 @@ BEGIN_SIGNALING_PROXY_MAP(PeerConnection) MediaStreamTrackInterface*, StatsOutputLevel) PROXY_METHOD1(void, GetStats, RTCStatsCollectorCallback*) + PROXY_METHOD2(void, + GetStats, + rtc::scoped_refptr, + rtc::scoped_refptr); + PROXY_METHOD2(void, + GetStats, + rtc::scoped_refptr, + rtc::scoped_refptr); PROXY_METHOD2(rtc::scoped_refptr, CreateDataChannel, const std::string&, diff --git a/api/stats/rtcstats_objects.h b/api/stats/rtcstats_objects.h index 208a3001da..842fca8cb2 100644 --- a/api/stats/rtcstats_objects.h +++ b/api/stats/rtcstats_objects.h @@ -11,6 +11,7 @@ #ifndef API_STATS_RTCSTATS_OBJECTS_H_ #define API_STATS_RTCSTATS_OBJECTS_H_ +#include #include #include @@ -217,11 +218,13 @@ class RTCIceCandidateStats : public RTCStats { // But here we define them as subclasses of |RTCIceCandidateStats| because the // |kType| need to be different ("RTCStatsType type") in the local/remote case. // https://w3c.github.io/webrtc-stats/#rtcstatstype-str* +// This forces us to have to override copy() and type(). class RTCLocalIceCandidateStats final : public RTCIceCandidateStats { public: static const char kType[]; RTCLocalIceCandidateStats(const std::string& id, int64_t timestamp_us); RTCLocalIceCandidateStats(std::string&& id, int64_t timestamp_us); + std::unique_ptr copy() const override; const char* type() const override; }; @@ -230,6 +233,7 @@ class RTCRemoteIceCandidateStats final : public RTCIceCandidateStats { static const char kType[]; RTCRemoteIceCandidateStats(const std::string& id, int64_t timestamp_us); RTCRemoteIceCandidateStats(std::string&& id, int64_t timestamp_us); + std::unique_ptr copy() const override; const char* type() const override; }; diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc index d8ec0c949a..6fb64fcbdd 100644 --- a/pc/peerconnection.cc +++ b/pc/peerconnection.cc @@ -1592,10 +1592,68 @@ bool PeerConnection::GetStats(StatsObserver* observer, } void PeerConnection::GetStats(RTCStatsCollectorCallback* callback) { + TRACE_EVENT0("webrtc", "PeerConnection::GetStats"); RTC_DCHECK(stats_collector_); + RTC_DCHECK(callback); stats_collector_->GetStatsReport(callback); } +void PeerConnection::GetStats( + rtc::scoped_refptr selector, + rtc::scoped_refptr callback) { + TRACE_EVENT0("webrtc", "PeerConnection::GetStats"); + RTC_DCHECK(callback); + RTC_DCHECK(stats_collector_); + rtc::scoped_refptr internal_sender; + if (selector) { + for (const auto& proxy_transceiver : transceivers_) { + for (const auto& proxy_sender : + proxy_transceiver->internal()->senders()) { + if (proxy_sender == selector) { + internal_sender = proxy_sender->internal(); + break; + } + } + if (internal_sender) + break; + } + } + // If there is no |internal_sender| then |selector| is either null or does not + // belong to the PeerConnection (in Plan B, senders can be removed from the + // PeerConnection). This means that "all the stats objects representing the + // selector" is an empty set. Invoking GetStatsReport() with a null selector + // produces an empty stats report. + stats_collector_->GetStatsReport(internal_sender, callback); +} + +void PeerConnection::GetStats( + rtc::scoped_refptr selector, + rtc::scoped_refptr callback) { + TRACE_EVENT0("webrtc", "PeerConnection::GetStats"); + RTC_DCHECK(callback); + RTC_DCHECK(stats_collector_); + rtc::scoped_refptr internal_receiver; + if (selector) { + for (const auto& proxy_transceiver : transceivers_) { + for (const auto& proxy_receiver : + proxy_transceiver->internal()->receivers()) { + if (proxy_receiver == selector) { + internal_receiver = proxy_receiver->internal(); + break; + } + } + if (internal_receiver) + break; + } + } + // If there is no |internal_receiver| then |selector| is either null or does + // not belong to the PeerConnection (in Plan B, receivers can be removed from + // the PeerConnection). This means that "all the stats objects representing + // the selector" is an empty set. Invoking GetStatsReport() with a null + // selector produces an empty stats report. + stats_collector_->GetStatsReport(internal_receiver, callback); +} + PeerConnectionInterface::SignalingState PeerConnection::signaling_state() { return signaling_state_; } diff --git a/pc/peerconnection.h b/pc/peerconnection.h index 9051910d15..d1ba212e9c 100644 --- a/pc/peerconnection.h +++ b/pc/peerconnection.h @@ -116,10 +116,18 @@ class PeerConnection : public PeerConnectionInternal, rtc::scoped_refptr CreateDataChannel( const std::string& label, const DataChannelInit* config) override; + // WARNING: LEGACY. See peerconnectioninterface.h bool GetStats(StatsObserver* observer, webrtc::MediaStreamTrackInterface* track, StatsOutputLevel level) override; + // Spec-complaint GetStats(). See peerconnectioninterface.h void GetStats(RTCStatsCollectorCallback* callback) override; + void GetStats( + rtc::scoped_refptr selector, + rtc::scoped_refptr callback) override; + void GetStats( + rtc::scoped_refptr selector, + rtc::scoped_refptr callback) override; void ClearStatsCache() override; SignalingState signaling_state() override; diff --git a/pc/rtcstats_integrationtest.cc b/pc/rtcstats_integrationtest.cc index 40d07f7607..cdc46d7d81 100644 --- a/pc/rtcstats_integrationtest.cc +++ b/pc/rtcstats_integrationtest.cc @@ -137,10 +137,26 @@ class RTCStatsIntegrationTest : public testing::Test { rtc::scoped_refptr GetStatsFromCaller() { return GetStats(caller_->pc()); } + rtc::scoped_refptr GetStatsFromCaller( + rtc::scoped_refptr selector) { + return GetStats(caller_->pc(), selector); + } + rtc::scoped_refptr GetStatsFromCaller( + rtc::scoped_refptr selector) { + return GetStats(caller_->pc(), selector); + } rtc::scoped_refptr GetStatsFromCallee() { return GetStats(callee_->pc()); } + rtc::scoped_refptr GetStatsFromCallee( + rtc::scoped_refptr selector) { + return GetStats(callee_->pc(), selector); + } + rtc::scoped_refptr GetStatsFromCallee( + rtc::scoped_refptr selector) { + return GetStats(callee_->pc(), selector); + } protected: static rtc::scoped_refptr GetStats( @@ -152,6 +168,17 @@ class RTCStatsIntegrationTest : public testing::Test { return stats_obtainer->report(); } + template + static rtc::scoped_refptr GetStats( + PeerConnectionInterface* pc, + rtc::scoped_refptr selector) { + rtc::scoped_refptr stats_obtainer = + RTCStatsObtainer::Create(); + pc->GetStats(selector, stats_obtainer); + EXPECT_TRUE_WAIT(stats_obtainer->report(), kGetStatsTimeoutMs); + return stats_obtainer->report(); + } + // |network_thread_| uses |virtual_socket_server_| so they must be // constructed/destructed in the correct order. rtc::VirtualSocketServer virtual_socket_server_; @@ -315,7 +342,7 @@ class RTCStatsReportVerifier { : report_(report) { } - void VerifyReport() { + void VerifyReport(std::vector allowed_missing_stats) { std::set missing_stats = StatsTypes(); bool verify_successful = true; std::vector transport_stats = @@ -367,9 +394,10 @@ class RTCStatsReportVerifier { verify_successful = false; } } - if (!missing_stats.empty()) { - verify_successful = false; - for (const char* missing : missing_stats) { + for (const char* missing : missing_stats) { + if (std::find(allowed_missing_stats.begin(), allowed_missing_stats.end(), + missing) == allowed_missing_stats.end()) { + verify_successful = false; EXPECT_TRUE(false) << "Missing expected stats type: " << missing; } } @@ -718,7 +746,7 @@ TEST_F(RTCStatsIntegrationTest, GetStatsFromCaller) { StartCall(); rtc::scoped_refptr report = GetStatsFromCaller(); - RTCStatsReportVerifier(report.get()).VerifyReport(); + RTCStatsReportVerifier(report.get()).VerifyReport({}); EXPECT_EQ(report->ToJson(), RTCStatsReportTraceListener::last_trace()); } @@ -726,10 +754,68 @@ TEST_F(RTCStatsIntegrationTest, GetStatsFromCallee) { StartCall(); rtc::scoped_refptr report = GetStatsFromCallee(); - RTCStatsReportVerifier(report.get()).VerifyReport(); + RTCStatsReportVerifier(report.get()).VerifyReport({}); EXPECT_EQ(report->ToJson(), RTCStatsReportTraceListener::last_trace()); } +// These tests exercise the integration of the stats selection algorithm inside +// of PeerConnection. See rtcstatstraveral_unittest.cc for more detailed stats +// traversal tests on particular stats graphs. +TEST_F(RTCStatsIntegrationTest, GetStatsWithSenderSelector) { + StartCall(); + ASSERT_FALSE(caller_->pc()->GetSenders().empty()); + rtc::scoped_refptr report = + GetStatsFromCaller(caller_->pc()->GetSenders()[0]); + std::vector allowed_missing_stats = { + // TODO(hbos): Include RTC[Audio/Video]ReceiverStats when implemented. + // TODO(hbos): Include RTCRemoteOutboundRtpStreamStats when implemented. + // TODO(hbos): Include RTCRtpContributingSourceStats when implemented. + RTCInboundRTPStreamStats::kType, RTCPeerConnectionStats::kType, + RTCMediaStreamStats::kType, RTCDataChannelStats::kType, + }; + RTCStatsReportVerifier(report.get()).VerifyReport(allowed_missing_stats); + EXPECT_TRUE(report->size()); +} + +TEST_F(RTCStatsIntegrationTest, GetStatsWithReceiverSelector) { + StartCall(); + + ASSERT_FALSE(caller_->pc()->GetReceivers().empty()); + rtc::scoped_refptr report = + GetStatsFromCaller(caller_->pc()->GetReceivers()[0]); + std::vector allowed_missing_stats = { + // TODO(hbos): Include RTC[Audio/Video]SenderStats when implemented. + // TODO(hbos): Include RTCRemoteInboundRtpStreamStats when implemented. + // TODO(hbos): Include RTCRtpContributingSourceStats when implemented. + RTCOutboundRTPStreamStats::kType, RTCPeerConnectionStats::kType, + RTCMediaStreamStats::kType, RTCDataChannelStats::kType, + }; + RTCStatsReportVerifier(report.get()).VerifyReport(allowed_missing_stats); + EXPECT_TRUE(report->size()); +} + +TEST_F(RTCStatsIntegrationTest, GetStatsWithInvalidSenderSelector) { + StartCall(); + + ASSERT_FALSE(callee_->pc()->GetSenders().empty()); + // The selector is invalid for the caller because it belongs to the callee. + auto invalid_selector = callee_->pc()->GetSenders()[0]; + rtc::scoped_refptr report = + GetStatsFromCaller(invalid_selector); + EXPECT_FALSE(report->size()); +} + +TEST_F(RTCStatsIntegrationTest, GetStatsWithInvalidReceiverSelector) { + StartCall(); + + ASSERT_FALSE(callee_->pc()->GetReceivers().empty()); + // The selector is invalid for the caller because it belongs to the callee. + auto invalid_selector = callee_->pc()->GetReceivers()[0]; + rtc::scoped_refptr report = + GetStatsFromCaller(invalid_selector); + EXPECT_FALSE(report->size()); +} + TEST_F(RTCStatsIntegrationTest, GetsStatsWhileDestroyingPeerConnections) { StartCall(); diff --git a/pc/test/fakepeerconnectionbase.h b/pc/test/fakepeerconnectionbase.h index 2a46bf5481..9f670484e5 100644 --- a/pc/test/fakepeerconnectionbase.h +++ b/pc/test/fakepeerconnectionbase.h @@ -110,6 +110,12 @@ class FakePeerConnectionBase : public PeerConnectionInternal { } void GetStats(RTCStatsCollectorCallback* callback) override {} + void GetStats( + rtc::scoped_refptr selector, + rtc::scoped_refptr callback) override {} + void GetStats( + rtc::scoped_refptr selector, + rtc::scoped_refptr callback) override {} void ClearStatsCache() override {} diff --git a/stats/rtcstats_objects.cc b/stats/rtcstats_objects.cc index 0a176b4f64..7b697f0f77 100644 --- a/stats/rtcstats_objects.cc +++ b/stats/rtcstats_objects.cc @@ -262,7 +262,7 @@ RTCIceCandidatePairStats::~RTCIceCandidatePairStats() { } // clang-format off -WEBRTC_RTCSTATS_IMPL(RTCIceCandidateStats, RTCStats, "ice-candidate", +WEBRTC_RTCSTATS_IMPL(RTCIceCandidateStats, RTCStats, "abstract-ice-candidate", &transport_id, &is_remote, &network_type, @@ -323,6 +323,10 @@ RTCLocalIceCandidateStats::RTCLocalIceCandidateStats( : RTCIceCandidateStats(std::move(id), timestamp_us, false) { } +std::unique_ptr RTCLocalIceCandidateStats::copy() const { + return std::unique_ptr(new RTCLocalIceCandidateStats(*this)); +} + const char* RTCLocalIceCandidateStats::type() const { return kType; } @@ -339,6 +343,10 @@ RTCRemoteIceCandidateStats::RTCRemoteIceCandidateStats( : RTCIceCandidateStats(std::move(id), timestamp_us, true) { } +std::unique_ptr RTCRemoteIceCandidateStats::copy() const { + return std::unique_ptr(new RTCRemoteIceCandidateStats(*this)); +} + const char* RTCRemoteIceCandidateStats::type() const { return kType; }