Add network cost as part of the connection comparison.

webrtc/p2p/base/port.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 177 matching lines @@
 
 void Port::Construct() {
   // TODO(pthatcher): Remove this old behavior once we're sure no one
   // relies on it.  If the username_fragment and password are empty,
   // we should just create one.
   if (ice_username_fragment_.empty()) {
     ASSERT(password_.empty());
     ice_username_fragment_ = rtc::CreateRandomString(ICE_UFRAG_LENGTH);
     password_ = rtc::CreateRandomString(ICE_PWD_LENGTH);
   }
+  // TODO(honghaiz): Make it configurable.
+  network_cost_ = (network_->type() == rtc::ADAPTER_TYPE_CELLULAR) ? 99 : 0;

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

Yeah, I definitely would prefer an enum, something like:

enum CandidateCost {
  CANDIDATE_COST_NONE = 0,  // use "none" in SDP
  CANDIDATE_COST_HIGH = 1, // use "high" in SDP
  CANDIDATE_COST_LAST_RESTORT = 2 // use "last-restort" in SDP
}

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

I don't really like the enum since it's not easily forward-extensible.

[pthatcher1, 2016/02/04 23:51:18]

The algorithm we have designed so far only works for certain know values: A/B/C,
or whatever we call them.  It's not designed for arbitrary cost values.  If we
choose A = 0 and C = 99 right now, we effectively have an enum, whether we call
it that or not.  If our algorithm works with specific values, I think it should
be an enum.  If it works with arbitrary values in a range, then we should make
it an integer.  But the later is not what we've designed so far.

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

Perhaps, we can use enum to define a few integer values which can be used. 
like enum CandidateCost {
  CANDIDATE_COST_NONE = 0,  
  CANDIDATE_COST_LOW = 2, 
  CANDIDATE_COST_MEDIUM = 4, 
  CANDIDATE_COST_HIGH = 10, 
  CANDIDATE_COST_MAX = 99 
}
Will leave this open depending on further discussion. 

[pthatcher1, 2016/02/05 01:42:35]

Thinking about this some more: I do now see merit in using the same thing in the
SDP and the STUN attr (an integer or a string in both), since the mapping is
much easier.  And an integer is clearly better for the STUN packets, so you're
right about that.  Let's make it an int.

+
   LOG_J(LS_INFO, this) << "Port created";
 }
 
 Port::~Port() {
   // Delete all of the remaining connections.  We copy the list up front
   // because each deletion will cause it to be modified.
 
   std::vector<Connection*> list;
 
   AddressMap::iterator iter = connections_.begin();
@@ skipping 35 matching lines @@
   c.set_protocol(protocol);
   c.set_relay_protocol(relay_protocol);
   c.set_tcptype(tcptype);
   c.set_address(address);
   c.set_priority(c.GetPriority(type_preference, network_->preference(),
                                relay_preference));
   c.set_username(username_fragment());
   c.set_password(password_);
   c.set_network_name(network_->name());
   c.set_network_type(network_->type());
+  c.set_network_cost(network_cost_);
   c.set_generation(generation_);
   c.set_related_address(related_address);
   c.set_foundation(
       ComputeFoundation(type, protocol, relay_protocol, base_address));
   candidates_.push_back(c);
   SignalCandidateReady(this, c);
 
   if (final) {
     SignalPortComplete(this);
   }
@@ skipping 422 matching lines @@
     request->AddAttribute(
         new StunByteStringAttribute(STUN_ATTR_USERNAME, username));
 
     // connection_ already holds this ping, so subtract one from count.
     if (connection_->port()->send_retransmit_count_attribute()) {
       request->AddAttribute(new StunUInt32Attribute(
           STUN_ATTR_RETRANSMIT_COUNT,
           static_cast<uint32_t>(connection_->pings_since_last_response_.size() -
                                 1)));
     }
+    uint32_t network_cost = connection_->port()->network_cost();
+    if (network_cost > 0) {
+      request->AddAttribute(
+          new StunUInt32Attribute(STUN_ATTR_NETWORK_COST, network_cost));
+    }
 
     // Adding ICE_CONTROLLED or ICE_CONTROLLING attribute based on the role.
     if (connection_->port()->GetIceRole() == ICEROLE_CONTROLLING) {
       request->AddAttribute(new StunUInt64Attribute(
           STUN_ATTR_ICE_CONTROLLING, connection_->port()->IceTiebreaker()));
       // Since we are trying aggressive nomination, sending USE-CANDIDATE
       // attribute in every ping.
       // If we are dealing with a ice-lite end point, nomination flag
       // in Connection will be set to false by default. Once the connection
       // becomes "best connection", nomination flag will be turned on.
@@ skipping 439 matching lines @@
   // networks would be up simultaneously but only for a brief period.
   return now > (time_created_ms_ + MIN_CONNECTION_LIFETIME);
 }
 
 std::string Connection::ToDebugId() const {
   std::stringstream ss;
   ss << std::hex << this;
   return ss.str();
 }
 
+int Connection::ComputeNetworkCost() const {

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

Everwhere you have "networkcost", please just make it "cost".  

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

Not sure about this. Network cost seems clearer than just 'cost', which seems
like an antonym for 'priority'. Maybe this should be 'network_priority'.

[pthatcher1, 2016/02/04 23:51:18]

I don't feel strongly one way or the other, I just want it consistent.  If it's
"network cost" then the SDP should have "network-cost", not just "cost".

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

When I put "cost" in the sdp, I intended to save a few bytes. Not sure if that
is super-critical though. 

[pthatcher1, 2016/02/12 00:07:12]

Saving bytes in SDP is of minor importance.  If someone wants to save bytes,
they shouldn't use SDP to start with.

Can you please just make it network-cost in the SDP to be consistent?

[honghaiz3, 2016/02/12 19:28:43]

Done.

+  // TODO(honghaiz): Will add rtt as part of the network cost.
+  return local_candidate().network_cost() + remote_candidate_.network_cost();
+}
+
 std::string Connection::ToString() const {
   const char CONNECT_STATE_ABBREV[2] = {
     '-',  // not connected (false)
     'C',  // connected (true)
   };
   const char RECEIVE_STATE_ABBREV[2] = {
     '-',  // not receiving (false)
     'R',  // receiving (true)
   };
   const char WRITE_STATE_ABBREV[4] = {
@@ skipping 218 matching lines @@
   new_local_candidate.set_id(id);
   new_local_candidate.set_component(local_candidate().component());
   new_local_candidate.set_type(PRFLX_PORT_TYPE);
   new_local_candidate.set_protocol(local_candidate().protocol());
   new_local_candidate.set_address(addr->GetAddress());
   new_local_candidate.set_priority(priority);
   new_local_candidate.set_username(local_candidate().username());
   new_local_candidate.set_password(local_candidate().password());
   new_local_candidate.set_network_name(local_candidate().network_name());
   new_local_candidate.set_network_type(local_candidate().network_type());
+  new_local_candidate.set_network_cost(local_candidate().network_cost());
   new_local_candidate.set_related_address(local_candidate().address());
   new_local_candidate.set_foundation(ComputeFoundation(
       PRFLX_PORT_TYPE, local_candidate().protocol(),
       local_candidate().relay_protocol(), local_candidate().address()));
 
   // Change the local candidate of this Connection to the new prflx candidate.
   local_candidate_index_ = port_->AddPrflxCandidate(new_local_candidate);
 
   // SignalStateChange to force a re-sort in P2PTransportChannel as this
   // Connection's local candidate has changed.
@@ skipping 18 matching lines @@
     ASSERT(sent < 0);
     error_ = port_->GetError();
     sent_packets_discarded_++;
   } else {
     send_rate_tracker_.AddSamples(sent);
   }
   return sent;
 }
 
 }  // namespace cricket