From acf26ce00a4641eaddcc6d1370c1b12bd69b210e Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Mon, 16 Dec 2024 11:37:36 +0100 Subject: [PATCH] Refactor PC tests to use non-global field trials In particular that avoids lifetime issues with the field trials passed into peerconnection, as now PC takes field trials object by unique_ptr and thus fully manages its lifetime. Bug: webrtc:42220378 Change-Id: Ia863e9703b5c76ae1866d0ff995b83286c0b947e Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/371480 Commit-Queue: Danil Chapovalov Reviewed-by: Per Kjellander Cr-Commit-Position: refs/heads/main@{#43576} --- pc/BUILD.gn | 5 ++- pc/congestion_control_integrationtest.cc | 25 +++----------- pc/peer_connection_crypto_unittest.cc | 6 ++-- ...er_connection_encodings_integrationtest.cc | 23 ++++++------- pc/peer_connection_factory_unittest.cc | 7 ++-- pc/peer_connection_field_trial_tests.cc | 16 ++++----- ...er_connection_header_extension_unittest.cc | 6 ++-- pc/peer_connection_ice_unittest.cc | 6 ++-- pc/peer_connection_interface_unittest.cc | 11 ++++--- pc/peer_connection_media_unittest.cc | 3 -- pc/rtp_sender_receiver_unittest.cc | 8 ++--- pc/sdp_offer_answer_unittest.cc | 33 +++++++++---------- pc/test/integration_test_helpers.cc | 4 +-- pc/test/integration_test_helpers.h | 25 +++++++------- pc/test/peer_connection_test_wrapper.cc | 24 +++----------- pc/test/peer_connection_test_wrapper.h | 10 ++---- 16 files changed, 86 insertions(+), 126 deletions(-) diff --git a/pc/BUILD.gn b/pc/BUILD.gn index d2c7a2f00e..a23ed01100 100644 --- a/pc/BUILD.gn +++ b/pc/BUILD.gn @@ -2308,6 +2308,7 @@ if (rtc_include_tests && !build_with_chromium) { "../api:enable_media_with_defaults", "../api:fake_frame_decryptor", "../api:fake_frame_encryptor", + "../api:field_trials", "../api:field_trials_view", "../api:function_view", "../api:ice_transport_interface", @@ -2416,10 +2417,8 @@ if (rtc_include_tests && !build_with_chromium) { "../rtc_base/third_party/base64", "../rtc_base/third_party/sigslot", "../system_wrappers:metrics", - "../test:field_trial", "../test:rtc_expect_death", "../test:run_loop", - "../test:scoped_key_value_config", "../test:wait_until", "../test/pc/sctp:fake_sctp_transport", "//testing/gtest", @@ -2550,6 +2549,7 @@ if (rtc_include_tests && !build_with_chromium) { "../api:enable_media_with_defaults", "../api:fake_frame_decryptor", "../api:fake_frame_encryptor", + "../api:field_trials", "../api:field_trials_view", "../api:function_view", "../api:ice_transport_interface", @@ -2580,7 +2580,6 @@ if (rtc_include_tests && !build_with_chromium) { "../api/task_queue", "../api/task_queue:default_task_queue_factory", "../api/task_queue:pending_task_safety_flag", - "../api/transport:field_trial_based_config", "../api/transport/rtp:rtp_source", "../api/units:time_delta", "../api/video:builtin_video_bitrate_allocator_factory", diff --git a/pc/congestion_control_integrationtest.cc b/pc/congestion_control_integrationtest.cc index c73cb2d1ed..d0cb4e0491 100644 --- a/pc/congestion_control_integrationtest.cc +++ b/pc/congestion_control_integrationtest.cc @@ -17,7 +17,6 @@ #include "api/peer_connection_interface.h" #include "pc/test/integration_test_helpers.h" #include "rtc_base/gunit.h" -#include "test/field_trial.h" #include "test/gmock.h" #include "test/gtest.h" @@ -35,21 +34,16 @@ class PeerConnectionCongestionControlTest }; TEST_F(PeerConnectionCongestionControlTest, OfferContainsCcfbIfEnabled) { - test::ScopedFieldTrials trials( - "WebRTC-RFC8888CongestionControlFeedback/Enabled/"); + SetFieldTrials("WebRTC-RFC8888CongestionControlFeedback/Enabled/"); ASSERT_TRUE(CreatePeerConnectionWrappers()); caller()->AddAudioVideoTracks(); auto offer = caller()->CreateOfferAndWait(); std::string offer_str = absl::StrCat(*offer); EXPECT_THAT(offer_str, HasSubstr("a=rtcp-fb:* ack ccfb\r\n")); - // Closing peer connections before ScopedFieldTrials gets out of scope to - // avoid a race condition. - ClosePeerConnections(); } TEST_F(PeerConnectionCongestionControlTest, ReceiveOfferSetsCcfbFlag) { - test::ScopedFieldTrials trials( - "WebRTC-RFC8888CongestionControlFeedback/Enabled/"); + SetFieldTrials("WebRTC-RFC8888CongestionControlFeedback/Enabled/"); ASSERT_TRUE(CreatePeerConnectionWrappers()); ConnectFakeSignalingForSdpOnly(); caller()->AddAudioVideoTracks(); @@ -72,14 +66,10 @@ TEST_F(PeerConnectionCongestionControlTest, ReceiveOfferSetsCcfbFlag) { // Check that the answer does not contain transport-cc std::string answer_str = absl::StrCat(*caller()->pc()->remote_description()); EXPECT_THAT(answer_str, Not(HasSubstr("transport-cc"))); - // Closing peer connections before ScopedFieldTrials gets out of scope to - // avoid a race condition. - ClosePeerConnections(); } TEST_F(PeerConnectionCongestionControlTest, CcfbGetsUsed) { - test::ScopedFieldTrials trials( - "WebRTC-RFC8888CongestionControlFeedback/Enabled/"); + SetFieldTrials("WebRTC-RFC8888CongestionControlFeedback/Enabled/"); ASSERT_TRUE(CreatePeerConnectionWrappers()); ConnectFakeSignaling(); caller()->AddAudioVideoTracks(); @@ -95,14 +85,10 @@ TEST_F(PeerConnectionCongestionControlTest, CcfbGetsUsed) { // There should be no transport-cc generated. EXPECT_THAT(pc_internal->FeedbackAccordingToTransportCcCountForTesting(), Eq(0)); - // Closing peer connections before ScopedFieldTrials gets out of scope to - // avoid a race condition. - ClosePeerConnections(); } TEST_F(PeerConnectionCongestionControlTest, TransportCcGetsUsed) { - test::ScopedFieldTrials trials( - "WebRTC-RFC8888CongestionControlFeedback/Disabled/"); + SetFieldTrials("WebRTC-RFC8888CongestionControlFeedback/Disabled/"); ASSERT_TRUE(CreatePeerConnectionWrappers()); ConnectFakeSignaling(); caller()->AddAudioVideoTracks(); @@ -118,9 +104,6 @@ TEST_F(PeerConnectionCongestionControlTest, TransportCcGetsUsed) { kDefaultTimeout); // Test that RFC 8888 feedback is NOT generated when field trial disabled. EXPECT_THAT(pc_internal->FeedbackAccordingToRfc8888CountForTesting(), Eq(0)); - // Closing peer connections before ScopedFieldTrials gets out of scope to - // avoid a race condition. - ClosePeerConnections(); } } // namespace webrtc diff --git a/pc/peer_connection_crypto_unittest.cc b/pc/peer_connection_crypto_unittest.cc index c808b0a27a..83c308dd5d 100644 --- a/pc/peer_connection_crypto_unittest.cc +++ b/pc/peer_connection_crypto_unittest.cc @@ -26,6 +26,7 @@ #include "api/audio_codecs/builtin_audio_encoder_factory.h" #include "api/create_peerconnection_factory.h" #include "api/crypto/crypto_options.h" +#include "api/field_trials.h" #include "api/jsep.h" #include "api/peer_connection_interface.h" #include "api/scoped_refptr.h" @@ -56,7 +57,6 @@ #include "rtc_base/thread.h" #include "test/gmock.h" #include "test/gtest.h" -#include "test/scoped_key_value_config.h" #ifdef WEBRTC_ANDROID #include "pc/test/android_test_initializer.h" #endif @@ -113,7 +113,7 @@ class PeerConnectionCryptoBaseTest : public ::testing::Test { auto fake_port_allocator = std::make_unique( rtc::Thread::Current(), std::make_unique(vss_.get()), - &field_trials_); + field_trials_.get()); auto observer = std::make_unique(); RTCConfiguration modified_config = config; modified_config.sdp_semantics = sdp_semantics_; @@ -163,7 +163,7 @@ class PeerConnectionCryptoBaseTest : public ::testing::Test { return transport_info->description.connection_role; } - test::ScopedKeyValueConfig field_trials_; + std::unique_ptr field_trials_ = FieldTrials::CreateNoGlobal(""); std::unique_ptr vss_; rtc::AutoSocketServerThread main_; rtc::scoped_refptr pc_factory_; diff --git a/pc/peer_connection_encodings_integrationtest.cc b/pc/peer_connection_encodings_integrationtest.cc index b6a8b08d79..28a4d203e4 100644 --- a/pc/peer_connection_encodings_integrationtest.cc +++ b/pc/peer_connection_encodings_integrationtest.cc @@ -25,6 +25,7 @@ #include "api/audio_codecs/builtin_audio_decoder_factory.h" #include "api/audio_codecs/builtin_audio_encoder_factory.h" #include "api/audio_options.h" +#include "api/field_trials.h" #include "api/field_trials_view.h" #include "api/jsep.h" #include "api/make_ref_counted.h" @@ -55,7 +56,6 @@ #include "rtc_base/thread.h" #include "test/gmock.h" #include "test/gtest.h" -#include "test/scoped_key_value_config.h" #include "test/wait_until.h" using ::testing::AllOf; @@ -143,12 +143,13 @@ class PeerConnectionEncodingsIntegrationTest : public ::testing::Test { RTC_CHECK(background_thread_->Start()); } - rtc::scoped_refptr CreatePc() { - auto pc_wrapper = rtc::make_ref_counted( - "pc", &pss_, background_thread_.get(), background_thread_.get(), - field_trials_); + scoped_refptr CreatePc( + std::unique_ptr field_trials = nullptr) { + auto pc_wrapper = make_ref_counted( + "pc", &pss_, background_thread_.get(), background_thread_.get()); pc_wrapper->CreatePc({}, CreateBuiltinAudioEncoderFactory(), - CreateBuiltinAudioDecoderFactory()); + CreateBuiltinAudioDecoderFactory(), + std::move(field_trials)); return pc_wrapper; } @@ -505,7 +506,6 @@ class PeerConnectionEncodingsIntegrationTest : public ::testing::Test { return true; } - test::ScopedKeyValueConfig field_trials_; rtc::PhysicalSocketServer pss_; std::unique_ptr background_thread_; }; @@ -2220,10 +2220,11 @@ TEST_F(PeerConnectionEncodingsIntegrationTest, TEST_F(PeerConnectionEncodingsIntegrationTest, AddTransceiverAcceptsMixedCodecSimulcast) { // Enable WIP mixed codec simulcast support - test::ScopedKeyValueConfig field_trials( - field_trials_, "WebRTC-MixedCodecSimulcast/Enabled/"); - rtc::scoped_refptr local_pc_wrapper = CreatePc(); - rtc::scoped_refptr remote_pc_wrapper = CreatePc(); + std::string field_trials = "WebRTC-MixedCodecSimulcast/Enabled/"; + scoped_refptr local_pc_wrapper = + CreatePc(FieldTrials::CreateNoGlobal(field_trials)); + scoped_refptr remote_pc_wrapper = + CreatePc(FieldTrials::CreateNoGlobal(field_trials)); ExchangeIceCandidates(local_pc_wrapper, remote_pc_wrapper); std::optional vp8 = diff --git a/pc/peer_connection_factory_unittest.cc b/pc/peer_connection_factory_unittest.cc index 3b294f75ba..0a32a36de4 100644 --- a/pc/peer_connection_factory_unittest.cc +++ b/pc/peer_connection_factory_unittest.cc @@ -25,6 +25,7 @@ #include "api/enable_media.h" #include "api/enable_media_with_defaults.h" #include "api/environment/environment_factory.h" +#include "api/field_trials.h" #include "api/jsep.h" #include "api/make_ref_counted.h" #include "api/media_stream_interface.h" @@ -56,7 +57,6 @@ #include "rtc_base/time_utils.h" #include "test/gmock.h" #include "test/gtest.h" -#include "test/scoped_key_value_config.h" #ifdef WEBRTC_ANDROID #include "pc/test/android_test_initializer.h" @@ -158,7 +158,8 @@ class PeerConnectionFactoryTest : public ::testing::Test { packet_socket_factory_.reset( new rtc::BasicPacketSocketFactory(socket_server_.get())); port_allocator_.reset(new cricket::FakePortAllocator( - rtc::Thread::Current(), packet_socket_factory_.get(), &field_trials_)); + rtc::Thread::Current(), packet_socket_factory_.get(), + field_trials_.get())); raw_port_allocator_ = port_allocator_.get(); } @@ -253,7 +254,7 @@ class PeerConnectionFactoryTest : public ::testing::Test { } } - test::ScopedKeyValueConfig field_trials_; + std::unique_ptr field_trials_ = FieldTrials::CreateNoGlobal(""); std::unique_ptr socket_server_; rtc::AutoSocketServerThread main_thread_; rtc::scoped_refptr factory_; diff --git a/pc/peer_connection_field_trial_tests.cc b/pc/peer_connection_field_trial_tests.cc index 5d390affe7..410aabeda6 100644 --- a/pc/peer_connection_field_trial_tests.cc +++ b/pc/peer_connection_field_trial_tests.cc @@ -11,12 +11,15 @@ // This file contains tests that verify that field trials do what they're // supposed to do. +#include #include #include "api/audio_codecs/builtin_audio_decoder_factory.h" #include "api/audio_codecs/builtin_audio_encoder_factory.h" #include "api/create_peerconnection_factory.h" #include "api/enable_media_with_defaults.h" +#include "api/field_trials.h" +#include "api/field_trials_view.h" #include "api/peer_connection_interface.h" #include "api/stats/rtcstats_objects.h" #include "api/task_queue/default_task_queue_factory.h" @@ -33,7 +36,6 @@ #include "rtc_base/physical_socket_server.h" #include "rtc_base/thread.h" #include "test/gtest.h" -#include "test/scoped_key_value_config.h" #ifdef WEBRTC_ANDROID #include "pc/test/android_test_initializer.h" @@ -100,10 +102,8 @@ class PeerConnectionFieldTrialTest : public ::testing::Test { // Tests for the dependency descriptor field trial. The dependency descriptor // field trial is implemented in media/engine/webrtc_video_engine.cc. TEST_F(PeerConnectionFieldTrialTest, EnableDependencyDescriptorAdvertised) { - std::unique_ptr field_trials = - std::make_unique( - "WebRTC-DependencyDescriptorAdvertised/Enabled/"); - CreatePCFactory(std::move(field_trials)); + CreatePCFactory(FieldTrials::CreateNoGlobal( + "WebRTC-DependencyDescriptorAdvertised/Enabled/")); WrapperPtr caller = CreatePeerConnection(); caller->AddTransceiver(cricket::MEDIA_TYPE_VIDEO); @@ -135,10 +135,8 @@ TEST_F(PeerConnectionFieldTrialTest, EnableDependencyDescriptorAdvertised) { #define MAYBE_InjectDependencyDescriptor InjectDependencyDescriptor #endif TEST_F(PeerConnectionFieldTrialTest, MAYBE_InjectDependencyDescriptor) { - std::unique_ptr field_trials = - std::make_unique( - "WebRTC-DependencyDescriptorAdvertised/Disabled/"); - CreatePCFactory(std::move(field_trials)); + CreatePCFactory(FieldTrials::CreateNoGlobal( + "WebRTC-DependencyDescriptorAdvertised/Disabled/")); WrapperPtr caller = CreatePeerConnection(); WrapperPtr callee = CreatePeerConnection(); diff --git a/pc/peer_connection_header_extension_unittest.cc b/pc/peer_connection_header_extension_unittest.cc index 61805a7337..f4dff4e948 100644 --- a/pc/peer_connection_header_extension_unittest.cc +++ b/pc/peer_connection_header_extension_unittest.cc @@ -16,6 +16,7 @@ #include #include "absl/strings/string_view.h" +#include "api/field_trials.h" #include "api/jsep.h" #include "api/media_types.h" #include "api/peer_connection_interface.h" @@ -42,7 +43,6 @@ #include "rtc_base/thread.h" #include "test/gmock.h" #include "test/gtest.h" -#include "test/scoped_key_value_config.h" namespace webrtc { @@ -98,7 +98,7 @@ class PeerConnectionHeaderExtensionTest auto fake_port_allocator = std::make_unique( rtc::Thread::Current(), std::make_unique(socket_server_.get()), - &field_trials_); + field_trials_.get()); auto observer = std::make_unique(); PeerConnectionInterface::RTCConfiguration config; if (semantics) @@ -113,7 +113,7 @@ class PeerConnectionHeaderExtensionTest pc_factory, result.MoveValue(), std::move(observer)); } - test::ScopedKeyValueConfig field_trials_; + std::unique_ptr field_trials_ = FieldTrials::CreateNoGlobal(""); std::unique_ptr socket_server_; rtc::AutoSocketServerThread main_thread_; std::vector extensions_; diff --git a/pc/peer_connection_ice_unittest.cc b/pc/peer_connection_ice_unittest.cc index ecf7da6149..613736a185 100644 --- a/pc/peer_connection_ice_unittest.cc +++ b/pc/peer_connection_ice_unittest.cc @@ -23,6 +23,7 @@ #include "api/audio/audio_mixer.h" #include "api/audio/audio_processing.h" #include "api/candidate.h" +#include "api/field_trials.h" #include "api/ice_transport_interface.h" #include "api/jsep.h" #include "api/media_types.h" @@ -54,7 +55,6 @@ #include "rtc_base/socket_address.h" #include "rtc_base/thread.h" #include "test/gtest.h" -#include "test/scoped_key_value_config.h" #ifdef WEBRTC_ANDROID #include "pc/test/android_test_initializer.h" #endif @@ -1445,7 +1445,7 @@ class PeerConnectionIceConfigTest : public ::testing::Test { std::unique_ptr port_allocator( new cricket::FakePortAllocator(rtc::Thread::Current(), packet_socket_factory_.get(), - &field_trials_)); + field_trials_.get())); port_allocator_ = port_allocator.get(); PeerConnectionDependencies pc_dependencies(&observer_); pc_dependencies.allocator = std::move(port_allocator); @@ -1455,7 +1455,7 @@ class PeerConnectionIceConfigTest : public ::testing::Test { pc_ = result.MoveValue(); } - test::ScopedKeyValueConfig field_trials_; + std::unique_ptr field_trials_ = FieldTrials::CreateNoGlobal(""); std::unique_ptr socket_server_; rtc::AutoSocketServerThread main_thread_; rtc::scoped_refptr pc_factory_ = nullptr; diff --git a/pc/peer_connection_interface_unittest.cc b/pc/peer_connection_interface_unittest.cc index 539b952215..63900dcd58 100644 --- a/pc/peer_connection_interface_unittest.cc +++ b/pc/peer_connection_interface_unittest.cc @@ -27,6 +27,7 @@ #include "api/create_peerconnection_factory.h" #include "api/data_channel_interface.h" #include "api/enable_media_with_defaults.h" +#include "api/field_trials.h" #include "api/jsep.h" #include "api/media_stream_interface.h" #include "api/media_types.h" @@ -85,7 +86,6 @@ #include "rtc_base/virtual_socket_server.h" #include "test/gmock.h" #include "test/gtest.h" -#include "test/scoped_key_value_config.h" #ifdef WEBRTC_ANDROID #include "pc/test/android_test_initializer.h" @@ -708,7 +708,7 @@ class PeerConnectionInterfaceBaseTest : public ::testing::Test { new cricket::FakePortAllocator( rtc::Thread::Current(), std::make_unique(vss_.get()), - &field_trials_)); + field_trials_.get())); port_allocator_ = port_allocator.get(); // Create certificate generator unless DTLS constraint is explicitly set to @@ -1227,7 +1227,7 @@ class PeerConnectionInterfaceBaseTest : public ::testing::Test { rtc::SocketServer* socket_server() const { return vss_.get(); } - test::ScopedKeyValueConfig field_trials_; + std::unique_ptr field_trials_ = FieldTrials::CreateNoGlobal(""); std::unique_ptr vss_; rtc::AutoSocketServerThread main_; rtc::scoped_refptr fake_audio_capture_module_; @@ -1336,8 +1336,9 @@ TEST_P(PeerConnectionInterfaceTest, std::unique_ptr packet_socket_factory( new rtc::BasicPacketSocketFactory(socket_server())); std::unique_ptr port_allocator( - new cricket::FakePortAllocator( - rtc::Thread::Current(), packet_socket_factory.get(), &field_trials_)); + new cricket::FakePortAllocator(rtc::Thread::Current(), + packet_socket_factory.get(), + field_trials_.get())); cricket::FakePortAllocator* raw_port_allocator = port_allocator.get(); // Create RTCConfiguration with some network-related fields relevant to diff --git a/pc/peer_connection_media_unittest.cc b/pc/peer_connection_media_unittest.cc index c34e98cb7b..bc129fdb77 100644 --- a/pc/peer_connection_media_unittest.cc +++ b/pc/peer_connection_media_unittest.cc @@ -62,7 +62,6 @@ #include "rtc_base/rtc_certificate_generator.h" #include "rtc_base/thread.h" #include "test/gtest.h" -#include "test/scoped_key_value_config.h" #ifdef WEBRTC_ANDROID #include "pc/test/android_test_initializer.h" #endif @@ -176,7 +175,6 @@ class PeerConnectionMediaBaseTest : public ::testing::Test { EnableFakeMedia(factory_dependencies, std::move(media_engine)); factory_dependencies.event_log_factory = std::make_unique(); - factory_dependencies.trials = std::move(field_trials_); auto pc_factory = CreateModularPeerConnectionFactory(std::move(factory_dependencies)); @@ -253,7 +251,6 @@ class PeerConnectionMediaBaseTest : public ::testing::Test { return sdp_semantics_ == SdpSemantics::kUnifiedPlan; } - std::unique_ptr field_trials_; std::unique_ptr vss_; rtc::AutoSocketServerThread main_; const SdpSemantics sdp_semantics_; diff --git a/pc/rtp_sender_receiver_unittest.cc b/pc/rtp_sender_receiver_unittest.cc index 50804e459d..743266ce61 100644 --- a/pc/rtp_sender_receiver_unittest.cc +++ b/pc/rtp_sender_receiver_unittest.cc @@ -67,7 +67,6 @@ #include "test/gmock.h" #include "test/gtest.h" #include "test/run_loop.h" -#include "test/scoped_key_value_config.h" using ::testing::_; using ::testing::ContainerEq; @@ -111,7 +110,7 @@ class RtpSenderReceiverTest // Create fake media engine/etc. so we can create channels to use to // test RtpSenders/RtpReceivers. media_engine_(std::make_unique()), - fake_call_(CreateEnvironment(), worker_thread_, network_thread_), + fake_call_(env_, worker_thread_, network_thread_), local_stream_(MediaStream::Create(kStreamId1)) { rtp_dtls_transport_ = std::make_unique( "fake_dtls_transport", cricket::ICE_CANDIDATE_COMPONENT_RTP); @@ -166,7 +165,7 @@ class RtpSenderReceiverTest std::unique_ptr CreateDtlsSrtpTransport() { auto dtls_srtp_transport = std::make_unique( - /*rtcp_mux_required=*/true, field_trials_); + /*rtcp_mux_required=*/true, env_.field_trials()); dtls_srtp_transport->SetDtlsTransports(rtp_dtls_transport_.get(), /*rtcp_dtls_transport=*/nullptr); return dtls_srtp_transport; @@ -519,7 +518,7 @@ class RtpSenderReceiverTest test::RunLoop run_loop_; rtc::Thread* const network_thread_; rtc::Thread* const worker_thread_; - RtcEventLogNull event_log_; + const Environment env_ = CreateEnvironment(); // The `rtp_dtls_transport_` and `rtp_transport_` should be destroyed after // the `channel_manager`. std::unique_ptr rtp_dtls_transport_; @@ -544,7 +543,6 @@ class RtpSenderReceiverTest rtc::scoped_refptr local_stream_; rtc::scoped_refptr video_track_; rtc::scoped_refptr audio_track_; - test::ScopedKeyValueConfig field_trials_; }; // Test that `voice_channel_` is updated when an audio track is associated diff --git a/pc/sdp_offer_answer_unittest.cc b/pc/sdp_offer_answer_unittest.cc index 5e8ae14816..a1bb5d85d6 100644 --- a/pc/sdp_offer_answer_unittest.cc +++ b/pc/sdp_offer_answer_unittest.cc @@ -21,6 +21,8 @@ #include "api/audio_codecs/builtin_audio_decoder_factory.h" #include "api/audio_codecs/builtin_audio_encoder_factory.h" #include "api/create_peerconnection_factory.h" +#include "api/field_trials.h" +#include "api/field_trials_view.h" #include "api/jsep.h" #include "api/media_types.h" #include "api/peer_connection_interface.h" @@ -51,7 +53,6 @@ #include "system_wrappers/include/metrics.h" #include "test/gmock.h" #include "test/gtest.h" -#include "test/scoped_key_value_config.h" // This file contains unit tests that relate to the behavior of the // SdpOfferAnswer module. @@ -98,22 +99,25 @@ class SdpOfferAnswerTest : public ::testing::Test { Dav1dDecoderTemplateAdapter>>(), nullptr /* audio_mixer */, nullptr /* audio_processing */, - nullptr /* audio_frame_processor */, - std::make_unique(field_trials_, ""))) { + nullptr /* audio_frame_processor */)) { metrics::Reset(); } - std::unique_ptr CreatePeerConnection() { + std::unique_ptr CreatePeerConnection( + std::unique_ptr field_trials = nullptr) { RTCConfiguration config; config.sdp_semantics = SdpSemantics::kUnifiedPlan; - return CreatePeerConnection(config); + return CreatePeerConnection(config, std::move(field_trials)); } std::unique_ptr CreatePeerConnection( - const RTCConfiguration& config) { + const RTCConfiguration& config, + std::unique_ptr field_trials) { auto observer = std::make_unique(); - auto result = pc_factory_->CreatePeerConnectionOrError( - config, PeerConnectionDependencies(observer.get())); + PeerConnectionDependencies pc_deps(observer.get()); + pc_deps.trials = std::move(field_trials); + auto result = + pc_factory_->CreatePeerConnectionOrError(config, std::move(pc_deps)); EXPECT_TRUE(result.ok()); observer->SetPeerConnectionInterface(result.value().get()); return std::make_unique( @@ -134,7 +138,6 @@ class SdpOfferAnswerTest : public ::testing::Test { } protected: - test::ScopedKeyValueConfig field_trials_; std::unique_ptr signaling_thread_; rtc::scoped_refptr pc_factory_; @@ -637,10 +640,8 @@ TEST_F(SdpOfferAnswerTest, SimulcastAnswerWithNoRidsIsRejected) { } TEST_F(SdpOfferAnswerTest, SimulcastOfferWithMixedCodec) { - test::ScopedKeyValueConfig field_trials( - field_trials_, "WebRTC-MixedCodecSimulcast/Enabled/"); - - auto pc = CreatePeerConnection(); + auto pc = CreatePeerConnection( + FieldTrials::CreateNoGlobal("WebRTC-MixedCodecSimulcast/Enabled/")); std::optional vp8_codec = FindFirstSendCodecWithName( cricket::MEDIA_TYPE_VIDEO, cricket::kVp8CodecName); @@ -691,10 +692,8 @@ TEST_F(SdpOfferAnswerTest, SimulcastOfferWithMixedCodec) { } TEST_F(SdpOfferAnswerTest, SimulcastAnswerWithPayloadType) { - test::ScopedKeyValueConfig field_trials( - field_trials_, "WebRTC-MixedCodecSimulcast/Enabled/"); - - auto pc = CreatePeerConnection(); + auto pc = CreatePeerConnection( + FieldTrials::CreateNoGlobal("WebRTC-MixedCodecSimulcast/Enabled/")); // A SDP offer with recv simulcast with payload type std::string sdp = diff --git a/pc/test/integration_test_helpers.cc b/pc/test/integration_test_helpers.cc index 332f084747..bf9f454746 100644 --- a/pc/test/integration_test_helpers.cc +++ b/pc/test/integration_test_helpers.cc @@ -28,7 +28,6 @@ #include "api/task_queue/default_task_queue_factory.h" #include "api/task_queue/pending_task_safety_flag.h" #include "api/task_queue/task_queue_base.h" -#include "api/transport/field_trial_based_config.h" #include "api/units/time_delta.h" #include "logging/rtc_event_log/fake_rtc_event_log_factory.h" #include "p2p/base/basic_packet_socket_factory.h" @@ -223,6 +222,7 @@ bool PeerConnectionIntegrationWrapper::Init( rtc::SocketServer* socket_server, rtc::Thread* network_thread, rtc::Thread* worker_thread, + std::unique_ptr field_trials, std::unique_ptr event_log_factory, bool reset_encoder_factory, bool reset_decoder_factory, @@ -251,7 +251,7 @@ bool PeerConnectionIntegrationWrapper::Init( pc_factory_dependencies.worker_thread = worker_thread; pc_factory_dependencies.signaling_thread = signaling_thread; pc_factory_dependencies.task_queue_factory = CreateDefaultTaskQueueFactory(); - pc_factory_dependencies.trials = std::make_unique(); + pc_factory_dependencies.trials = std::move(field_trials); pc_factory_dependencies.decode_metronome = std::make_unique(TimeDelta::Millis(8)); diff --git a/pc/test/integration_test_helpers.h b/pc/test/integration_test_helpers.h index 7afb8a9449..d68f05f681 100644 --- a/pc/test/integration_test_helpers.h +++ b/pc/test/integration_test_helpers.h @@ -33,6 +33,7 @@ #include "api/candidate.h" #include "api/crypto/crypto_options.h" #include "api/data_channel_interface.h" +#include "api/field_trials.h" #include "api/field_trials_view.h" #include "api/ice_transport_interface.h" #include "api/jsep.h" @@ -94,7 +95,6 @@ #include "system_wrappers/include/metrics.h" #include "test/gmock.h" #include "test/gtest.h" -#include "test/scoped_key_value_config.h" namespace webrtc { @@ -741,6 +741,7 @@ class PeerConnectionIntegrationWrapper : public PeerConnectionObserver, rtc::SocketServer* socket_server, rtc::Thread* network_thread, rtc::Thread* worker_thread, + std::unique_ptr field_trials, std::unique_ptr event_log_factory, bool reset_encoder_factory, bool reset_decoder_factory, @@ -1322,17 +1323,12 @@ class MockIceTransportFactory : public IceTransportFactory { // of everything else (including "PeerConnectionFactory"s). class PeerConnectionIntegrationBaseTest : public ::testing::Test { public: - PeerConnectionIntegrationBaseTest( - SdpSemantics sdp_semantics, - std::optional field_trials = std::nullopt) + explicit PeerConnectionIntegrationBaseTest(SdpSemantics sdp_semantics) : sdp_semantics_(sdp_semantics), ss_(new rtc::VirtualSocketServer()), fss_(new rtc::FirewallSocketServer(ss_.get())), network_thread_(new rtc::Thread(fss_.get())), - worker_thread_(rtc::Thread::Create()), - // TODO(bugs.webrtc.org/10335): Pass optional ScopedKeyValueConfig. - field_trials_(new test::ScopedKeyValueConfig( - field_trials.has_value() ? *field_trials : "")) { + worker_thread_(rtc::Thread::Create()) { network_thread_->SetName("PCNetworkThread", this); worker_thread_->SetName("PCWorkerThread", this); RTC_CHECK(network_thread_->Start()); @@ -1383,6 +1379,14 @@ class PeerConnectionIntegrationBaseTest : public ::testing::Test { PeerConnectionInterface::kIceConnectionCompleted); } + // Sets field trials to pass to created PeerConnectionWrapper. + // Must be called before PeerConnectionWrappers are created. + void SetFieldTrials(absl::string_view field_trials) { + RTC_CHECK(caller_ == nullptr); + RTC_CHECK(callee_ == nullptr); + field_trials_ = std::string(field_trials); + } + // When `event_log_factory` is null, the default implementation of the event // log factory will be used. std::unique_ptr CreatePeerConnectionWrapper( @@ -1408,6 +1412,7 @@ class PeerConnectionIntegrationBaseTest : public ::testing::Test { if (!client->Init(options, &modified_config, std::move(dependencies), fss_.get(), network_thread_.get(), worker_thread_.get(), + FieldTrials::CreateNoGlobal(field_trials_), std::move(event_log_factory), reset_encoder_factory, reset_decoder_factory, create_media_engine)) { return nullptr; @@ -1837,8 +1842,6 @@ class PeerConnectionIntegrationBaseTest : public ::testing::Test { expected_cipher_suite); } - const FieldTrialsView& trials() const { return *field_trials_.get(); } - protected: SdpSemantics sdp_semantics_; @@ -1859,7 +1862,7 @@ class PeerConnectionIntegrationBaseTest : public ::testing::Test { std::vector> turn_customizers_; std::unique_ptr caller_; std::unique_ptr callee_; - std::unique_ptr field_trials_; + std::string field_trials_; }; } // namespace webrtc diff --git a/pc/test/peer_connection_test_wrapper.cc b/pc/test/peer_connection_test_wrapper.cc index 987d0d70c6..aa190b9243 100644 --- a/pc/test/peer_connection_test_wrapper.cc +++ b/pc/test/peer_connection_test_wrapper.cc @@ -51,12 +51,12 @@ #include "rtc_base/string_encode.h" #include "rtc_base/time_utils.h" #include "test/gtest.h" -#include "test/scoped_key_value_config.h" namespace { using ::webrtc::Environment; using ::webrtc::FakeVideoTrackRenderer; +using ::webrtc::FieldTrialsView; using ::webrtc::IceCandidateInterface; using ::webrtc::MediaStreamInterface; using ::webrtc::MediaStreamTrackInterface; @@ -134,21 +134,6 @@ PeerConnectionTestWrapper::PeerConnectionTestWrapper( pc_thread_checker_.Detach(); } -PeerConnectionTestWrapper::PeerConnectionTestWrapper( - const std::string& name, - rtc::SocketServer* socket_server, - rtc::Thread* network_thread, - rtc::Thread* worker_thread, - webrtc::test::ScopedKeyValueConfig& field_trials) - : field_trials_(field_trials, ""), - name_(name), - socket_server_(socket_server), - network_thread_(network_thread), - worker_thread_(worker_thread), - pending_negotiation_(false) { - pc_thread_checker_.Detach(); -} - PeerConnectionTestWrapper::~PeerConnectionTestWrapper() { RTC_DCHECK_RUN_ON(&pc_thread_checker_); // To avoid flaky bot failures, make sure fake sources are stopped prior to @@ -166,12 +151,13 @@ PeerConnectionTestWrapper::~PeerConnectionTestWrapper() { bool PeerConnectionTestWrapper::CreatePc( const webrtc::PeerConnectionInterface::RTCConfiguration& config, rtc::scoped_refptr audio_encoder_factory, - rtc::scoped_refptr audio_decoder_factory) { + rtc::scoped_refptr audio_decoder_factory, + std::unique_ptr field_trials) { std::unique_ptr port_allocator( new cricket::FakePortAllocator( network_thread_, std::make_unique(socket_server_), - &field_trials_)); + field_trials.get())); RTC_DCHECK_RUN_ON(&pc_thread_checker_); @@ -191,7 +177,7 @@ bool PeerConnectionTestWrapper::CreatePc( webrtc::OpenH264DecoderTemplateAdapter, webrtc::Dav1dDecoderTemplateAdapter>>(), nullptr /* audio_mixer */, nullptr /* audio_processing */, nullptr, - std::make_unique(field_trials_, "")); + std::move(field_trials)); if (!peer_connection_factory_) { return false; } diff --git a/pc/test/peer_connection_test_wrapper.h b/pc/test/peer_connection_test_wrapper.h index bf21e99ea4..0a3a689e94 100644 --- a/pc/test/peer_connection_test_wrapper.h +++ b/pc/test/peer_connection_test_wrapper.h @@ -35,7 +35,6 @@ #include "pc/test/fake_video_track_renderer.h" #include "rtc_base/third_party/sigslot/sigslot.h" #include "rtc_base/thread.h" -#include "test/scoped_key_value_config.h" class PeerConnectionTestWrapper : public webrtc::PeerConnectionObserver, @@ -49,17 +48,13 @@ class PeerConnectionTestWrapper rtc::SocketServer* socket_server, rtc::Thread* network_thread, rtc::Thread* worker_thread); - PeerConnectionTestWrapper(const std::string& name, - rtc::SocketServer* socket_server, - rtc::Thread* network_thread, - rtc::Thread* worker_thread, - webrtc::test::ScopedKeyValueConfig& field_trials); virtual ~PeerConnectionTestWrapper(); bool CreatePc( const webrtc::PeerConnectionInterface::RTCConfiguration& config, rtc::scoped_refptr audio_encoder_factory, - rtc::scoped_refptr audio_decoder_factory); + rtc::scoped_refptr audio_decoder_factory, + std::unique_ptr field_trials = nullptr); rtc::scoped_refptr pc_factory() const { @@ -136,7 +131,6 @@ class PeerConnectionTestWrapper bool CheckForAudio(); bool CheckForVideo(); - webrtc::test::ScopedKeyValueConfig field_trials_; std::string name_; rtc::SocketServer* const socket_server_; rtc::Thread* const network_thread_;