From 6c7f98472e8c629ace8ab60eb8ea50b615856902 Mon Sep 17 00:00:00 2001 From: Jonas Oreland Date: Tue, 19 Apr 2022 17:24:10 +0200 Subject: [PATCH] WebRTC-DeprecateGlobalFieldTrialString/Enabled/ - part 16/inf This cl/ adds the feature actually injecting a FieldTrialsView into PeerConnectionFactory, or into a PeerConnection or both. The field trials used for a PeerConnection is those specified in PeerConnectionDependencies. Otherwise will those from PeerConnectionFactoryDependencies be used (and until we're finished with this conversion, the global string fallback is used as last resort). Note that it is currently not possible to create 2 FieldTrials objects concurrently...due to global string, so this cl/ is mostly (but entirely) for show, i.e one _can_ realistically inject them into a PeerConnectionFactory. Bug: webrtc:10335 Change-Id: Id2e60525f48a1f8293c1dd0be771e3ed03790963 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/258134 Reviewed-by: Mirko Bonadei Reviewed-by: Harald Alvestrand Commit-Queue: Jonas Oreland Cr-Commit-Position: refs/heads/main@{#36578} --- api/create_peerconnection_factory.cc | 9 ++++- api/create_peerconnection_factory.h | 3 +- api/peer_connection_interface.h | 3 ++ pc/connection_context.cc | 2 +- pc/connection_context.h | 6 ++- pc/peer_connection.cc | 9 ++--- pc/peer_connection.h | 8 +++- pc/peer_connection_factory.cc | 16 +++++--- pc/peer_connection_factory.h | 7 +++- pc/peer_connection_internal.h | 4 +- pc/sdp_offer_answer.cc | 5 ++- pc/test/fake_peer_connection_base.h | 2 +- pc/webrtc_session_description_factory.cc | 5 ++- pc/webrtc_session_description_factory.h | 3 +- rtc_base/memory/always_valid_pointer.h | 30 +++++++++++++++ .../memory/always_valid_pointer_unittest.cc | 38 +++++++++++++++++++ 16 files changed, 123 insertions(+), 27 deletions(-) diff --git a/api/create_peerconnection_factory.cc b/api/create_peerconnection_factory.cc index c41b6d6fb2..4a01b2f3c9 100644 --- a/api/create_peerconnection_factory.cc +++ b/api/create_peerconnection_factory.cc @@ -38,7 +38,8 @@ rtc::scoped_refptr CreatePeerConnectionFactory( std::unique_ptr video_decoder_factory, rtc::scoped_refptr audio_mixer, rtc::scoped_refptr audio_processing, - AudioFrameProcessor* audio_frame_processor) { + AudioFrameProcessor* audio_frame_processor, + std::unique_ptr field_trials) { PeerConnectionFactoryDependencies dependencies; dependencies.network_thread = network_thread; dependencies.worker_thread = worker_thread; @@ -47,7 +48,11 @@ rtc::scoped_refptr CreatePeerConnectionFactory( dependencies.call_factory = CreateCallFactory(); dependencies.event_log_factory = std::make_unique( dependencies.task_queue_factory.get()); - dependencies.trials = std::make_unique(); + if (field_trials) { + dependencies.trials = std::move(field_trials); + } else { + dependencies.trials = std::make_unique(); + } if (network_thread) { // TODO(bugs.webrtc.org/13145): Add an rtc::SocketFactory* argument. diff --git a/api/create_peerconnection_factory.h b/api/create_peerconnection_factory.h index 4eb0a00e54..efebc5f3ea 100644 --- a/api/create_peerconnection_factory.h +++ b/api/create_peerconnection_factory.h @@ -49,7 +49,8 @@ CreatePeerConnectionFactory( std::unique_ptr video_decoder_factory, rtc::scoped_refptr audio_mixer, rtc::scoped_refptr audio_processing, - AudioFrameProcessor* audio_frame_processor = nullptr); + AudioFrameProcessor* audio_frame_processor = nullptr, + std::unique_ptr field_trials = nullptr); } // namespace webrtc diff --git a/api/peer_connection_interface.h b/api/peer_connection_interface.h index 5f1f09937e..c326799edb 100644 --- a/api/peer_connection_interface.h +++ b/api/peer_connection_interface.h @@ -1398,6 +1398,9 @@ struct RTC_EXPORT PeerConnectionDependencies final { std::unique_ptr tls_cert_verifier; std::unique_ptr video_bitrate_allocator_factory; + // Optional field trials to use. + // Overrides those from PeerConnectionFactoryDependencies. + std::unique_ptr trials; }; // PeerConnectionFactoryDependencies holds all of the PeerConnectionFactory diff --git a/pc/connection_context.cc b/pc/connection_context.cc index 736a6d79b3..d7c80ebfda 100644 --- a/pc/connection_context.cc +++ b/pc/connection_context.cc @@ -145,7 +145,7 @@ ConnectionContext::ConnectionContext( // If network_monitor_factory_ is non-null, it will be used to create a // network monitor while on the network thread. default_network_manager_ = std::make_unique( - network_monitor_factory_.get(), socket_factory, &trials()); + network_monitor_factory_.get(), socket_factory, &field_trials()); default_socket_factory_ = std::make_unique(socket_factory); diff --git a/pc/connection_context.h b/pc/connection_context.h index 3b8ac07fc9..e4aa715985 100644 --- a/pc/connection_context.h +++ b/pc/connection_context.h @@ -75,7 +75,11 @@ class ConnectionContext final rtc::Thread* network_thread() { return network_thread_; } const rtc::Thread* network_thread() const { return network_thread_; } - const FieldTrialsView& trials() const { return *trials_.get(); } + // Field trials associated with the PeerConnectionFactory. + // Note: that there can be different field trials for different + // PeerConnections (but they are not supposed change after creating the + // PeerConnection). + const FieldTrialsView& field_trials() const { return *trials_.get(); } // Accessors only used from the PeerConnectionFactory class rtc::BasicNetworkManager* default_network_manager() { diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index 0705c84a97..e7891796b5 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -509,6 +509,7 @@ PeerConnection::PeerConnection( PeerConnectionDependencies& dependencies, bool dtls_enabled) : context_(context), + trials_(std::move(dependencies.trials), &context->field_trials()), options_(options), observer_(dependencies.observer), is_unified_plan_(is_unified_plan), @@ -719,7 +720,7 @@ JsepTransportController* PeerConnection::InitializeTransportController_n( } }; - config.field_trials = &context_->trials(); + config.field_trials = trials_.get(); transport_controller_.reset( new JsepTransportController(network_thread(), port_allocator_.get(), @@ -1692,8 +1693,7 @@ bool PeerConnection::StartRtcEventLog(std::unique_ptr output, bool PeerConnection::StartRtcEventLog( std::unique_ptr output) { int64_t output_period_ms = webrtc::RtcEventLog::kImmediateOutput; - if (absl::StartsWith(context_->trials().Lookup("WebRTC-RtcEventLogNewFormat"), - "Enabled")) { + if (trials().IsEnabled("WebRTC-RtcEventLogNewFormat")) { output_period_ms = 5000; } return StartRtcEventLog(std::move(output), output_period_ms); @@ -2045,8 +2045,7 @@ PeerConnection::InitializePortAllocator_n( // by experiment. if (configuration.disable_ipv6) { port_allocator_flags &= ~(cricket::PORTALLOCATOR_ENABLE_IPV6); - } else if (absl::StartsWith(context_->trials().Lookup("WebRTC-IPv6Default"), - "Disabled")) { + } else if (trials().IsDisabled("WebRTC-IPv6Default")) { port_allocator_flags &= ~(cricket::PORTALLOCATOR_ENABLE_IPV6); } if (configuration.disable_ipv6_on_wifi) { diff --git a/pc/peer_connection.h b/pc/peer_connection.h index 919d377b2b..84a881083f 100644 --- a/pc/peer_connection.h +++ b/pc/peer_connection.h @@ -437,7 +437,7 @@ class PeerConnection : public PeerConnectionInternal, } void RequestUsagePatternReportForTesting(); - const FieldTrialsView& trials() override { return context_->trials(); } + const FieldTrialsView& trials() const override { return *trials_; } protected: // Available for rtc::scoped_refptr creation @@ -592,6 +592,12 @@ class PeerConnection : public PeerConnectionInternal, InitializeRtcpCallback(); const rtc::scoped_refptr context_; + // Field trials active for this PeerConnection is the first of: + // a) Specified in PeerConnectionDependencies (owned). + // b) Accessed via ConnectionContext (e.g PeerConnectionFactoryDependencies> + // c) Created as Default (FieldTrialBasedConfig). + const webrtc::AlwaysValidPointer + trials_; const PeerConnectionFactoryInterface::Options options_; PeerConnectionObserver* observer_ RTC_GUARDED_BY(signaling_thread()) = nullptr; diff --git a/pc/peer_connection_factory.cc b/pc/peer_connection_factory.cc index c598440201..aa9f328231 100644 --- a/pc/peer_connection_factory.cc +++ b/pc/peer_connection_factory.cc @@ -243,9 +243,12 @@ PeerConnectionFactory::CreatePeerConnectionOrError( worker_thread()->Invoke>( RTC_FROM_HERE, [this] { return CreateRtcEventLog_w(); }); + const FieldTrialsView* trials = + dependencies.trials ? dependencies.trials.get() : &field_trials(); std::unique_ptr call = worker_thread()->Invoke>( - RTC_FROM_HERE, - [this, &event_log] { return CreateCall_w(event_log.get()); }); + RTC_FROM_HERE, [this, &event_log, trials] { + return CreateCall_w(event_log.get(), *trials); + }); auto result = PeerConnection::Create(context_, options_, std::move(event_log), std::move(call), configuration, @@ -307,7 +310,8 @@ std::unique_ptr PeerConnectionFactory::CreateRtcEventLog_w() { } std::unique_ptr PeerConnectionFactory::CreateCall_w( - RtcEventLog* event_log) { + RtcEventLog* event_log, + const FieldTrialsView& field_trials) { RTC_DCHECK_RUN_ON(worker_thread()); webrtc::Call::Config call_config(event_log, network_thread()); @@ -324,7 +328,7 @@ std::unique_ptr PeerConnectionFactory::CreateCall_w( FieldTrialParameter max_bandwidth("max", DataRate::KilobitsPerSec(2000)); ParseFieldTrial({&min_bandwidth, &start_bandwidth, &max_bandwidth}, - trials().Lookup("WebRTC-PcFactoryDefaultBitrates")); + field_trials.Lookup("WebRTC-PcFactoryDefaultBitrates")); call_config.bitrate_config.min_bitrate_bps = rtc::saturated_cast(min_bandwidth->bps()); @@ -347,7 +351,7 @@ std::unique_ptr PeerConnectionFactory::CreateCall_w( RTC_LOG(LS_INFO) << "Using default network controller factory"; } - call_config.trials = &trials(); + call_config.trials = &field_trials; call_config.rtp_transport_controller_send_factory = transport_controller_send_factory_.get(); call_config.metronome = metronome_.get(); @@ -356,7 +360,7 @@ std::unique_ptr PeerConnectionFactory::CreateCall_w( } bool PeerConnectionFactory::IsTrialEnabled(absl::string_view key) const { - return absl::StartsWith(trials().Lookup(key), "Enabled"); + return absl::StartsWith(field_trials().Lookup(key), "Enabled"); } } // namespace webrtc diff --git a/pc/peer_connection_factory.h b/pc/peer_connection_factory.h index 4ee102a2fb..16695f234a 100644 --- a/pc/peer_connection_factory.h +++ b/pc/peer_connection_factory.h @@ -115,7 +115,9 @@ class PeerConnectionFactory : public PeerConnectionFactoryInterface { return options_; } - const FieldTrialsView& trials() const { return context_->trials(); } + const FieldTrialsView& field_trials() const { + return context_->field_trials(); + } protected: // Constructor used by the static Create() method. Modifies the dependencies. @@ -138,7 +140,8 @@ class PeerConnectionFactory : public PeerConnectionFactoryInterface { } std::unique_ptr CreateRtcEventLog_w(); - std::unique_ptr CreateCall_w(RtcEventLog* event_log); + std::unique_ptr CreateCall_w(RtcEventLog* event_log, + const FieldTrialsView& field_trials); rtc::scoped_refptr context_; PeerConnectionFactoryInterface::Options options_ diff --git a/pc/peer_connection_internal.h b/pc/peer_connection_internal.h index 66897ee758..762f9b1e08 100644 --- a/pc/peer_connection_internal.h +++ b/pc/peer_connection_internal.h @@ -123,6 +123,8 @@ class PeerConnectionSdpMethods { virtual void TeardownDataChannelTransport_n() = 0; virtual void SetSctpDataMid(const std::string& mid) = 0; virtual void ResetSctpDataMid() = 0; + + virtual const FieldTrialsView& trials() const = 0; }; // Functions defined in this class are called by other objects, @@ -178,8 +180,6 @@ class PeerConnectionInternal : public PeerConnectionInterface, virtual void NoteDataAddedEvent() {} // Handler for the "channel closed" signal virtual void OnSctpDataChannelClosed(DataChannelInterface* channel) {} - - virtual const FieldTrialsView& trials() = 0; }; } // namespace webrtc diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc index d942548cbd..1ad793c841 100644 --- a/pc/sdp_offer_answer.cc +++ b/pc/sdp_offer_answer.cc @@ -1201,7 +1201,7 @@ void SdpOfferAnswerHandler::Initialize( // Use 100 kbps as the default minimum screencast bitrate unless this path is // kill-switched. if (!video_options_.screencast_min_bitrate_kbps.has_value() && - !context_->trials().IsEnabled(kDefaultScreencastMinBitrateKillSwitch)) { + !pc_->trials().IsEnabled(kDefaultScreencastMinBitrateKillSwitch)) { video_options_.screencast_min_bitrate_kbps = 100; } audio_options_.combined_audio_video_bwe = @@ -1235,7 +1235,8 @@ void SdpOfferAnswerHandler::Initialize( [this](const rtc::scoped_refptr& certificate) { RTC_DCHECK_RUN_ON(signaling_thread()); transport_controller_s()->SetLocalCertificate(certificate); - }); + }, + pc_->trials()); if (pc_->options()->disable_encryption) { webrtc_session_desc_factory_->SetSdesPolicy(cricket::SEC_DISABLED); diff --git a/pc/test/fake_peer_connection_base.h b/pc/test/fake_peer_connection_base.h index c8863059db..f629f04e6b 100644 --- a/pc/test/fake_peer_connection_base.h +++ b/pc/test/fake_peer_connection_base.h @@ -360,7 +360,7 @@ class FakePeerConnectionBase : public PeerConnectionInternal { void SetSctpDataMid(const std::string& mid) override {} void ResetSctpDataMid() override {} - const FieldTrialsView& trials() override { return field_trials_; } + const FieldTrialsView& trials() const override { return field_trials_; } protected: webrtc::test::ScopedKeyValueConfig field_trials_; diff --git a/pc/webrtc_session_description_factory.cc b/pc/webrtc_session_description_factory.cc index 09f2ee2f00..7ccc3bf252 100644 --- a/pc/webrtc_session_description_factory.cc +++ b/pc/webrtc_session_description_factory.cc @@ -135,9 +135,10 @@ WebRtcSessionDescriptionFactory::WebRtcSessionDescriptionFactory( std::unique_ptr cert_generator, const rtc::scoped_refptr& certificate, std::function&)> - on_certificate_ready) + on_certificate_ready, + const FieldTrialsView& field_trials) : signaling_thread_(context->signaling_thread()), - transport_desc_factory_(context->trials()), + transport_desc_factory_(field_trials), session_desc_factory_(context->channel_manager(), &transport_desc_factory_), // RFC 4566 suggested a Network Time Protocol (NTP) format timestamp diff --git a/pc/webrtc_session_description_factory.h b/pc/webrtc_session_description_factory.h index 79171f797b..3f1519082b 100644 --- a/pc/webrtc_session_description_factory.h +++ b/pc/webrtc_session_description_factory.h @@ -86,7 +86,8 @@ class WebRtcSessionDescriptionFactory : public rtc::MessageHandler, std::unique_ptr cert_generator, const rtc::scoped_refptr& certificate, std::function&)> - on_certificate_ready); + on_certificate_ready, + const FieldTrialsView& field_trials); virtual ~WebRtcSessionDescriptionFactory(); WebRtcSessionDescriptionFactory(const WebRtcSessionDescriptionFactory&) = diff --git a/rtc_base/memory/always_valid_pointer.h b/rtc_base/memory/always_valid_pointer.h index c6e0a70d6b..a878083fc0 100644 --- a/rtc_base/memory/always_valid_pointer.h +++ b/rtc_base/memory/always_valid_pointer.h @@ -37,6 +37,36 @@ class AlwaysValidPointer { RTC_DCHECK(pointer_); } + // Create a pointer by + // a) taking over ownership of |instance| + // b) or fallback to |pointer|, without taking ownership. + // c) or Default. + AlwaysValidPointer(std::unique_ptr&& instance, Interface* pointer) + : owned_instance_( + instance + ? std::move(instance) + : (pointer == nullptr ? std::make_unique() : nullptr)), + pointer_(owned_instance_ ? owned_instance_.get() : pointer) { + RTC_DCHECK(pointer_); + } + + // Create a pointer by + // a) taking over ownership of |instance| + // b) or fallback to |pointer|, without taking ownership. + // c) or Default (with forwarded args). + template + AlwaysValidPointer(std::unique_ptr&& instance, + Interface* pointer, + Args... args) + : owned_instance_( + instance ? std::move(instance) + : (pointer == nullptr + ? std::make_unique(std::move(args...)) + : nullptr)), + pointer_(owned_instance_ ? owned_instance_.get() : pointer) { + RTC_DCHECK(pointer_); + } + Interface* get() { return pointer_; } Interface* operator->() { return pointer_; } Interface& operator*() { return *pointer_; } diff --git a/rtc_base/memory/always_valid_pointer_unittest.cc b/rtc_base/memory/always_valid_pointer_unittest.cc index dbb0671e32..92cf56bffe 100644 --- a/rtc_base/memory/always_valid_pointer_unittest.cc +++ b/rtc_base/memory/always_valid_pointer_unittest.cc @@ -46,4 +46,42 @@ TEST(AlwaysValidPointerTest, NonDefaultValue) { EXPECT_EQ(*ptr, "keso"); } +TEST(AlwaysValidPointerTest, TakeOverOwnershipOfInstance) { + std::string str("keso"); + std::unique_ptr str2 = std::make_unique("kent"); + AlwaysValidPointer ptr(std::move(str2), &str); + EXPECT_EQ(*ptr, "kent"); + EXPECT_EQ(str2, nullptr); +} + +TEST(AlwaysValidPointerTest, TakeOverOwnershipFallbackOnPointer) { + std::string str("keso"); + std::unique_ptr str2; + AlwaysValidPointer ptr(std::move(str2), &str); + EXPECT_EQ(*ptr, "keso"); +} + +TEST(AlwaysValidPointerTest, TakeOverOwnershipFallbackOnDefault) { + std::unique_ptr str; + std::string* str_ptr = nullptr; + AlwaysValidPointer ptr(std::move(str), str_ptr); + EXPECT_EQ(*ptr, ""); +} + +TEST(AlwaysValidPointerTest, + TakeOverOwnershipFallbackOnDefaultWithForwardedArgument) { + std::unique_ptr str2; + AlwaysValidPointer ptr(std::move(str2), nullptr, "keso"); + EXPECT_EQ(*ptr, "keso"); +} + +TEST(AlwaysValidPointerTest, TakeOverOwnershipDoesNotForwardDefaultArguments) { + std::unique_ptr str = std::make_unique("kalle"); + std::unique_ptr str2 = std::make_unique("anka"); + AlwaysValidPointer ptr(std::move(str), nullptr, *str2); + EXPECT_EQ(*ptr, "kalle"); + EXPECT_TRUE(!str); + EXPECT_EQ(*str2, "anka"); +} + } // namespace webrtc