From f2443a79717f2cb5eae251ad68104c215ced2804 Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Mon, 9 Oct 2023 16:39:40 +0200 Subject: [PATCH] Replace WebRTC-QuickPerfTest field trial with a flag This field trial is configured via command line flag, so may use flag system directly, reducing dependency on global field trial string. Bug: webrtc:7101, webrtc:10335 Change-Id: I1e48e0e3fdc251b73a375c6d7f1a46fa4f8a179b Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/322624 Reviewed-by: Mirko Bonadei Reviewed-by: Harald Alvestrand Commit-Queue: Danil Chapovalov Cr-Commit-Position: refs/heads/main@{#40897} --- call/BUILD.gn | 1 + call/call_perf_tests.cc | 11 ++++---- call/rampup_tests.cc | 2 +- modules/audio_coding/BUILD.gn | 3 ++- .../neteq/test/neteq_performance_unittest.cc | 11 ++++---- modules/video_coding/BUILD.gn | 7 +++-- .../codecs/test/video_codec_test.cc | 9 ++++--- test/BUILD.gn | 26 +++++++++++-------- test/pc/e2e/BUILD.gn | 6 ++++- test/pc/e2e/peer_connection_quality_test.cc | 4 ++- test/rtp_rtcp_observer.h | 10 +++---- test/test_flags.cc | 5 ++++ test/test_flags.h | 1 + test/test_main_lib.cc | 5 ++++ video/BUILD.gn | 4 +++ video/screenshare_loopback.cc | 10 +------ video/sv_loopback.cc | 10 +------ video/video_loopback.cc | 10 +------ video/video_quality_test.cc | 3 ++- 19 files changed, 73 insertions(+), 65 deletions(-) diff --git a/call/BUILD.gn b/call/BUILD.gn index 39cbc0a9c1..17c797c163 100644 --- a/call/BUILD.gn +++ b/call/BUILD.gn @@ -606,6 +606,7 @@ if (rtc_include_tests) { "../test:frame_generator_capturer", "../test:null_transport", "../test:test_common", + "../test:test_flags", "../test:test_support", "../test:video_test_common", "../test:video_test_constants", diff --git a/call/call_perf_tests.cc b/call/call_perf_tests.cc index f1ea970db8..3c6562caa8 100644 --- a/call/call_perf_tests.cc +++ b/call/call_perf_tests.cc @@ -13,6 +13,7 @@ #include #include +#include "absl/flags/flag.h" #include "absl/strings/string_view.h" #include "api/audio_codecs/builtin_audio_encoder_factory.h" #include "api/numerics/samples_stats_counter.h" @@ -52,6 +53,7 @@ #include "test/gtest.h" #include "test/null_transport.h" #include "test/rtp_rtcp_observer.h" +#include "test/test_flags.h" #include "test/testsupport/file_utils.h" #include "test/video_encoder_proxy_factory.h" #include "test/video_test_constants.h" @@ -361,7 +363,7 @@ void CallPerfTest::TestAudioVideoSync(FecMode fec, observer->PrintResults(); // In quick test synchronization may not be achieved in time. - if (!field_trial::IsEnabled("WebRTC-QuickPerfTest")) { + if (!absl::GetFlag(FLAGS_webrtc_quick_perf_test)) { // TODO(bugs.webrtc.org/10417): Reenable this for iOS #if !defined(WEBRTC_IOS) EXPECT_METRIC_EQ(1, metrics::NumSamples("WebRTC.Video.AVSyncOffsetInMs")); @@ -975,8 +977,8 @@ void CallPerfTest::TestMinAudioVideoBitrate(int test_bitrate_from, void PerformTest() override { // Quick test mode, just to exercise all the code paths without actually // caring about performance measurements. - const bool quick_perf_test = - field_trial::IsEnabled("WebRTC-QuickPerfTest"); + const bool quick_perf_test = absl::GetFlag(FLAGS_webrtc_quick_perf_test); + int last_passed_test_bitrate = -1; for (int test_bitrate = test_bitrate_from_; test_bitrate_from_ < test_bitrate_to_ @@ -1125,8 +1127,7 @@ void CallPerfTest::TestEncodeFramerate(VideoEncoderFactory* encoder_factory, } void VerifyStats() const { - const bool quick_perf_test = - field_trial::IsEnabled("WebRTC-QuickPerfTest"); + const bool quick_perf_test = absl::GetFlag(FLAGS_webrtc_quick_perf_test); double input_fps = 0.0; for (const auto& configured_framerate : configured_framerates_) { input_fps = std::max(configured_framerate.second, input_fps); diff --git a/call/rampup_tests.cc b/call/rampup_tests.cc index 8ddce83a2e..232fe0b3fe 100644 --- a/call/rampup_tests.cc +++ b/call/rampup_tests.cc @@ -329,7 +329,7 @@ void RampUpTester::TriggerTestDone() { RTC_DCHECK_GE(test_start_ms_, 0); // Stop polling stats. - // Corner case for field_trials=WebRTC-QuickPerfTest/Enabled/ + // Corner case for webrtc_quick_perf_test SendTask(task_queue_, [this] { pending_task_.Stop(); }); // TODO(holmer): Add audio send stats here too when those APIs are available. diff --git a/modules/audio_coding/BUILD.gn b/modules/audio_coding/BUILD.gn index 8f5019b4c2..61fecea691 100644 --- a/modules/audio_coding/BUILD.gn +++ b/modules/audio_coding/BUILD.gn @@ -1122,10 +1122,11 @@ if (rtc_include_tests) { "../../rtc_base:macromagic", "../../rtc_base:timeutils", "../../system_wrappers", - "../../system_wrappers:field_trial", "../../test:fileutils", + "../../test:test_flags", "../../test:test_support", ] + absl_deps = [ "//third_party/abseil-cpp/absl/flags:flag" ] } rtc_library("acm_receive_test") { diff --git a/modules/audio_coding/neteq/test/neteq_performance_unittest.cc b/modules/audio_coding/neteq/test/neteq_performance_unittest.cc index 961f74ab66..1b453cf7bf 100644 --- a/modules/audio_coding/neteq/test/neteq_performance_unittest.cc +++ b/modules/audio_coding/neteq/test/neteq_performance_unittest.cc @@ -8,11 +8,12 @@ * be found in the AUTHORS file in the root of the source tree. */ +#include "absl/flags/flag.h" #include "api/test/metrics/global_metrics_logger_and_exporter.h" #include "api/test/metrics/metric.h" #include "modules/audio_coding/neteq/tools/neteq_performance_test.h" -#include "system_wrappers/include/field_trial.h" #include "test/gtest.h" +#include "test/test_flags.h" namespace webrtc { namespace { @@ -29,8 +30,8 @@ TEST(NetEqPerformanceTest, 10_Pl_10_Drift) { const int kLossPeriod = 10; // Drop every 10th packet. const double kDriftFactor = 0.1; int64_t runtime = test::NetEqPerformanceTest::Run( - field_trial::IsEnabled("WebRTC-QuickPerfTest") ? kQuickSimulationTimeMs - : kSimulationTimeMs, + absl::GetFlag(FLAGS_webrtc_quick_perf_test) ? kQuickSimulationTimeMs + : kSimulationTimeMs, kLossPeriod, kDriftFactor); ASSERT_GT(runtime, 0); GetGlobalMetricsLogger()->LogSingleValueMetric( @@ -47,8 +48,8 @@ TEST(NetEqPerformanceTest, 0_Pl_0_Drift) { const int kLossPeriod = 0; // No losses. const double kDriftFactor = 0.0; // No clock drift. int64_t runtime = test::NetEqPerformanceTest::Run( - field_trial::IsEnabled("WebRTC-QuickPerfTest") ? kQuickSimulationTimeMs - : kSimulationTimeMs, + absl::GetFlag(FLAGS_webrtc_quick_perf_test) ? kQuickSimulationTimeMs + : kSimulationTimeMs, kLossPeriod, kDriftFactor); ASSERT_GT(runtime, 0); GetGlobalMetricsLogger()->LogSingleValueMetric( diff --git a/modules/video_coding/BUILD.gn b/modules/video_coding/BUILD.gn index f0bce34e3e..e305ddfcf4 100644 --- a/modules/video_coding/BUILD.gn +++ b/modules/video_coding/BUILD.gn @@ -1055,8 +1055,8 @@ if (rtc_include_tests) { "../../api/video_codecs:video_codecs_api", "../../media:rtc_internal_video_codecs", "../../rtc_base:logging", - "../../system_wrappers:field_trial", "../../test:fileutils", + "../../test:test_flags", "../../test:test_main", "../../test:test_support", "../../test:video_test_support", @@ -1076,7 +1076,10 @@ if (rtc_include_tests) { shard_timeout = 900 } - absl_deps = [ "//third_party/abseil-cpp/absl/functional:any_invocable" ] + absl_deps = [ + "//third_party/abseil-cpp/absl/flags:flag", + "//third_party/abseil-cpp/absl/functional:any_invocable", + ] data = [ "../../resources/FourPeople_1280x720_30.yuv" ] } diff --git a/modules/video_coding/codecs/test/video_codec_test.cc b/modules/video_coding/codecs/test/video_codec_test.cc index e838a6cbfd..1c8fe97e84 100644 --- a/modules/video_coding/codecs/test/video_codec_test.cc +++ b/modules/video_coding/codecs/test/video_codec_test.cc @@ -15,6 +15,7 @@ #include #include +#include "absl/flags/flag.h" #include "absl/functional/any_invocable.h" #include "api/test/create_video_codec_tester.h" #include "api/test/metrics/global_metrics_logger_and_exporter.h" @@ -38,8 +39,8 @@ #include "modules/video_coding/codecs/test/android_codec_factory_helper.h" #endif #include "rtc_base/logging.h" -#include "system_wrappers/include/field_trial.h" #include "test/gtest.h" +#include "test/test_flags.h" #include "test/testsupport/file_utils.h" #include "test/testsupport/frame_reader.h" @@ -612,7 +613,7 @@ TEST_P(SpatialQualityTest, SpatialQuality) { std::vector frames = stats->Slice(); SetTargetRates(frame_settings, frames); stream = stats->Aggregate(frames); - if (field_trial::IsEnabled("WebRTC-QuickPerfTest")) { + if (absl::GetFlag(FLAGS_webrtc_quick_perf_test)) { EXPECT_GE(stream.psnr.y.GetAverage(), psnr); } } @@ -697,7 +698,7 @@ TEST_P(BitrateAdaptationTest, BitrateAdaptation) { stats->Slice(VideoCodecStats::Filter{.first_frame = first_frame}); SetTargetRates(frame_settings, frames); stream = stats->Aggregate(frames); - if (field_trial::IsEnabled("WebRTC-QuickPerfTest")) { + if (absl::GetFlag(FLAGS_webrtc_quick_perf_test)) { EXPECT_NEAR(stream.bitrate_mismatch_pct.GetAverage(), 0, 10); EXPECT_NEAR(stream.framerate_mismatch_pct.GetAverage(), 0, 10); } @@ -776,7 +777,7 @@ TEST_P(FramerateAdaptationTest, FramerateAdaptation) { stats->Slice(VideoCodecStats::Filter{.first_frame = first_frame}); SetTargetRates(frame_settings, frames); stream = stats->Aggregate(frames); - if (field_trial::IsEnabled("WebRTC-QuickPerfTest")) { + if (absl::GetFlag(FLAGS_webrtc_quick_perf_test)) { EXPECT_NEAR(stream.bitrate_mismatch_pct.GetAverage(), 0, 10); EXPECT_NEAR(stream.framerate_mismatch_pct.GetAverage(), 0, 10); } diff --git a/test/BUILD.gn b/test/BUILD.gn index e8d4fdbc04..6f0671821d 100644 --- a/test/BUILD.gn +++ b/test/BUILD.gn @@ -588,16 +588,6 @@ if (rtc_include_tests) { absl_deps = [ "//third_party/abseil-cpp/absl/flags:flag" ] } - rtc_library("test_flags") { - visibility = [ "*" ] - testonly = true - sources = [ - "test_flags.cc", - "test_flags.h", - ] - absl_deps = [ "//third_party/abseil-cpp/absl/flags:flag" ] - } - rtc_library("test_main_lib") { visibility = [ "*" ] testonly = true @@ -1245,6 +1235,16 @@ if (!build_with_chromium) { ] } + rtc_library("test_flags") { + visibility = [ "*" ] + testonly = true + sources = [ + "test_flags.cc", + "test_flags.h", + ] + absl_deps = [ "//third_party/abseil-cpp/absl/flags:flag" ] + } + rtc_library("test_common") { testonly = true sources = [ @@ -1266,6 +1266,7 @@ if (!build_with_chromium) { ":mock_transport", ":run_loop", ":scoped_key_value_config", + ":test_flags", ":test_support", ":test_video_capturer", ":video_test_common", @@ -1312,7 +1313,10 @@ if (!build_with_chromium) { "../system_wrappers:field_trial", "../video/config:encoder_config", ] - absl_deps = [ "//third_party/abseil-cpp/absl/types:optional" ] + absl_deps = [ + "//third_party/abseil-cpp/absl/flags:flag", + "//third_party/abseil-cpp/absl/types:optional", + ] if (!is_android && !build_with_chromium) { deps += [ "../modules/video_capture:video_capture_internal_impl" ] } diff --git a/test/pc/e2e/BUILD.gn b/test/pc/e2e/BUILD.gn index 75db40eef5..86a9110f58 100644 --- a/test/pc/e2e/BUILD.gn +++ b/test/pc/e2e/BUILD.gn @@ -226,6 +226,7 @@ if (!build_with_chromium) { "../..:field_trial", "../..:fileutils", "../..:perf_test", + "../..:test_flags", "../../../api:audio_quality_analyzer_api", "../../../api:libjingle_peerconnection_api", "../../../api:media_stream_interface", @@ -260,7 +261,10 @@ if (!build_with_chromium) { "analyzer/video:video_quality_analyzer_injection_helper", "analyzer/video:video_quality_metrics_reporter", ] - absl_deps = [ "//third_party/abseil-cpp/absl/strings" ] + absl_deps = [ + "//third_party/abseil-cpp/absl/flags:flag", + "//third_party/abseil-cpp/absl/strings", + ] } peer_connection_e2e_smoke_test_resources = [ diff --git a/test/pc/e2e/peer_connection_quality_test.cc b/test/pc/e2e/peer_connection_quality_test.cc index 83613118f9..5eb47b4682 100644 --- a/test/pc/e2e/peer_connection_quality_test.cc +++ b/test/pc/e2e/peer_connection_quality_test.cc @@ -14,6 +14,7 @@ #include #include +#include "absl/flags/flag.h" #include "absl/strings/string_view.h" #include "api/jsep.h" #include "api/media_stream_interface.h" @@ -44,6 +45,7 @@ #include "test/pc/e2e/peer_params_preprocessor.h" #include "test/pc/e2e/stats_poller.h" #include "test/pc/e2e/test_peer_factory.h" +#include "test/test_flags.h" #include "test/testsupport/file_utils.h" namespace webrtc { @@ -384,7 +386,7 @@ void PeerConnectionE2EQualityTest::Run(RunParams run_params) { executor_->Start(task_queue_.get()); Timestamp start_time = Now(); - bool is_quick_test_enabled = field_trial::IsEnabled("WebRTC-QuickPerfTest"); + bool is_quick_test_enabled = absl::GetFlag(FLAGS_webrtc_quick_perf_test); if (is_quick_test_enabled) { time_controller_.AdvanceTime(kQuickTestModeRunDuration); } else { diff --git a/test/rtp_rtcp_observer.h b/test/rtp_rtcp_observer.h index cbbff1abfc..14c5ce3641 100644 --- a/test/rtp_rtcp_observer.h +++ b/test/rtp_rtcp_observer.h @@ -15,6 +15,7 @@ #include #include +#include "absl/flags/flag.h" #include "api/array_view.h" #include "api/test/simulated_network.h" #include "api/units/time_delta.h" @@ -25,10 +26,7 @@ #include "system_wrappers/include/field_trial.h" #include "test/direct_transport.h" #include "test/gtest.h" - -namespace { -constexpr webrtc::TimeDelta kShortTimeout = webrtc::TimeDelta::Millis(500); -} +#include "test/test_flags.h" namespace webrtc { namespace test { @@ -45,8 +43,8 @@ class RtpRtcpObserver { virtual ~RtpRtcpObserver() {} virtual bool Wait() { - if (field_trial::IsEnabled("WebRTC-QuickPerfTest")) { - observation_complete_.Wait(kShortTimeout); + if (absl::GetFlag(FLAGS_webrtc_quick_perf_test)) { + observation_complete_.Wait(TimeDelta::Millis(500)); return true; } return observation_complete_.Wait(timeout_); diff --git a/test/test_flags.cc b/test/test_flags.cc index a0fff747fe..4df2583672 100644 --- a/test/test_flags.cc +++ b/test/test_flags.cc @@ -49,3 +49,8 @@ ABSL_FLAG(bool, export_perf_results_new_api, false, "Tells to initialize new API for exporting performance metrics"); + +ABSL_FLAG(bool, + webrtc_quick_perf_test, + false, + "Runs webrtc perfomance tests in quick mode."); diff --git a/test/test_flags.h b/test/test_flags.h index 30f918fc7d..84f1c29503 100644 --- a/test/test_flags.h +++ b/test/test_flags.h @@ -20,5 +20,6 @@ ABSL_DECLARE_FLAG(std::vector, plot); ABSL_DECLARE_FLAG(std::string, isolated_script_test_perf_output); ABSL_DECLARE_FLAG(std::string, webrtc_test_metrics_output_path); ABSL_DECLARE_FLAG(bool, export_perf_results_new_api); +ABSL_DECLARE_FLAG(bool, webrtc_quick_perf_test); #endif // TEST_TEST_FLAGS_H_ diff --git a/test/test_main_lib.cc b/test/test_main_lib.cc index 4c80315ac5..331a2d3bb6 100644 --- a/test/test_main_lib.cc +++ b/test/test_main_lib.cc @@ -150,6 +150,11 @@ class TestMainImpl : public TestMain { // outlive the application. field_trials_ = absl::GetFlag(FLAGS_force_fieldtrials); webrtc::field_trial::InitFieldTrialsFromString(field_trials_.c_str()); + // TODO(bugs.webrtc.org/7101): Remove when all invocation would use + // webrtc_quick_perf_test flag directly instead of setting the field trial. + if (webrtc::field_trial::IsEnabled("WebRTC-QuickPerfTest")) { + absl::SetFlag(&FLAGS_webrtc_quick_perf_test, true); + } webrtc::metrics::Enable(); #if defined(WEBRTC_WIN) diff --git a/video/BUILD.gn b/video/BUILD.gn index d696445db2..9346175831 100644 --- a/video/BUILD.gn +++ b/video/BUILD.gn @@ -561,6 +561,7 @@ if (rtc_include_tests) { "../test:platform_video_capturer", "../test:rtp_test_utils", "../test:test_common", + "../test:test_flags", "../test:test_renderer", "../test:test_support", "../test:test_support_test_artifacts", @@ -661,6 +662,7 @@ if (rtc_include_tests) { "../test:run_test", "../test:run_test_interface", "../test:test_common", + "../test:test_flags", "../test:test_renderer", "../test:test_support", "//testing/gtest", @@ -706,6 +708,7 @@ if (rtc_include_tests) { "../test:run_test", "../test:run_test_interface", "../test:test_common", + "../test:test_flags", "../test:test_renderer", "../test:test_support", "//third_party/abseil-cpp/absl/flags:flag", @@ -732,6 +735,7 @@ if (rtc_include_tests) { "../test:run_test", "../test:run_test_interface", "../test:test_common", + "../test:test_flags", "../test:test_renderer", "../test:test_support", "//testing/gtest", diff --git a/video/screenshare_loopback.cc b/video/screenshare_loopback.cc index 239e472f6e..40bf9ee496 100644 --- a/video/screenshare_loopback.cc +++ b/video/screenshare_loopback.cc @@ -28,6 +28,7 @@ #include "test/field_trial.h" #include "test/gtest.h" #include "test/run_test.h" +#include "test/test_flags.h" #include "video/video_quality_test.h" using ::webrtc::BitrateConstraints; @@ -256,15 +257,6 @@ ABSL_FLAG(bool, generic_descriptor, false, "Use the generic frame descriptor."); ABSL_FLAG(bool, allow_reordering, false, "Allow packet reordering to occur"); -ABSL_FLAG( - std::string, - force_fieldtrials, - "", - "Field trials control experimental feature code which can be forced. " - "E.g. running with --force_fieldtrials=WebRTC-FooFeature/Enable/" - " will assign the group Enable to field trial WebRTC-FooFeature. Multiple " - "trials are separated by \"/\""); - // Screenshare-specific flags. ABSL_FLAG(int, min_transmit_bitrate, diff --git a/video/sv_loopback.cc b/video/sv_loopback.cc index af475ae4eb..2a8f9743e8 100644 --- a/video/sv_loopback.cc +++ b/video/sv_loopback.cc @@ -28,6 +28,7 @@ #include "test/field_trial.h" #include "test/gtest.h" #include "test/run_test.h" +#include "test/test_flags.h" #include "video/video_quality_test.h" // Flags for video. @@ -311,15 +312,6 @@ ABSL_FLAG(bool, ABSL_FLAG(bool, video, true, "Add video stream"); -ABSL_FLAG( - std::string, - force_fieldtrials, - "", - "Field trials control experimental feature code which can be forced. " - "E.g. running with --force_fieldtrials=WebRTC-FooFeature/Enable/" - " will assign the group Enable to field trial WebRTC-FooFeature. Multiple " - "trials are separated by \"/\""); - // Video-specific flags. ABSL_FLAG(std::string, vclip, diff --git a/video/video_loopback.cc b/video/video_loopback.cc index ba0a0e5745..b02184a685 100644 --- a/video/video_loopback.cc +++ b/video/video_loopback.cc @@ -28,6 +28,7 @@ #include "test/field_trial.h" #include "test/gtest.h" #include "test/run_test.h" +#include "test/test_flags.h" #include "video/video_quality_test.h" // Flags common with screenshare loopback, with different default values. @@ -199,15 +200,6 @@ ABSL_FLAG(bool, ABSL_FLAG(bool, video, true, "Add video stream"); -ABSL_FLAG( - std::string, - force_fieldtrials, - "", - "Field trials control experimental feature code which can be forced. " - "E.g. running with --force_fieldtrials=WebRTC-FooFeature/Enabled/" - " will assign the group Enable to field trial WebRTC-FooFeature. Multiple " - "trials are separated by \"/\""); - // Video-specific flags. ABSL_FLAG(std::string, clip, diff --git a/video/video_quality_test.cc b/video/video_quality_test.cc index 8759f3bb43..5eae8ebda0 100644 --- a/video/video_quality_test.cc +++ b/video/video_quality_test.cc @@ -48,6 +48,7 @@ #include "rtc_base/strings/string_builder.h" #include "rtc_base/task_queue_for_test.h" #include "test/platform_video_capturer.h" +#include "test/test_flags.h" #include "test/testsupport/file_utils.h" #include "test/video_renderer.h" #include "video/frame_dumping_decoder.h" @@ -1262,7 +1263,7 @@ void VideoQualityTest::RunWithAnalyzer(const Params& params) { std::string graph_title = params_.analyzer.graph_title; if (graph_title.empty()) graph_title = VideoQualityTest::GenerateGraphTitle(); - bool is_quick_test_enabled = field_trial::IsEnabled("WebRTC-QuickPerfTest"); + bool is_quick_test_enabled = absl::GetFlag(FLAGS_webrtc_quick_perf_test); analyzer_ = std::make_unique( send_transport.get(), params_.analyzer.test_label, params_.analyzer.avg_psnr_threshold, params_.analyzer.avg_ssim_threshold,