From ee1e01565570b2e0a20a7f0c07e879d63aa09293 Mon Sep 17 00:00:00 2001 From: Konrad Hofbauer Date: Thu, 5 Dec 2019 16:25:40 +0100 Subject: [PATCH] Expose methods to validate and merge FieldTrial strings. Bug: webrtc:11177 Change-Id: I0514d82bc904b1548c64fdef8b0a2a99a8dbd735 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/161309 Reviewed-by: Niels Moller Reviewed-by: Mirko Bonadei Commit-Queue: Konrad Hofbauer Cr-Commit-Position: refs/heads/master@{#30027} --- system_wrappers/BUILD.gn | 1 + system_wrappers/include/field_trial.h | 18 +++++-- system_wrappers/source/field_trial.cc | 37 ++++++++++++- .../source/field_trial_unittest.cc | 52 +++++++++++++++++++ 4 files changed, 103 insertions(+), 5 deletions(-) diff --git a/system_wrappers/BUILD.gn b/system_wrappers/BUILD.gn index 2f30a16395..7fc29c929d 100644 --- a/system_wrappers/BUILD.gn +++ b/system_wrappers/BUILD.gn @@ -96,6 +96,7 @@ rtc_library("field_trial") { deps = [ "../rtc_base:checks", "../rtc_base:logging", + "../rtc_base:stringutils", "//third_party/abseil-cpp/absl/strings", ] } diff --git a/system_wrappers/include/field_trial.h b/system_wrappers/include/field_trial.h index 1d0cef447d..52db33b0e9 100644 --- a/system_wrappers/include/field_trial.h +++ b/system_wrappers/include/field_trial.h @@ -16,7 +16,7 @@ // Field trials allow webrtc clients (such as Chrome) to turn on feature code // in binaries out in the field and gather information with that. // -// By default WebRTC provides an implementaion of field trials that can be +// By default WebRTC provides an implementation of field trials that can be // found in system_wrappers/source/field_trial.cc. If clients want to provide // a custom version, they will have to: // @@ -45,10 +45,10 @@ // // Notes: // - NOT every feature is a candidate to be controlled by this mechanism as -// it may require negotation between involved parties (e.g. SDP). +// it may require negotiation between involved parties (e.g. SDP). // // TODO(andresp): since chrome --force-fieldtrials does not marks the trial -// as active it does not gets propaged to renderer process. For now one +// as active it does not get propagated to the renderer process. For now one // needs to push a config with start_active:true or run a local finch // server. // @@ -84,6 +84,18 @@ void InitFieldTrialsFromString(const char* trials_string); const char* GetFieldTrialString(); +#ifndef WEBRTC_EXCLUDE_FIELD_TRIAL_DEFAULT +// Validates the given field trial string. +bool FieldTrialsStringIsValid(const char* trials_string); + +// Merges two field trial strings. +// +// If a key (trial) exists twice with conflicting values (groups), the value +// in 'second' takes precedence. +// Shall only be called with valid FieldTrial strings. +std::string MergeFieldTrialsStrings(const char* first, const char* second); +#endif // WEBRTC_EXCLUDE_FIELD_TRIAL_DEFAULT + } // namespace field_trial } // namespace webrtc diff --git a/system_wrappers/source/field_trial.cc b/system_wrappers/source/field_trial.cc index 5b8a7562c9..f1dccc987b 100644 --- a/system_wrappers/source/field_trial.cc +++ b/system_wrappers/source/field_trial.cc @@ -17,6 +17,7 @@ #include "absl/strings/string_view.h" #include "rtc_base/checks.h" #include "rtc_base/logging.h" +#include "rtc_base/string_encode.h" // Simple field trial implementation, which allows client to // specify desired flags in InitFieldTrialsFromString. @@ -36,7 +37,7 @@ constexpr char kPersistentStringSeparator = '/'; // // E.g. invalid config: // "WebRTC-experiment1/Enabled" (note missing / separator at the end). -bool FieldTrialsStringIsValid(const absl::string_view trials) { +bool FieldTrialsStringIsValidInternal(const absl::string_view trials) { if (trials.empty()) return true; @@ -69,6 +70,38 @@ bool FieldTrialsStringIsValid(const absl::string_view trials) { } } // namespace +bool FieldTrialsStringIsValid(const char* trials_string) { + return FieldTrialsStringIsValidInternal(trials_string); +} + +void InsertOrReplaceFieldTrialStringsInMap( + std::map* fieldtrial_map, + const absl::string_view trials_string) { + if (FieldTrialsStringIsValidInternal(trials_string)) { + std::vector tokens; + rtc::split(std::string(trials_string), '/', &tokens); + // Skip last token which is empty due to trailing '/'. + for (size_t idx = 0; idx < tokens.size() - 1; idx += 2) { + (*fieldtrial_map)[tokens[idx]] = tokens[idx + 1]; + } + } else { + RTC_DCHECK(false) << "Invalid field trials string:" << trials_string; + } +} + +std::string MergeFieldTrialsStrings(const char* first, const char* second) { + std::map fieldtrial_map; + InsertOrReplaceFieldTrialStringsInMap(&fieldtrial_map, first); + InsertOrReplaceFieldTrialStringsInMap(&fieldtrial_map, second); + + // Merge into fieldtrial string. + std::string merged = ""; + for (auto const& fieldtrial : fieldtrial_map) { + merged += fieldtrial.first + '/' + fieldtrial.second + '/'; + } + return merged; +} + std::string FindFullName(const std::string& name) { if (trials_init_string == NULL) return std::string(); @@ -107,7 +140,7 @@ void InitFieldTrialsFromString(const char* trials_string) { RTC_LOG(LS_INFO) << "Setting field trial string:" << trials_string; #ifndef WEBRTC_EXCLUDE_FIELD_TRIAL_DEFAULT if (trials_string) { - RTC_DCHECK(FieldTrialsStringIsValid(trials_string)) + RTC_DCHECK(FieldTrialsStringIsValidInternal(trials_string)) << "Invalid field trials string:" << trials_string; }; #endif // WEBRTC_EXCLUDE_FIELD_TRIAL_DEFAULT diff --git a/system_wrappers/source/field_trial_unittest.cc b/system_wrappers/source/field_trial_unittest.cc index 67b841df4c..fdabe1b7e6 100644 --- a/system_wrappers/source/field_trial_unittest.cc +++ b/system_wrappers/source/field_trial_unittest.cc @@ -21,10 +21,15 @@ TEST(FieldTrialValidationTest, AcceptsValidInputs) { InitFieldTrialsFromString(""); InitFieldTrialsFromString("Audio/Enabled/"); InitFieldTrialsFromString("Audio/Enabled/Video/Disabled/"); + EXPECT_TRUE(FieldTrialsStringIsValid("")); + EXPECT_TRUE(FieldTrialsStringIsValid("Audio/Enabled/")); + EXPECT_TRUE(FieldTrialsStringIsValid("Audio/Enabled/Video/Disabled/")); // Duplicate trials with the same value is fine InitFieldTrialsFromString("Audio/Enabled/Audio/Enabled/"); InitFieldTrialsFromString("Audio/Enabled/B/C/Audio/Enabled/"); + EXPECT_TRUE(FieldTrialsStringIsValid("Audio/Enabled/Audio/Enabled/")); + EXPECT_TRUE(FieldTrialsStringIsValid("Audio/Enabled/B/C/Audio/Enabled/")); } TEST(FieldTrialValidationTest, RejectsBadInputs) { @@ -40,6 +45,26 @@ TEST(FieldTrialValidationTest, RejectsBadInputs) { RTC_EXPECT_DEATH( InitFieldTrialsFromString("Audio/Enabled/Video/Disabled/garbage"), "Invalid field trials string:"); + EXPECT_FALSE(FieldTrialsStringIsValid("Audio/EnabledVideo/Disabled/")); + EXPECT_FALSE(FieldTrialsStringIsValid("Audio/Enabled//Video/Disabled/")); + EXPECT_FALSE(FieldTrialsStringIsValid("/Audio/Enabled/Video/Disabled/")); + EXPECT_FALSE(FieldTrialsStringIsValid("Audio/Enabled/Video/Disabled")); + EXPECT_FALSE( + FieldTrialsStringIsValid("Audio/Enabled/Video/Disabled/garbage")); + + // Empty trial or group + RTC_EXPECT_DEATH(InitFieldTrialsFromString("Audio//"), + "Invalid field trials string:"); + RTC_EXPECT_DEATH(InitFieldTrialsFromString("/Enabled/"), + "Invalid field trials string:"); + RTC_EXPECT_DEATH(InitFieldTrialsFromString("//"), + "Invalid field trials string:"); + RTC_EXPECT_DEATH(InitFieldTrialsFromString("//Enabled"), + "Invalid field trials string:"); + EXPECT_FALSE(FieldTrialsStringIsValid("Audio//")); + EXPECT_FALSE(FieldTrialsStringIsValid("/Enabled/")); + EXPECT_FALSE(FieldTrialsStringIsValid("//")); + EXPECT_FALSE(FieldTrialsStringIsValid("//Enabled")); // Duplicate trials with different values is not fine RTC_EXPECT_DEATH(InitFieldTrialsFromString("Audio/Enabled/Audio/Disabled/"), @@ -47,6 +72,33 @@ TEST(FieldTrialValidationTest, RejectsBadInputs) { RTC_EXPECT_DEATH( InitFieldTrialsFromString("Audio/Enabled/B/C/Audio/Disabled/"), "Invalid field trials string:"); + EXPECT_FALSE(FieldTrialsStringIsValid("Audio/Enabled/Audio/Disabled/")); + EXPECT_FALSE(FieldTrialsStringIsValid("Audio/Enabled/B/C/Audio/Disabled/")); +} + +TEST(FieldTrialMergingTest, MergesValidInput) { + EXPECT_EQ(MergeFieldTrialsStrings("Video/Enabled/", "Audio/Enabled/"), + "Audio/Enabled/Video/Enabled/"); + EXPECT_EQ(MergeFieldTrialsStrings("Audio/Disabled/Video/Enabled/", + "Audio/Enabled/"), + "Audio/Enabled/Video/Enabled/"); + EXPECT_EQ( + MergeFieldTrialsStrings("Audio/Enabled/Video/Enabled/", "Audio/Enabled/"), + "Audio/Enabled/Video/Enabled/"); + EXPECT_EQ( + MergeFieldTrialsStrings("Audio/Enabled/Audio/Enabled/", "Video/Enabled/"), + "Audio/Enabled/Video/Enabled/"); +} + +TEST(FieldTrialMergingTest, DchecksBadInput) { + RTC_EXPECT_DEATH(MergeFieldTrialsStrings("Audio/Enabled/", "garbage"), + "Invalid field trials string:"); +} + +TEST(FieldTrialMergingTest, HandlesEmptyInput) { + EXPECT_EQ(MergeFieldTrialsStrings("", "Audio/Enabled/"), "Audio/Enabled/"); + EXPECT_EQ(MergeFieldTrialsStrings("Audio/Enabled/", ""), "Audio/Enabled/"); + EXPECT_EQ(MergeFieldTrialsStrings("", ""), ""); } #endif // GTEST_HAS_DEATH_TEST && RTC_DCHECK_IS_ON && !defined(WEBRTC_ANDROID) // && !defined(WEBRTC_EXCLUDE_FIELD_TRIAL_DEFAULT)