diff --git a/webrtc/p2p/base/p2ptransportchannel.cc b/webrtc/p2p/base/p2ptransportchannel.cc index 5238da83ca..c12e8dbf66 100644 --- a/webrtc/p2p/base/p2ptransportchannel.cc +++ b/webrtc/p2p/base/p2ptransportchannel.cc @@ -136,9 +136,7 @@ void P2PTransportChannel::AddAllocatorSession( session->set_generation(static_cast(allocator_sessions_.size())); session->SignalPortReady.connect(this, &P2PTransportChannel::OnPortReady); - session->SignalPortsRemoved.connect(this, - &P2PTransportChannel::OnPortsRemoved); - session->SignalPortPruned.connect(this, &P2PTransportChannel::OnPortPruned); + session->SignalPortsPruned.connect(this, &P2PTransportChannel::OnPortsPruned); session->SignalCandidatesReady.connect( this, &P2PTransportChannel::OnCandidatesReady); session->SignalCandidatesRemoved.connect( @@ -149,7 +147,7 @@ void P2PTransportChannel::AddAllocatorSession( // We now only want to apply new candidates that we receive to the ports // created by this new session because these are replacing those of the // previous sessions. - removed_ports_.insert(removed_ports_.end(), ports_.begin(), ports_.end()); + pruned_ports_.insert(pruned_ports_.end(), ports_.begin(), ports_.end()); ports_.clear(); allocator_sessions_.push_back(std::move(session)); @@ -242,7 +240,7 @@ void P2PTransportChannel::SetIceRole(IceRole ice_role) { } // Update role on removed ports as well, because they may still have // connections alive that should be using the correct role. - for (PortInterface* port : removed_ports_) { + for (PortInterface* port : pruned_ports_) { port->SetIceRole(ice_role); } } @@ -250,7 +248,7 @@ void P2PTransportChannel::SetIceRole(IceRole ice_role) { void P2PTransportChannel::SetIceTiebreaker(uint64_t tiebreaker) { ASSERT(worker_thread_ == rtc::Thread::Current()); - if (!ports_.empty() || !removed_ports_.empty()) { + if (!ports_.empty() || !pruned_ports_.empty()) { LOG(LS_ERROR) << "Attempt to change tiebreaker after Port has been allocated."; return; @@ -1678,20 +1676,19 @@ void P2PTransportChannel::OnPortDestroyed(PortInterface* port) { ASSERT(worker_thread_ == rtc::Thread::Current()); ports_.erase(std::remove(ports_.begin(), ports_.end(), port), ports_.end()); - removed_ports_.erase( - std::remove(removed_ports_.begin(), removed_ports_.end(), port), - removed_ports_.end()); + pruned_ports_.erase( + std::remove(pruned_ports_.begin(), pruned_ports_.end(), port), + pruned_ports_.end()); LOG(INFO) << "Removed port because it is destroyed: " << ports_.size() << " remaining"; } -void P2PTransportChannel::OnPortsRemoved( +void P2PTransportChannel::OnPortsPruned( PortAllocatorSession* session, const std::vector& ports) { ASSERT(worker_thread_ == rtc::Thread::Current()); - LOG(LS_INFO) << "Remove " << ports.size() << " ports"; for (PortInterface* port : ports) { - if (RemovePort(port)) { + if (OnPortPruned(port)) { LOG(INFO) << "Removed port: " << port->ToString() << " " << ports_.size() << " remaining"; } @@ -1730,22 +1727,14 @@ void P2PTransportChannel::OnRegatherOnFailedNetworks() { MSG_REGATHER_ON_FAILED_NETWORKS); } -void P2PTransportChannel::OnPortPruned(PortAllocatorSession* session, - PortInterface* port) { - if (RemovePort(port)) { - LOG(INFO) << "Removed port because it is pruned: " << port->ToString() - << " " << ports_.size() << " remaining"; - } -} - -bool P2PTransportChannel::RemovePort(PortInterface* port) { +bool P2PTransportChannel::OnPortPruned(PortInterface* port) { auto it = std::find(ports_.begin(), ports_.end(), port); // Don't need to do anything if the port has been deleted from the port list. if (it == ports_.end()) { return false; } ports_.erase(it); - removed_ports_.push_back(port); + pruned_ports_.push_back(port); return true; } diff --git a/webrtc/p2p/base/p2ptransportchannel.h b/webrtc/p2p/base/p2ptransportchannel.h index 692b09843b..f363adc600 100644 --- a/webrtc/p2p/base/p2ptransportchannel.h +++ b/webrtc/p2p/base/p2ptransportchannel.h @@ -276,11 +276,8 @@ class P2PTransportChannel : public TransportChannelImpl, void AddConnection(Connection* connection); void OnPortReady(PortAllocatorSession *session, PortInterface* port); - // TODO(honghaiz): Merge the two methods OnPortsRemoved and OnPortPruned but - // still log the reason of removing. - void OnPortsRemoved(PortAllocatorSession* session, - const std::vector& ports); - void OnPortPruned(PortAllocatorSession* session, PortInterface* port); + void OnPortsPruned(PortAllocatorSession* session, + const std::vector& ports); void OnCandidatesReady(PortAllocatorSession *session, const std::vector& candidates); void OnCandidatesRemoved(PortAllocatorSession* session, @@ -294,11 +291,11 @@ class P2PTransportChannel : public TransportChannelImpl, bool port_muxed); // When a port is destroyed, remove it from both lists |ports_| - // and |removed_ports_|. + // and |pruned_ports_|. void OnPortDestroyed(PortInterface* port); - // When removing a port, move it from |ports_| to |removed_ports_|. + // When pruning a port, move it from |ports_| to |pruned_ports_|. // Returns true if the port is found and removed from |ports_|. - bool RemovePort(PortInterface* port); + bool OnPortPruned(PortInterface* port); void OnRoleConflict(PortInterface* port); void OnConnectionStateChange(Connection* connection); @@ -366,11 +363,11 @@ class P2PTransportChannel : public TransportChannelImpl, // |ports_| contains ports that are used to form new connections when // new remote candidates are added. std::vector ports_; - // |removed_ports_| contains ports that have been removed from |ports_| and + // |pruned_ports_| contains ports that have been removed from |ports_| and // are not being used to form new connections, but that aren't yet destroyed. // They may have existing connections, and they still fire signals such as // SignalUnknownAddress. - std::vector removed_ports_; + std::vector pruned_ports_; // |connections_| is a sorted list with the first one always be the // |selected_connection_| when it's not nullptr. The combination of diff --git a/webrtc/p2p/base/p2ptransportchannel_unittest.cc b/webrtc/p2p/base/p2ptransportchannel_unittest.cc index 6673f8f54d..c9600ddddd 100644 --- a/webrtc/p2p/base/p2ptransportchannel_unittest.cc +++ b/webrtc/p2p/base/p2ptransportchannel_unittest.cc @@ -3550,7 +3550,7 @@ TEST_F(P2PTransportChannelPingTest, TestIceRoleUpdatedOnRemovedPort) { // Make a fake signal to remove the ports in the p2ptransportchannel. then // change the ICE role and expect it to be updated. std::vector ports(1, conn->port()); - ch.allocator_session()->SignalPortsRemoved(ch.allocator_session(), ports); + ch.allocator_session()->SignalPortsPruned(ch.allocator_session(), ports); ch.SetIceRole(ICEROLE_CONTROLLED); EXPECT_EQ(ICEROLE_CONTROLLED, conn->port()->GetIceRole()); } diff --git a/webrtc/p2p/base/portallocator.h b/webrtc/p2p/base/portallocator.h index 152474d4f4..5deae7f736 100644 --- a/webrtc/p2p/base/portallocator.h +++ b/webrtc/p2p/base/portallocator.h @@ -191,11 +191,12 @@ class PortAllocatorSession : public sigslot::has_slots<> { virtual bool CandidatesAllocationDone() const = 0; sigslot::signal2 SignalPortReady; - // Ports should be signaled to be removed when the networks of the ports - // failed (either because the interface is down, or because there is no - // connection on the interface). + // Fires this signal when the network of the ports failed (either because the + // interface is down, or because there is no connection on the interface), + // or when TURN ports are pruned because a higher-priority TURN port becomes + // ready(pairable). sigslot::signal2&> - SignalPortsRemoved; + SignalPortsPruned; sigslot::signal2&> SignalCandidatesReady; // Candidates should be signaled to be removed when the port that generated @@ -203,11 +204,6 @@ class PortAllocatorSession : public sigslot::has_slots<> { sigslot::signal2&> SignalCandidatesRemoved; sigslot::signal1 SignalCandidatesAllocationDone; - // A TURN port is pruned if a higher-priority TURN port becomes ready - // (pairable). When it is pruned, it will not be used for creating - // connections and its candidates will not be sent to the remote side - // if they have not been sent. - sigslot::signal2 SignalPortPruned; virtual uint32_t generation() { return generation_; } virtual void set_generation(uint32_t generation) { generation_ = generation; } diff --git a/webrtc/p2p/client/basicportallocator.cc b/webrtc/p2p/client/basicportallocator.cc index 5e4930c1cb..46c28926c8 100644 --- a/webrtc/p2p/client/basicportallocator.cc +++ b/webrtc/p2p/client/basicportallocator.cc @@ -695,12 +695,12 @@ void BasicPortAllocatorSession::OnCandidateReady( // Note: We should check whether any candidates may become ready after this // because there we will check whether the candidate is generated by the ready // ports, which may include this port. - bool pruned_port = false; + bool pruned = false; if (CandidatePairable(c, port) && !data->has_pairable_candidate()) { data->set_has_pairable_candidate(true); if (prune_turn_ports_ && port->Type() == RELAY_PORT_TYPE) { - pruned_port = PruneTurnPorts(port); + pruned = PruneTurnPorts(port); } // If the current port is not pruned yet, SignalPortReady. if (!data->pruned()) { @@ -726,7 +726,7 @@ void BasicPortAllocatorSession::OnCandidateReady( } // If we have pruned any port, maybe need to signal port allocation done. - if (pruned_port) { + if (pruned) { MaybeSignalCandidatesAllocationDone(); } } @@ -745,7 +745,6 @@ Port* BasicPortAllocatorSession::GetBestTurnPortForNetwork( } bool BasicPortAllocatorSession::PruneTurnPorts(Port* newly_pairable_turn_port) { - bool pruned_port = false; // Note: We determine the same network based only on their network names. So // if an IPv4 address and an IPv6 address have the same network name, they // are considered the same network here. @@ -754,18 +753,24 @@ bool BasicPortAllocatorSession::PruneTurnPorts(Port* newly_pairable_turn_port) { // |port| is already in the list of ports, so the best port cannot be nullptr. RTC_CHECK(best_turn_port != nullptr); + bool pruned = false; + std::vector pruned_ports; for (PortData& data : ports_) { if (data.port()->Network()->name() == network_name && data.port()->Type() == RELAY_PORT_TYPE && !data.pruned() && ComparePort(data.port(), best_turn_port) < 0) { data.set_pruned(); - pruned_port = true; + pruned = true; if (data.port() != newly_pairable_turn_port) { - SignalPortPruned(this, data.port()); + pruned_ports.push_back(data.port()); } } } - return pruned_port; + if (!pruned_ports.empty()) { + LOG(LS_INFO) << "Pruned " << pruned_ports.size() << " ports"; + SignalPortsPruned(this, pruned_ports); + } + return pruned; } void BasicPortAllocatorSession::OnPortComplete(Port* port) { @@ -946,7 +951,8 @@ void BasicPortAllocatorSession::RemovePortsAndCandidates( } } if (!ports_to_remove.empty()) { - SignalPortsRemoved(this, ports_to_remove); + LOG(LS_INFO) << "Removed " << ports_to_remove.size() << " ports"; + SignalPortsPruned(this, ports_to_remove); } if (!candidates_to_remove.empty()) { SignalCandidatesRemoved(this, candidates_to_remove); diff --git a/webrtc/p2p/client/basicportallocator_unittest.cc b/webrtc/p2p/client/basicportallocator_unittest.cc index 4696f2399c..9f91aa0857 100644 --- a/webrtc/p2p/client/basicportallocator_unittest.cc +++ b/webrtc/p2p/client/basicportallocator_unittest.cc @@ -239,8 +239,8 @@ class BasicPortAllocatorTest : public testing::Test, sid, content_name, component, ice_ufrag, ice_pwd); session->SignalPortReady.connect(this, &BasicPortAllocatorTest::OnPortReady); - session->SignalPortPruned.connect(this, - &BasicPortAllocatorTest::OnPortPruned); + session->SignalPortsPruned.connect(this, + &BasicPortAllocatorTest::OnPortsPruned); session->SignalCandidatesReady.connect( this, &BasicPortAllocatorTest::OnCandidatesReady); session->SignalCandidatesAllocationDone.connect( @@ -415,13 +415,18 @@ class BasicPortAllocatorTest : public testing::Test, EXPECT_NE(ready_ports.end(), std::find(ready_ports.begin(), ready_ports.end(), port)); } - void OnPortPruned(PortAllocatorSession* ses, PortInterface* port) { - LOG(LS_INFO) << "OnPortPruned: " << port->ToString(); - ports_.erase(std::remove(ports_.begin(), ports_.end(), port), ports_.end()); - // Make sure the pruned port is not in ReadyPorts. + void OnPortsPruned(PortAllocatorSession* ses, + const std::vector& ports_pruned) { + LOG(LS_INFO) << "Number of ports pruned: " << ports_pruned.size(); auto ready_ports = ses->ReadyPorts(); - EXPECT_EQ(ready_ports.end(), - std::find(ready_ports.begin(), ready_ports.end(), port)); + auto new_end = ports_.end(); + for (PortInterface* port : ports_pruned) { + new_end = std::remove(ports_.begin(), new_end, port); + // Make sure the pruned port is not in ReadyPorts. + EXPECT_EQ(ready_ports.end(), + std::find(ready_ports.begin(), ready_ports.end(), port)); + } + ports_.erase(new_end, ports_.end()); } void OnCandidatesReady(PortAllocatorSession* ses,