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

Don't re-determine ICE role on an ICE restart.

Browse Source 
This only causes an increased likelihood of role conflicts as each peer
is picking a new role. It also means that if ICE is restarted for only
one media stream, roles can be different across media streams (which isn't
even allowed).

For more explanation of why this is unnecessary and should be changed,
see this discussion:
https://mailarchive.ietf.org/arch/msg/ice/C0_QRCTNcwtvUF12y28jQicPR10

Review-Url: https://codereview.webrtc.org/2046123003
Cr-Commit-Position: refs/heads/master@{#13077}
This commit is contained in:
deadbeef 2016-06-08 13:13:00 -07:00 committed by Commit bot
parent 3cd9a30f96
commit 73fbcf9237
2 changed files with 18 additions and 24 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

16
webrtc/p2p/base/transport.cc
View File

@ -58,12 +58,6 @@ bool IceCredentialsChanged(const std::string& old_ufrag,
return (old_ufrag != new_ufrag) || (old_pwd != new_pwd);
}
static bool IceCredentialsChanged(const TransportDescription& old_desc,
const TransportDescription& new_desc) {
return IceCredentialsChanged(old_desc.ice_ufrag, old_desc.ice_pwd,
new_desc.ice_ufrag, new_desc.ice_pwd);
}
Transport::Transport(const std::string& name, PortAllocator* allocator)
: name_(name), allocator_(allocator) {}
@ -105,16 +99,6 @@ bool Transport::SetLocalTransportDescription(
error_desc);
}
if (local_description_ &&
IceCredentialsChanged(*local_description_, description)) {
IceRole new_ice_role =
(action == CA_OFFER) ? ICEROLE_CONTROLLING : ICEROLE_CONTROLLED;
// It must be called before ApplyLocalTransportDescription, which may
// trigger an ICE restart and depends on the new ICE role.
SetIceRole(new_ice_role);
}
local_description_.reset(new TransportDescription(description));
for (const auto& kv : channels_) {

26
webrtc/p2p/base/transport_unittest.cc
View File

@ -97,8 +97,13 @@ TEST_F(TransportTest, TestIceCredentialsChanged) {
EXPECT_FALSE(cricket::IceCredentialsChanged("u1", "p1", "u1", "p1"));
}
// This test verifies that the callee's ICE role changes from controlled to
// controlling when the callee triggers an ICE restart.
// This test verifies that the callee's ICE role remains the same when the
// callee triggers an ICE restart.
//
// RFC5245 currently says that the role *should* change on an ICE restart,
// but this rule was intended for an ICE restart that occurs when an endpoint
// is changing to ICE lite (which we already handle). See discussion here:
// https://mailarchive.ietf.org/arch/msg/ice/C0_QRCTNcwtvUF12y28jQicPR10
TEST_F(TransportTest, TestIceControlledToControllingOnIceRestart) {
EXPECT_TRUE(SetupChannel());
transport_->SetIceRole(cricket::ICEROLE_CONTROLLED);
@ -116,12 +121,17 @@ TEST_F(TransportTest, TestIceControlledToControllingOnIceRestart) {
ASSERT_TRUE(transport_->SetLocalTransportDescription(new_local_desc,
cricket::CA_OFFER,
NULL));
EXPECT_EQ(cricket::ICEROLE_CONTROLLING, transport_->ice_role());
EXPECT_EQ(cricket::ICEROLE_CONTROLLING, channel_->GetIceRole());
EXPECT_EQ(cricket::ICEROLE_CONTROLLED, transport_->ice_role());
EXPECT_EQ(cricket::ICEROLE_CONTROLLED, channel_->GetIceRole());
}
// This test verifies that the caller's ICE role changes from controlling to
// controlled when the callee triggers an ICE restart.
// This test verifies that the caller's ICE role remains the same when the
// callee triggers an ICE restart.
//
// RFC5245 currently says that the role *should* change on an ICE restart,
// but this rule was intended for an ICE restart that occurs when an endpoint
// is changing to ICE lite (which we already handle). See discussion here:
// https://mailarchive.ietf.org/arch/msg/ice/C0_QRCTNcwtvUF12y28jQicPR10
TEST_F(TransportTest, TestIceControllingToControlledOnIceRestart) {
EXPECT_TRUE(SetupChannel());
transport_->SetIceRole(cricket::ICEROLE_CONTROLLING);
@ -139,8 +149,8 @@ TEST_F(TransportTest, TestIceControllingToControlledOnIceRestart) {
ASSERT_TRUE(transport_->SetLocalTransportDescription(new_local_desc,
cricket::CA_ANSWER,
NULL));
EXPECT_EQ(cricket::ICEROLE_CONTROLLED, transport_->ice_role());
EXPECT_EQ(cricket::ICEROLE_CONTROLLED, channel_->GetIceRole());
EXPECT_EQ(cricket::ICEROLE_CONTROLLING, transport_->ice_role());
EXPECT_EQ(cricket::ICEROLE_CONTROLLING, channel_->GetIceRole());
}
// This test verifies that the caller's ICE role is still controlling after the