diff --git a/webrtc/api/webrtcsession_unittest.cc b/webrtc/api/webrtcsession_unittest.cc index c0d8a3bbdf..6c0170e704 100644 --- a/webrtc/api/webrtcsession_unittest.cc +++ b/webrtc/api/webrtcsession_unittest.cc @@ -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. diff --git a/webrtc/p2p/base/p2ptransportchannel.cc b/webrtc/p2p/base/p2ptransportchannel.cc index d4d0e5e327..03b0ed426a 100644 --- a/webrtc/p2p/base/p2ptransportchannel.cc +++ b/webrtc/p2p/base/p2ptransportchannel.cc @@ -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"; diff --git a/webrtc/p2p/base/p2ptransportchannel_unittest.cc b/webrtc/p2p/base/p2ptransportchannel_unittest.cc index 9342a201a1..26db621a9d 100644 --- a/webrtc/p2p/base/p2ptransportchannel_unittest.cc +++ b/webrtc/p2p/base/p2ptransportchannel_unittest.cc @@ -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(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(), diff --git a/webrtc/p2p/base/portallocator.h b/webrtc/p2p/base/portallocator.h index 8eef714b09..8befbadcc7 100644 --- a/webrtc/p2p/base/portallocator.h +++ b/webrtc/p2p/base/portallocator.h @@ -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). diff --git a/webrtc/p2p/client/basicportallocator.cc b/webrtc/p2p/client/basicportallocator.cc index 6234d77b75..e94feb4d15 100644 --- a/webrtc/p2p/client/basicportallocator.cc +++ b/webrtc/p2p/client/basicportallocator.cc @@ -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; } diff --git a/webrtc/p2p/client/basicportallocator_unittest.cc b/webrtc/p2p/client/basicportallocator_unittest.cc index 93951ff80b..c9831f525b 100644 --- a/webrtc/p2p/client/basicportallocator_unittest.cc +++ b/webrtc/p2p/client/basicportallocator_unittest.cc @@ -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