admin/webrtc_m130
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Simplify PeerConnection::SetConfiguration

Browse Source 
* 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 <hta@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#40984}
This commit is contained in:
Tommi 2023-10-22 13:00:14 +02:00 committed by WebRTC LUCI CQ
parent 90db6ddbaf
commit aea49c953c
5 changed files with 148 additions and 192 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

21
p2p/base/port_allocator.cc
View File

@ -141,6 +141,8 @@ bool PortAllocator::SetConfiguration(
webrtc::PortPrunePolicy turn_port_prune_policy,
webrtc::TurnCustomizer* turn_customizer,
const absl::optional<int>& stun_candidate_keepalive_interval) {
RTC_DCHECK_GE(candidate_pool_size, 0);
RTC_DCHECK_LE(candidate_pool_size, static_cast<int>(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();

10
p2p/base/port_allocator.h
View File

@ -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<RelayServerConfig> turn_servers_;
int candidate_pool_size_ = 0; // Last value passed into SetConfiguration.
std::vector<std::unique_ptr<PortAllocatorSession>> pooled_sessions_;
bool candidate_pool_frozen_ = false;
webrtc::PortPrunePolicy turn_port_prune_policy_ = webrtc::NO_PRUNE;
// Customizer for TURN messages.

32
p2p/base/port_allocator_unittest.cc
View File

@ -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<cricket::RelayServerConfig> 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<cricket::RelayServerConfig> 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();

5
pc/jsep_transport_controller.cc
View File

@ -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: "

272
pc/peer_connection.cc
View File

@ -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<int> 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<int>(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<PeerConnectionInterface::RTCConfiguration> 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<cricket::RelayServerConfig>& 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<cricket::RelayServerConfig> 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<int>(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<int>(UINT16_MAX)) {
return RTCError(RTCErrorType::INVALID_RANGE);
// Create a new, configuration object whose Ice config will have been
// validated..
RTCErrorOr<RTCConfiguration> 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<cricket::RelayServerConfig> 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<cricket::VideoMediaSendChannelInterface*> 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) {