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);
         return;
       }
     }
 
     Connection* connection = port->CreateConnection(
         remote_candidate, cricket::PortInterface::ORIGIN_THIS_PORT);
     if (!connection) {
       ASSERT(false);
       port->SendBindingErrorResponse(stun_msg, address,
                                      STUN_ERROR_SERVER_ERROR,
                                      STUN_ERROR_REASON_SERVER_ERROR);
       return;
     }
+    bool received_use_candidate =
+        stun_msg->GetByteString(STUN_ATTR_USE_CANDIDATE) != nullptr;
+    if (received_use_candidate && ice_role_ == ICEROLE_CONTROLLED &&
+        protocol_type_ == ICEPROTO_RFC5245) {
+      connection->set_received_use_candidate(true);

[pthatcher1, 2015/08/06 01:41:42]

I think we should call this set_nominated.

[honghaiz3, 2015/08/06 18:22:57]

Done.

+      // Since the connection is not writable yet, it will only set the
+      // pending_best_connection_ there and will not incur double sorting.
+      OnUseCandidate(connection);

[pthatcher1, 2015/08/06 01:41:42]

We can call OnUseCandidate *before* calling AddConnection?  That seems
backwards.   I was thinking that instead of calling OnUseCandidate here, we
would have AddConnection do something like:

if (connection->nominated()) {
  OnUseCandidate(connection);
}

[honghaiz3, 2015/08/06 18:22:57]

I move this after AddConnection. Plus, hid the call to set_nominated() method. 

+    }
 
     LOG(LS_INFO) << "Adding connection from "
                  << (remote_candidate_is_new ? "peer reflexive" : "resurrected")
                  << " candidate: " << remote_candidate.ToString();
     AddConnection(connection);
     connection->ReceivedPing();
 
     // Send the pinger a successful stun response.
     port->SendBindingResponse(stun_msg, address);
 
@@ skipping 411 matching lines @@
   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.
-
   // If necessary, switch to the new choice.
-  if (protocol_type_ != ICEPROTO_RFC5245 || ice_role_ == ICEROLE_CONTROLLING) {
+  // As we are moving toward passive-aggressive nomination, a channel may be
+  // selected as the best connection even if it is using RFC5245 and its mode
+  // is CONTROLLED. Note that top_connection don't have to be writable to be
+  // switched to although it will have higher priority if it is writable.
+  if (protocol_type_ != ICEPROTO_RFC5245 || ice_role_ == ICEROLE_CONTROLLING ||
+      (ice_role_ == ICEROLE_CONTROLLED &&
+       !(best_connection_ && best_connection_->received_use_candidate()))) {

[pthatcher1, 2015/08/06 01:41:42]

I think a best_nominated_connection() == (best_connection_ &&
best_connection_->nominated()) ? best_connection_ : nullptr would be helpful
here.

Then this could just be:

if (protocol_type_ != ICEPROTO_RFC5245 || ice_role_ = ICEROLE_CONTROLLING ||
!best_nominated_connection()) {
  // ...
}

[honghaiz3, 2015/08/06 18:22:57]

Done.

     if (ShouldSwitch(best_connection_, top_connection)) {

[pthatcher1, 2015/08/06 01:41:42]

I think we should move all of this logic together into ShouldSwitch (checking
the role and if best_connection_ is nominated)

[honghaiz3, 2015/08/06 18:22:57]

ShouldSwitch is defined as a helper function outside of the class. 
Unless we want to change it to a member function of the class, we cannot check
the role there. I tried to change that to a member function and tried to add a
separate function for it. But at the end, I feel it is best to just leave it
there, especially if you see in the next lines we have had similar conditions
(if you put some tests in a separate function and some tests here, it does not
improve readability much.   

-      LOG(LS_INFO) << "Switching best connection on controlling side: "
+      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;
+  // 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
+  // connection that will be selected by the controlling side.
+  if (ice_role_ != ICEROLE_CONTROLLED ||
+      (best_connection_ && best_connection_->received_use_candidate())) {

[pthatcher1, 2015/08/06 01:41:42]

I think we want to make this 

if (best_connection_nominated()) {
  PruneLowerPriorityCandidates();
}

1.  Having a helper method would make this more readable.

2.  The controlling side should mark the best_connection_ as nominated when it
nominates it.  Then the logic is very easy to read.  

[honghaiz3, 2015/08/06 18:22:57]

Done. I think calling it PruneConnections is probably enough. When you prune
connections, you only prune low priority or equal priority ones. 

+    std::set<rtc::Network*>::iterator network;
+    for (network = networks.begin(); network != networks.end(); ++network) {
+      Connection* primier = GetBestConnectionOnNetwork(*network);
+      if (!(primier && primier->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();
+      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();
+        }
       }
     }
   }
 
   // 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;
@@ skipping 343 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 controlled side.
+  if (best_connection_ != connection && ice_role_ == ICEROLE_CONTROLLED &&
+      !(best_connection_ && best_connection_->received_use_candidate()) &&
+      connection->writable()) {

[pthatcher1, 2015/08/06 01:41:42]

I think if this would be easier to read:

if (ice_role_ == ICEROLE_CONTROLLED && connection !=
best_connection_nominated()) {
  SwitchBestConnectionTo(connection)
}

[honghaiz3, 2015/08/06 18:22:57]

That is different from what I put. 
If the connection is not writable but receiving data, we should not use it to
send. 
If the connection and best_connection is equal but the best_connection is not
nominated, we should not (need not) switch. 
I put the best_connection_ != connection as the first test as most of time this
will return false and shortcut the rest of the tests. 

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