Fixing problems with ICE candidate pair prioritization.

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 758 matching lines @@
     } else {
       ASSERT(false);
     }
 
     // Adding PRIORITY Attribute.
     // Changing the type preference to Peer Reflexive and local preference
     // and component id information is unchanged from the original priority.
     // priority = (2^24)*(type preference) +
     //           (2^8)*(local preference) +
     //           (2^0)*(256 - component ID)
+    uint32_t type_preference =
+        (connection_->local_candidate().protocol() == TCP_PROTOCOL_NAME)
+            ? ICE_TYPE_PREFERENCE_PRFLX_TCP
+            : ICE_TYPE_PREFERENCE_PRFLX;

[pthatcher1, 2016/08/08 22:20:30]

It seems like it would make sense to make this fix into its own CL.  What do you
think?

     uint32_t prflx_priority =
-        ICE_TYPE_PREFERENCE_PRFLX << 24 |
+        type_preference << 24 |
         (connection_->local_candidate().priority() & 0x00FFFFFF);
     request->AddAttribute(
         new StunUInt32Attribute(STUN_ATTR_PRIORITY, prflx_priority));
 
     // Adding Message Integrity attribute.
     request->AddMessageIntegrity(connection_->remote_candidate().password());
     // Adding Fingerprint.
     request->AddFingerprint();
   }
 
@@ skipping 527 matching lines @@
     LOG_JV(sev, this) << "Received STUN ping response"
                       << ", id=" << rtc::hex_encode(request->id())
                       << ", code=0"  // Makes logging easier to parse.
                       << ", rtt=" << rtt
                       << ", use_candidate=" << use_candidate
                       << ", pings_since_last_response=" << pings;
   }
 
   stats_.recv_ping_responses++;
 
-  MaybeAddPrflxCandidate(request, response);
+  MaybeUpdateLocalCandidate(request, response);
 }
 
 void Connection::OnConnectionRequestErrorResponse(ConnectionRequest* request,
                                                   StunMessage* response) {
   const StunErrorCodeAttribute* error_attr = response->GetErrorCode();
   int error_code = STUN_ERROR_GLOBAL_FAILURE;
   if (error_attr) {
     error_code = error_attr->code();
   }
 
@@ skipping 88 matching lines @@
 }
 
 ConnectionInfo Connection::stats() {
   stats_.recv_bytes_second = round(recv_rate_tracker_.ComputeRate());
   stats_.recv_total_bytes = recv_rate_tracker_.TotalSampleCount();
   stats_.sent_bytes_second = round(send_rate_tracker_.ComputeRate());
   stats_.sent_total_bytes = send_rate_tracker_.TotalSampleCount();
   return stats_;
 }
 
-void Connection::MaybeAddPrflxCandidate(ConnectionRequest* request,
-                                        StunMessage* response) {
+void Connection::MaybeUpdateLocalCandidate(ConnectionRequest* request,
+                                           StunMessage* response) {
   // RFC 5245
   // The agent checks the mapped address from the STUN response.  If the
   // transport address does not match any of the local candidates that the
   // agent knows about, the mapped address represents a new candidate -- a
   // peer reflexive candidate.
   const StunAddressAttribute* addr =
       response->GetAddress(STUN_ATTR_XOR_MAPPED_ADDRESS);
   if (!addr) {
     LOG(LS_WARNING) << "Connection::OnConnectionRequestResponse - "
                     << "No MAPPED-ADDRESS or XOR-MAPPED-ADDRESS found in the "
                     << "stun response message";
     return;
   }
 
-  bool known_addr = false;
   for (size_t i = 0; i < port_->Candidates().size(); ++i) {
     if (port_->Candidates()[i].address() == addr->GetAddress()) {
-      known_addr = true;
-      break;
+      if (local_candidate_index_ != i) {
+        LOG_J(LS_INFO, this) << "Updating local candidate type to srflx.";
+        local_candidate_index_ = i;
+        // SignalStateChange to force a re-sort in P2PTransportChannel as this
+        // Connection's local candidate has changed.
+        SignalStateChange(this);
+      }
+      return;
     }
   }
-  if (known_addr) {
-    return;
-  }
 
   // RFC 5245
   // Its priority is set equal to the value of the PRIORITY attribute
   // in the Binding request.
   const StunUInt32Attribute* priority_attr =
       request->msg()->GetUInt32(STUN_ATTR_PRIORITY);
   if (!priority_attr) {
     LOG(LS_WARNING) << "Connection::OnConnectionRequestResponse - "
                     << "No STUN_ATTR_PRIORITY found in the "
                     << "stun response message";
@@ skipping 15 matching lines @@
   new_local_candidate.set_network_type(local_candidate().network_type());
   new_local_candidate.set_related_address(local_candidate().address());
   new_local_candidate.set_generation(local_candidate().generation());
   new_local_candidate.set_foundation(ComputeFoundation(
       PRFLX_PORT_TYPE, local_candidate().protocol(),
       local_candidate().relay_protocol(), local_candidate().address()));
   new_local_candidate.set_network_id(local_candidate().network_id());
   new_local_candidate.set_network_cost(local_candidate().network_cost());
 
   // Change the local candidate of this Connection to the new prflx candidate.
+  LOG_J(LS_INFO, this) << "Updating local candidate type to prflx.";
   local_candidate_index_ = port_->AddPrflxCandidate(new_local_candidate);
 
   // SignalStateChange to force a re-sort in P2PTransportChannel as this
   // Connection's local candidate has changed.
   SignalStateChange(this);
 }
 
 bool Connection::rtt_converged() const {
   return rtt_samples_ > (RTT_RATIO + 1);
 }
@@ skipping 21 matching lines @@
     ASSERT(sent < 0);
     error_ = port_->GetError();
     stats_.sent_discarded_packets++;
   } else {
     send_rate_tracker_.AddSamples(sent);
   }
   return sent;
 }
 
 }  // namespace cricket