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

Fixing inconsistency with behavior of ClearGettingPorts.

Browse Source 
I found that, depending on when it's called, ClearGettingPorts may or
may not signal CandidatesAllocationDone, and may or may not continue
to gather more ports/candidates.

I'm fixing this inconsistency by having it always signal
CandidatesAllocationDone (if needed), and always stop gathering until
the next network change event. This makes it equivalent to
StopGettingPorts, except that it allows gathering to be restarted if
a network change occurs.

I also found that P2PTransportChannel was signaling "gathering
complete" even when continual gathering was enabled. This wasn't caught
by the unit tests due to the inconsistency of ClearGettingPorts as
described above.

Review-Url: https://codereview.webrtc.org/2124283003
Cr-Commit-Position: refs/heads/master@{#13908}
This commit is contained in:
deadbeef 2016-08-24 15:15:00 -07:00 committed by Commit bot
parent 824f586213
commit b60a8198f1
6 changed files with 77 additions and 54 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

11
webrtc/api/webrtcsession_unittest.cc
View File

@ -2258,6 +2258,10 @@ TEST_F(WebRtcSessionTest,
TestLocalCandidatesAddedAndRemovedIfGatherContinually) {
AddInterface(rtc::SocketAddress(kClientAddrHost1, kClientAddrPort));
Init();
// Enable Continual Gathering.
cricket::IceConfig config;
config.continual_gathering_policy = cricket::GATHER_CONTINUALLY;
session_->SetIceConfig(config);
SendAudioVideoStream1();
CreateAndSetRemoteOfferAndLocalAnswer();
@ -2267,7 +2271,8 @@ TEST_F(WebRtcSessionTest,
ASSERT_TRUE(candidates != NULL);
EXPECT_EQ(0u, candidates->count());
EXPECT_TRUE_WAIT(observer_.oncandidatesready_, kIceCandidatesTimeout);
// Since we're using continual gathering, we won't get "gathering done".
EXPECT_EQ_WAIT(2u, candidates->count(), kIceCandidatesTimeout);
local_desc = session_->local_description();
candidates = local_desc->candidates(kMediaContentIndex0);
@ -2291,10 +2296,6 @@ TEST_F(WebRtcSessionTest,
candidates = local_desc->candidates(kMediaContentIndex0);
size_t num_local_candidates = candidates->count();
// Enable Continual Gathering
cricket::IceConfig config;
config.continual_gathering_policy = cricket::GATHER_CONTINUALLY;
session_->SetIceConfig(config);
// Bring down the network interface to trigger candidate removals.
RemoveInterface(rtc::SocketAddress(kClientAddrHost1, kClientAddrPort));
// Verify that all local candidates are removed.

18
webrtc/p2p/base/p2ptransportchannel.cc
View File

@ -351,9 +351,14 @@ void P2PTransportChannel::SetRemoteIceMode(IceMode mode) {
void P2PTransportChannel::SetIceConfig(const IceConfig& config) {
if (config_.continual_gathering_policy != config.continual_gathering_policy) {
config_.continual_gathering_policy = config.continual_gathering_policy;
LOG(LS_INFO) << "Set continual_gathering_policy to "
<< config_.continual_gathering_policy;
if (!allocator_sessions_.empty()) {
LOG(LS_ERROR) << "Trying to change continual gathering policy "
<< "when gathering has already started!";
} else {
config_.continual_gathering_policy = config.continual_gathering_policy;
LOG(LS_INFO) << "Set continual_gathering_policy to "
<< config_.continual_gathering_policy;
}
}
if (config.backup_connection_ping_interval >= 0 &&
@ -524,6 +529,13 @@ void P2PTransportChannel::OnCandidatesReady(
void P2PTransportChannel::OnCandidatesAllocationDone(
PortAllocatorSession* session) {
ASSERT(worker_thread_ == rtc::Thread::Current());
if (config_.gather_continually()) {
LOG(LS_INFO) << "P2PTransportChannel: " << transport_name()
<< ", component " << component()
<< " gathering complete, but using continual "
<< "gathering so not changing gathering state.";
return;
}
gathering_state_ = kIceGatheringComplete;
LOG(LS_INFO) << "P2PTransportChannel: " << transport_name() << ", component "
<< component() << " gathering complete";

78
webrtc/p2p/base/p2ptransportchannel_unittest.cc
View File

@ -305,7 +305,9 @@ class P2PTransportChannelTestBase : public testing::Test,
return ep2_.GetChannelData(channel);
}
void CreateChannels(int num) {
void CreateChannels(int num,
const IceConfig& ep1_config,
const IceConfig& ep2_config) {
std::string ice_ufrag_ep1_cd1_ch = kIceUfrag[0];
std::string ice_pwd_ep1_cd1_ch = kIcePwd[0];
std::string ice_ufrag_ep2_cd1_ch = kIceUfrag[1];
@ -316,6 +318,8 @@ class P2PTransportChannelTestBase : public testing::Test,
ep2_.cd1_.ch_.reset(CreateChannel(
1, ICE_CANDIDATE_COMPONENT_DEFAULT, ice_ufrag_ep2_cd1_ch,
ice_pwd_ep2_cd1_ch, ice_ufrag_ep1_cd1_ch, ice_pwd_ep1_cd1_ch));
ep1_.cd1_.ch_->SetIceConfig(ep1_config);
ep2_.cd1_.ch_->SetIceConfig(ep2_config);
ep1_.cd1_.ch_->MaybeStartGathering();
ep2_.cd1_.ch_->MaybeStartGathering();
if (num == 2) {
@ -329,10 +333,18 @@ class P2PTransportChannelTestBase : public testing::Test,
ep2_.cd2_.ch_.reset(CreateChannel(
1, ICE_CANDIDATE_COMPONENT_DEFAULT, ice_ufrag_ep2_cd2_ch,
ice_pwd_ep2_cd2_ch, ice_ufrag_ep1_cd2_ch, ice_pwd_ep1_cd2_ch));
ep1_.cd2_.ch_->SetIceConfig(ep1_config);
ep2_.cd2_.ch_->SetIceConfig(ep2_config);
ep1_.cd2_.ch_->MaybeStartGathering();
ep2_.cd2_.ch_->MaybeStartGathering();
}
}
void CreateChannels(int num) {
IceConfig default_config;
CreateChannels(num, default_config, default_config);
}
P2PTransportChannel* CreateChannel(int endpoint,
int component,
const std::string& local_ice_ufrag,
@ -1573,10 +1585,11 @@ TEST_F(P2PTransportChannelTest, TestContinualGathering) {
kDefaultPortAllocatorFlags);
SetAllocationStepDelay(0, kDefaultStepDelay);
SetAllocationStepDelay(1, kDefaultStepDelay);
CreateChannels(1);
IceConfig config = CreateIceConfig(1000, GATHER_CONTINUALLY);
ep1_ch1()->SetIceConfig(config);
IceConfig continual_gathering_config =
CreateIceConfig(1000, GATHER_CONTINUALLY);
// By default, ep2 does not gather continually.
IceConfig default_config;
CreateChannels(1, continual_gathering_config, default_config);
EXPECT_TRUE_WAIT_MARGIN(ep1_ch1() != NULL && ep2_ch1() != NULL &&
ep1_ch1()->receiving() && ep1_ch1()->writable() &&
@ -1908,8 +1921,10 @@ TEST_F(P2PTransportChannelMultihomedTest, TestFailoverControlledSide) {
SetAllocatorFlags(0, kOnlyLocalPorts);
SetAllocatorFlags(1, kOnlyLocalPorts);
// Make the receiving timeout shorter for testing.
IceConfig config = CreateIceConfig(1000, GATHER_ONCE);
// Create channels and let them go writable, as usual.
CreateChannels(1);
CreateChannels(1, config, config);
EXPECT_TRUE_SIMULATED_WAIT(ep1_ch1()->receiving() && ep1_ch1()->writable() &&
ep2_ch1()->receiving() &&
@ -1920,11 +1935,6 @@ TEST_F(P2PTransportChannelMultihomedTest, TestFailoverControlledSide) {
LocalCandidate(ep1_ch1())->address().EqualIPs(kPublicAddrs[0]) &&
RemoteCandidate(ep1_ch1())->address().EqualIPs(kPublicAddrs[1]));
// Make the receiving timeout shorter for testing.
IceConfig config = CreateIceConfig(1000, GATHER_ONCE);
ep1_ch1()->SetIceConfig(config);
ep2_ch1()->SetIceConfig(config);
// Blackhole any traffic to or from the public addrs.
LOG(LS_INFO) << "Failing over...";
fw()->AddRule(false, rtc::FP_ANY, rtc::FD_ANY, kPublicAddrs[1]);
@ -1964,8 +1974,10 @@ TEST_F(P2PTransportChannelMultihomedTest, TestFailoverControllingSide) {
SetAllocatorFlags(0, kOnlyLocalPorts);
SetAllocatorFlags(1, kOnlyLocalPorts);
// Make the receiving timeout shorter for testing.
IceConfig config = CreateIceConfig(1000, GATHER_ONCE);
// Create channels and let them go writable, as usual.
CreateChannels(1);
CreateChannels(1, config, config);
EXPECT_TRUE_SIMULATED_WAIT(ep1_ch1()->receiving() && ep1_ch1()->writable() &&
ep2_ch1()->receiving() &&
ep2_ch1()->writable(),
@ -1975,11 +1987,6 @@ TEST_F(P2PTransportChannelMultihomedTest, TestFailoverControllingSide) {
LocalCandidate(ep1_ch1())->address().EqualIPs(kPublicAddrs[0]) &&
RemoteCandidate(ep1_ch1())->address().EqualIPs(kPublicAddrs[1]));
// Make the receiving timeout shorter for testing.
IceConfig config = CreateIceConfig(1000, GATHER_ONCE);
ep1_ch1()->SetIceConfig(config);
ep2_ch1()->SetIceConfig(config);
// Blackhole any traffic to or from the public addrs.
LOG(LS_INFO) << "Failing over...";
fw()->AddRule(false, rtc::FP_ANY, rtc::FD_ANY, kPublicAddrs[0]);
@ -2020,8 +2027,10 @@ TEST_F(P2PTransportChannelMultihomedTest, TestIceRenomination) {
SetAllocatorFlags(0, kOnlyLocalPorts);
SetAllocatorFlags(1, kOnlyLocalPorts);
// Make the receiving timeout shorter for testing.
IceConfig config = CreateIceConfig(1000, GATHER_ONCE);
// Create channels and let them go writable, as usual.
CreateChannels(1);
CreateChannels(1, config, config);
ep1_ch1()->set_remote_supports_renomination(true);
EXPECT_TRUE_SIMULATED_WAIT(ep1_ch1()->receiving() && ep1_ch1()->writable() &&
ep2_ch1()->receiving() &&
@ -2041,11 +2050,6 @@ TEST_F(P2PTransportChannelMultihomedTest, TestIceRenomination) {
SIMULATED_WAIT(nominated(), 3000, clock);
EXPECT_FALSE(nominated());
// Make the receiving timeout shorter for testing.
IceConfig config = CreateIceConfig(1000, GATHER_ONCE);
ep1_ch1()->SetIceConfig(config);
ep2_ch1()->SetIceConfig(config);
// Blackhole any traffic to or from the public addrs.
LOG(LS_INFO) << "Failing over...";
fw()->AddRule(false, rtc::FP_ANY, rtc::FD_ANY, kPublicAddrs[0]);
@ -2362,9 +2366,9 @@ TEST_F(P2PTransportChannelMultihomedTest, TestNetworkBecomesInactive) {
AddAddress(0, kPublicAddrs[0]);
AddAddress(1, kPublicAddrs[1]);
// Create channels and let them go writable, as usual.
CreateChannels(1);
ep1_ch1()->SetIceConfig(CreateIceConfig(2000, GATHER_CONTINUALLY));
ep2_ch1()->SetIceConfig(CreateIceConfig(2000, GATHER_ONCE));
IceConfig ep1_config = CreateIceConfig(2000, GATHER_CONTINUALLY);
IceConfig ep2_config = CreateIceConfig(2000, GATHER_ONCE);
CreateChannels(1, ep1_config, ep2_config);
SetAllocatorFlags(0, kOnlyLocalPorts);
SetAllocatorFlags(1, kOnlyLocalPorts);
@ -2406,10 +2410,10 @@ TEST_F(P2PTransportChannelMultihomedTest,
auto& cellular = kPublicAddrs;
AddAddress(0, wifi[0], "test_wifi0", rtc::ADAPTER_TYPE_WIFI);
AddAddress(1, cellular[1], "test_cell1", rtc::ADAPTER_TYPE_CELLULAR);
CreateChannels(1);
// Set continual gathering policy.
ep1_ch1()->SetIceConfig(CreateIceConfig(1000, GATHER_CONTINUALLY));
ep2_ch1()->SetIceConfig(CreateIceConfig(1000, GATHER_CONTINUALLY));
IceConfig continual_gathering_config =
CreateIceConfig(1000, GATHER_CONTINUALLY);
CreateChannels(1, continual_gathering_config, continual_gathering_config);
SetAllocatorFlags(0, kOnlyLocalPorts);
SetAllocatorFlags(1, kOnlyLocalPorts);
EXPECT_TRUE_WAIT_MARGIN(ep1_ch1()->receiving() && ep1_ch1()->writable() &&
@ -2456,11 +2460,11 @@ TEST_F(P2PTransportChannelMultihomedTest,
SetAllocatorFlags(0, kOnlyLocalPorts);
SetAllocatorFlags(1, kOnlyLocalPorts);
// Create channels and let them go writable, as usual.
CreateChannels(1);
// Set continual gathering policy.
ep1_ch1()->SetIceConfig(CreateIceConfig(1000, GATHER_CONTINUALLY));
ep2_ch1()->SetIceConfig(CreateIceConfig(1000, GATHER_CONTINUALLY));
IceConfig continual_gathering_config =
CreateIceConfig(1000, GATHER_CONTINUALLY);
// Create channels and let them go writable, as usual.
CreateChannels(1, continual_gathering_config, continual_gathering_config);
EXPECT_TRUE_SIMULATED_WAIT(ep1_ch1()->receiving() && ep1_ch1()->writable() &&
ep2_ch1()->receiving() &&
ep2_ch1()->writable(),
@ -2514,12 +2518,10 @@ TEST_F(P2PTransportChannelMultihomedTest,
SetAllocatorFlags(0, kOnlyLocalPorts);
SetAllocatorFlags(1, kOnlyLocalPorts);
// Create channels and let them go writable, as usual.
CreateChannels(1);
// Set continual gathering policy.
IceConfig config = CreateIceConfig(1000, GATHER_CONTINUALLY_AND_RECOVER);
ep1_ch1()->SetIceConfig(config);
ep2_ch1()->SetIceConfig(config);
// Create channels and let them go writable, as usual.
CreateChannels(1, config, config);
EXPECT_TRUE_SIMULATED_WAIT(ep1_ch1()->receiving() && ep1_ch1()->writable() &&
ep2_ch1()->receiving() && ep2_ch1()->writable(),
3000, clock);
@ -2558,11 +2560,9 @@ TEST_F(P2PTransportChannelMultihomedTest, TestRestoreBackupConnection) {
SetAllocatorFlags(1, kOnlyLocalPorts);
// Create channels and let them go writable, as usual.
CreateChannels(1);
IceConfig config = CreateIceConfig(1000, GATHER_CONTINUALLY);
config.regather_on_failed_networks_interval = rtc::Optional<int>(2000);
ep1_ch1()->SetIceConfig(config);
ep2_ch1()->SetIceConfig(config);
CreateChannels(1, config, config);
EXPECT_TRUE_SIMULATED_WAIT(ep1_ch1()->receiving() && ep1_ch1()->writable() &&
ep2_ch1()->receiving() &&
ep2_ch1()->writable(),

16
webrtc/p2p/base/portallocator.h
View File

@ -158,14 +158,22 @@ class PortAllocatorSession : public sigslot::has_slots<> {
// Default filter should be CF_ALL.
virtual void SetCandidateFilter(uint32_t filter) = 0;
// Starts gathering STUN and Relay configurations.
// Starts gathering ports and ICE candidates.
virtual void StartGettingPorts() = 0;
// Completely stops the gathering process and will not start new ones.
// Completely stops gathering. Will not gather again unless StartGettingPorts
// is called again.
virtual void StopGettingPorts() = 0;
// Whether the session is actively getting ports.
virtual bool IsGettingPorts() = 0;
// ClearGettingPorts and IsCleared are used by continual gathering.
// Only stops the existing gathering process but may start new ones if needed.
//
// NOTE: The group of methods below is only used for continual gathering.
//
// ClearGettingPorts should have the same immediate effect as
// StopGettingPorts, but if the implementation supports continual gathering,
// ClearGettingPorts allows additional ports/candidates to be gathered if the
// network conditions change.
virtual void ClearGettingPorts() = 0;
// Whether it is in the state where the existing gathering process is stopped,
// but new ones may be started (basically after calling ClearGettingPorts).

2
webrtc/p2p/client/basicportallocator.cc
View File

@ -253,7 +253,6 @@ void BasicPortAllocatorSession::StartGettingPorts() {
void BasicPortAllocatorSession::StopGettingPorts() {
ASSERT(rtc::Thread::Current() == network_thread_);
network_thread_->Post(RTC_FROM_HERE, this, MSG_CONFIG_STOP);
ClearGettingPorts();
// Note: this must be called after ClearGettingPorts because both may set the
// session state and we should set the state to STOPPED.
@ -266,6 +265,7 @@ void BasicPortAllocatorSession::ClearGettingPorts() {
for (uint32_t i = 0; i < sequences_.size(); ++i) {
sequences_[i]->Stop();
}
network_thread_->Post(RTC_FROM_HERE, this, MSG_CONFIG_STOP);
state_ = SessionState::CLEARED;
}

6
webrtc/p2p/client/basicportallocator_unittest.cc
View File

@ -1608,6 +1608,7 @@ TEST_F(BasicPortAllocatorTest, TestStopGettingPorts) {
// After stopping getting ports, adding a new interface will not start
// getting ports again.
allocator_->set_step_delay(kMinimumStepDelay);
candidates_.clear();
ports_.clear();
candidate_allocation_done_ = false;
@ -1625,17 +1626,18 @@ TEST_F(BasicPortAllocatorTest, TestClearGettingPorts) {
ASSERT_EQ_WAIT(2U, candidates_.size(), 1000);
EXPECT_EQ(2U, ports_.size());
session_->ClearGettingPorts();
WAIT(candidate_allocation_done_, 1000);
EXPECT_FALSE(candidate_allocation_done_);
EXPECT_TRUE_WAIT(candidate_allocation_done_, 1000);
// After clearing getting ports, adding a new interface will start getting
// ports again.
allocator_->set_step_delay(kMinimumStepDelay);
candidates_.clear();
ports_.clear();
candidate_allocation_done_ = false;
network_manager_.AddInterface(kClientAddr2);
ASSERT_EQ_WAIT(2U, candidates_.size(), 1000);
EXPECT_EQ(2U, ports_.size());
EXPECT_TRUE_WAIT(candidate_allocation_done_, kDefaultAllocationTimeout);
}
// Test that the ports and candidates are updated with new ufrag/pwd/etc. when