First step of passive aggressive nomination.

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 547 matching lines @@
 
   if (port->IceProtocol() == ICEPROTO_RFC5245) {
     // 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
-    // simply ignore the signal othewise send server error.
+    // simply ignore the signal otherwise send server error.
     if (port->GetConnection(remote_candidate.address())) {
       if (port_muxed) {
         LOG(LS_INFO) << "Connection already exists for peer reflexive "
                      << "candidate: " << remote_candidate.ToString();
         return;
       } else {
         ASSERT(false);
         port->SendBindingErrorResponse(stun_msg, address,
                                        STUN_ERROR_SERVER_ERROR,
                                        STUN_ERROR_REASON_SERVER_ERROR);
@@ skipping 10 matching lines @@
                                      STUN_ERROR_REASON_SERVER_ERROR);
       return;
     }
 
     LOG(LS_INFO) << "Adding connection from "
                  << (remote_candidate_is_new ? "peer reflexive" : "resurrected")
                  << " candidate: " << remote_candidate.ToString();
     AddConnection(connection);
     connection->ReceivedPing();
 
+    bool received_use_candidate =
+        stun_msg->GetByteString(STUN_ATTR_USE_CANDIDATE) != nullptr;
+    if (received_use_candidate && ice_role_ == ICEROLE_CONTROLLED &&
+        protocol_type_ == ICEPROTO_RFC5245) {

[pthatcher1, 2015/08/07 22:50:22]

We are already in a "== ICEPROTO_RFC5245" block, so you can remove that.

[honghaiz3, 2015/08/10 20:15:43]

Done.

+      connection->set_nominated(true);
+      OnUseCandidate(connection);

[pthatcher1, 2015/08/07 22:50:22]

Firing OnUseCandidate after we call AddConnection is kind of nice.  

While you're there, do you think you could rename OnUseCandidate to be
OnNominated?  It would match so much better now.

[honghaiz3, 2015/08/10 20:15:42]

Done. Also changed the SignalUseCandidate to SignalNominated.

+    }
+
     // Send the pinger a successful stun response.
     port->SendBindingResponse(stun_msg, address);
 
     // Update the list of connections since we just added another.  We do this
     // after sending the response since it could (in principle) delete the
     // connection in question.
     SortConnections();
   } else {
     // Check for connectivity to this address. Create connections
     // to this address across all local ports. First, add this as a new remote
@@ skipping 383 matching lines @@
     // is no point in sorting the connections. In hybrid state, ports can have
     // different protocol than hybrid and protocol may differ from one another.
     // Instead just update the state of this channel
     UpdateChannelState();
     return;
   }
 
   // Any changes after this point will require a re-sort.
   sort_dirty_ = false;
 
-  // Get a list of the networks that we are using.
-  std::set<rtc::Network*> networks;
-  for (uint32 i = 0; i < connections_.size(); ++i)
-    networks.insert(connections_[i]->port()->Network());
-
   // Find the best alternative connection by sorting.  It is important to note
   // that amongst equal preference, writable connections, this will choose the
   // one whose estimated latency is lowest.  So it is the only one that we
   // need to consider switching to.
-
   ConnectionCompare cmp;
   std::stable_sort(connections_.begin(), connections_.end(), cmp);
   LOG(LS_VERBOSE) << "Sorting available connections:";
   for (uint32 i = 0; i < connections_.size(); ++i) {
     LOG(LS_VERBOSE) << connections_[i]->ToString();
   }
-
-  Connection* top_connection = NULL;
-  if (connections_.size() > 0)
-    top_connection = connections_[0];
-
-  // We don't want to pick the best connections if channel is using RFC5245
-  // and it's mode is CONTROLLED, as connections will be selected by the
-  // CONTROLLING agent.
+  Connection* top_connection =
+      (connections_.size() > 0) ? connections_[0] : nullptr;
 
   // If necessary, switch to the new choice.
-  if (protocol_type_ != ICEPROTO_RFC5245 || ice_role_ == ICEROLE_CONTROLLING) {
-    if (ShouldSwitch(best_connection_, top_connection)) {
-      LOG(LS_INFO) << "Switching best connection on controlling side: "
-                   << top_connection->ToString();
-      SwitchBestConnectionTo(top_connection);
-    }
+  // Note that top_connection doesn't have to be writable to be

[pthatcher1, 2015/08/07 22:50:22]

|top_connection|

[honghaiz3, 2015/08/10 20:15:43]

Done.

+  // switched to although it will have higher priority if it is writable.

[pthatcher1, 2015/08/07 22:50:22]

to be switched to => to become the best_connection_

[honghaiz3, 2015/08/10 20:15:42]

Done.

+  // Controlled side can switch the best connection only if the best connection
+  // was not nominated yet.

[pthatcher1, 2015/08/07 22:50:22]

Controlled side => The controlled side
if the best connection was not nominated yet => if the current
|best_connection_| has not been nominated by the controlling side.

[honghaiz3, 2015/08/10 20:15:43]

Done.

+  if ((protocol_type_ != ICEPROTO_RFC5245 || ice_role_ == ICEROLE_CONTROLLING ||
+       !best_nominated_connection()) &&
+      ShouldSwitch(best_connection_, top_connection)) {
+    LOG(LS_INFO) << "Switching best connection: " << top_connection->ToString();
+    SwitchBestConnectionTo(top_connection);
   }
 
-  // We can prune any connection for which there is a connected, writable
-  // connection on the same network with better or equal priority.  We leave
-  // those with better priority just in case they become writable later (at
-  // which point, we would prune out the current best connection).  We leave
-  // connections on other networks because they may not be using the same
-  // resources and they may represent very distinct paths over which we can
-  // switch. If the |primier| connection is not connected, we may be
-  // reconnecting a TCP connection and temporarily do not prune connections in
-  // this network. See the big comment in CompareConnections.
-  std::set<rtc::Network*>::iterator network;
-  for (network = networks.begin(); network != networks.end(); ++network) {
-    Connection* primier = GetBestConnectionOnNetwork(*network);
-    if (!primier || (primier->write_state() != Connection::STATE_WRITABLE) ||
-        !primier->connected())
-      continue;
-
-    for (uint32 i = 0; i < connections_.size(); ++i) {
-      if ((connections_[i] != primier) &&
-          (connections_[i]->port()->Network() == *network) &&
-          (CompareConnectionCandidates(primier, connections_[i]) >= 0)) {
-        connections_[i]->Prune();
-      }
-    }
+  // Controlled side can prune only if the best connection has been nominated.
+  if (ice_role_ == ICEROLE_CONTROLLING || best_nominated_connection()) {
+    PruneConnections();
   }
 
   // Check if all connections are timedout.
   bool all_connections_timedout = true;
   for (uint32 i = 0; i < connections_.size(); ++i) {
     if (connections_[i]->write_state() != Connection::STATE_WRITE_TIMEOUT) {
       all_connections_timedout = false;
       break;
     }
   }
 
   // Now update the writable state of the channel with the information we have
   // so far.
   if (best_connection_ && best_connection_->writable()) {
     HandleWritable();
   } else if (all_connections_timedout) {
     HandleAllTimedOut();
   } else {
     HandleNotWritable();
   }
 
   // Update the state of this channel.  This method is called whenever the
   // state of any connection changes, so this is a good place to do this.
   UpdateChannelState();
 }
 
+Connection* P2PTransportChannel::best_nominated_connection() const {
+  return (best_connection_ && best_connection_->nominated()) ? best_connection_
+                                                             : nullptr;
+}
+
+void P2PTransportChannel::PruneConnections() {
+  // We can prune any connection for which there is a connected, writable
+  // connection on the same network with better or equal priority.  We leave
+  // those with better priority just in case they become writable later (at
+  // which point, we would prune out the current best connection).  We leave
+  // connections on other networks because they may not be using the same
+  // resources and they may represent very distinct paths over which we can
+  // switch. If the |primier| connection is not connected, we may be
+  // reconnecting a TCP connection and temporarily do not prune connections in
+  // this network. See the big comment in CompareConnections.
+  // NOTE: If the ICE role is controlled and it has not received requests with
+  // use_candidate yet, do not prune the connections because it may delete the

[pthatcher1, 2015/08/07 22:50:22]

I'd change "it has not received requests with use_candidate yet" to "and the
controlling side has not nominated a connection yet", 

[honghaiz3, 2015/08/10 20:15:42]

Done.

+  // connection that will be selected by the controlling side.

[pthatcher1, 2015/08/07 22:50:22]

Shouldn't you move this NOTE to where we call PruneConnections, since that is
where the code to not prune is?

[honghaiz3, 2015/08/10 20:15:43]

Done. And merged with the comments there. 

+
+  // Get a list of the networks that we are using.
+  std::set<rtc::Network*> networks;
+  for (uint32 i = 0; i < connections_.size(); ++i)
+    networks.insert(connections_[i]->port()->Network());

[pthatcher1, 2015/08/07 22:50:22]

Might as well switch go c++11:

for (const Connection* conn : connections_) {
  networks.insert(conn->port()->Network());
}

[honghaiz3, 2015/08/10 20:15:42]

Done.

+  for (auto network : networks) {

[pthatcher1, 2015/08/07 22:50:22]

I'd prefer "for (const Network* network : networks)".

[honghaiz3, 2015/08/10 20:15:43]

Done.

+    Connection* primier = GetBestConnectionOnNetwork(network);
+    if (!(primier && primier->writable() && primier->connected()))
+      continue;

[pthatcher1, 2015/08/07 22:50:22]

Can you add some {}s around this?

[honghaiz3, 2015/08/10 20:15:42]

Done.

+
+    for (auto connection : connections_) {

[pthatcher1, 2015/08/07 22:50:22]

Same here:

for (Connection* conn : connections_) {
}

[honghaiz3, 2015/08/10 20:15:43]

Done.

+      if ((connection != primier) &&
+          (connection->port()->Network() == network) &&
+          (CompareConnectionCandidates(primier, connection) >= 0)) {
+        connection->Prune();
+      }
+    }
+  }
+}
 
 // Track the best connection, and let listeners know
 void P2PTransportChannel::SwitchBestConnectionTo(Connection* conn) {
   // Note: if conn is NULL, the previous best_connection_ has been destroyed,
   // so don't use it.
   Connection* old_best_connection = best_connection_;
   best_connection_ = conn;
   if (best_connection_) {
     if (old_best_connection) {
       LOG_J(LS_INFO, this) << "Previous best connection: "
@@ skipping 315 matching lines @@
     Connection *connection, const char *data, size_t len,
     const rtc::PacketTime& packet_time) {
   ASSERT(worker_thread_ == rtc::Thread::Current());
 
   // Do not deliver, if packet doesn't belong to the correct transport channel.
   if (!FindConnection(connection))
     return;
 
   // Let the client know of an incoming packet
   SignalReadPacket(this, data, len, packet_time, 0);
+
+  // May need to switch the sending connection based on the receiving media path
+  // if this is the controlled side.
+  if (best_connection_ != connection && ice_role_ == ICEROLE_CONTROLLED &&
+      !best_nominated_connection() && connection->writable()) {

[pthatcher1, 2015/08/07 22:50:22]

I think the order would be a little more clear as:

if (ice_role_ == ICEROLE_CONTROLLED && 
    !best_nominated_connection() &&
    connection->writable() &&
    best_connection_ != connection) {
}

[honghaiz3, 2015/08/10 20:15:42]

Done. I put it in that order because I thought in most cases, best_connection_
will be equal to connection so the rest tests need not be checked. But it looks
like that the readability is more important. 

+    SwitchBestConnectionTo(connection);
+  }
 }
 
 void P2PTransportChannel::OnReadyToSend(Connection* connection) {
   if (connection == best_connection_ && writable()) {
     SignalReadyToSend(this);
   }
 }
 
 }  // namespace cricket