From aea49c953c644ebd5fe8a75dee884b9c821c5257 Mon Sep 17 00:00:00 2001 From: Tommi Date: Sun, 22 Oct 2023 13:00:14 +0200 Subject: [PATCH] Simplify PeerConnection::SetConfiguration * Consolidate ice candidate pool size checks (was in 3 places) * Consolidate ICE server configuration parsing (was in 2 locations) * Remove separate blocking call in PC for SetActiveResetSrtpParams(). * Remove unnecessary blocking call inside SetActiveResetSrtpParams implementation. Bug: none Change-Id: I38c8964f82f91c77c1fd18c407aefaab1d0c7c0d Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/324303 Reviewed-by: Harald Alvestrand Commit-Queue: Tomas Gunnarsson Cr-Commit-Position: refs/heads/main@{#40984} --- p2p/base/port_allocator.cc | 21 +-- p2p/base/port_allocator.h | 10 - p2p/base/port_allocator_unittest.cc | 32 ---- pc/jsep_transport_controller.cc | 5 - pc/peer_connection.cc | 272 +++++++++++++++------------- 5 files changed, 148 insertions(+), 192 deletions(-) diff --git a/p2p/base/port_allocator.cc b/p2p/base/port_allocator.cc index d8ff04fe20..9292319b56 100644 --- a/p2p/base/port_allocator.cc +++ b/p2p/base/port_allocator.cc @@ -141,6 +141,8 @@ bool PortAllocator::SetConfiguration( webrtc::PortPrunePolicy turn_port_prune_policy, webrtc::TurnCustomizer* turn_customizer, const absl::optional& stun_candidate_keepalive_interval) { + RTC_DCHECK_GE(candidate_pool_size, 0); + RTC_DCHECK_LE(candidate_pool_size, static_cast(UINT16_MAX)); CheckRunOnValidThreadIfInitialized(); // A positive candidate pool size would lead to the creation of a pooled // allocator session and starting getting ports, which we should only do on @@ -152,20 +154,6 @@ bool PortAllocator::SetConfiguration( turn_servers_ = turn_servers; turn_port_prune_policy_ = turn_port_prune_policy; - if (candidate_pool_frozen_) { - if (candidate_pool_size != candidate_pool_size_) { - RTC_LOG(LS_ERROR) - << "Trying to change candidate pool size after pool was frozen."; - return false; - } - return true; - } - - if (candidate_pool_size < 0) { - RTC_LOG(LS_ERROR) << "Can't set negative pool size."; - return false; - } - candidate_pool_size_ = candidate_pool_size; // If ICE servers changed, throw away any existing pooled sessions and create @@ -286,11 +274,6 @@ PortAllocator::FindPooledSession(const IceParameters* ice_credentials) const { return pooled_sessions_.end(); } -void PortAllocator::FreezeCandidatePool() { - CheckRunOnValidThreadAndInitialized(); - candidate_pool_frozen_ = true; -} - void PortAllocator::DiscardCandidatePool() { CheckRunOnValidThreadIfInitialized(); pooled_sessions_.clear(); diff --git a/p2p/base/port_allocator.h b/p2p/base/port_allocator.h index 3ca63cbd41..11462f78f2 100644 --- a/p2p/base/port_allocator.h +++ b/p2p/base/port_allocator.h @@ -443,15 +443,6 @@ class RTC_EXPORT PortAllocator : public sigslot::has_slots<> { const PortAllocatorSession* GetPooledSession( const IceParameters* ice_credentials = nullptr) const; - // After FreezeCandidatePool is called, changing the candidate pool size will - // no longer be allowed, and changing ICE servers will not cause pooled - // sessions to be recreated. - // - // Expected to be called when SetLocalDescription is called on a - // PeerConnection. Can be called safely on any thread as long as not - // simultaneously with SetConfiguration. - void FreezeCandidatePool(); - // Discard any remaining pooled sessions. void DiscardCandidatePool(); @@ -655,7 +646,6 @@ class RTC_EXPORT PortAllocator : public sigslot::has_slots<> { std::vector turn_servers_; int candidate_pool_size_ = 0; // Last value passed into SetConfiguration. std::vector> pooled_sessions_; - bool candidate_pool_frozen_ = false; webrtc::PortPrunePolicy turn_port_prune_policy_ = webrtc::NO_PRUNE; // Customizer for TURN messages. diff --git a/p2p/base/port_allocator_unittest.cc b/p2p/base/port_allocator_unittest.cc index 48d0bc8a6e..836a2fa494 100644 --- a/p2p/base/port_allocator_unittest.cc +++ b/p2p/base/port_allocator_unittest.cc @@ -150,11 +150,6 @@ TEST_F(PortAllocatorTest, SetConfigurationUpdatesCandidatePoolSize) { EXPECT_EQ(4, allocator_->candidate_pool_size()); } -// A negative pool size should just be treated as zero. -TEST_F(PortAllocatorTest, SetConfigurationWithNegativePoolSizeFails) { - SetConfigurationWithPoolSizeExpectFailure(-1); -} - // Test that if the candidate pool size is nonzero, pooled sessions are // created, and StartGettingPorts is called on them. TEST_F(PortAllocatorTest, SetConfigurationCreatesPooledSessions) { @@ -213,33 +208,6 @@ TEST_F(PortAllocatorTest, EXPECT_EQ(0, GetAllPooledSessionsReturnCount()); } -// According to JSEP, after SetLocalDescription, setting different ICE servers -// will not cause the pool to be refilled. This is implemented by the -// PeerConnection calling FreezeCandidatePool when a local description is set. -TEST_F(PortAllocatorTest, - SetConfigurationDoesNotRecreatePooledSessionsAfterFreezeCandidatePool) { - cricket::ServerAddresses stun_servers_1 = {stun_server_1}; - std::vector turn_servers_1 = {turn_server_1}; - allocator_->SetConfiguration(stun_servers_1, turn_servers_1, 1, - webrtc::NO_PRUNE); - EXPECT_EQ(stun_servers_1, allocator_->stun_servers()); - EXPECT_EQ(turn_servers_1, allocator_->turn_servers()); - - // Update with a different set of servers, but first freeze the pool. - allocator_->FreezeCandidatePool(); - cricket::ServerAddresses stun_servers_2 = {stun_server_2}; - std::vector turn_servers_2 = {turn_server_2}; - allocator_->SetConfiguration(stun_servers_2, turn_servers_2, 2, - webrtc::NO_PRUNE); - EXPECT_EQ(stun_servers_2, allocator_->stun_servers()); - EXPECT_EQ(turn_servers_2, allocator_->turn_servers()); - auto session = TakePooledSession(); - ASSERT_NE(nullptr, session.get()); - EXPECT_EQ(stun_servers_1, session->stun_servers()); - EXPECT_EQ(turn_servers_1, session->turn_servers()); - EXPECT_EQ(0, GetAllPooledSessionsReturnCount()); -} - TEST_F(PortAllocatorTest, GetPooledSessionReturnsNextSession) { SetConfigurationWithPoolSize(2); auto peeked_session_1 = GetPooledSession(); diff --git a/pc/jsep_transport_controller.cc b/pc/jsep_transport_controller.cc index fdf6598eea..7c669a5ae3 100644 --- a/pc/jsep_transport_controller.cc +++ b/pc/jsep_transport_controller.cc @@ -360,11 +360,6 @@ bool JsepTransportController::GetStats(const std::string& transport_name, void JsepTransportController::SetActiveResetSrtpParams( bool active_reset_srtp_params) { - if (!network_thread_->IsCurrent()) { - network_thread_->BlockingCall( - [=] { SetActiveResetSrtpParams(active_reset_srtp_params); }); - return; - } RTC_DCHECK_RUN_ON(network_thread_); RTC_LOG(LS_INFO) << "Updating the active_reset_srtp_params for JsepTransportController: " diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index a8d1d70362..20ed8db445 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -264,6 +264,87 @@ RTCError ValidateConfiguration( ParseIceConfig(config)); } +// Checks for valid pool size range and if a previous value has already been +// set, which is done via SetLocalDescription. +RTCError ValidateIceCandidatePoolSize( + int ice_candidate_pool_size, + absl::optional previous_ice_candidate_pool_size) { + // Note that this isn't possible through chromium, since it's an unsigned + // short in WebIDL. + if (ice_candidate_pool_size < 0 || + ice_candidate_pool_size > static_cast(UINT16_MAX)) { + return RTCError(RTCErrorType::INVALID_RANGE); + } + + // According to JSEP, after setLocalDescription, changing the candidate pool + // size is not allowed, and changing the set of ICE servers will not result + // in new candidates being gathered. + if (previous_ice_candidate_pool_size.has_value() && + ice_candidate_pool_size != previous_ice_candidate_pool_size.value()) { + LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_MODIFICATION, + "Can't change candidate pool size after calling " + "SetLocalDescription."); + } + + return RTCError::OK(); +} + +// The simplest (and most future-compatible) way to tell if a config was +// modified in an invalid way is to copy each property we do support modifying, +// then use operator==. There are far more properties we don't support modifying +// than those we do, and more could be added. +// This helper function accepts a proposed new `configuration` object, an +// existing configuration and returns a valid, modified, configuration that's +// based on the existing configuration, with modified properties copied from +// `configuration`. +// If the result of creating a modified configuration doesn't pass the above +// `operator==` test or a call to `ValidateConfiguration()`, then the function +// will return an error. Otherwise, the return value will be the new config. +RTCErrorOr ApplyConfiguration( + const PeerConnectionInterface::RTCConfiguration& configuration, + const PeerConnectionInterface::RTCConfiguration& existing_configuration) { + PeerConnectionInterface::RTCConfiguration modified_config = + existing_configuration; + modified_config.servers = configuration.servers; + modified_config.type = configuration.type; + modified_config.ice_candidate_pool_size = + configuration.ice_candidate_pool_size; + modified_config.prune_turn_ports = configuration.prune_turn_ports; + modified_config.turn_port_prune_policy = configuration.turn_port_prune_policy; + modified_config.surface_ice_candidates_on_ice_transport_type_changed = + configuration.surface_ice_candidates_on_ice_transport_type_changed; + modified_config.ice_check_min_interval = configuration.ice_check_min_interval; + modified_config.ice_check_interval_strong_connectivity = + configuration.ice_check_interval_strong_connectivity; + modified_config.ice_check_interval_weak_connectivity = + configuration.ice_check_interval_weak_connectivity; + modified_config.ice_unwritable_timeout = configuration.ice_unwritable_timeout; + modified_config.ice_unwritable_min_checks = + configuration.ice_unwritable_min_checks; + modified_config.ice_inactive_timeout = configuration.ice_inactive_timeout; + modified_config.stun_candidate_keepalive_interval = + configuration.stun_candidate_keepalive_interval; + modified_config.turn_customizer = configuration.turn_customizer; + modified_config.network_preference = configuration.network_preference; + modified_config.active_reset_srtp_params = + configuration.active_reset_srtp_params; + modified_config.turn_logging_id = configuration.turn_logging_id; + modified_config.allow_codec_switching = configuration.allow_codec_switching; + modified_config.stable_writable_connection_ping_interval_ms = + configuration.stable_writable_connection_ping_interval_ms; + if (configuration != modified_config) { + LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_MODIFICATION, + "Modifying the configuration in an unsupported way."); + } + + RTCError err = ValidateConfiguration(modified_config); + if (!err.ok()) { + return err; + } + + return modified_config; +} + bool HasRtcpMuxEnabled(const cricket::ContentInfo* content) { return content->media_description()->rtcp_mux(); } @@ -286,6 +367,46 @@ bool DtlsEnabled(const PeerConnectionInterface::RTCConfiguration& configuration, #endif } +// Calls `ParseIceServersOrError` to extract ice server information from the +// `configuration` and then validates the extracted configuration. For a +// non-empty list of servers, usage gets recorded via `usage_pattern`. +RTCError ParseAndValidateIceServersFromConfiguration( + const PeerConnectionInterface::RTCConfiguration& configuration, + cricket::ServerAddresses& stun_servers, + std::vector& turn_servers, + UsagePattern& usage_pattern) { + RTC_DCHECK(stun_servers.empty()); + RTC_DCHECK(turn_servers.empty()); + RTCError err = ParseIceServersOrError(configuration.servers, &stun_servers, + &turn_servers); + if (!err.ok()) { + return err; + } + + // Restrict number of TURN servers. + if (turn_servers.size() > cricket::kMaxTurnServers) { + RTC_LOG(LS_WARNING) << "Number of configured TURN servers is " + << turn_servers.size() + << " which exceeds the maximum allowed number of " + << cricket::kMaxTurnServers; + turn_servers.resize(cricket::kMaxTurnServers); + } + + // Add the turn logging id to all turn servers + for (cricket::RelayServerConfig& turn_server : turn_servers) { + turn_server.turn_logging_id = configuration.turn_logging_id; + } + + // Note if STUN or TURN servers were supplied. + if (!stun_servers.empty()) { + usage_pattern.NoteUsageEvent(UsageEvent::STUN_SERVER_ADDED); + } + if (!turn_servers.empty()) { + usage_pattern.NoteUsageEvent(UsageEvent::TURN_SERVER_ADDED); + } + return RTCError::OK(); +} + } // namespace bool PeerConnectionInterface::RTCConfiguration::operator==( @@ -605,35 +726,12 @@ RTCError PeerConnection::Initialize( cricket::ServerAddresses stun_servers; std::vector turn_servers; - - RTCError parse_error = ParseIceServersOrError(configuration.servers, - &stun_servers, &turn_servers); + RTCError parse_error = ParseAndValidateIceServersFromConfiguration( + configuration, stun_servers, turn_servers, usage_pattern_); if (!parse_error.ok()) { return parse_error; } - // Restrict number of TURN servers. - if (turn_servers.size() > cricket::kMaxTurnServers) { - RTC_LOG(LS_WARNING) << "Number of configured TURN servers is " - << turn_servers.size() - << " which exceeds the maximum allowed number of " - << cricket::kMaxTurnServers; - turn_servers.resize(cricket::kMaxTurnServers); - } - - // Add the turn logging id to all turn servers - for (cricket::RelayServerConfig& turn_server : turn_servers) { - turn_server.turn_logging_id = configuration.turn_logging_id; - } - - // Note if STUN or TURN servers were supplied. - if (!stun_servers.empty()) { - NoteUsageEvent(UsageEvent::STUN_SERVER_ADDED); - } - if (!turn_servers.empty()) { - NoteUsageEvent(UsageEvent::TURN_SERVER_ADDED); - } - // Network thread initialization. transport_controller_copy_ = network_thread()->BlockingCall([&] { RTC_DCHECK_RUN_ON(network_thread()); @@ -1505,106 +1603,42 @@ RTCError PeerConnection::SetConfiguration( "SetConfiguration: PeerConnection is closed."); } - // According to JSEP, after setLocalDescription, changing the candidate pool - // size is not allowed, and changing the set of ICE servers will not result - // in new candidates being gathered. - if (local_description() && configuration.ice_candidate_pool_size != - configuration_.ice_candidate_pool_size) { - LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_MODIFICATION, - "Can't change candidate pool size after calling " - "SetLocalDescription."); + const bool has_local_description = local_description() != nullptr; + + RTCError validate_error = ValidateIceCandidatePoolSize( + configuration.ice_candidate_pool_size, + has_local_description + ? absl::optional(configuration_.ice_candidate_pool_size) + : absl::nullopt); + if (!validate_error.ok()) { + return validate_error; } - if (local_description() && + if (has_local_description && configuration.crypto_options != configuration_.crypto_options) { LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_MODIFICATION, "Can't change crypto_options after calling " "SetLocalDescription."); } - // The simplest (and most future-compatible) way to tell if the config was - // modified in an invalid way is to copy each property we do support - // modifying, then use operator==. There are far more properties we don't - // support modifying than those we do, and more could be added. - RTCConfiguration modified_config = configuration_; - modified_config.servers = configuration.servers; - modified_config.type = configuration.type; - modified_config.ice_candidate_pool_size = - configuration.ice_candidate_pool_size; - modified_config.prune_turn_ports = configuration.prune_turn_ports; - modified_config.turn_port_prune_policy = configuration.turn_port_prune_policy; - modified_config.surface_ice_candidates_on_ice_transport_type_changed = - configuration.surface_ice_candidates_on_ice_transport_type_changed; - modified_config.ice_check_min_interval = configuration.ice_check_min_interval; - modified_config.ice_check_interval_strong_connectivity = - configuration.ice_check_interval_strong_connectivity; - modified_config.ice_check_interval_weak_connectivity = - configuration.ice_check_interval_weak_connectivity; - modified_config.ice_unwritable_timeout = configuration.ice_unwritable_timeout; - modified_config.ice_unwritable_min_checks = - configuration.ice_unwritable_min_checks; - modified_config.ice_inactive_timeout = configuration.ice_inactive_timeout; - modified_config.stun_candidate_keepalive_interval = - configuration.stun_candidate_keepalive_interval; - modified_config.turn_customizer = configuration.turn_customizer; - modified_config.network_preference = configuration.network_preference; - modified_config.active_reset_srtp_params = - configuration.active_reset_srtp_params; - modified_config.turn_logging_id = configuration.turn_logging_id; - modified_config.allow_codec_switching = configuration.allow_codec_switching; - modified_config.stable_writable_connection_ping_interval_ms = - configuration.stable_writable_connection_ping_interval_ms; - if (configuration != modified_config) { - LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_MODIFICATION, - "Modifying the configuration in an unsupported way."); - } - - // Validate the modified configuration. - RTCError validate_error = ValidateConfiguration(modified_config); - if (!validate_error.ok()) { - return validate_error; - } - - // Note that this isn't possible through chromium, since it's an unsigned - // short in WebIDL. - if (configuration.ice_candidate_pool_size < 0 || - configuration.ice_candidate_pool_size > static_cast(UINT16_MAX)) { - return RTCError(RTCErrorType::INVALID_RANGE); + // Create a new, configuration object whose Ice config will have been + // validated.. + RTCErrorOr validated_config = + ApplyConfiguration(configuration, configuration_); + if (!validated_config.ok()) { + return validated_config.error(); } // Parse ICE servers before hopping to network thread. cricket::ServerAddresses stun_servers; std::vector turn_servers; - RTCError parse_error = ParseIceServersOrError(configuration.servers, - &stun_servers, &turn_servers); - if (!parse_error.ok()) { - return parse_error; + validate_error = ParseAndValidateIceServersFromConfiguration( + configuration, stun_servers, turn_servers, usage_pattern_); + if (!validate_error.ok()) { + return validate_error; } - // Restrict number of TURN servers. - if (turn_servers.size() > cricket::kMaxTurnServers) { - RTC_LOG(LS_WARNING) << "Number of configured TURN servers is " - << turn_servers.size() - << " which exceeds the maximum allowed number of " - << cricket::kMaxTurnServers; - turn_servers.resize(cricket::kMaxTurnServers); - } - - // Add the turn logging id to all turn servers - for (cricket::RelayServerConfig& turn_server : turn_servers) { - turn_server.turn_logging_id = configuration.turn_logging_id; - } - - // Note if STUN or TURN servers were supplied. - if (!stun_servers.empty()) { - NoteUsageEvent(UsageEvent::STUN_SERVER_ADDED); - } - if (!turn_servers.empty()) { - NoteUsageEvent(UsageEvent::TURN_SERVER_ADDED); - } - - const bool has_local_description = local_description() != nullptr; - + const RTCConfiguration& modified_config = validated_config.value(); const bool needs_ice_restart = modified_config.servers != configuration_.servers || NeedIceRestart( @@ -1628,6 +1662,8 @@ RTCError PeerConnection::SetConfiguration( transport_controller_->SetNeedsIceRestartFlag(); transport_controller_->SetIceConfig(ice_config); + transport_controller_->SetActiveResetSrtpParams( + modified_config.active_reset_srtp_params); return ReconfigurePortAllocator_n( stun_servers, turn_servers, modified_config.type, modified_config.ice_candidate_pool_size, @@ -1640,16 +1676,6 @@ RTCError PeerConnection::SetConfiguration( "Failed to apply configuration to PortAllocator."); } - if (configuration_.active_reset_srtp_params != - modified_config.active_reset_srtp_params) { - // TODO(tommi): merge BlockingCalls - network_thread()->BlockingCall([this, &modified_config] { - RTC_DCHECK_RUN_ON(network_thread()); - transport_controller_->SetActiveResetSrtpParams( - modified_config.active_reset_srtp_params); - }); - } - if (modified_config.allow_codec_switching.has_value()) { std::vector channels; for (const auto& transceiver : rtp_manager()->transceivers()->List()) { @@ -2230,12 +2256,6 @@ bool PeerConnection::ReconfigurePortAllocator_n( RTC_DCHECK_RUN_ON(network_thread()); port_allocator_->SetCandidateFilter( ConvertIceTransportTypeToCandidateFilter(type)); - // According to JSEP, after setLocalDescription, changing the candidate pool - // size is not allowed, and changing the set of ICE servers will not result - // in new candidates being gathered. - if (have_local_description) { - port_allocator_->FreezeCandidatePool(); - } // Add the custom tls turn servers if they exist. auto turn_servers_copy = turn_servers; for (auto& turn_server : turn_servers_copy) {