Add FieldTrialsRegistry that verifies looked up field trials
This new class implements the existing FieldTrialsView interface,
extending it with the verification functionality. For now, the
verification will only be performed if the rtc_strict_field_trials GN
arg is set.
Most classes extending FieldTrialsView today have been converted to
extend from FieldTrialsRegistry instead to automatically perform
verification.
Bug: webrtc:14154
Change-Id: I4819724cd66a04507e62fcc2bb1019187b6ba8c7
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/276270
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
Commit-Queue: Emil Lundmark <lndmrk@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#38453}
diff --git a/api/BUILD.gn b/api/BUILD.gn
index f0bc7ba..1251151 100644
--- a/api/BUILD.gn
+++ b/api/BUILD.gn
@@ -1362,6 +1362,25 @@
}
}
+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 @@
"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 c8c8ac0..5f01204 100644
--- a/api/DEPS
+++ b/api/DEPS
@@ -312,6 +312,10 @@
"+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 @@
"+common_audio",
"+common_video",
"+examples",
+ "+experiments",
"+logging",
"+media",
"+modules",
diff --git a/api/field_trials.cc b/api/field_trials.cc
index d6b53ac..4bd1127 100644
--- a/api/field_trials.cc
+++ b/api/field_trials.cc
@@ -90,7 +90,7 @@
}
}
-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 0bfa4b7..bf7a7cc 100644
--- a/api/field_trials.h
+++ b/api/field_trials.h
@@ -15,7 +15,7 @@
#include <string>
#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 @@
// 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 @@
// global variable (i.e can not be used for all parts of webrtc).
static std::unique_ptr<FieldTrials> 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 0000000..f97e8a8
--- /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 <string>
+
+#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 0000000..dc7e844
--- /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 <string>
+#include <utility>
+
+#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<std::string> 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<std::string> 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 5f0188e..2616a2e 100644
--- a/api/field_trials_unittest.cc
+++ b/api/field_trials_unittest.cc
@@ -34,6 +34,8 @@
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 @@
"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 @@
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 @@
std::unique_ptr<FieldTrials> 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 @@
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 @@
std::unique_ptr<FieldTrials> 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 @@
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 299205d..45e6f78 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 3cc3559..86a7c8a 100644
--- a/api/transport/BUILD.gn
+++ b/api/transport/BUILD.gn
@@ -52,7 +52,7 @@
"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 4a3a179..0cef30f 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 f0063ff..d47140e 100644
--- a/api/transport/field_trial_based_config.h
+++ b/api/transport/field_trial_based_config.h
@@ -13,13 +13,13 @@
#include <string>
#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 736ca58..ead114b 100644
--- a/modules/rtp_rtcp/BUILD.gn
+++ b/modules/rtp_rtcp/BUILD.gn
@@ -604,6 +604,7 @@
":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 4c08ce5..4dece66 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 <utility>
#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 @@
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 @@
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 982f3fe..19e122f 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 @@
(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 a4d669e..1352712 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 <vector>
#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 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 @@
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 @@
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 8d226d9..f2e574a 100644
--- a/test/BUILD.gn
+++ b/test/BUILD.gn
@@ -255,7 +255,7 @@
]
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 @@
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 c9e5ac1..90690c0 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 @@
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 5685c13..f14a104 100644
--- a/test/explicit_key_value_config.h
+++ b/test/explicit_key_value_config.h
@@ -16,17 +16,18 @@
#include <string>
#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::string>, std::less<> is transparent and allows
// heterogeneous lookup directly with absl::string_view.
std::map<std::string, std::string, std::less<>> key_value_map_;
diff --git a/test/scoped_key_value_config.cc b/test/scoped_key_value_config.cc
index 449d5f0..df84462 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 @@
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 db90ca3..c0023f8 100644
--- a/test/scoped_key_value_config.h
+++ b/test/scoped_key_value_config.h
@@ -17,24 +17,23 @@
#include <string>
#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_;
