Reland "Let port allocator create ice tie breaker"
This is a reland of commit 3f3f991c03bb4073a06da37c822daaa9deed9307
Original change's description:
> Let port allocator create ice tie breaker
>
> Moves the responsibility for creating the ICE tie breaker from the JSEP transport controller to the port allocator. This will allow a future change to separate the ICE tie breaker (which is sent over the network and hence known to the peer) from the "port allocator random" (that is used to seed the ICE candidate foundation crc32 checksum) as an implementation detail.
>
> BUG=webrtc:14626
>
> Change-Id: I3a9a0980238d6108b1b154f45de2975b08793b1c
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/281660
> Reviewed-by: Harald Alvestrand <hta@webrtc.org>
> Commit-Queue: Philipp Hancke <phancke@microsoft.com>
> Cr-Commit-Position: refs/heads/main@{#41707}
Bug: webrtc:14626
Change-Id: Id3c8f257c5611958551bd66d7ce7a885bf8ba2f9
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/339320
Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Philipp Hancke <phancke@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#41720}
diff --git a/p2p/base/fake_port_allocator.h b/p2p/base/fake_port_allocator.h
index 0bb7dd0..a51a7ca 100644
--- a/p2p/base/fake_port_allocator.h
+++ b/p2p/base/fake_port_allocator.h
@@ -93,6 +93,7 @@
ice_ufrag,
ice_pwd,
allocator->flags()),
+ allocator_(allocator),
network_thread_(network_thread),
factory_(factory),
ipv4_network_("network",
@@ -110,7 +111,6 @@
field_trials_(field_trials) {
ipv4_network_.AddIP(rtc::IPAddress(INADDR_LOOPBACK));
ipv6_network_.AddIP(rtc::IPAddress(in6addr_loopback));
- set_ice_tiebreaker(/*kTiebreakerDefault = */ 44444);
}
void SetCandidateFilter(uint32_t filter) override {
@@ -127,7 +127,7 @@
username(), password(), false,
field_trials_));
RTC_DCHECK(port_);
- port_->SetIceTiebreaker(ice_tiebreaker());
+ port_->SetIceTiebreaker(allocator_->ice_tiebreaker());
port_->SubscribePortDestroyed(
[this](PortInterface* port) { OnPortDestroyed(port); });
AddPort(port_.get());
@@ -199,6 +199,7 @@
port_.release();
}
+ PortAllocator* allocator_;
rtc::Thread* network_thread_;
rtc::PacketSocketFactory* factory_;
rtc::Network ipv4_network_;
diff --git a/p2p/base/p2p_transport_channel.cc b/p2p/base/p2p_transport_channel.cc
index 0bccb67..2f18f1d 100644
--- a/p2p/base/p2p_transport_channel.cc
+++ b/p2p/base/p2p_transport_channel.cc
@@ -164,7 +164,7 @@
error_(0),
remote_ice_mode_(ICEMODE_FULL),
ice_role_(ICEROLE_UNKNOWN),
- tiebreaker_(0),
+ ice_tiebreaker_(0),
gathering_state_(kIceGatheringNew),
weak_ping_interval_(GetWeakPingIntervalInFieldTrial(field_trials)),
config_(RECEIVING_TIMEOUT,
@@ -326,7 +326,7 @@
return;
}
- tiebreaker_ = tiebreaker;
+ ice_tiebreaker_ = tiebreaker;
}
IceTransportState P2PTransportChannel::GetState() const {
@@ -885,7 +885,6 @@
ice_parameters_.ufrag,
ice_parameters_.pwd);
if (pooled_session) {
- pooled_session->set_ice_tiebreaker(tiebreaker_);
AddAllocatorSession(std::move(pooled_session));
PortAllocatorSession* raw_pooled_session =
allocator_sessions_.back().get();
@@ -902,7 +901,6 @@
AddAllocatorSession(allocator_->CreateSession(
transport_name(), component(), ice_parameters_.ufrag,
ice_parameters_.pwd));
- allocator_sessions_.back()->set_ice_tiebreaker(tiebreaker_);
allocator_sessions_.back()->StartGettingPorts();
}
}
@@ -930,7 +928,7 @@
// if one is pending.
port->SetIceRole(ice_role_);
- port->SetIceTiebreaker(tiebreaker_);
+ port->SetIceTiebreaker(ice_tiebreaker_);
ports_.push_back(port);
port->SignalUnknownAddress.connect(this,
&P2PTransportChannel::OnUnknownAddress);
diff --git a/p2p/base/p2p_transport_channel.h b/p2p/base/p2p_transport_channel.h
index da7933f..f7472df 100644
--- a/p2p/base/p2p_transport_channel.h
+++ b/p2p/base/p2p_transport_channel.h
@@ -439,7 +439,7 @@
RTC_GUARDED_BY(network_thread_);
IceMode remote_ice_mode_ RTC_GUARDED_BY(network_thread_);
IceRole ice_role_ RTC_GUARDED_BY(network_thread_);
- uint64_t tiebreaker_ RTC_GUARDED_BY(network_thread_);
+ uint64_t ice_tiebreaker_ RTC_GUARDED_BY(network_thread_);
IceGatheringState gathering_state_ RTC_GUARDED_BY(network_thread_);
std::unique_ptr<webrtc::BasicRegatheringController> regathering_controller_
RTC_GUARDED_BY(network_thread_);
diff --git a/p2p/base/p2p_transport_channel_unittest.cc b/p2p/base/p2p_transport_channel_unittest.cc
index 5bcfee0..79e984c 100644
--- a/p2p/base/p2p_transport_channel_unittest.cc
+++ b/p2p/base/p2p_transport_channel_unittest.cc
@@ -141,7 +141,6 @@
const uint64_t kLowTiebreaker = 11111;
const uint64_t kHighTiebreaker = 22222;
-const uint64_t kTiebreakerDefault = 44444;
cricket::IceConfig CreateIceConfig(
int receiving_timeout,
@@ -299,10 +298,6 @@
&ep2_.network_manager_, socket_factory_.get(), stun_servers,
kTurnUdpIntAddr, rtc::SocketAddress()));
- ep1_.SetIceTiebreaker(kTiebreakerDefault);
- ep1_.allocator_->SetIceTiebreaker(kTiebreakerDefault);
- ep2_.SetIceTiebreaker(kTiebreakerDefault);
- ep2_.allocator_->SetIceTiebreaker(kTiebreakerDefault);
webrtc::metrics::Reset();
}
@@ -3347,7 +3342,6 @@
protected:
void PrepareChannel(P2PTransportChannel* ch) {
ch->SetIceRole(ICEROLE_CONTROLLING);
- ch->SetIceTiebreaker(kTiebreakerDefault);
ch->SetIceParameters(kIceParams[0]);
ch->SetRemoteIceParameters(kIceParams[1]);
ch->SignalNetworkRouteChanged.connect(
@@ -3715,7 +3709,6 @@
&field_trials_);
P2PTransportChannel ch("TestChannel", 1, &pa, &field_trials_);
ch.SetIceRole(ICEROLE_CONTROLLING);
- ch.SetIceTiebreaker(kTiebreakerDefault);
ch.SetIceParameters(kIceParams[0]);
ch.MaybeStartGathering();
EXPECT_EQ_WAIT(IceGatheringState::kIceGatheringComplete, ch.gathering_state(),
diff --git a/p2p/base/port_allocator.cc b/p2p/base/port_allocator.cc
index 3745717..52fc8c1 100644
--- a/p2p/base/port_allocator.cc
+++ b/p2p/base/port_allocator.cc
@@ -68,8 +68,7 @@
content_name_(content_name),
component_(component),
ice_ufrag_(ice_ufrag),
- ice_pwd_(ice_pwd),
- tiebreaker_(0) {
+ ice_pwd_(ice_pwd) {
// Pooled sessions are allowed to be created with empty content name,
// component, ufrag and password.
RTC_DCHECK(ice_ufrag.empty() == ice_pwd.empty());
@@ -101,7 +100,7 @@
step_delay_(kDefaultStepDelay),
allow_tcp_listen_(true),
candidate_filter_(CF_ALL),
- tiebreaker_(0) {
+ tiebreaker_(rtc::CreateRandomId64()) {
// The allocator will be attached to a thread in Initialize.
thread_checker_.Detach();
}
@@ -189,7 +188,6 @@
PortAllocatorSession* pooled_session =
CreateSessionInternal("", 0, iceCredentials.ufrag, iceCredentials.pwd);
pooled_session->set_pooled(true);
- pooled_session->set_ice_tiebreaker(tiebreaker_);
pooled_session->StartGettingPorts();
pooled_sessions_.push_back(
std::unique_ptr<PortAllocatorSession>(pooled_session));
@@ -197,13 +195,6 @@
return true;
}
-void PortAllocator::SetIceTiebreaker(uint64_t tiebreaker) {
- tiebreaker_ = tiebreaker;
- for (auto& pooled_session : pooled_sessions_) {
- pooled_session->set_ice_tiebreaker(tiebreaker_);
- }
-}
-
std::unique_ptr<PortAllocatorSession> PortAllocator::CreateSession(
absl::string_view content_name,
int component,
@@ -213,7 +204,6 @@
auto session = std::unique_ptr<PortAllocatorSession>(
CreateSessionInternal(content_name, component, ice_ufrag, ice_pwd));
session->SetCandidateFilter(candidate_filter());
- session->set_ice_tiebreaker(tiebreaker_);
return session;
}
diff --git a/p2p/base/port_allocator.h b/p2p/base/port_allocator.h
index b8cffca..63ecfc6 100644
--- a/p2p/base/port_allocator.h
+++ b/p2p/base/port_allocator.h
@@ -212,10 +212,6 @@
const std::string& ice_pwd() const { return ice_pwd_; }
bool pooled() const { return pooled_; }
- // TODO(bugs.webrtc.org/14605): move this to the constructor
- void set_ice_tiebreaker(uint64_t tiebreaker) { tiebreaker_ = tiebreaker; }
- uint64_t ice_tiebreaker() const { return tiebreaker_; }
-
// Setting this filter should affect not only candidates gathered in the
// future, but candidates already gathered and ports already "ready",
// which would be returned by ReadyCandidates() and ReadyPorts().
@@ -332,9 +328,6 @@
bool pooled_ = false;
- // TODO(bugs.webrtc.org/14605): move this to the constructor
- uint64_t tiebreaker_;
-
// SetIceParameters is an implementation detail which only PortAllocator
// should be able to call.
friend class PortAllocator;
@@ -387,9 +380,6 @@
const absl::optional<int>&
stun_candidate_keepalive_interval = absl::nullopt);
- void SetIceTiebreaker(uint64_t tiebreaker);
- uint64_t IceTiebreaker() const { return tiebreaker_; }
-
const ServerAddresses& stun_servers() const {
CheckRunOnValidThreadIfInitialized();
return stun_servers_;
@@ -461,6 +451,8 @@
// 3. mDNS concealment of private IPs is enabled.
Candidate SanitizeCandidate(const Candidate& c) const;
+ uint64_t ice_tiebreaker() const { return tiebreaker_; }
+
uint32_t flags() const {
CheckRunOnValidThreadIfInitialized();
return flags_;
diff --git a/p2p/base/port_allocator_unittest.cc b/p2p/base/port_allocator_unittest.cc
index 836a2fa..2df8662 100644
--- a/p2p/base/port_allocator_unittest.cc
+++ b/p2p/base/port_allocator_unittest.cc
@@ -26,7 +26,6 @@
static const char kIcePwd[] = "TESTICEPWD00000000000000";
static const char kTurnUsername[] = "test";
static const char kTurnPassword[] = "test";
-constexpr uint64_t kTiebreakerDefault = 44444;
class PortAllocatorTest : public ::testing::Test, public sigslot::has_slots<> {
public:
@@ -38,9 +37,7 @@
allocator_(std::make_unique<cricket::FakePortAllocator>(
rtc::Thread::Current(),
packet_socket_factory_.get(),
- &field_trials_)) {
- allocator_->SetIceTiebreaker(kTiebreakerDefault);
- }
+ &field_trials_)) {}
protected:
void SetConfigurationWithPoolSize(int candidate_pool_size) {
diff --git a/p2p/base/regathering_controller_unittest.cc b/p2p/base/regathering_controller_unittest.cc
index 91b7270..573c0fd 100644
--- a/p2p/base/regathering_controller_unittest.cc
+++ b/p2p/base/regathering_controller_unittest.cc
@@ -40,7 +40,6 @@
const cricket::RelayCredentials kRelayCredentials("test", "test");
const char kIceUfrag[] = "UF00";
const char kIcePwd[] = "TESTICEPWD00000000000000";
-constexpr uint64_t kTiebreakerDefault = 44444;
} // namespace
@@ -59,7 +58,6 @@
rtc::Thread::Current(),
packet_socket_factory_.get(),
&field_trials_)) {
- allocator_->SetIceTiebreaker(kTiebreakerDefault);
BasicRegatheringController::Config regathering_config;
regathering_config.regather_on_failed_networks_interval = 0;
regathering_controller_.reset(new BasicRegatheringController(
diff --git a/p2p/client/basic_port_allocator.cc b/p2p/client/basic_port_allocator.cc
index e95033e..cc38a66 100644
--- a/p2p/client/basic_port_allocator.cc
+++ b/p2p/client/basic_port_allocator.cc
@@ -1483,7 +1483,7 @@
}
if (port) {
- port->SetIceTiebreaker(session_->ice_tiebreaker());
+ port->SetIceTiebreaker(session_->allocator()->ice_tiebreaker());
// If shared socket is enabled, STUN candidate will be allocated by the
// UDPPort.
if (IsFlagSet(PORTALLOCATOR_ENABLE_SHARED_SOCKET)) {
@@ -1519,7 +1519,7 @@
session_->allocator()->allow_tcp_listen(),
session_->allocator()->field_trials());
if (port) {
- port->SetIceTiebreaker(session_->ice_tiebreaker());
+ port->SetIceTiebreaker(session_->allocator()->ice_tiebreaker());
session_->AddAllocatedPort(port.release(), this);
// Since TCPPort is not created using shared socket, `port` will not be
// added to the dequeue.
@@ -1549,7 +1549,7 @@
session_->allocator()->stun_candidate_keepalive_interval(),
session_->allocator()->field_trials());
if (port) {
- port->SetIceTiebreaker(session_->ice_tiebreaker());
+ port->SetIceTiebreaker(session_->allocator()->ice_tiebreaker());
session_->AddAllocatedPort(port.release(), this);
// Since StunPort is not created using shared socket, `port` will not be
// added to the dequeue.
@@ -1652,7 +1652,7 @@
}
}
RTC_DCHECK(port != NULL);
- port->SetIceTiebreaker(session_->ice_tiebreaker());
+ port->SetIceTiebreaker(session_->allocator()->ice_tiebreaker());
session_->AddAllocatedPort(port.release(), this);
}
}
diff --git a/p2p/client/basic_port_allocator_unittest.cc b/p2p/client/basic_port_allocator_unittest.cc
index 77443ee..f77040d 100644
--- a/p2p/client/basic_port_allocator_unittest.cc
+++ b/p2p/client/basic_port_allocator_unittest.cc
@@ -112,8 +112,6 @@
// Add some margin of error for slow bots.
static const int kStunTimeoutMs = cricket::STUN_TOTAL_TIMEOUT;
-constexpr uint64_t kTiebreakerDefault = 44444;
-
namespace {
void CheckStunKeepaliveIntervalOfAllReadyPorts(
@@ -176,7 +174,6 @@
&network_manager_, &socket_factory_, stun_servers, &field_trials_);
allocator_->Initialize();
allocator_->set_step_delay(kMinimumStepDelay);
- allocator_->SetIceTiebreaker(kTiebreakerDefault);
webrtc::metrics::Reset();
}
@@ -214,7 +211,6 @@
allocator_.reset(
new BasicPortAllocator(&network_manager_, &socket_factory_));
allocator_->Initialize();
- allocator_->SetIceTiebreaker(kTiebreakerDefault);
allocator_->set_step_delay(kMinimumStepDelay);
}
// Endpoint is behind a NAT, with STUN specified.
@@ -299,7 +295,6 @@
this, &BasicPortAllocatorTestBase::OnCandidatesRemoved);
session->SignalCandidatesAllocationDone.connect(
this, &BasicPortAllocatorTestBase::OnCandidatesAllocationDone);
- session->set_ice_tiebreaker(kTiebreakerDefault);
return session;
}
diff --git a/pc/jsep_transport_controller.cc b/pc/jsep_transport_controller.cc
index 9473d96..d5eb0c6 100644
--- a/pc/jsep_transport_controller.cc
+++ b/pc/jsep_transport_controller.cc
@@ -55,6 +55,7 @@
}),
config_(std::move(config)),
active_reset_srtp_params_(config.active_reset_srtp_params),
+ ice_tiebreaker_(port_allocator ? port_allocator->ice_tiebreaker() : 0),
bundles_(config.bundle_policy) {
// The `transport_observer` is assumed to be non-null.
RTC_DCHECK(config_.transport_observer);
@@ -62,9 +63,6 @@
RTC_DCHECK(config_.ice_transport_factory);
RTC_DCHECK(config_.on_dtls_handshake_error_);
RTC_DCHECK(config_.field_trials);
- if (port_allocator_) {
- port_allocator_->SetIceTiebreaker(ice_tiebreaker_);
- }
}
JsepTransportController::~JsepTransportController() {
diff --git a/pc/jsep_transport_controller.h b/pc/jsep_transport_controller.h
index 448844a..7f06c22 100644
--- a/pc/jsep_transport_controller.h
+++ b/pc/jsep_transport_controller.h
@@ -507,7 +507,7 @@
cricket::IceConfig ice_config_;
cricket::IceRole ice_role_ = cricket::ICEROLE_CONTROLLING;
- uint64_t ice_tiebreaker_ = rtc::CreateRandomId64();
+ uint64_t ice_tiebreaker_;
rtc::scoped_refptr<rtc::RTCCertificate> certificate_;
BundleManager bundles_;
diff --git a/pc/peer_connection_ice_unittest.cc b/pc/peer_connection_ice_unittest.cc
index 492e108..bd4848c 100644
--- a/pc/peer_connection_ice_unittest.cc
+++ b/pc/peer_connection_ice_unittest.cc
@@ -94,7 +94,6 @@
constexpr int kIceCandidatesTimeout = 10000;
constexpr int64_t kWaitTimeout = 10000;
-constexpr uint64_t kTiebreakerDefault = 44444;
class PeerConnectionWrapperForIceTest : public PeerConnectionWrapper {
public:
@@ -1448,7 +1447,6 @@
packet_socket_factory_.get(),
&field_trials_));
port_allocator_ = port_allocator.get();
- port_allocator_->SetIceTiebreaker(kTiebreakerDefault);
PeerConnectionDependencies pc_dependencies(&observer_);
pc_dependencies.allocator = std::move(port_allocator);
auto result = pc_factory_->CreatePeerConnectionOrError(
diff --git a/pc/peer_connection_interface_unittest.cc b/pc/peer_connection_interface_unittest.cc
index 08fb163..71c85d4 100644
--- a/pc/peer_connection_interface_unittest.cc
+++ b/pc/peer_connection_interface_unittest.cc
@@ -114,7 +114,6 @@
static const char kRecvonly[] = "recvonly";
static const char kSendrecv[] = "sendrecv";
-constexpr uint64_t kTiebreakerDefault = 44444;
// Reference SDP with a MediaStream with label "stream1" and audio track with
// id "audio_1" and a video track with id "video_1;
@@ -711,7 +710,6 @@
std::make_unique<rtc::BasicPacketSocketFactory>(vss_.get()),
&field_trials_));
port_allocator_ = port_allocator.get();
- port_allocator_->SetIceTiebreaker(kTiebreakerDefault);
// Create certificate generator unless DTLS constraint is explicitly set to
// false.
