Add network cost as part of the connection comparison.

webrtc/p2p/base/p2ptransportchannel.cc

 /*
  *  Copyright 2004 The WebRTC Project Authors. All rights reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 21 matching lines @@
 cricket::PortInterface::CandidateOrigin GetOrigin(cricket::PortInterface* port,
                                          cricket::PortInterface* origin_port) {
   if (!origin_port)
     return cricket::PortInterface::ORIGIN_MESSAGE;
   else if (port == origin_port)
     return cricket::PortInterface::ORIGIN_THIS_PORT;
   else
     return cricket::PortInterface::ORIGIN_OTHER_PORT;
 }
 
-// Compares two connections based only on static information about them.
+// Compares two connections based only on the candidate and network information.
+// Returns positive if |a| is better than |b|.
 int CompareConnectionCandidates(cricket::Connection* a,
                                 cricket::Connection* b) {
+  int a_cost = a->ComputeNetworkCost();
+  int b_cost = b->ComputeNetworkCost();
+  // Smaller cost is better.
+  if (a_cost < b_cost) {

[juberti2, 2016/02/04 23:31:10]

Can simply do

return (b_cost - a_cost);

[honghaiz3, 2016/02/05 01:36:46]

Not really because in the case that they are equal, it should not return but
continue to the next step. 
I thought about doing something like
cost_diff = a->ComputeNetworkCost() - b->ComputeNetworkCost()
if (cost_diff != 0) return cost_diff.
This is slightly simpler but easier to make mistake about the sign. 

+    return 1;
+  }
+  if (a_cost > b_cost) {
+    return -1;
+  }
+
   // Compare connection priority. Lower values get sorted last.
   if (a->priority() > b->priority())
     return 1;
   if (a->priority() < b->priority())
     return -1;
 
   // If we're still tied at this point, prefer a younger generation.
   return (a->remote_candidate().generation() + a->port()->generation()) -
          (b->remote_candidate().generation() + b->port()->generation());
 }
@@ skipping 490 matching lines @@
 
   Candidate remote_candidate;
   bool remote_candidate_is_new = (candidate == nullptr);
   if (!remote_candidate_is_new) {
     remote_candidate = *candidate;
     if (ufrag_per_port) {
       remote_candidate.set_address(address);
     }
   } else {
     // Create a new candidate with this address.
-    int remote_candidate_priority;
-
     // The priority of the candidate is set to the PRIORITY attribute
     // from the request.
     const StunUInt32Attribute* priority_attr =
         stun_msg->GetUInt32(STUN_ATTR_PRIORITY);
     if (!priority_attr) {
       LOG(LS_WARNING) << "P2PTransportChannel::OnUnknownAddress - "
                       << "No STUN_ATTR_PRIORITY found in the "
                       << "stun request message";
       port->SendBindingErrorResponse(stun_msg, address, STUN_ERROR_BAD_REQUEST,
                                      STUN_ERROR_REASON_BAD_REQUEST);
       return;
     }
-    remote_candidate_priority = priority_attr->value();
+    int remote_candidate_priority = priority_attr->value();
+
+    const StunUInt32Attribute* cost_attr =
+        stun_msg->GetUInt32(STUN_ATTR_NETWORK_COST);
+    uint32_t network_cost = (cost_attr) ? cost_attr->value() : 0;

[pthatcher1, 2016/02/04 23:16:20]

Hmm... I didn't think about peer-reflexive candidates.  That's a good point.  We
do need an attribute for that case.   It's kind of sad that it will make the
packets larger.  I guess at least we can avoid sending it if it's zero/default. 

[juberti2, 2016/02/04 23:31:10]

It could also be inferred from the base, although that breaks down when we don't
signal any candidates at all.

[honghaiz3, 2016/02/05 01:36:46]

Guess we'd better have a reliable way to get it in any circumstance. 

 
     // RFC 5245
     // If the source transport address of the request does not match any
     // existing remote candidates, it represents a new peer reflexive remote
     // candidate.
     remote_candidate = Candidate(component(), ProtoToString(proto), address, 0,
                                  remote_username, remote_password,
                                  PRFLX_PORT_TYPE, remote_generation, "");
 
     // From RFC 5245, section-7.2.1.3:
     // The foundation of the candidate is set to an arbitrary value, different
     // from the foundation for all other remote candidates.
     remote_candidate.set_foundation(
         rtc::ToString<uint32_t>(rtc::ComputeCrc32(remote_candidate.id())));
-
     remote_candidate.set_priority(remote_candidate_priority);
+    remote_candidate.set_network_cost(network_cost);
   }
 
   // RFC5245, the agent constructs a pair whose local candidate is equal to
   // the transport address on which the STUN request was received, and a
   // remote candidate equal to the source transport address where the
   // request came from.
 
   // There shouldn't be an existing connection with this remote address.
   // When ports are muxed, this channel might get multiple unknown address
   // signals. In that case if the connection is already exists, we should
@@ skipping 708 matching lines @@
 //      a.3) the best connection is unwritable OR
 //      a.4) |conn| has higher priority than best_connection.
 //    b) we're doing LITE ICE AND
 //      b.1) |conn| is the best_connection AND
 //      b.2) |conn| is writable.
 void P2PTransportChannel::PingConnection(Connection* conn) {
   bool use_candidate = false;
   if (remote_ice_mode_ == ICEMODE_FULL && ice_role_ == ICEROLE_CONTROLLING) {
     use_candidate = (conn == best_connection_) || (best_connection_ == NULL) ||
                     (!best_connection_->writable()) ||
-                    (conn->priority() > best_connection_->priority());
+                    (CompareConnectionCandidates(best_connection_, conn) < 0);
   } else if (remote_ice_mode_ == ICEMODE_LITE && conn == best_connection_) {
     use_candidate = best_connection_->writable();
   }
   conn->set_use_candidate_attr(use_candidate);
   last_ping_sent_ms_ = rtc::Time();
   conn->Ping(last_ping_sent_ms_);
 }
 
 // When a connection's state changes, we need to figure out who to use as
 // the best connection again.  It could have become usable, or become unusable.
@@ skipping 105 matching lines @@
   SignalSentPacket(this, sent_packet);
 }
 
 void P2PTransportChannel::OnReadyToSend(Connection* connection) {
   if (connection == best_connection_ && writable()) {
     SignalReadyToSend(this);
   }
 }
 
 }  // namespace cricket