diff --git a/api/BUILD.gn b/api/BUILD.gn index f0bc7ba322..12511518f5 100644 --- a/api/BUILD.gn +++ b/api/BUILD.gn @@ -1362,6 +1362,25 @@ if (rtc_include_tests) { } } +rtc_source_set("field_trials_registry") { + visibility = [ "*" ] + sources = [ + "field_trials_registry.cc", + "field_trials_registry.h", + ] + deps = [ + ":field_trials_view", + "../experiments:registered_field_trials", + "../rtc_base:checks", + "../rtc_base/containers:flat_set", + "../rtc_base/system:rtc_export", + ] + absl_deps = [ + "//third_party/abseil-cpp/absl/algorithm:container", + "//third_party/abseil-cpp/absl/strings", + ] +} + rtc_source_set("field_trials_view") { visibility = [ "*" ] sources = [ "field_trials_view.h" ] @@ -1382,7 +1401,7 @@ rtc_library("field_trials") { "field_trials.h", ] deps = [ - ":field_trials_view", + ":field_trials_registry", "../rtc_base:checks", "../rtc_base/containers:flat_map", "../system_wrappers:field_trial", diff --git a/api/DEPS b/api/DEPS index c8c8ac0517..5f01204068 100644 --- a/api/DEPS +++ b/api/DEPS @@ -312,6 +312,10 @@ specific_include_rules = { "+rtc_base/thread.h", ], + "field_trials_registry\.h": [ + "+rtc_base/containers/flat_set.h", + ], + # .cc files in api/ should not be restricted in what they can #include, # so we re-add all the top-level directories here. (That's because .h # files leak their #includes to whoever's #including them, but .cc files @@ -322,6 +326,7 @@ specific_include_rules = { "+common_audio", "+common_video", "+examples", + "+experiments", "+logging", "+media", "+modules", diff --git a/api/field_trials.cc b/api/field_trials.cc index d6b53acafb..4bd11271dc 100644 --- a/api/field_trials.cc +++ b/api/field_trials.cc @@ -90,7 +90,7 @@ FieldTrials::~FieldTrials() { } } -std::string FieldTrials::Lookup(absl::string_view key) const { +std::string FieldTrials::GetValue(absl::string_view key) const { auto it = key_value_map_.find(std::string(key)); if (it != key_value_map_.end()) return it->second; diff --git a/api/field_trials.h b/api/field_trials.h index 0bfa4b7871..bf7a7cc625 100644 --- a/api/field_trials.h +++ b/api/field_trials.h @@ -15,7 +15,7 @@ #include #include "absl/strings/string_view.h" -#include "api/field_trials_view.h" +#include "api/field_trials_registry.h" #include "rtc_base/containers/flat_map.h" namespace webrtc { @@ -34,7 +34,7 @@ namespace webrtc { // NOTE: Creating multiple FieldTrials-object is currently prohibited // until we remove the global string (TODO(bugs.webrtc.org/10335)) // (unless using CreateNoGlobal): -class FieldTrials : public FieldTrialsView { +class FieldTrials : public FieldTrialsRegistry { public: explicit FieldTrials(const std::string& s); ~FieldTrials(); @@ -43,10 +43,11 @@ class FieldTrials : public FieldTrialsView { // global variable (i.e can not be used for all parts of webrtc). static std::unique_ptr CreateNoGlobal(const std::string& s); - std::string Lookup(absl::string_view key) const override; - private: explicit FieldTrials(const std::string& s, bool); + + std::string GetValue(absl::string_view key) const override; + const bool uses_global_; const std::string field_trial_string_; const char* const previous_field_trial_string_; diff --git a/api/field_trials_registry.cc b/api/field_trials_registry.cc new file mode 100644 index 0000000000..f97e8a85a9 --- /dev/null +++ b/api/field_trials_registry.cc @@ -0,0 +1,31 @@ +/* + * Copyright 2022 The WebRTC project authors. All Rights Reserved. + * + * Use of this source code is governed by a BSD-style license + * that can be found in the LICENSE file in the root of the source + * tree. An additional intellectual property rights grant can be found + * in the file PATENTS. All contributing project authors may + * be found in the AUTHORS file in the root of the source tree. + */ +#include "api/field_trials_registry.h" + +#include + +#include "absl/algorithm/container.h" +#include "absl/strings/string_view.h" +#include "experiments/registered_field_trials.h" +#include "rtc_base/checks.h" +#include "rtc_base/containers/flat_set.h" + +namespace webrtc { + +std::string FieldTrialsRegistry::Lookup(absl::string_view key) const { +#if WEBRTC_STRICT_FIELD_TRIALS + RTC_DCHECK(absl::c_linear_search(kRegisteredFieldTrials, key) || + test_keys_.contains(key)) + << key << " is not registered."; +#endif + return GetValue(key); +} + +} // namespace webrtc diff --git a/api/field_trials_registry.h b/api/field_trials_registry.h new file mode 100644 index 0000000000..dc7e8445b1 --- /dev/null +++ b/api/field_trials_registry.h @@ -0,0 +1,54 @@ +/* + * Copyright 2022 The WebRTC project authors. All Rights Reserved. + * + * Use of this source code is governed by a BSD-style license + * that can be found in the LICENSE file in the root of the source + * tree. An additional intellectual property rights grant can be found + * in the file PATENTS. All contributing project authors may + * be found in the AUTHORS file in the root of the source tree. + */ +#ifndef API_FIELD_TRIALS_REGISTRY_H_ +#define API_FIELD_TRIALS_REGISTRY_H_ + +#include +#include + +#include "absl/strings/string_view.h" +#include "api/field_trials_view.h" +#include "rtc_base/containers/flat_set.h" +#include "rtc_base/system/rtc_export.h" + +namespace webrtc { + +// Abstract base class for a field trial registry that verifies that any looked +// up key has been pre-registered in accordance with `g3doc/field-trials.md`. +class RTC_EXPORT FieldTrialsRegistry : public FieldTrialsView { + public: + FieldTrialsRegistry() = default; + + FieldTrialsRegistry(const FieldTrialsRegistry&) = default; + FieldTrialsRegistry& operator=(const FieldTrialsRegistry&) = default; + + ~FieldTrialsRegistry() override = default; + + // Verifies that `key` is a registered field trial and then returns the + // configured value for `key` or an empty string if the field trial isn't + // configured. + std::string Lookup(absl::string_view key) const override; + + // Register additional `keys` for testing. This should only be used for + // imaginary keys that are never used outside test code. + void RegisterKeysForTesting(flat_set keys) { + test_keys_ = std::move(keys); + } + + private: + virtual std::string GetValue(absl::string_view key) const = 0; + + // Imaginary keys only used for testing. + flat_set test_keys_; +}; + +} // namespace webrtc + +#endif // API_FIELD_TRIALS_REGISTRY_H_ diff --git a/api/field_trials_unittest.cc b/api/field_trials_unittest.cc index 5f0188e557..2616a2e30c 100644 --- a/api/field_trials_unittest.cc +++ b/api/field_trials_unittest.cc @@ -34,6 +34,8 @@ using ::webrtc::field_trial::ScopedGlobalFieldTrialsForTesting; TEST(FieldTrialsTest, EmptyStringHasNoEffect) { ScopedGlobalFieldTrialsForTesting g({"MyCoolTrial"}); FieldTrials f(""); + f.RegisterKeysForTesting({"MyCoolTrial"}); + EXPECT_FALSE(f.IsEnabled("MyCoolTrial")); EXPECT_FALSE(f.IsDisabled("MyCoolTrial")); } @@ -43,6 +45,8 @@ TEST(FieldTrialsTest, EnabledDisabledMustBeFirstInValue) { "MyCoolTrial/EnabledFoo/" "MyUncoolTrial/DisabledBar/" "AnotherTrial/BazEnabled/"); + f.RegisterKeysForTesting({"MyCoolTrial", "MyUncoolTrial", "AnotherTrial"}); + EXPECT_TRUE(f.IsEnabled("MyCoolTrial")); EXPECT_TRUE(f.IsDisabled("MyUncoolTrial")); EXPECT_FALSE(f.IsEnabled("AnotherTrial")); @@ -53,6 +57,8 @@ TEST(FieldTrialsTest, FieldTrialsDoesNotReadGlobalString) { static constexpr char s[] = "MyCoolTrial/Enabled/MyUncoolTrial/Disabled/"; InitFieldTrialsFromString(s); FieldTrials f(""); + f.RegisterKeysForTesting({"MyCoolTrial", "MyUncoolTrial"}); + EXPECT_FALSE(f.IsEnabled("MyCoolTrial")); EXPECT_FALSE(f.IsDisabled("MyUncoolTrial")); } @@ -93,6 +99,8 @@ TEST(FieldTrialsTest, NonGlobalFieldTrialsInstanceDoesNotModifyGlobalString) { std::unique_ptr f = FieldTrials::CreateNoGlobal("SomeString/Enabled/"); ASSERT_THAT(f, NotNull()); + f->RegisterKeysForTesting({"SomeString"}); + EXPECT_TRUE(f->IsEnabled("SomeString")); EXPECT_FALSE(webrtc::field_trial::IsEnabled("SomeString")); } @@ -104,6 +112,8 @@ TEST(FieldTrialsTest, NonGlobalFieldTrialsSupportSimultaneousInstances) { FieldTrials::CreateNoGlobal("SomeOtherString/Enabled/"); ASSERT_THAT(f1, NotNull()); ASSERT_THAT(f2, NotNull()); + f1->RegisterKeysForTesting({"SomeString", "SomeOtherString"}); + f2->RegisterKeysForTesting({"SomeString", "SomeOtherString"}); EXPECT_TRUE(f1->IsEnabled("SomeString")); EXPECT_FALSE(f1->IsEnabled("SomeOtherString")); @@ -118,6 +128,8 @@ TEST(FieldTrialsTest, GlobalAndNonGlobalFieldTrialsAreDisjoint) { std::unique_ptr f2 = FieldTrials::CreateNoGlobal("SomeOtherString/Enabled/"); ASSERT_THAT(f2, NotNull()); + f1.RegisterKeysForTesting({"SomeString", "SomeOtherString"}); + f2->RegisterKeysForTesting({"SomeString", "SomeOtherString"}); EXPECT_TRUE(f1.IsEnabled("SomeString")); EXPECT_FALSE(f1.IsEnabled("SomeOtherString")); @@ -131,6 +143,8 @@ TEST(FieldTrialsTest, FieldTrialBasedConfigReadsGlobalString) { static constexpr char s[] = "MyCoolTrial/Enabled/MyUncoolTrial/Disabled/"; InitFieldTrialsFromString(s); FieldTrialBasedConfig f; + f.RegisterKeysForTesting({"MyCoolTrial", "MyUncoolTrial"}); + EXPECT_TRUE(f.IsEnabled("MyCoolTrial")); EXPECT_TRUE(f.IsDisabled("MyUncoolTrial")); } diff --git a/api/field_trials_view.h b/api/field_trials_view.h index 299205d1d3..45e6f7899b 100644 --- a/api/field_trials_view.h +++ b/api/field_trials_view.h @@ -17,15 +17,18 @@ namespace webrtc { -// An interface that provides a key-value mapping for configuring internal -// details of WebRTC. Note that there's no guarantess that the meaning of a -// particular key value mapping will be preserved over time and no announcements -// will be made if they are changed. It's up to the library user to ensure that -// the behavior does not break. +// An interface that provides the means to access field trials. +// +// Note that there are no guarantess that the meaning of a particular key-value +// mapping will be preserved over time and no announcements will be made if they +// are changed. It's up to the library user to ensure that the behavior does not +// break. class RTC_EXPORT FieldTrialsView { public: virtual ~FieldTrialsView() = default; - // The configured value for the given key. Defaults to an empty string. + + // Returns the configured value for `key` or an empty string if the field + // trial isn't configured. virtual std::string Lookup(absl::string_view key) const = 0; bool IsEnabled(absl::string_view key) const { diff --git a/api/transport/BUILD.gn b/api/transport/BUILD.gn index 3cc3559f30..86a7c8acf8 100644 --- a/api/transport/BUILD.gn +++ b/api/transport/BUILD.gn @@ -52,7 +52,7 @@ rtc_library("field_trial_based_config") { "field_trial_based_config.h", ] deps = [ - "../../api:field_trials_view", + "../../api:field_trials_registry", "../../system_wrappers:field_trial", ] absl_deps = [ "//third_party/abseil-cpp/absl/strings" ] diff --git a/api/transport/field_trial_based_config.cc b/api/transport/field_trial_based_config.cc index 4a3a179240..0cef30f054 100644 --- a/api/transport/field_trial_based_config.cc +++ b/api/transport/field_trial_based_config.cc @@ -12,7 +12,7 @@ #include "system_wrappers/include/field_trial.h" namespace webrtc { -std::string FieldTrialBasedConfig::Lookup(absl::string_view key) const { +std::string FieldTrialBasedConfig::GetValue(absl::string_view key) const { return webrtc::field_trial::FindFullName(std::string(key)); } } // namespace webrtc diff --git a/api/transport/field_trial_based_config.h b/api/transport/field_trial_based_config.h index f0063ff95e..d47140e579 100644 --- a/api/transport/field_trial_based_config.h +++ b/api/transport/field_trial_based_config.h @@ -13,13 +13,13 @@ #include #include "absl/strings/string_view.h" -#include "api/field_trials_view.h" +#include "api/field_trials_registry.h" namespace webrtc { // Implementation using the field trial API fo the key value lookup. -class FieldTrialBasedConfig : public FieldTrialsView { - public: - std::string Lookup(absl::string_view key) const override; +class FieldTrialBasedConfig : public FieldTrialsRegistry { + private: + std::string GetValue(absl::string_view key) const override; }; } // namespace webrtc diff --git a/modules/rtp_rtcp/BUILD.gn b/modules/rtp_rtcp/BUILD.gn index 736ca58fd1..ead114bc91 100644 --- a/modules/rtp_rtcp/BUILD.gn +++ b/modules/rtp_rtcp/BUILD.gn @@ -604,6 +604,7 @@ if (rtc_include_tests) { ":rtp_rtcp_legacy", "../../api:array_view", "../../api:create_time_controller", + "../../api:field_trials_registry", "../../api:libjingle_peerconnection_api", "../../api:mock_frame_encryptor", "../../api:rtp_headers", diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl2_unittest.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl2_unittest.cc index 4c08ce5c13..4dece662ec 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl2_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl2_unittest.cc @@ -17,7 +17,7 @@ #include #include "absl/types/optional.h" -#include "api/transport/field_trial_based_config.h" +#include "api/field_trials_registry.h" #include "api/units/time_delta.h" #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" #include "modules/rtp_rtcp/source/rtcp_packet.h" @@ -157,7 +157,7 @@ struct TestConfig { bool with_overhead = false; }; -class FieldTrialConfig : public FieldTrialsView { +class FieldTrialConfig : public FieldTrialsRegistry { public: static FieldTrialConfig GetFromTestConfig(const TestConfig& config) { FieldTrialConfig trials; @@ -170,14 +170,14 @@ class FieldTrialConfig : public FieldTrialsView { void SetOverHeadEnabled(bool enabled) { overhead_enabled_ = enabled; } - std::string Lookup(absl::string_view key) const override { + private: + std::string GetValue(absl::string_view key) const override { if (key == "WebRTC-SendSideBwe-WithOverhead") { return overhead_enabled_ ? "Enabled" : "Disabled"; } return ""; } - private: bool overhead_enabled_; }; diff --git a/modules/rtp_rtcp/source/rtp_sender_egress_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_egress_unittest.cc index 982f3fed9d..19e122f8e2 100644 --- a/modules/rtp_rtcp/source/rtp_sender_egress_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_sender_egress_unittest.cc @@ -15,6 +15,7 @@ #include "absl/types/optional.h" #include "api/array_view.h" #include "api/call/transport.h" +#include "api/field_trials_registry.h" #include "api/units/data_size.h" #include "api/units/timestamp.h" #include "logging/rtc_event_log/mock/mock_rtc_event_log.h" @@ -87,21 +88,21 @@ class MockSendSideDelayObserver : public SendSideDelayObserver { (override)); }; -class FieldTrialConfig : public FieldTrialsView { +class FieldTrialConfig : public FieldTrialsRegistry { public: FieldTrialConfig() : overhead_enabled_(false) {} ~FieldTrialConfig() override {} void SetOverHeadEnabled(bool enabled) { overhead_enabled_ = enabled; } - std::string Lookup(absl::string_view key) const override { + private: + std::string GetValue(absl::string_view key) const override { if (key == "WebRTC-SendSideBwe-WithOverhead") { return overhead_enabled_ ? "Enabled" : "Disabled"; } return ""; } - private: bool overhead_enabled_; }; diff --git a/modules/rtp_rtcp/source/rtp_sender_video_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_video_unittest.cc index a4d669edc5..13527128c9 100644 --- a/modules/rtp_rtcp/source/rtp_sender_video_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_sender_video_unittest.cc @@ -16,6 +16,7 @@ #include #include "absl/memory/memory.h" +#include "api/field_trials_registry.h" #include "api/rtp_headers.h" #include "api/task_queue/task_queue_base.h" #include "api/task_queue/task_queue_factory.h" @@ -148,7 +149,7 @@ class TestRtpSenderVideo : public RTPSenderVideo { } }; -class FieldTrials : public FieldTrialsView { +class FieldTrials : public FieldTrialsRegistry { public: explicit FieldTrials(bool use_send_side_bwe_with_overhead) : use_send_side_bwe_with_overhead_(use_send_side_bwe_with_overhead), @@ -158,7 +159,8 @@ class FieldTrials : public FieldTrialsView { include_capture_clock_offset_ = include_capture_clock_offset; } - std::string Lookup(absl::string_view key) const override { + private: + std::string GetValue(absl::string_view key) const override { if (key == "WebRTC-SendSideBwe-WithOverhead") { return use_send_side_bwe_with_overhead_ ? "Enabled" : ""; } else if (key == "WebRTC-IncludeCaptureClockOffset") { @@ -167,7 +169,6 @@ class FieldTrials : public FieldTrialsView { return ""; } - private: bool use_send_side_bwe_with_overhead_; bool include_capture_clock_offset_; }; diff --git a/test/BUILD.gn b/test/BUILD.gn index 8d226d9c72..f2e574a7c7 100644 --- a/test/BUILD.gn +++ b/test/BUILD.gn @@ -255,7 +255,7 @@ rtc_library("explicit_key_value_config") { ] deps = [ - "../api:field_trials_view", + "../api:field_trials_registry", "../rtc_base:checks", ] absl_deps = [ "//third_party/abseil-cpp/absl/strings:strings" ] @@ -271,7 +271,7 @@ rtc_library("scoped_key_value_config") { deps = [ ":field_trial", - "../api:field_trials_view", + "../api:field_trials_registry", "../rtc_base:checks", "../system_wrappers:field_trial", ] diff --git a/test/explicit_key_value_config.cc b/test/explicit_key_value_config.cc index c9e5ac1c28..90690c0514 100644 --- a/test/explicit_key_value_config.cc +++ b/test/explicit_key_value_config.cc @@ -11,7 +11,6 @@ #include "test/explicit_key_value_config.h" #include "absl/strings/string_view.h" -#include "api/field_trials_view.h" #include "rtc_base/checks.h" namespace webrtc { @@ -46,7 +45,7 @@ ExplicitKeyValueConfig::ExplicitKeyValueConfig(absl::string_view s) { RTC_CHECK_EQ(field_start, s.size()); } -std::string ExplicitKeyValueConfig::Lookup(absl::string_view key) const { +std::string ExplicitKeyValueConfig::GetValue(absl::string_view key) const { auto it = key_value_map_.find(key); if (it != key_value_map_.end()) return it->second; diff --git a/test/explicit_key_value_config.h b/test/explicit_key_value_config.h index 5685c13604..f14a10432c 100644 --- a/test/explicit_key_value_config.h +++ b/test/explicit_key_value_config.h @@ -16,17 +16,18 @@ #include #include "absl/strings/string_view.h" -#include "api/field_trials_view.h" +#include "api/field_trials_registry.h" namespace webrtc { namespace test { -class ExplicitKeyValueConfig : public FieldTrialsView { +class ExplicitKeyValueConfig : public FieldTrialsRegistry { public: explicit ExplicitKeyValueConfig(absl::string_view s); - std::string Lookup(absl::string_view key) const override; private: + std::string GetValue(absl::string_view key) const override; + // Unlike std::less, std::less<> is transparent and allows // heterogeneous lookup directly with absl::string_view. std::map> key_value_map_; diff --git a/test/scoped_key_value_config.cc b/test/scoped_key_value_config.cc index 449d5f0722..df84462637 100644 --- a/test/scoped_key_value_config.cc +++ b/test/scoped_key_value_config.cc @@ -10,7 +10,6 @@ #include "test/scoped_key_value_config.h" -#include "api/field_trials_view.h" #include "rtc_base/checks.h" #include "system_wrappers/include/field_trial.h" #include "test/field_trial.h" @@ -97,7 +96,7 @@ ScopedKeyValueConfig* ScopedKeyValueConfig::GetRoot(ScopedKeyValueConfig* n) { return n; } -std::string ScopedKeyValueConfig::Lookup(absl::string_view key) const { +std::string ScopedKeyValueConfig::GetValue(absl::string_view key) const { if (parent_ == nullptr) { return leaf_->LookupRecurse(key); } else { diff --git a/test/scoped_key_value_config.h b/test/scoped_key_value_config.h index db90ca3533..c0023f8228 100644 --- a/test/scoped_key_value_config.h +++ b/test/scoped_key_value_config.h @@ -17,24 +17,23 @@ #include #include "absl/strings/string_view.h" -#include "api/field_trials_view.h" +#include "api/field_trials_registry.h" #include "test/field_trial.h" namespace webrtc { namespace test { -class ScopedKeyValueConfig : public FieldTrialsView { +class ScopedKeyValueConfig : public FieldTrialsRegistry { public: virtual ~ScopedKeyValueConfig(); ScopedKeyValueConfig(); explicit ScopedKeyValueConfig(absl::string_view s); ScopedKeyValueConfig(ScopedKeyValueConfig& parent, absl::string_view s); - std::string Lookup(absl::string_view key) const override; - private: ScopedKeyValueConfig(ScopedKeyValueConfig* parent, absl::string_view s); ScopedKeyValueConfig* GetRoot(ScopedKeyValueConfig* n); + std::string GetValue(absl::string_view key) const override; std::string LookupRecurse(absl::string_view key) const; ScopedKeyValueConfig* const parent_;