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

Revert "Let port allocator create ice tie breaker"

Browse Source 
This reverts commit 3f3f991c03bb4073a06da37c822daaa9deed9307.

Reason for revert: API breaking change on PortAllocatorSession.
Is it possible to duplicate the ctor of PortAllocatorSession and remove
the deprecated one (the one without ice_tiebreaker) in another CL?

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: I342c9a96ac1909244aedea6a7779f5682088a5fc
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/339280
Bot-Commit: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Mirko Bonadei <mbonadei@webrtc.org>
Reviewed-by: Björn Terelius <terelius@webrtc.org>
Owners-Override: Mirko Bonadei <mbonadei@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#41715}
This commit is contained in:
Mirko Bonadei 2024-02-12 08:52:04 +00:00 committed by WebRTC LUCI CQ
parent 52fec7d3e9
commit 407367d053
15 changed files with 53 additions and 42 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
p2p/base/fake_port_allocator.h
View File

@ -92,7 +92,6 @@ class FakePortAllocatorSession : public PortAllocatorSession {
component,
ice_ufrag,
ice_pwd,
/*kTiebreakerDefault = */ 44444,
allocator->flags()),
network_thread_(network_thread),
factory_(factory),
@ -111,6 +110,7 @@ class FakePortAllocatorSession : public PortAllocatorSession {
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 {

8
p2p/base/p2p_transport_channel.cc
View File

@ -164,7 +164,7 @@ P2PTransportChannel::P2PTransportChannel(
error_(0),
remote_ice_mode_(ICEMODE_FULL),
ice_role_(ICEROLE_UNKNOWN),
ice_tiebreaker_(0),
tiebreaker_(0),
gathering_state_(kIceGatheringNew),
weak_ping_interval_(GetWeakPingIntervalInFieldTrial(field_trials)),
config_(RECEIVING_TIMEOUT,
@ -326,7 +326,7 @@ void P2PTransportChannel::SetIceTiebreaker(uint64_t tiebreaker) {
return;
}
ice_tiebreaker_ = tiebreaker;
tiebreaker_ = tiebreaker;
}
IceTransportState P2PTransportChannel::GetState() const {
@ -885,6 +885,7 @@ void P2PTransportChannel::MaybeStartGathering() {
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();
@ -901,6 +902,7 @@ void P2PTransportChannel::MaybeStartGathering() {
AddAllocatorSession(allocator_->CreateSession(
transport_name(), component(), ice_parameters_.ufrag,
ice_parameters_.pwd));
allocator_sessions_.back()->set_ice_tiebreaker(tiebreaker_);
allocator_sessions_.back()->StartGettingPorts();
}
}
@ -928,7 +930,7 @@ void P2PTransportChannel::OnPortReady(PortAllocatorSession* session,
// if one is pending.
port->SetIceRole(ice_role_);
port->SetIceTiebreaker(ice_tiebreaker_);
port->SetIceTiebreaker(tiebreaker_);
ports_.push_back(port);
port->SignalUnknownAddress.connect(this,
&P2PTransportChannel::OnUnknownAddress);

2
p2p/base/p2p_transport_channel.h
View File

@ -439,7 +439,7 @@ class RTC_EXPORT P2PTransportChannel : public IceTransportInternal,
RTC_GUARDED_BY(network_thread_);
IceMode remote_ice_mode_ RTC_GUARDED_BY(network_thread_);
IceRole ice_role_ RTC_GUARDED_BY(network_thread_);
uint64_t ice_tiebreaker_ RTC_GUARDED_BY(network_thread_);
uint64_t 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_);

7
p2p/base/p2p_transport_channel_unittest.cc
View File

@ -141,6 +141,7 @@ const cricket::IceParameters kIceParams[4] = {
const uint64_t kLowTiebreaker = 11111;
const uint64_t kHighTiebreaker = 22222;
const uint64_t kTiebreakerDefault = 44444;
cricket::IceConfig CreateIceConfig(
int receiving_timeout,
@ -298,6 +299,10 @@ class P2PTransportChannelTestBase : public ::testing::Test,
&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();
}
@ -3342,6 +3347,7 @@ class P2PTransportChannelPingTest : public ::testing::Test,
protected:
void PrepareChannel(P2PTransportChannel* ch) {
ch->SetIceRole(ICEROLE_CONTROLLING);
ch->SetIceTiebreaker(kTiebreakerDefault);
ch->SetIceParameters(kIceParams[0]);
ch->SetRemoteIceParameters(kIceParams[1]);
ch->SignalNetworkRouteChanged.connect(
@ -3709,6 +3715,7 @@ TEST_F(P2PTransportChannelPingTest, PingingStartedAsSoonAsPossible) {
&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(),

12
p2p/base/port_allocator.cc
View File

@ -62,7 +62,6 @@ PortAllocatorSession::PortAllocatorSession(absl::string_view content_name,
int component,
absl::string_view ice_ufrag,
absl::string_view ice_pwd,
uint64_t ice_tiebreaker,
uint32_t flags)
: flags_(flags),
generation_(0),
@ -70,7 +69,7 @@ PortAllocatorSession::PortAllocatorSession(absl::string_view content_name,
component_(component),
ice_ufrag_(ice_ufrag),
ice_pwd_(ice_pwd),
ice_tiebreaker_(ice_tiebreaker) {
tiebreaker_(0) {
// Pooled sessions are allowed to be created with empty content name,
// component, ufrag and password.
RTC_DCHECK(ice_ufrag.empty() == ice_pwd.empty());
@ -102,7 +101,7 @@ PortAllocator::PortAllocator()
step_delay_(kDefaultStepDelay),
allow_tcp_listen_(true),
candidate_filter_(CF_ALL),
tiebreaker_(rtc::CreateRandomId64()) {
tiebreaker_(0) {
// The allocator will be attached to a thread in Initialize.
thread_checker_.Detach();
}
@ -198,6 +197,13 @@ bool PortAllocator::SetConfiguration(
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,

16
p2p/base/port_allocator.h
View File

@ -199,7 +199,6 @@ class RTC_EXPORT PortAllocatorSession : public sigslot::has_slots<> {
int component,
absl::string_view ice_ufrag,
absl::string_view ice_pwd,
uint64_t ice_tiebreaker,
uint32_t flags);
// Subclasses should clean up any ports created.
@ -213,9 +212,9 @@ class RTC_EXPORT PortAllocatorSession : public sigslot::has_slots<> {
const std::string& ice_pwd() const { return ice_pwd_; }
bool pooled() const { return pooled_; }
// TODO(bugs.webrtc.org/14605): remove once downstream code has been updated.
void set_ice_tiebreaker(uint64_t tiebreaker) { ice_tiebreaker_ = tiebreaker; }
uint64_t ice_tiebreaker() const { return ice_tiebreaker_; }
// 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",
@ -330,10 +329,12 @@ class RTC_EXPORT PortAllocatorSession : public sigslot::has_slots<> {
int component_;
std::string ice_ufrag_;
std::string ice_pwd_;
uint64_t ice_tiebreaker_;
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;
@ -386,6 +387,9 @@ class RTC_EXPORT PortAllocator : public sigslot::has_slots<> {
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_;
@ -457,8 +461,6 @@ class RTC_EXPORT PortAllocator : public sigslot::has_slots<> {
// 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_;

5
p2p/base/port_allocator_unittest.cc
View File

@ -26,6 +26,7 @@ static const char kIceUfrag[] = "UF00";
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:
@ -37,7 +38,9 @@ class PortAllocatorTest : public ::testing::Test, public sigslot::has_slots<> {
allocator_(std::make_unique<cricket::FakePortAllocator>(
rtc::Thread::Current(),
packet_socket_factory_.get(),
&field_trials_)) {}
&field_trials_)) {
allocator_->SetIceTiebreaker(kTiebreakerDefault);
}
protected:
void SetConfigurationWithPoolSize(int candidate_pool_size) {

2
p2p/base/regathering_controller_unittest.cc
View File

@ -40,6 +40,7 @@ const rtc::SocketAddress kTurnUdpIntAddr("99.99.99.3",
const cricket::RelayCredentials kRelayCredentials("test", "test");
const char kIceUfrag[] = "UF00";
const char kIcePwd[] = "TESTICEPWD00000000000000";
constexpr uint64_t kTiebreakerDefault = 44444;
} // namespace
@ -58,6 +59,7 @@ class RegatheringControllerTest : public ::testing::Test,
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(

19
p2p/client/basic_port_allocator.cc
View File

@ -243,7 +243,7 @@ PortAllocatorSession* BasicPortAllocator::CreateSessionInternal(
CheckRunOnValidThreadAndInitialized();
return new BasicPortAllocatorSession(this, std::string(content_name),
component, std::string(ice_ufrag),
std::string(ice_pwd), ice_tiebreaker());
std::string(ice_pwd));
}
void BasicPortAllocator::AddTurnServerForTesting(
@ -261,13 +261,11 @@ BasicPortAllocatorSession::BasicPortAllocatorSession(
absl::string_view content_name,
int component,
absl::string_view ice_ufrag,
absl::string_view ice_pwd,
uint64_t ice_tiebreaker)
absl::string_view ice_pwd)
: PortAllocatorSession(content_name,
component,
ice_ufrag,
ice_pwd,
ice_tiebreaker,
allocator->flags()),
allocator_(allocator),
network_thread_(rtc::Thread::Current()),
@ -283,19 +281,6 @@ BasicPortAllocatorSession::BasicPortAllocatorSession(
allocator_->network_manager()->StartUpdating();
}
BasicPortAllocatorSession::BasicPortAllocatorSession(
BasicPortAllocator* allocator,
absl::string_view content_name,
int component,
absl::string_view ice_ufrag,
absl::string_view ice_pwd)
: BasicPortAllocatorSession(allocator,
content_name,
component,
ice_ufrag,
ice_pwd,
0) {}
BasicPortAllocatorSession::~BasicPortAllocatorSession() {
TRACE_EVENT0("webrtc",
"BasicPortAllocatorSession::~BasicPortAllocatorSession");

7
p2p/client/basic_port_allocator.h
View File

@ -116,13 +116,6 @@ enum class SessionState {
// destroyed on the network thread.
class RTC_EXPORT BasicPortAllocatorSession : public PortAllocatorSession {
public:
BasicPortAllocatorSession(BasicPortAllocator* allocator,
absl::string_view content_name,
int component,
absl::string_view ice_ufrag,
absl::string_view ice_pwd,
uint64_t ice_tiebreaker);
// TODO(bugs.webrtc.org/14605): remove this constructor
BasicPortAllocatorSession(BasicPortAllocator* allocator,
absl::string_view content_name,
int component,

5
p2p/client/basic_port_allocator_unittest.cc
View File

@ -112,6 +112,8 @@ static const char kTurnPassword[] = "test";
// Add some margin of error for slow bots.
static const int kStunTimeoutMs = cricket::STUN_TOTAL_TIMEOUT;
constexpr uint64_t kTiebreakerDefault = 44444;
namespace {
void CheckStunKeepaliveIntervalOfAllReadyPorts(
@ -174,6 +176,7 @@ class BasicPortAllocatorTestBase : public ::testing::Test,
&network_manager_, &socket_factory_, stun_servers, &field_trials_);
allocator_->Initialize();
allocator_->set_step_delay(kMinimumStepDelay);
allocator_->SetIceTiebreaker(kTiebreakerDefault);
webrtc::metrics::Reset();
}
@ -211,6 +214,7 @@ class BasicPortAllocatorTestBase : public ::testing::Test,
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.
@ -295,6 +299,7 @@ class BasicPortAllocatorTestBase : public ::testing::Test,
this, &BasicPortAllocatorTestBase::OnCandidatesRemoved);
session->SignalCandidatesAllocationDone.connect(
this, &BasicPortAllocatorTestBase::OnCandidatesAllocationDone);
session->set_ice_tiebreaker(kTiebreakerDefault);
return session;
}

4
pc/jsep_transport_controller.cc
View File

@ -55,7 +55,6 @@ JsepTransportController::JsepTransportController(
}),
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);
@ -63,6 +62,9 @@ JsepTransportController::JsepTransportController(
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() {

2
pc/jsep_transport_controller.h
View File

@ -507,7 +507,7 @@ class JsepTransportController : public sigslot::has_slots<> {
cricket::IceConfig ice_config_;
cricket::IceRole ice_role_ = cricket::ICEROLE_CONTROLLING;
uint64_t ice_tiebreaker_;
uint64_t ice_tiebreaker_ = rtc::CreateRandomId64();
rtc::scoped_refptr<rtc::RTCCertificate> certificate_;
BundleManager bundles_;

2
pc/peer_connection_ice_unittest.cc
View File

@ -94,6 +94,7 @@ using ::testing::Values;
constexpr int kIceCandidatesTimeout = 10000;
constexpr int64_t kWaitTimeout = 10000;
constexpr uint64_t kTiebreakerDefault = 44444;
class PeerConnectionWrapperForIceTest : public PeerConnectionWrapper {
public:
@ -1447,6 +1448,7 @@ class PeerConnectionIceConfigTest : public ::testing::Test {
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(

2
pc/peer_connection_interface_unittest.cc
View File

@ -114,6 +114,7 @@ static const char kVideoTracks[][32] = {"videotrack0", "videotrack1"};
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;
@ -710,6 +711,7 @@ class PeerConnectionInterfaceBaseTest : public ::testing::Test {
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.