From 0f1376529a18810280fb4b0e641e01254aaba048 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Bostr=C3=B6m?= Date: Wed, 24 May 2023 17:57:12 +0200 Subject: [PATCH] Delete RTC[NonStandard/Restricted]StatsMember. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Whether a metric is to be exposed to JavaScript or not is a blink implementation detail that the WebRTC repository does not need to be concerned with. This CL removes unused code and paves the way for the possibility of making the one and only RTCStatsMember class be absl::optional<>-based in the future. Bug: webrtc:15162 Change-Id: I578715f48b8fcc3534b72b4c700fd6567f8d553e Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/304722 Reviewed-by: Harald Alvestrand Commit-Queue: Henrik Boström Cr-Commit-Position: refs/heads/main@{#40139} --- api/stats/rtc_stats.h | 167 +------------------------------- api/stats/rtcstats_objects.h | 40 ++++---- pc/rtc_stats_integrationtest.cc | 6 -- stats/rtc_stats.cc | 77 --------------- stats/rtc_stats_unittest.cc | 21 ---- 5 files changed, 21 insertions(+), 290 deletions(-) diff --git a/api/stats/rtc_stats.h b/api/stats/rtc_stats.h index 0bd70a0fb5..6cc39a309f 100644 --- a/api/stats/rtc_stats.h +++ b/api/stats/rtc_stats.h @@ -206,19 +206,6 @@ class RTC_EXPORT RTCStats { return parent_class::MembersOfThisObjectAndAncestors(0); \ } -// Certain stat members should only be exposed to the JavaScript API in -// certain circumstances as to avoid passive fingerprinting. -enum class StatExposureCriteria : uint8_t { - // The stat should always be exposed. This is the default. - kAlways, - // The stat exposes hardware capabilities and thus should has limited exposure - // to JavaScript. The requirements for exposure are written in the spec at - // https://w3c.github.io/webrtc-stats/#limiting-exposure-of-hardware-capabilities. - kHardwareCapability, - // The stat is non-standard so user agents should filter these. - kNonStandard, -}; - // Interface for `RTCStats` members, which have a name and a value of a type // defined in a subclass. Only the types listed in `Type` are supported, these // are implemented by `RTCStatsMember`. The value of a member may be @@ -254,17 +241,6 @@ class RTCStatsMemberInterface { virtual bool is_sequence() const = 0; virtual bool is_string() const = 0; virtual bool is_defined() const = 0; - // Is this part of the stats spec? Used so that chromium can easily filter - // out anything unstandardized. - bool is_standardized() const { - return exposure_criteria() != StatExposureCriteria::kNonStandard; - } - // The conditions for exposing the statistic to JavaScript. Stats with - // criteria that is not kAlways has some restriction and should be filtered - // in accordance to the spec. - virtual StatExposureCriteria exposure_criteria() const { - return StatExposureCriteria::kAlways; - } // Type and value comparator. The names are not compared. These operators are // exposed for testing. bool operator==(const RTCStatsMemberInterface& other) const { @@ -360,9 +336,7 @@ class RTCStatsMember : public RTCStatsMemberInterface { protected: bool IsEqual(const RTCStatsMemberInterface& other) const override { - if (type() != other.type() || - is_standardized() != other.is_standardized() || - exposure_criteria() != other.exposure_criteria()) + if (type() != other.type()) return false; const RTCStatsMember& other_t = static_cast&>(other); @@ -411,145 +385,6 @@ WEBRTC_DECLARE_RTCSTATSMEMBER(std::vector); WEBRTC_DECLARE_RTCSTATSMEMBER(rtc_stats_internal::MapStringUint64); WEBRTC_DECLARE_RTCSTATSMEMBER(rtc_stats_internal::MapStringDouble); -// For stats with restricted exposure. -template -class RTCRestrictedStatsMember : public RTCStatsMember { - public: - explicit RTCRestrictedStatsMember(const char* name) - : RTCStatsMember(name) {} - RTCRestrictedStatsMember(const char* name, const T& value) - : RTCStatsMember(name, value) {} - RTCRestrictedStatsMember(const char* name, T&& value) - : RTCStatsMember(name, std::move(value)) {} - RTCRestrictedStatsMember(const RTCRestrictedStatsMember& other) - : RTCStatsMember(other) {} - RTCRestrictedStatsMember(RTCRestrictedStatsMember&& other) - : RTCStatsMember(std::move(other)) {} - - StatExposureCriteria exposure_criteria() const override { return E; } - - T& operator=(const T& value) { return RTCStatsMember::operator=(value); } - T& operator=(const T&& value) { - return RTCStatsMember::operator=(std::move(value)); - } - - private: - static_assert(E != StatExposureCriteria::kAlways, - "kAlways is the default exposure criteria. Use " - "RTCStatMember instead."); -}; - -extern template class RTC_EXPORT_TEMPLATE_DECLARE(RTC_EXPORT) - RTCRestrictedStatsMember; -extern template class RTC_EXPORT_TEMPLATE_DECLARE(RTC_EXPORT) - RTCRestrictedStatsMember; -extern template class RTC_EXPORT_TEMPLATE_DECLARE(RTC_EXPORT) - RTCRestrictedStatsMember; -extern template class RTC_EXPORT_TEMPLATE_DECLARE(RTC_EXPORT) - RTCRestrictedStatsMember; -extern template class RTC_EXPORT_TEMPLATE_DECLARE(RTC_EXPORT) - RTCRestrictedStatsMember; -extern template class RTC_EXPORT_TEMPLATE_DECLARE(RTC_EXPORT) - RTCRestrictedStatsMember; -extern template class RTC_EXPORT_TEMPLATE_DECLARE(RTC_EXPORT) - RTCRestrictedStatsMember; -extern template class RTC_EXPORT_TEMPLATE_DECLARE(RTC_EXPORT) - RTCRestrictedStatsMember, - StatExposureCriteria::kHardwareCapability>; -extern template class RTC_EXPORT_TEMPLATE_DECLARE(RTC_EXPORT) - RTCRestrictedStatsMember, - StatExposureCriteria::kHardwareCapability>; -extern template class RTC_EXPORT_TEMPLATE_DECLARE(RTC_EXPORT) - RTCRestrictedStatsMember, - StatExposureCriteria::kHardwareCapability>; -extern template class RTC_EXPORT_TEMPLATE_DECLARE(RTC_EXPORT) - RTCRestrictedStatsMember, - StatExposureCriteria::kHardwareCapability>; -extern template class RTC_EXPORT_TEMPLATE_DECLARE(RTC_EXPORT) - RTCRestrictedStatsMember, - StatExposureCriteria::kHardwareCapability>; -extern template class RTC_EXPORT_TEMPLATE_DECLARE(RTC_EXPORT) - RTCRestrictedStatsMember, - StatExposureCriteria::kHardwareCapability>; -extern template class RTC_EXPORT_TEMPLATE_DECLARE(RTC_EXPORT) - RTCRestrictedStatsMember, - StatExposureCriteria::kHardwareCapability>; -extern template class RTC_EXPORT_TEMPLATE_DECLARE(RTC_EXPORT) - RTCRestrictedStatsMember, - StatExposureCriteria::kHardwareCapability>; -extern template class RTC_EXPORT_TEMPLATE_DECLARE(RTC_EXPORT) - RTCRestrictedStatsMember, - StatExposureCriteria::kHardwareCapability>; - -// Using inheritance just so that it's obvious from the member's declaration -// whether it's standardized or not. -template -class RTCNonStandardStatsMember - : public RTCRestrictedStatsMember { - public: - explicit RTCNonStandardStatsMember(const char* name) - : RTCRestrictedStatsBase(name) {} - RTCNonStandardStatsMember(const char* name, const T& value) - : RTCRestrictedStatsBase(name, value) {} - RTCNonStandardStatsMember(const char* name, T&& value) - : RTCRestrictedStatsBase(name, std::move(value)) {} - RTCNonStandardStatsMember(const RTCNonStandardStatsMember& other) - : RTCRestrictedStatsBase(other) {} - RTCNonStandardStatsMember(RTCNonStandardStatsMember&& other) - : RTCRestrictedStatsBase(std::move(other)) {} - - T& operator=(const T& value) { - return RTCRestrictedStatsMember< - T, StatExposureCriteria::kNonStandard>::operator=(value); - } - T& operator=(const T&& value) { - return RTCRestrictedStatsMember< - T, StatExposureCriteria::kNonStandard>::operator=(std::move(value)); - } - - private: - using RTCRestrictedStatsBase = - RTCRestrictedStatsMember; -}; - -extern template class RTC_EXPORT_TEMPLATE_DECLARE(RTC_EXPORT) - RTCNonStandardStatsMember; -extern template class RTC_EXPORT_TEMPLATE_DECLARE(RTC_EXPORT) - RTCNonStandardStatsMember; -extern template class RTC_EXPORT_TEMPLATE_DECLARE(RTC_EXPORT) - RTCNonStandardStatsMember; -extern template class RTC_EXPORT_TEMPLATE_DECLARE(RTC_EXPORT) - RTCNonStandardStatsMember; -extern template class RTC_EXPORT_TEMPLATE_DECLARE(RTC_EXPORT) - RTCNonStandardStatsMember; -extern template class RTC_EXPORT_TEMPLATE_DECLARE(RTC_EXPORT) - RTCNonStandardStatsMember; -extern template class RTC_EXPORT_TEMPLATE_DECLARE(RTC_EXPORT) - RTCNonStandardStatsMember; -extern template class RTC_EXPORT_TEMPLATE_DECLARE(RTC_EXPORT) - RTCNonStandardStatsMember>; -extern template class RTC_EXPORT_TEMPLATE_DECLARE(RTC_EXPORT) - RTCNonStandardStatsMember>; -extern template class RTC_EXPORT_TEMPLATE_DECLARE(RTC_EXPORT) - RTCNonStandardStatsMember>; -extern template class RTC_EXPORT_TEMPLATE_DECLARE(RTC_EXPORT) - RTCNonStandardStatsMember>; -extern template class RTC_EXPORT_TEMPLATE_DECLARE(RTC_EXPORT) - RTCNonStandardStatsMember>; -extern template class RTC_EXPORT_TEMPLATE_DECLARE(RTC_EXPORT) - RTCNonStandardStatsMember>; -extern template class RTC_EXPORT_TEMPLATE_DECLARE(RTC_EXPORT) - RTCNonStandardStatsMember>; -extern template class RTC_EXPORT_TEMPLATE_DECLARE(RTC_EXPORT) - RTCNonStandardStatsMember>; -extern template class RTC_EXPORT_TEMPLATE_DECLARE(RTC_EXPORT) - RTCNonStandardStatsMember>; - } // namespace webrtc #endif // API_STATS_RTC_STATS_H_ diff --git a/api/stats/rtcstats_objects.h b/api/stats/rtcstats_objects.h index 38aa57b48f..58ca165d82 100644 --- a/api/stats/rtcstats_objects.h +++ b/api/stats/rtcstats_objects.h @@ -251,8 +251,10 @@ class RTC_EXPORT RTCIceCandidateStats : public RTCStats { // Enum type RTCIceTcpCandidateType. RTCStatsMember tcp_type; - RTCNonStandardStatsMember vpn; - RTCNonStandardStatsMember network_adapter_type; + // The following metrics are NOT exposed to JavaScript. We should consider + // standardizing or removing them. + RTCStatsMember vpn; + RTCStatsMember network_adapter_type; protected: RTCIceCandidateStats(std::string id, Timestamp timestamp, bool is_remote); @@ -402,9 +404,8 @@ class RTC_EXPORT RTCInboundRtpStreamStats final // TODO(https://crbug.com/webrtc/14177): Expose even if A/V sync is off? RTCStatsMember estimated_playout_timestamp; // Only defined for video. - RTCRestrictedStatsMember - decoder_implementation; + // In JavaScript, this is only exposed if HW exposure is allowed. + RTCStatsMember decoder_implementation; // FIR and PLI counts are only defined for |kind == "video"|. RTCStatsMember fir_count; RTCStatsMember pli_count; @@ -417,17 +418,17 @@ class RTC_EXPORT RTCInboundRtpStreamStats final // TimingFrameInfo::ToString(). // TODO(https://crbug.com/webrtc/14586): Unship or standardize this metric. RTCStatsMember goog_timing_frame_info; - RTCRestrictedStatsMember - power_efficient_decoder; - // Non-standard audio metrics. - RTCNonStandardStatsMember jitter_buffer_flushes; - RTCNonStandardStatsMember delayed_packet_outage_samples; - RTCNonStandardStatsMember relative_packet_arrival_delay; - RTCNonStandardStatsMember interruption_count; - RTCNonStandardStatsMember total_interruption_duration; + // In JavaScript, this is only exposed if HW exposure is allowed. + RTCStatsMember power_efficient_decoder; - // The former googMinPlayoutDelayMs (in seconds). - RTCNonStandardStatsMember min_playout_delay; + // The following metrics are NOT exposed to JavaScript. We should consider + // standardizing or removing them. + RTCStatsMember jitter_buffer_flushes; + RTCStatsMember delayed_packet_outage_samples; + RTCStatsMember relative_packet_arrival_delay; + RTCStatsMember interruption_count; + RTCStatsMember total_interruption_duration; + RTCStatsMember min_playout_delay; }; // https://w3c.github.io/webrtc-stats/#outboundrtpstats-dict* @@ -465,19 +466,18 @@ class RTC_EXPORT RTCOutboundRtpStreamStats final RTCStatsMember quality_limitation_resolution_changes; // https://w3c.github.io/webrtc-provisional-stats/#dom-rtcoutboundrtpstreamstats-contenttype RTCStatsMember content_type; + // In JavaScript, this is only exposed if HW exposure is allowed. // Only implemented for video. // TODO(https://crbug.com/webrtc/14178): Implement for audio as well. - RTCRestrictedStatsMember - encoder_implementation; + RTCStatsMember encoder_implementation; // FIR and PLI counts are only defined for |kind == "video"|. RTCStatsMember fir_count; RTCStatsMember pli_count; RTCStatsMember nack_count; RTCStatsMember qp_sum; RTCStatsMember active; - RTCRestrictedStatsMember - power_efficient_encoder; + // In JavaScript, this is only exposed if HW exposure is allowed. + RTCStatsMember power_efficient_encoder; RTCStatsMember scalability_mode; }; diff --git a/pc/rtc_stats_integrationtest.cc b/pc/rtc_stats_integrationtest.cc index 34958075bc..fa9f2263f3 100644 --- a/pc/rtc_stats_integrationtest.cc +++ b/pc/rtc_stats_integrationtest.cc @@ -595,14 +595,10 @@ class RTCStatsReportVerifier { verifier.TestMemberIsNonNegative(inbound_stream.qp_sum); verifier.TestMemberIsDefined(inbound_stream.decoder_implementation); verifier.TestMemberIsDefined(inbound_stream.power_efficient_decoder); - EXPECT_EQ(inbound_stream.power_efficient_decoder.exposure_criteria(), - StatExposureCriteria::kHardwareCapability); } else { verifier.TestMemberIsUndefined(inbound_stream.qp_sum); verifier.TestMemberIsUndefined(inbound_stream.decoder_implementation); verifier.TestMemberIsUndefined(inbound_stream.power_efficient_decoder); - EXPECT_EQ(inbound_stream.power_efficient_decoder.exposure_criteria(), - StatExposureCriteria::kHardwareCapability); } verifier.TestMemberIsNonNegative(inbound_stream.packets_received); if (inbound_stream.kind.is_defined() && *inbound_stream.kind == "audio") { @@ -829,8 +825,6 @@ class RTCStatsReportVerifier { verifier.MarkMemberTested(outbound_stream.content_type, true); verifier.TestMemberIsDefined(outbound_stream.encoder_implementation); verifier.TestMemberIsDefined(outbound_stream.power_efficient_encoder); - EXPECT_EQ(outbound_stream.power_efficient_encoder.exposure_criteria(), - StatExposureCriteria::kHardwareCapability); // Unless an implementation-specific amount of time has passed and at // least one frame has been encoded, undefined is reported. Because it // is hard to tell what is the case here, we treat FPS as optional. diff --git a/stats/rtc_stats.cc b/stats/rtc_stats.cc index 421d9690d7..25bde289c2 100644 --- a/stats/rtc_stats.cc +++ b/stats/rtc_stats.cc @@ -294,81 +294,4 @@ WEBRTC_DEFINE_RTCSTATSMEMBER(rtc_stats_internal::MapStringDouble, MapToString(*value_), MapToStringAsDouble(*value_)); -// Restricted members that expose hardware capabilites. -template class RTC_EXPORT_TEMPLATE_DEFINE(RTC_EXPORT) - RTCRestrictedStatsMember; -template class RTC_EXPORT_TEMPLATE_DEFINE(RTC_EXPORT) - RTCRestrictedStatsMember; -template class RTC_EXPORT_TEMPLATE_DEFINE(RTC_EXPORT) - RTCRestrictedStatsMember; -template class RTC_EXPORT_TEMPLATE_DEFINE(RTC_EXPORT) - RTCRestrictedStatsMember; -template class RTC_EXPORT_TEMPLATE_DEFINE(RTC_EXPORT) - RTCRestrictedStatsMember; -template class RTC_EXPORT_TEMPLATE_DEFINE(RTC_EXPORT) - RTCRestrictedStatsMember; -template class RTC_EXPORT_TEMPLATE_DEFINE(RTC_EXPORT) - RTCRestrictedStatsMember; -template class RTC_EXPORT_TEMPLATE_DEFINE(RTC_EXPORT) - RTCRestrictedStatsMember, - StatExposureCriteria::kHardwareCapability>; -template class RTC_EXPORT_TEMPLATE_DEFINE(RTC_EXPORT) - RTCRestrictedStatsMember, - StatExposureCriteria::kHardwareCapability>; -template class RTC_EXPORT_TEMPLATE_DEFINE(RTC_EXPORT) - RTCRestrictedStatsMember, - StatExposureCriteria::kHardwareCapability>; -template class RTC_EXPORT_TEMPLATE_DEFINE(RTC_EXPORT) - RTCRestrictedStatsMember, - StatExposureCriteria::kHardwareCapability>; -template class RTC_EXPORT_TEMPLATE_DEFINE(RTC_EXPORT) - RTCRestrictedStatsMember, - StatExposureCriteria::kHardwareCapability>; -template class RTC_EXPORT_TEMPLATE_DEFINE(RTC_EXPORT) - RTCRestrictedStatsMember, - StatExposureCriteria::kHardwareCapability>; -template class RTC_EXPORT_TEMPLATE_DEFINE(RTC_EXPORT) - RTCRestrictedStatsMember, - StatExposureCriteria::kHardwareCapability>; -template class RTC_EXPORT_TEMPLATE_DEFINE(RTC_EXPORT) - RTCRestrictedStatsMember, - StatExposureCriteria::kHardwareCapability>; -template class RTC_EXPORT_TEMPLATE_DEFINE(RTC_EXPORT) - RTCRestrictedStatsMember, - StatExposureCriteria::kHardwareCapability>; - -template class RTC_EXPORT_TEMPLATE_DEFINE(RTC_EXPORT) - RTCNonStandardStatsMember; -template class RTC_EXPORT_TEMPLATE_DEFINE(RTC_EXPORT) - RTCNonStandardStatsMember; -template class RTC_EXPORT_TEMPLATE_DEFINE(RTC_EXPORT) - RTCNonStandardStatsMember; -template class RTC_EXPORT_TEMPLATE_DEFINE(RTC_EXPORT) - RTCNonStandardStatsMember; -template class RTC_EXPORT_TEMPLATE_DEFINE(RTC_EXPORT) - RTCNonStandardStatsMember; -template class RTC_EXPORT_TEMPLATE_DEFINE(RTC_EXPORT) - RTCNonStandardStatsMember; -template class RTC_EXPORT_TEMPLATE_DEFINE(RTC_EXPORT) - RTCNonStandardStatsMember; -template class RTC_EXPORT_TEMPLATE_DEFINE(RTC_EXPORT) - RTCNonStandardStatsMember>; -template class RTC_EXPORT_TEMPLATE_DEFINE(RTC_EXPORT) - RTCNonStandardStatsMember>; -template class RTC_EXPORT_TEMPLATE_DEFINE(RTC_EXPORT) - RTCNonStandardStatsMember>; -template class RTC_EXPORT_TEMPLATE_DEFINE(RTC_EXPORT) - RTCNonStandardStatsMember>; -template class RTC_EXPORT_TEMPLATE_DEFINE(RTC_EXPORT) - RTCNonStandardStatsMember>; -template class RTC_EXPORT_TEMPLATE_DEFINE(RTC_EXPORT) - RTCNonStandardStatsMember>; -template class RTC_EXPORT_TEMPLATE_DEFINE(RTC_EXPORT) - RTCNonStandardStatsMember>; - } // namespace webrtc diff --git a/stats/rtc_stats_unittest.cc b/stats/rtc_stats_unittest.cc index 5e4b5f12a4..249491effd 100644 --- a/stats/rtc_stats_unittest.cc +++ b/stats/rtc_stats_unittest.cc @@ -387,13 +387,6 @@ TEST(RTCStatsTest, RTCStatsPrintsValidJson) { std::cout << stats.ToJson() << std::endl; } -TEST(RTCStatsTest, IsStandardized) { - RTCStatsMember standardized("standardized"); - RTCNonStandardStatsMember unstandardized("unstandardized"); - EXPECT_TRUE(standardized.is_standardized()); - EXPECT_FALSE(unstandardized.is_standardized()); -} - TEST(RTCStatsTest, IsSequence) { RTCTestStats stats("statsId", Timestamp::Micros(42)); EXPECT_FALSE(stats.m_bool.is_sequence()); @@ -504,20 +497,6 @@ TEST(RTCStatsTest, ValueToString) { EXPECT_EQ("{bar:0.25,foo:0.5}", stats.m_map_string_double.ValueToString()); } -TEST(RTCStatsTest, RestrictedStatsTest) { - RTCStatsMember unrestricted("unrestricted"); - EXPECT_EQ(unrestricted.exposure_criteria(), StatExposureCriteria::kAlways); - RTCRestrictedStatsMember - restricted("restricted"); - EXPECT_EQ(restricted.exposure_criteria(), - StatExposureCriteria::kHardwareCapability); - - unrestricted = true; - restricted = true; - EXPECT_NE(unrestricted, restricted) - << "These can not be equal as they have different exposure criteria."; -} - // Death tests. // Disabled on Android because death tests misbehave on Android, see // base/test/gtest_util.h.