Add config to prune TURN ports

webrtc/p2p/client/basicportallocator.cc

Index: webrtc/p2p/client/basicportallocator.cc
diff --git a/webrtc/p2p/client/basicportallocator.cc b/webrtc/p2p/client/basicportallocator.cc
index e39a44013cdcf4f5b4cf627e225c499afb1e1687..f60807869a63f852209176d7f9b29ba064827fff 100644
--- a/webrtc/p2p/client/basicportallocator.cc
+++ b/webrtc/p2p/client/basicportallocator.cc
@@ -47,6 +47,45 @@ const int PHASE_SSLTCP = 3;
 
 const int kNumPhases = 4;
 
+enum class AddressFamily { UNSPEC, IPV4, IPV6 };
+
+AddressFamily ToAddressFamily(int ip_family) {
+  switch (ip_family) {
+    case AF_UNSPEC:
+      return AddressFamily::UNSPEC;
+    case AF_INET:
+      return AddressFamily::IPV4;
+    case AF_INET6:
+      return AddressFamily::IPV6;
+    default:
+      RTC_DCHECK(false);
+      return AddressFamily::UNSPEC;
+  }
+}

[pthatcher1, 2016/06/27 20:35:55]

Is this just for sorting?  If so, instead of making an emum for address family,
it seems like we should just make it part a int GetAddressFamilyPriority(int
ip_family).  UNSPEC -> 0, IPV4 -> 1, IPV6 -> 2.

Actually, why are we preferring ipv4 over ipv6?  Shouldn't be the other way
around?  Or should we just prefer which ever TURN candidate works first (lowest
RTT)?

[honghaiz3, 2016/06/28 01:49:25]

You are right. Make it return an integer. 
I am giving IPv6 higher priority than Ipv4 already. 
If we choose whichever candidate works first, it will essentially choose the
first port becoming ready. I think we discussed that and we don't want that
approach, although not particular about IpV4 vs IPv6.  

+
+// Returns positive if a is better, negative if b is better, and 0 otherwise.
+int ComparePort(const cricket::Port* a, const cricket::Port* b) {
+  static constexpr int a_is_better = 1;
+  static constexpr int b_is_better = -1;
+  // Protocol type is defined as UDP = 0, TCP = 1, SSLTCP = 2.
+  if (a->GetProtocol() < b->GetProtocol()) {
+    return a_is_better;
+  }
+  if (a->GetProtocol() > b->GetProtocol()) {
+    return b_is_better;
+  }
+
+  AddressFamily a_family = ToAddressFamily(a->Network()->GetBestIP().family());
+  AddressFamily b_family = ToAddressFamily(b->Network()->GetBestIP().family());
+  if (a_family > b_family) {
+    return a_is_better;
+  }
+  if (a_family < b_family) {
+    return b_is_better;
+  }
+  return 0;
+}
+
 }  // namespace
 
 namespace cricket {
@@ -74,7 +113,7 @@ BasicPortAllocator::BasicPortAllocator(rtc::NetworkManager* network_manager,
                                        const ServerAddresses& stun_servers)
     : network_manager_(network_manager), socket_factory_(socket_factory) {
   ASSERT(socket_factory_ != NULL);
-  SetConfiguration(stun_servers, std::vector<RelayServerConfig>(), 0);
+  SetConfiguration(stun_servers, std::vector<RelayServerConfig>(), 0, false);
   Construct();
 }
 
@@ -101,7 +140,7 @@ BasicPortAllocator::BasicPortAllocator(
     turn_servers.push_back(config);
   }
 
-  SetConfiguration(stun_servers, turn_servers, 0);
+  SetConfiguration(stun_servers, turn_servers, 0, false);
   Construct();
 }
 
@@ -122,7 +161,8 @@ PortAllocatorSession* BasicPortAllocator::CreateSessionInternal(
 void BasicPortAllocator::AddTurnServer(const RelayServerConfig& turn_server) {
   std::vector<RelayServerConfig> new_turn_servers = turn_servers();
   new_turn_servers.push_back(turn_server);
-  SetConfiguration(stun_servers(), new_turn_servers, candidate_pool_size());
+  SetConfiguration(stun_servers(), new_turn_servers, candidate_pool_size(),
+                   disable_low_priority_turn_ports());
 }
 
 // BasicPortAllocatorSession
@@ -217,8 +257,10 @@ void BasicPortAllocatorSession::ClearGettingPorts() {
 
 std::vector<PortInterface*> BasicPortAllocatorSession::ReadyPorts() const {
   std::vector<PortInterface*> ret;
+  // Add all non-TURN ports.
   for (const PortData& port : ports_) {
-    if (port.has_pairable_candidate() && !port.error()) {
+    if (IsPortInUse(port.port()) && port.has_pairable_candidate() &&
+        !port.error()) {
       ret.push_back(port.port());
     }
   }
@@ -228,6 +270,10 @@ std::vector<PortInterface*> BasicPortAllocatorSession::ReadyPorts() const {
 std::vector<Candidate> BasicPortAllocatorSession::ReadyCandidates() const {
   std::vector<Candidate> candidates;
   for (const PortData& data : ports_) {
+    if (!IsPortInUse(data.port())) {
+      continue;
+    }
+
     for (const Candidate& candidate : data.port()->Candidates()) {
       if (!CheckCandidateFilter(candidate)) {
         continue;
@@ -438,16 +484,14 @@ void BasicPortAllocatorSession::DoAllocate() {
   bool done_signal_needed = false;
   std::vector<rtc::Network*> networks;
   GetNetworks(&networks);
+  turn_ports_in_use_by_network_names_.clear();
 
   if (networks.empty()) {
     LOG(LS_WARNING) << "Machine has no networks; no ports will be allocated";
     done_signal_needed = true;
   } else {
+    PortConfiguration* config = configs_.empty() ? nullptr : configs_.back();
     for (uint32_t i = 0; i < networks.size(); ++i) {
-      PortConfiguration* config = NULL;
-      if (configs_.size() > 0)
-        config = configs_.back();
-
       uint32_t sequence_flags = flags();
       if ((sequence_flags & DISABLE_ALL_PHASES) == DISABLE_ALL_PHASES) {
         // If all the ports are disabled we should just fire the allocation
@@ -573,6 +617,19 @@ void BasicPortAllocatorSession::OnCandidateReady(
     return;
   }
 
+  // Mark that the port has a pairable candidate, either because we have a
+  // usable candidate from the port, or simply because the port is bound to the
+  // any address and therefore has no host candidate. This will trigger the port
+  // to start creating candidate pairs (connections) and issue connectivity
+  // checks. If port has already been marked as having a pairable candidate,
+  // do nothing here.
+  // Note: Need to do MaybeSignalCandidateReady after this because we will
+  // check whether the candidate is generated by the port in use.
+  if (!data->has_pairable_candidate() && CandidatePairable(c, port)) {
+    data->set_has_pairable_candidate(true);
+    MaybeSignalPortReady(port);

[pthatcher1, 2016/06/27 20:35:55]

If this is only called once, can we just inline it?

[honghaiz3, 2016/06/28 01:49:25]

A separate method will make it more readable here. 
Plus it allows early return in the method to be called. 

+  }
+
   ProtocolType pvalue;
   bool candidate_protocol_enabled =
       StringToProto(c.protocol().c_str(), &pvalue) &&
@@ -581,26 +638,72 @@ void BasicPortAllocatorSession::OnCandidateReady(
   if (CheckCandidateFilter(c) && candidate_protocol_enabled) {
     std::vector<Candidate> candidates;
     candidates.push_back(SanitizeRelatedAddress(c));
-    SignalCandidatesReady(this, candidates);
+    MaybeSignalCandidatesReady(port, candidates);

[pthatcher1, 2016/06/27 20:35:54]

Since this is only called once, can you inline it?

[honghaiz3, 2016/06/28 01:49:25]

Done.

   }
+}
 
-  // Port has already been marked as having a pairable candidate.
-  // Nothing to do here.
-  if (data->has_pairable_candidate()) {
+void BasicPortAllocatorSession::MaybeSignalPortReady(Port* port) {
+  if (!allocator_->disable_low_priority_turn_ports() ||
+      port->Type() != RELAY_PORT_TYPE) {
+    SignalPortReady(this, port);
     return;
   }
-
-  // Mark that the port has a pairable candidate, either because we have a
-  // usable candidate from the port, or simply because the port is bound to the
-  // any address and therefore has no host candidate. This will trigger the port
-  // to start creating candidate pairs (connections) and issue connectivity
-  // checks.
-  if (CandidatePairable(c, port)) {
-    data->set_has_pairable_candidate(true);
+  std::vector<Port*>& turn_ports =
+      turn_ports_in_use_by_network_names_[port->Network()->name()];

[pthatcher1, 2016/06/27 20:35:55]

Shouldn't we do a find() just in case it's not in the map?

[honghaiz3, 2016/06/28 01:49:25]

If it is not in the map, we create a new vector and add it to the map. 
That's what we want. 

+
+  bool use_port = true;
+  auto iter = turn_ports.begin();
+  while (iter != turn_ports.end()) {
+    int cmp = ComparePort(port, *iter);
+    if (cmp > 0) {
+      // The new port has higher precedence. Deprecate the existing port.
+      SignalPortDeprecated(this, *iter);
+      iter = turn_ports.erase(iter);
+      // TODO(honghaiz): Maybe also remove the candidates in the port.
+    } else {
+      ++iter;
+      if (cmp < 0) {
+        // If this new port has a lower precedence than any existing port, do
+        // not use it.
+        use_port = false;
+      }
+    }
+  }

[pthatcher1, 2016/06/27 20:35:54]

Correct me if I'm wrong, but it looks like turn_ports are always equal.  In that
case, you don't need to check all of them.  Just check the first. So all you
have to do is something like this:

int cmp = ComparePorts(port, turn_ports[0]);
if (cmp > 0) {
  // Ignore the port;  It's worse than ones we already have.
  return;  
} 
if (cmp < 0) {
  // Stop using all the rest;  They are worse than the new one we have.
  for (Port* deprecated_port : turn_ports) {
    SignalPortDeprecated(this, deprecated_port);
  }
  turn_ports.Clear();
}
turn_ports.push_back(port);
SignalPortReady(port);

[honghaiz3, 2016/06/28 01:49:25]

You are right, although we have removed the map now. 

+  if (use_port) {

[pthatcher1, 2016/06/27 20:35:55]

Actually, if we just store all ports in ports_ and mark them deprecated, then
this would be much more simple.  Just add this one, then use std::max to find
the best relay port.  Then, go through ports_ and mark all relay ports worse
than the best one as deprecated. 

[honghaiz3, 2016/06/28 01:49:25]

Done. Did not use std::max as it is not really much more easier than find the
max directly. 

+    turn_ports.push_back(port);
     SignalPortReady(this, port);

[pthatcher1, 2016/06/27 20:35:54]

I think we need a better pair of names than
SignalPortReady/SignalPortDeprecated.  How about
SignalPortPairable/SignalPortUnpairable?

[honghaiz3, 2016/06/28 01:49:25]

SignalPortReady is used elsewhere by downstream code. 
Changing this will break the other usage. 
Should we keep it (until we want to change the name everywhere) and perhaps just
change the name of the newly added signal?

   }
 }
 
+void BasicPortAllocatorSession::MaybeSignalCandidatesReady(
+    Port* port,
+    const std::vector<Candidate>& candidates) {
+  if (IsPortInUse(port)) {
+    SignalCandidatesReady(this, candidates);
+  }
+}
+
+bool BasicPortAllocatorSession::IsPortInUse(Port* port) const {
+  // If the port is not disabling low priority ports, every port is in use.
+  if (!allocator_->disable_low_priority_turn_ports()) {
+    return true;
+  }
+  // If it is not a turn port, it is in use.
+  if (port->Type() != RELAY_PORT_TYPE) {
+    return true;
+  }
+  // For TURN port, if it can be found in |turn_ports_in_use_by_network_name_|,
+  // it is in use.
+  const auto iter =
+      turn_ports_in_use_by_network_names_.find(port->Network()->name());
+  if (iter == turn_ports_in_use_by_network_names_.end()) {
+    return false;
+  }
+  const std::vector<Port*>& turn_ports = iter->second;
+  return std::find(turn_ports.begin(), turn_ports.end(), port) !=
+         turn_ports.end();

[pthatcher1, 2016/06/27 20:35:54]

If we store all port in ports_ and just mark them deprecated, then this would be
as simple as finding the port in ports_ and then looking at the state.   

[honghaiz3, 2016/06/28 01:49:26]

Done.

+}
+
 void BasicPortAllocatorSession::OnPortComplete(Port* port) {
   ASSERT(rtc::Thread::Current() == network_thread_);
   PortData* data = FindPort(port);
@@ -1028,13 +1131,11 @@ void AllocationSequence::CreateRelayPorts() {
     return;
   }
 
-  PortConfiguration::RelayList::const_iterator relay;
-  for (relay = config_->relays.begin();
-       relay != config_->relays.end(); ++relay) {
-    if (relay->type == RELAY_GTURN) {
-      CreateGturnPort(*relay);
-    } else if (relay->type == RELAY_TURN) {
-      CreateTurnPort(*relay);
+  for (RelayServerConfig& relay : config_->relays) {
+    if (relay.type == RELAY_GTURN) {
+      CreateGturnPort(relay);
+    } else if (relay.type == RELAY_TURN) {
+      CreateTurnPort(relay);
     } else {
       ASSERT(false);
     }
@@ -1084,6 +1185,18 @@ void AllocationSequence::CreateTurnPort(const RelayServerConfig& config) {
       continue;
     }
 
+    // Do not create the port if the server address family is known and does
+    // not match the local ip address family.
+    int server_ip_family = relay_port->address.ipaddr().family();
+    int local_ip_family = network_->GetBestIP().family();
+    if (server_ip_family != AF_UNSPEC && server_ip_family != local_ip_family) {
+      LOG(LS_INFO) << "Server and local address families are not compatible. "
+                   << "Server address: "
+                   << relay_port->address.ipaddr().ToString()
+                   << " Local address: " << network_->GetBestIP().ToString();
+      continue;
+    }

[pthatcher1, 2016/06/27 20:35:54]

Should this be a separate CL?  It seems like a bug fix.  

[honghaiz3, 2016/06/28 01:49:26]

Done.

+
     // Shared socket mode must be enabled only for UDP based ports. Hence
     // don't pass shared socket for ports which will create TCP sockets.
     // TODO(mallinath) - Enable shared socket mode for TURN ports. Disabled