Cache ICE credentials and remove signaling thread blocking calls Avoid thread hops to the network thread by caching pooled ICE credentials on the signaling thread. Previously, generating SDP offers or answers required synchronous blocking calls to retrieve credentials from the PortAllocator. Key changes: * Update JsepTransportController::MaybeStartGathering to return pooled credentials from the network thread. * Extend PeerConnection and SdpOfferAnswerHandler to store and propagate these credentials to session options without blocking. * Update tests and fakes to support the new credential retrieval flow and proper PortAllocator lifecycle management. Bug: webrtc:42222117 Change-Id: Ib66bb074ae9b7ee33b6033ce31c3f0746897a4f3 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/463560 Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org> Reviewed-by: Danil Chapovalov <danilchap@webrtc.org> Cr-Commit-Position: refs/heads/main@{#47443}
diff --git a/pc/codec_vendor.cc b/pc/codec_vendor.cc index 312dc87..113a881 100644 --- a/pc/codec_vendor.cc +++ b/pc/codec_vendor.cc
@@ -174,12 +174,7 @@ absl::string_view mid, CodecList& offered_codecs, PayloadTypeSuggester& pt_suggester) { - // TODO: bugs.webrtc.org/360058654 - This method makes blocking calls to - // the network thread. Once those calls run on the current (signaling) thread - // change `RTC_LOG_THREAD_BLOCK_COUNT()` to - // `RTC_DCHECK_DISALLOW_THREAD_BLOCKING_CALLS()`. - RTC_LOG_THREAD_BLOCK_COUNT(); - + RTC_DCHECK_DISALLOW_THREAD_BLOCKING_CALLS(); // Add all new codecs that are not RTX/RED codecs. // The two-pass splitting of the loops means preferring payload types // of actual codecs with respect to collisions. @@ -752,11 +747,7 @@ const std::vector<Codec> codecs_from_offer, PayloadTypeSuggester& pt_suggester) { RTC_DCHECK_RUN_ON(&sequence_checker_); - // TODO: bugs.webrtc.org/360058654 - This method makes blocking calls to - // the network thread. Once those calls run on the current (signaling) thread - // change `RTC_LOG_THREAD_BLOCK_COUNT()` to - // `RTC_DCHECK_DISALLOW_THREAD_BLOCKING_CALLS()`. - RTC_LOG_THREAD_BLOCK_COUNT(); + RTC_DCHECK_DISALLOW_THREAD_BLOCKING_CALLS(); CodecList codecs; std::string mid = media_description_options.mid; if (current_content && current_content->mid() == mid &&
diff --git a/pc/jsep_transport_controller.cc b/pc/jsep_transport_controller.cc index 9612501..636e7ad 100644 --- a/pc/jsep_transport_controller.cc +++ b/pc/jsep_transport_controller.cc
@@ -101,6 +101,7 @@ role_update_safety_flag_n_(role_update_safety_flag_s_) { RTC_DCHECK(signaling_thread_); RTC_DCHECK(network_thread_); + RTC_DCHECK(port_allocator_); // The `transport_observer` is assumed to be non-null. RTC_DCHECK(config_.transport_observer); RTC_DCHECK(config_.rtcp_handler); @@ -370,11 +371,12 @@ return dtls->GetRemoteSSLCertChain(); } -void JsepTransportController::MaybeStartGathering() { +std::vector<IceParameters> JsepTransportController::MaybeStartGathering() { RTC_DCHECK_RUN_ON(signaling_thread_); - network_thread_->BlockingCall([&] { + return network_thread_->BlockingCall([&] { RTC_DCHECK_RUN_ON(network_thread_); MaybeStartGathering_n(); + return port_allocator_->GetPooledIceCredentials(); }); }
diff --git a/pc/jsep_transport_controller.h b/pc/jsep_transport_controller.h index 804dc60..0a334e7 100644 --- a/pc/jsep_transport_controller.h +++ b/pc/jsep_transport_controller.h
@@ -228,10 +228,10 @@ // Must be called on the signaling thread. bool NeedsIceRestart(absl::string_view mid) const; // Start gathering candidates for any new transports, or transports doing an - // ICE restart. + // ICE restart. Returns the pooled ICE credentials from the port allocator. // // Must be called on the signaling thread. - void MaybeStartGathering(); + std::vector<IceParameters> MaybeStartGathering(); RTCError AddRemoteCandidates(absl::string_view mid, const std::vector<Candidate>& candidates); // Must be called on the signaling thread.
diff --git a/pc/jsep_transport_controller_unittest.cc b/pc/jsep_transport_controller_unittest.cc index 136d2e4..4e90705 100644 --- a/pc/jsep_transport_controller_unittest.cc +++ b/pc/jsep_transport_controller_unittest.cc
@@ -45,6 +45,7 @@ #include "p2p/dtls/dtls_transport_internal.h" #include "p2p/dtls/fake_dtls_transport.h" #include "p2p/test/fake_ice_transport.h" +#include "p2p/test/fake_port_allocator.h" #include "pc/dtls_transport.h" #include "pc/rtp_transport.h" #include "pc/rtp_transport_internal.h" @@ -122,10 +123,22 @@ fake_ice_transport_factory_ = std::make_unique<FakeIceTransportFactory>(); fake_dtls_transport_factory_ = std::make_unique<FakeDtlsTransportFactory>(); } + ~JsepTransportControllerTest() override { + if (network_thread_ != nullptr && port_allocator_) { + network_thread_->BlockingCall([&] { port_allocator_ = std::nullopt; }); + } + } void CreateJsepTransportController(JsepTransportController::Config config, Thread* network_thread = Thread::Current(), PortAllocator* port_allocator = nullptr) { + if (port_allocator == nullptr) { + if (!port_allocator_) { + port_allocator_.emplace(env_, network_thread->socketserver(), + network_thread); + } + port_allocator = &*port_allocator_; + } config.transport_observer = this; config.rtcp_handler = [](const CopyOnWriteBuffer& packet, int64_t packet_time_us) { @@ -381,6 +394,7 @@ std::unique_ptr<Thread> network_thread_; std::unique_ptr<FakeIceTransportFactory> fake_ice_transport_factory_; std::unique_ptr<FakeDtlsTransportFactory> fake_dtls_transport_factory_; + std::optional<FakePortAllocator> port_allocator_; Thread* const signaling_thread_ = nullptr; Thread* ice_signaled_on_thread_ = nullptr; // Used to verify the SignalRtpTransportChanged/SignalDtlsTransportChanged are
diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index af1d3bf..b094f34 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc
@@ -19,6 +19,7 @@ #include <optional> #include <set> #include <string> +#include <tuple> #include <type_traits> #include <utility> #include <vector> @@ -624,7 +625,8 @@ // Field trials specific to the peerconnection should be owned by the `env`, RTC_DCHECK(dependencies.trials == nullptr); - transport_controller_copy_ = + std::vector<IceParameters> pooled_credentials; + std::tie(transport_controller_copy_, pooled_credentials) = InitializeNetworkThread(stun_servers, turn_servers); if (call_ptr_) { @@ -641,6 +643,7 @@ env_, this, configuration_, std::move(dependencies.cert_generator), std::move(dependencies.video_bitrate_allocator_factory), context_.get(), codec_lookup_helper_.get()); + sdp_handler_->UpdateCachedIceCredentials(std::move(pooled_credentials)); rtp_manager_ = std::make_unique<RtpTransmissionManager>( env_, call_ptr_, IsUnifiedPlan(), context_.get(), codec_lookup_helper_.get(), &usage_pattern_, observer_, @@ -717,7 +720,8 @@ data_channel_controller_.PrepareForShutdown(); } -JsepTransportController* PeerConnection::InitializeNetworkThread( +std::pair<JsepTransportController*, std::vector<IceParameters>> +PeerConnection::InitializeNetworkThread( const ServerAddresses& stun_servers, const std::vector<RelayServerConfig>& turn_servers) { RTC_DCHECK_RUN_ON(signaling_thread()); @@ -844,7 +848,11 @@ pa_result.enable_ipv6 ? kPeerConnection_IPv6 : kPeerConnection_IPv4; RTC_HISTOGRAM_ENUMERATION("WebRTC.PeerConnection.IPMetrics", address_family, kPeerConnectionAddressFamilyCounter_Max); - return InitializeTransportController_n(std::move(controller), *config); + JsepTransportController* controller_ptr = + InitializeTransportController_n(std::move(controller), *config); + std::vector<IceParameters> credentials = + port_allocator_->GetPooledIceCredentials(); + return std::make_pair(controller_ptr, credentials); }); } @@ -1673,30 +1681,32 @@ // Apply part of the configuration on the network thread. In theory this // shouldn't fail. - if (!network_thread()->BlockingCall( - [this, needs_ice_restart, &ice_config, &stun_servers, &turn_servers, - &modified_config, has_local_description] { - RTC_DCHECK_RUN_ON(network_thread()); - // As described in JSEP, calling setConfiguration with new ICE - // servers or candidate policy must set a "needs-ice-restart" bit so - // that the next offer triggers an ICE restart which will pick up - // the changes. - if (needs_ice_restart) - transport_controller_->SetNeedsIceRestartFlag(); + std::vector<IceParameters> pooled_credentials; + if (!network_thread()->BlockingCall([&] { + RTC_DCHECK_RUN_ON(network_thread()); + // As described in JSEP, calling setConfiguration with new ICE + // servers or candidate policy must set a "needs-ice-restart" bit so + // that the next offer triggers an ICE restart which will pick up + // the changes. + if (needs_ice_restart) + transport_controller_->SetNeedsIceRestartFlag(); - transport_controller_->SetIceConfig(ice_config); - return ReconfigurePortAllocator_n( - stun_servers, turn_servers, modified_config.type, - modified_config.ice_candidate_pool_size, - modified_config.GetTurnPortPrunePolicy(), - modified_config.turn_customizer, - modified_config.stun_candidate_keepalive_interval, - has_local_description); - })) { + transport_controller_->SetIceConfig(ice_config); + bool result = ReconfigurePortAllocator_n( + stun_servers, turn_servers, modified_config.type, + modified_config.ice_candidate_pool_size, + modified_config.GetTurnPortPrunePolicy(), + modified_config.turn_customizer, + modified_config.stun_candidate_keepalive_interval, + has_local_description); + pooled_credentials = port_allocator_->GetPooledIceCredentials(); + return result; + })) { return LOG_ERROR(RTCError(RTCErrorType::INTERNAL_ERROR) << "Failed to apply configuration to PortAllocator."); } + sdp_handler_->UpdateCachedIceCredentials(std::move(pooled_credentials)); configuration_ = modified_config; return RTCError::OK(); }
diff --git a/pc/peer_connection.h b/pc/peer_connection.h index 2b68531..e1d1353 100644 --- a/pc/peer_connection.h +++ b/pc/peer_connection.h
@@ -67,6 +67,7 @@ #include "p2p/base/ice_transport_internal.h" #include "p2p/base/port.h" #include "p2p/base/port_allocator.h" +#include "p2p/base/transport_description.h" #include "p2p/dtls/dtls_transport_factory.h" #include "pc/channel_interface.h" #include "pc/codec_vendor.h" @@ -488,9 +489,9 @@ // network thread and initialize network thread related state (see // InitializeTransportController_n). The return value of this function is used // to set the initial value of `transport_controller_copy_`. - JsepTransportController* InitializeNetworkThread( - const ServerAddresses& stun_servers, - const std::vector<RelayServerConfig>& turn_servers); + std::pair<JsepTransportController*, std::vector<IceParameters>> + InitializeNetworkThread(const ServerAddresses& stun_servers, + const std::vector<RelayServerConfig>& turn_servers); ScopedOperationsBatcher::BatchTaskWithFinalizer MakeCloseOnNetworkThreadTask(); JsepTransportController* InitializeTransportController_n(
diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc index dd66408..8ea2195 100644 --- a/pc/sdp_offer_answer.cc +++ b/pc/sdp_offer_answer.cc
@@ -1692,6 +1692,12 @@ weak_ptr_factory_.InvalidateWeakPtrs(); } +void SdpOfferAnswerHandler::UpdateCachedIceCredentials( + std::vector<IceParameters> credentials) { + RTC_DCHECK_RUN_ON(signaling_thread()); + cached_pooled_ice_credentials_ = std::move(credentials); +} + void SdpOfferAnswerHandler::Close() { RTC_LOG_THREAD_BLOCK_COUNT(); ChangeSignalingState(PeerConnectionInterface::kClosed); @@ -2774,7 +2780,8 @@ // MaybeStartGathering needs to be called after informing the observer so // that we don't signal any candidates before signaling that // SetLocalDescription completed. - transport_controller_s()->MaybeStartGathering(); + cached_pooled_ice_credentials_ = + transport_controller_s()->MaybeStartGathering(); } void SdpOfferAnswerHandler::DoCreateOffer( @@ -4512,9 +4519,7 @@ session_options->rtcp_cname = rtcp_cname_; session_options->crypto_options = pc_->GetCryptoOptions(); - session_options->pooled_ice_credentials = - context_->network_thread()->BlockingCall( - [this] { return port_allocator()->GetPooledIceCredentials(); }); + session_options->pooled_ice_credentials = cached_pooled_ice_credentials_; session_options->offer_extmap_allow_mixed = pc_->configuration()->offer_extmap_allow_mixed; @@ -4807,9 +4812,7 @@ session_options->rtcp_cname = rtcp_cname_; session_options->crypto_options = pc_->GetCryptoOptions(); - session_options->pooled_ice_credentials = - context_->network_thread()->BlockingCall( - [this] { return port_allocator()->GetPooledIceCredentials(); }); + session_options->pooled_ice_credentials = cached_pooled_ice_credentials_; // draft-hancke-tsvwg-snap. session_options->use_sctp_snap = pc_->trials().IsEnabled("WebRTC-Sctp-Snap"); }
diff --git a/pc/sdp_offer_answer.h b/pc/sdp_offer_answer.h index 0b3997b..683acd0 100644 --- a/pc/sdp_offer_answer.h +++ b/pc/sdp_offer_answer.h
@@ -42,6 +42,7 @@ #include "media/base/media_engine.h" #include "media/base/stream_params.h" #include "p2p/base/port_allocator.h" +#include "p2p/base/transport_description.h" #include "pc/codec_vendor.h" #include "pc/connection_context.h" #include "pc/data_channel_controller.h" @@ -166,6 +167,7 @@ PeerConnectionInterface::RTCConfiguration GetConfiguration(); RTCError SetConfiguration( const PeerConnectionInterface::RTCConfiguration& configuration); + void UpdateCachedIceCredentials(std::vector<IceParameters> credentials); bool AddIceCandidate(const IceCandidate* candidate); void AddIceCandidate(std::unique_ptr<IceCandidate> candidate, std::function<void(RTCError)> callback); @@ -705,6 +707,8 @@ // Member variables for caching global options. AudioOptions audio_options_ RTC_GUARDED_BY(signaling_thread()); VideoOptions video_options_ RTC_GUARDED_BY(signaling_thread()); + std::vector<IceParameters> cached_pooled_ice_credentials_ + RTC_GUARDED_BY(signaling_thread()); // A video bitrate allocator factory. // This can be injected using the PeerConnectionDependencies,
diff --git a/pc/test/fake_peer_connection_for_stats.h b/pc/test/fake_peer_connection_for_stats.h index 6dcfbfb..96f1dfb 100644 --- a/pc/test/fake_peer_connection_for_stats.h +++ b/pc/test/fake_peer_connection_for_stats.h
@@ -49,6 +49,7 @@ #include "p2p/base/transport_description.h" #include "p2p/base/transport_info.h" #include "p2p/test/fake_ice_transport.h" +#include "p2p/test/fake_port_allocator.h" #include "pc/channel.h" #include "pc/channel_interface.h" #include "pc/connection_context.h" @@ -361,8 +362,10 @@ int64_t packet_time_us) {}; config.un_demuxable_packet_handler = [](const RtpPacketReceived& parsed_packet) {}; + port_allocator_.emplace(env, network_thread_->socketserver(), + network_thread_); transport_controller_ = std::make_unique<JsepTransportController>( - env, signaling_thread_, network_thread_, /*port_allocator=*/nullptr, + env, signaling_thread_, network_thread_, &(*port_allocator_), /*async_dns_resolver_factory=*/nullptr, /*lna_permission_factory=*/nullptr, std::move(config)); } @@ -371,7 +374,10 @@ for (auto transceiver : transceivers_) { transceiver->internal()->ClearChannel(); } - network_thread_->BlockingCall([&]() { transport_controller_.reset(); }); + network_thread_->BlockingCall([&]() { + transport_controller_.reset(); + port_allocator_ = std::nullopt; + }); } static PeerConnectionFactoryDependencies MakeDependencies( @@ -795,6 +801,7 @@ PayloadTypePicker payload_type_picker_; FakeCodecLookupHelper codec_lookup_helper_; std::unique_ptr<IceTransportFactory> ice_transport_factory_; + std::optional<FakePortAllocator> port_allocator_; std::unique_ptr<JsepTransportController> transport_controller_; std::map<std::string, std::string> transport_names_by_mid_; };