Do not switch best connection on the controlled side too frequently

webrtc/p2p/base/p2ptransportchannel.cc

Index: webrtc/p2p/base/p2ptransportchannel.cc
diff --git a/webrtc/p2p/base/p2ptransportchannel.cc b/webrtc/p2p/base/p2ptransportchannel.cc
index d83726eab5a5b53c1af9edbda62f93907ac5e22c..df8ec913df76c1897f52f5755bde2d2110392bdb 100644
--- a/webrtc/p2p/base/p2ptransportchannel.cc
+++ b/webrtc/p2p/base/p2ptransportchannel.cc
@@ -53,8 +53,8 @@ cricket::PortInterface::CandidateOrigin GetOrigin(cricket::PortInterface* port,
 
 // 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 CompareConnectionCandidates(const cricket::Connection* a,
+                                const cricket::Connection* b) {
   uint32_t a_cost = a->ComputeNetworkCost();
   uint32_t b_cost = b->ComputeNetworkCost();
   // Smaller cost is better.
@@ -78,7 +78,8 @@ int CompareConnectionCandidates(cricket::Connection* a,
 
 // Compare two connections based on their writing, receiving, and connected
 // states.
-int CompareConnectionStates(cricket::Connection* a, cricket::Connection* b) {
+int CompareConnectionStates(const cricket::Connection* a,
+                            const cricket::Connection* b) {
   // Sort based on write-state.  Better states have lower values.
   if (a->write_state() < b->write_state())
     return 1;
@@ -102,7 +103,7 @@ int CompareConnectionStates(cricket::Connection* a, cricket::Connection* b) {
   // side connects.  At that point, there are two TCP connections on the passive
   // side: 1. the old, disconnected one that is pretending to be writable, and
   // 2.  the new, connected one that is maybe not yet writable.  For purposes of
-  // pruning, pinging, and selecting the best connection, we want to treat the
+  // pruning, pinging, and selecting the connection to use, we want to treat the
   // new connection as "better" than the old one.  We could add a method called
   // something like Connection::ImReallyBadEvenThoughImWritable, but that is
   // equivalent to the existing Connection::connected(), which we already have.
@@ -127,79 +128,6 @@ int CompareConnectionStates(cricket::Connection* a, cricket::Connection* b) {
   return 0;
 }
 
-int CompareConnections(cricket::Connection* a, cricket::Connection* b) {
-  int state_cmp = CompareConnectionStates(a, b);
-  if (state_cmp != 0) {
-    return state_cmp;
-  }
-  // Compare the candidate information.
-  return CompareConnectionCandidates(a, b);
-}
-
-// Wraps the comparison connection into a less than operator that puts higher
-// priority writable connections first.
-class ConnectionCompare {
- public:
-  bool operator()(const cricket::Connection *ca,
-                  const cricket::Connection *cb) {
-    cricket::Connection* a = const_cast<cricket::Connection*>(ca);
-    cricket::Connection* b = const_cast<cricket::Connection*>(cb);
-
-    // Compare first on writability and static preferences.
-    int cmp = CompareConnections(a, b);
-    if (cmp > 0)
-      return true;
-    if (cmp < 0)
-      return false;
-
-    // Otherwise, sort based on latency estimate.
-    return a->rtt() < b->rtt();
-
-    // Should we bother checking for the last connection that last received
-    // data? It would help rendezvous on the connection that is also receiving
-    // packets.
-    //
-    // TODO: Yes we should definitely do this.  The TCP protocol gains
-    // efficiency by being used bidirectionally, as opposed to two separate
-    // unidirectional streams.  This test should probably occur before
-    // comparison of local prefs (assuming combined prefs are the same).  We
-    // need to be careful though, not to bounce back and forth with both sides
-    // trying to rendevous with the other.
-  }
-};
-
-// Determines whether we should switch between two connections, based first on
-// connection states, static preferences, and then (if those are equal) on
-// latency estimates.
-bool ShouldSwitch(cricket::Connection* a_conn,
-                  cricket::Connection* b_conn,
-                  cricket::IceRole ice_role) {
-  if (a_conn == b_conn)
-    return false;
-
-  if (!a_conn || !b_conn)  // don't think the latter should happen
-    return true;
-
-  // We prefer to switch to a writable and receiving connection over a
-  // non-writable or non-receiving connection, even if the latter has
-  // been nominated by the controlling side.
-  int state_cmp = CompareConnectionStates(a_conn, b_conn);
-  if (state_cmp != 0) {
-    return state_cmp < 0;
-  }
-  if (ice_role == cricket::ICEROLE_CONTROLLED && a_conn->nominated()) {
-    LOG(LS_VERBOSE) << "Controlled side did not switch due to nominated status";
-    return false;
-  }
-
-  int prefs_cmp = CompareConnectionCandidates(a_conn, b_conn);
-  if (prefs_cmp != 0) {
-    return prefs_cmp < 0;
-  }
-
-  return b_conn->rtt() <= a_conn->rtt() + kMinImprovement;
-}
-
 }  // unnamed namespace
 
 namespace cricket {
@@ -210,16 +138,16 @@ namespace cricket {
 // well on a 28.8K modem, which is the slowest connection on which the voice
 // quality is reasonable at all.
 static const int PING_PACKET_SIZE = 60 * 8;
-// STRONG_PING_INTERVAL (480ms) is applied when the best connection is both
+// STRONG_PING_INTERVAL (480ms) is applied when the selected connection is both
 // writable and receiving.
 static const int STRONG_PING_INTERVAL = 1000 * PING_PACKET_SIZE / 1000;
-// WEAK_PING_INTERVAL (48ms) is applied when the best connection is either not
-// writable or not receiving.
+// WEAK_PING_INTERVAL (48ms) is applied when the selected connection is either
+// not writable or not receiving.
 const int WEAK_PING_INTERVAL = 1000 * PING_PACKET_SIZE / 10000;
 
-// If the current best connection is both writable and receiving, then we will
-// also try hard to make sure it is pinged at this rate (a little less than
-// 2 * STRONG_PING_INTERVAL).
+// If the current selected connection is both writable and receiving, then we
+// will also try hard to make sure it is pinged at this rate (a little less
+// than 2 * STRONG_PING_INTERVAL).
 static const int MAX_CURRENT_STRONG_INTERVAL = 900;  // ms
 
 static const int MIN_CHECK_RECEIVING_INTERVAL = 50;  // ms
@@ -238,8 +166,6 @@ P2PTransportChannel::P2PTransportChannel(const std::string& transport_name,
       worker_thread_(rtc::Thread::Current()),
       incoming_only_(false),
       error_(0),
-      best_connection_(NULL),
-      pending_best_connection_(NULL),
       sort_dirty_(false),
       remote_ice_mode_(ICEMODE_FULL),
       ice_role_(ICEROLE_UNKNOWN),
@@ -301,6 +227,75 @@ void P2PTransportChannel::AddConnection(Connection* connection) {
   had_connection_ = true;
 }
 
+int P2PTransportChannel::CompareConnections(

[pthatcher1, 2016/06/21 07:16:53]

Can you call this CompareConnectionIncludingStates?

[honghaiz3, 2016/06/22 08:03:16]

Done.

+    const cricket::Connection* a,
+    const cricket::Connection* b) const {
+  RTC_CHECK(a != nullptr);
+  RTC_CHECK(b != nullptr);
+
+  // We prefer to switch to a writable and receiving connection over a
+  // non-writable or non-receiving connection, even if the latter has
+  // been nominated by the controlling side.
+  int state_cmp = CompareConnectionStates(a, b);
+  if (state_cmp != 0) {
+    return state_cmp;
+  }
+  return CompareConnectionsBase(a, b);
+}
+
+int P2PTransportChannel::CompareConnectionsBase(

[pthatcher1, 2016/06/21 07:16:53]

Can you call this CompareConnectionsExcludingStates?

[honghaiz3, 2016/06/22 08:03:16]

Done.

+    const cricket::Connection* a,
+    const cricket::Connection* b) const {
+  if (ice_role_ == cricket::ICEROLE_CONTROLLED) {
+    // Compare the connections based on the nomination states and the last data
+    // received time if this is on the controlled side.
+    if (a->nominated() != b->nominated()) {
+      return b->nominated() ? -1 : 1;
+    }
+
+    if (a->last_data_received() > b->last_data_received()) {

[Taylor Brandstetter, 2016/06/21 18:33:25]

What happens if the controlled side stops sending data (call is put on hold)?
Then we would be stuck with one connection, right?

[pthatcher1, 2016/06/22 06:36:34]

We could still switch to a higher priority candidate pair, but generally, yes,
you're right.  And that's exactly what the RFC says to do.

And it's exactly why our follow-up to this is to implement the renomination
draft: so we can safely switch selected candidate pairs without relying on media
being sent. 

[honghaiz3, 2016/06/22 08:03:16]

If the controlling side stops sending data, the controlled side won't switch
until writable/receiving state changes. We discussed this and felt it might be
OK to have  asymmetric path if the controlling side is not sending data. 

[honghaiz3, 2016/06/22 08:03:16]

Acknowledged.

+      return 1;
+    }
+    if (a->last_data_received() < b->last_data_received()) {
+      return -1;
+    }
+  }
+
+  // Compare the network cost and priority.
+  return CompareConnectionCandidates(a, b);
+}
+
+// Determines whether we should switch the selected connection to
+// |new_connection| based the writable/receiving state, the nomination state,
+// and the last data received time. This prevents the controlled side from
+// switching the selected connection too frequently when the controlling side
+// is doing aggressive nominations. The precedence of the connection switching
+// criteria is as follows:
+// i) write/receiving/connected states
+// ii) For controlled side,
+//        a) nomination state,
+//        b) last data received time.
+// iii) Lower cost / higher priority.
+// iv) rtt.
+// TODO(honghaiz): Stop the aggressive nomination on the controlling side and
+// implement the ice-renomination option.
+bool P2PTransportChannel::ShouldSwitchConnection(

[pthatcher1, 2016/06/21 07:16:53]

Can you call this ShouldSwitchSelectedConnection?

[honghaiz3, 2016/06/22 08:03:16]

Done.

+    cricket::Connection* new_connection) const {
+  RTC_CHECK(new_connection != nullptr);
+  RTC_DCHECK(selected_connection_ != new_connection);

[pthatcher1, 2016/06/21 07:16:52]

I think it would simply be better to do this:

if (!new_connection || selcted_connection_ ==  new_connection) {
  return false;
}

[honghaiz3, 2016/06/22 08:03:16]

Done.

+
+  if (selected_connection_ == nullptr) {
+    return true;
+  }
+  int cmp = CompareConnections(selected_connection_, new_connection);
+  if (cmp != 0) {
+    return cmp < 0;
+  }
+
+  // Lastly, switch only if rtt has improved by a margin.

[Taylor Brandstetter, 2016/06/21 18:33:25]

Odd to say "lastly" if there's no "firstly". Maybe above there should be a
comment saying "First, compare based on write state, and candidates".

[pthatcher1, 2016/06/22 06:36:34]

I think that's what the big comment before the method says.

[honghaiz3, 2016/06/22 08:03:16]

Acknowledged.

[honghaiz3, 2016/06/22 08:03:16]

Revised the comments. 

+  return new_connection->rtt() <= selected_connection_->rtt() + kMinImprovement;
+}
+
 void P2PTransportChannel::SetIceRole(IceRole ice_role) {
   ASSERT(worker_thread_ == rtc::Thread::Current());
   if (ice_role_ != ice_role) {
@@ -729,21 +724,24 @@ void P2PTransportChannel::OnNominated(Connection* conn) {
   ASSERT(worker_thread_ == rtc::Thread::Current());
   ASSERT(ice_role_ == ICEROLE_CONTROLLED);
 
-  if (conn->write_state() == Connection::STATE_WRITABLE) {
-    if (best_connection_ != conn) {
-      pending_best_connection_ = NULL;
-      LOG(LS_INFO) << "Switching best connection on controlled side: "
-                   << conn->ToString();
-      SwitchBestConnectionTo(conn);
-      // Now we have selected the best connection, time to prune other existing
-      // connections and update the read/write state of the channel.
-      RequestSort();
-    }
-  } else {
-    LOG(LS_INFO) << "Not switching the best connection on controlled side yet,"
-                 << " because it's not writable: " << conn->ToString();
-    pending_best_connection_ = conn;
+  if (selected_connection_ == conn) {
+    return;
+  }
+
+  if (!ShouldSwitchConnection(conn)) {
+    LOG(LS_INFO)
+        << "Not switching the selected connection on controlled side yet: "
+        << conn->ToString();
+    return;
   }
+
+  LOG(LS_INFO)
+      << "Switching selected connection on controlled side due to nomination: "
+      << conn->ToString();
+  SwitchSelectedConnection(conn);
+  // Now we have selected the selected connection, time to prune other existing

[Taylor Brandstetter, 2016/06/21 18:33:25]

"Now that we have selected a connection, it's time to prune..."

[honghaiz3, 2016/06/22 08:03:16]

Done.

+  // connections and update the read/write state of the channel.
+  RequestSort();
 }
 
 void P2PTransportChannel::AddRemoteCandidate(const Candidate& candidate) {
@@ -990,7 +988,7 @@ bool P2PTransportChannel::GetOption(rtc::Socket::Option opt, int* value) {
   return true;
 }
 
-// Send data to the other side, using our best connection.
+// Send data to the other side, using our selected connection.
 int P2PTransportChannel::SendPacket(const char *data, size_t len,
                                     const rtc::PacketOptions& options,
                                     int flags) {
@@ -999,16 +997,16 @@ int P2PTransportChannel::SendPacket(const char *data, size_t len,
     error_ = EINVAL;
     return -1;
   }
-  if (best_connection_ == NULL) {
+  if (selected_connection_ == NULL) {
     error_ = EWOULDBLOCK;
     return -1;
   }
 
   last_sent_packet_id_ = options.packet_id;
-  int sent = best_connection_->Send(data, len, options);
+  int sent = selected_connection_->Send(data, len, options);
   if (sent <= 0) {
     ASSERT(sent < 0);
-    error_ = best_connection_->GetError();
+    error_ = selected_connection_->GetError();
   }
   return sent;
 }
@@ -1020,7 +1018,7 @@ bool P2PTransportChannel::GetStats(ConnectionInfos *infos) {
 
   for (Connection* connection : connections_) {
     ConnectionInfo info = connection->stats();
-    info.best_connection = (best_connection_ == connection);
+    info.best_connection = (selected_connection_ == connection);
     info.receiving = connection->receiving();
     info.writable = (connection->write_state() == Connection::STATE_WRITABLE);
     info.timeout =
@@ -1079,8 +1077,17 @@ void P2PTransportChannel::SortConnections() {
   // 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);
+  std::stable_sort(
+      connections_.begin(), connections_.end(),
+      [this](const cricket::Connection* a, const cricket::Connection* b) {
+        int cmp = CompareConnections(a, b);
+        if (cmp != 0) {
+          return cmp > 0;
+        }
+
+        // Otherwise, sort based on latency estimate.
+        return a->rtt() < b->rtt();
+      });
   LOG(LS_VERBOSE) << "Sorting " << connections_.size()
                   << " available connections:";
   for (size_t i = 0; i < connections_.size(); ++i) {
@@ -1090,18 +1097,25 @@ void P2PTransportChannel::SortConnections() {
   Connection* top_connection =
       (connections_.size() > 0) ? connections_[0] : nullptr;
 
-  // If necessary, switch to the new choice.
-  // Note that |top_connection| doesn't have to be writable to become the best
-  // connection although it will have higher priority if it is writable.
-  if (ShouldSwitch(best_connection_, top_connection, ice_role_)) {
-    LOG(LS_INFO) << "Switching best connection: " << top_connection->ToString();
-    SwitchBestConnectionTo(top_connection);
-  }
-
-  // Controlled side can prune only if the best connection has been nominated.
-  // because otherwise it may delete the connection that will be selected by
-  // the controlling side.
-  if (ice_role_ == ICEROLE_CONTROLLING || best_nominated_connection()) {
+  // If necessary, switch to the new choice. Note that |top_connection| doesn't
+  // have to be writable to become the selected connection although it will
+  // have higher priority if it is writable.
+  if (top_connection != selected_connection_ &&
+      ShouldSwitchConnection(top_connection)) {

[pthatcher1, 2016/06/21 07:16:53]

Can you move this line into ShouldSwitchSelectedConnection?

if (selected_connection_ == connection) {
  return false;
}

And then this would simplify to:

if (ShouldSwitchSelectedConnection(top_connection) {
  ...
}

[honghaiz3, 2016/06/22 08:03:16]

Done.

+    LOG(LS_INFO) << "Switching selected connection after sorting: "
+                 << top_connection->ToString();
+    SwitchSelectedConnection(top_connection);
+  }
+
+  // Controlled side can prune only if the selected connection has been

[pthatcher1, 2016/06/21 07:16:53]

Controlled side => The controlled side

[honghaiz3, 2016/06/22 08:03:16]

Done.

+  // nominated because otherwise it may prune the connection that will be
+  // selected by the controlling side.
+  // TODO(honghaiz): This is not enough to prevent a connection from being
+  // pruned too early because with aggressive nomination, the controlling side
+  // will nominate every connection until it becomes writable. What we do now
+  // is to un-prune a connection if a ping request comes with a nomination, or
+  // new data is received on a nominated connection.
+  if (ice_role_ == ICEROLE_CONTROLLING || selected_nominated_connection()) {
     PruneConnections();
   }
 
@@ -1125,16 +1139,17 @@ void P2PTransportChannel::SortConnections() {
   UpdateState();
 }
 
-Connection* P2PTransportChannel::best_nominated_connection() const {
-  return (best_connection_ && best_connection_->nominated()) ? best_connection_
-                                                             : nullptr;
+Connection* P2PTransportChannel::selected_nominated_connection() const {

[pthatcher1, 2016/06/21 07:16:53]

Since this is only called once, I suggest we inline it as:

if (ice_role_ == ICEROLE_CONTROLLING || (selected_connection_ &&
selected_connection->nominated())) {
  PruneConnections();
}

[honghaiz3, 2016/06/22 08:03:16]

Done.

+  return (selected_connection_ && selected_connection_->nominated())
+             ? selected_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
+  // which point, we would prune out the current selected 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 |premier| connection is not connected, we may be
@@ -1148,49 +1163,49 @@ void P2PTransportChannel::PruneConnections() {
   }
   for (rtc::Network* network : networks) {
     Connection* premier = GetBestConnectionOnNetwork(network);
-    // Do not prune connections if the current best connection is weak on this
-    // network. Otherwise, it may delete connections prematurely.
+    // Do not prune connections if the current selected connection is weak on
+    // this network. Otherwise, it may delete connections prematurely.
     if (!premier || premier->weak()) {
       continue;
     }
 
     for (Connection* conn : connections_) {
       if ((conn != premier) && (conn->port()->Network() == network) &&
-          (CompareConnectionCandidates(premier, conn) >= 0)) {
+          (CompareConnectionsBase(premier, conn) >= 0)) {

[Taylor Brandstetter, 2016/06/21 18:33:25]

Why can't you just use CompareConnections here? Isn't |premier| guaranteed to
have a better or equal write state than |conn|, since connections are always
sorted by the time this function is called?

Also, this change means that the controlled side will prune connections based on
which last received packets... This seems wrong to me. The RTT could change,
making the controlling side choose a different connection. Or a race condition
could result in the wrong connection being pruned when ICE is first starting.

[pthatcher1, 2016/06/22 06:36:33]

I think this is so we don't prune a better candidate pair just because it's not
writable yet.  Otherwise, the first writable candidate pair would always prune
of all the other candidate pairs, even if we'd rather have one of the others
once they become writable.
Good catch!  Perhaps instead of having CompareConnections and
CompareConnectionsBase, we should just have CompareConnections for sorting and
then have a separate method just for ShouldPrune(conn, premier), which should
take into account the writable and nominated state of premier, but not the
writable and nominated state of conn, and not the receiving state of either.  

And if don't have a unit test that checks that we don't prune a connection until
a better one is writable and nominated, then we really should add one.

[honghaiz3, 2016/06/22 08:03:16]

Done. I just use the CompareConnectionCandidates to determine whether we should
prune.  The same as before. 

[honghaiz3, 2016/06/22 08:03:16]

CompareConnections (now CompareConnectionsIncludingStates) will first compare
the write/receiving/connected states. Basically you don't want a high-priority
connection is pruned simply because it does not writable yet. Pruning it means
setting it write_timeout state. It may not get a chance to be pinged again. 

Yes it could happen if a connection received data later it may get pruned. But
we have the issue of incorrectly pruning connections any way, see my comments in
SortConnections (near the end). To mitigate this, once a connection received
data or stun ping with nomination, we un-prune it. 

[Taylor Brandstetter, 2016/06/22 16:12:22]

Ah yes, that's obvious now, my mistake.

         conn->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: "
-                           << old_best_connection->ToString();
+// Track the selected connection, and let listeners know.

[Taylor Brandstetter, 2016/06/21 18:33:25]

Maybe "Change the" instead of "Track the".

[honghaiz3, 2016/06/22 08:03:16]

Done.

+void P2PTransportChannel::SwitchSelectedConnection(Connection* conn) {
+  // Note: if conn is NULL, the previous |selected_connection_| has been
+  // destroyed, so don't use it.
+  Connection* old_selected_connection = selected_connection_;
+  selected_connection_ = conn;
+  if (selected_connection_) {
+    if (old_selected_connection) {
+      LOG_J(LS_INFO, this) << "Previous selected connection: "
+                           << old_selected_connection->ToString();
     }
-    LOG_J(LS_INFO, this) << "New best connection: "
-                         << best_connection_->ToString();
-    SignalRouteChange(this, best_connection_->remote_candidate());
+    LOG_J(LS_INFO, this) << "New selected connection: "
+                         << selected_connection_->ToString();
+    SignalRouteChange(this, selected_connection_->remote_candidate());
     // This is a temporary, but safe fix to webrtc issue 5705.
     // TODO(honghaiz): Make all EWOULDBLOCK error routed through the transport
     // channel so that it knows whether the media channel is allowed to
     // send; then it will only signal ready-to-send if the media channel
     // has been disallowed to send.
-    if (best_connection_->writable()) {
+    if (selected_connection_->writable()) {
       SignalReadyToSend(this);
     }
   } else {
-    LOG_J(LS_INFO, this) << "No best connection";
+    LOG_J(LS_INFO, this) << "No selected connection";
   }
-  // TODO(honghaiz): rename best_connection_ with selected_connection_ or
+  // TODO(honghaiz): rename selected_connection_ with selected_connection_ or
   // selected_candidate pair_.
-  SignalSelectedCandidatePairChanged(this, best_connection_,
+  SignalSelectedCandidatePairChanged(this, selected_connection_,
                                      last_sent_packet_id_);
 }
 
@@ -1237,7 +1252,7 @@ void P2PTransportChannel::UpdateState() {
     SignalStateChanged(this);
   }
 
-  bool writable = best_connection_ && best_connection_->writable();
+  bool writable = selected_connection_ && selected_connection_->writable();
   set_writable(writable);
 
   bool receiving = false;
@@ -1277,16 +1292,17 @@ void P2PTransportChannel::HandleAllTimedOut() {
 }
 
 bool P2PTransportChannel::weak() const {
-  return !best_connection_ || best_connection_->weak();
+  return !selected_connection_ || selected_connection_->weak();
 }
 
-// If we have a best connection, return it, otherwise return top one in the
+// If we have a selected connection, return it, otherwise return top one in the
 // list (later we will mark it best).
 Connection* P2PTransportChannel::GetBestConnectionOnNetwork(
     rtc::Network* network) const {
-  // If the best connection is on this network, then it wins.
-  if (best_connection_ && (best_connection_->port()->Network() == network))
-    return best_connection_;
+  // If the selected connection is on this network, then it wins.
+  if (selected_connection_ &&
+      (selected_connection_->port()->Network() == network))

[Taylor Brandstetter, 2016/06/21 18:33:25]

Nit: As long as you're changing this code, can you add {}s

[honghaiz3, 2016/06/22 08:03:16]

Done.

+    return selected_connection_;
 
   // Otherwise, we return the top-most in sorted order.
   for (size_t i = 0; i < connections_.size(); ++i) {
@@ -1323,8 +1339,9 @@ void P2PTransportChannel::OnCheckAndPing() {
   // Make sure the states of the connections are up-to-date (since this affects
   // which ones are pingable).
   UpdateConnectionStates();
-  // When the best connection is not receiving or not writable, or any active
-  // connection has not been pinged enough times, use the weak ping interval.
+  // When the selected connection is not receiving or not writable, or any
+  // active connection has not been pinged enough times, use the weak ping
+  // interval.
   bool need_more_pings_at_weak_interval = std::any_of(
       connections_.begin(), connections_.end(), [](Connection* conn) {
         return conn->active() &&
@@ -1345,9 +1362,9 @@ void P2PTransportChannel::OnCheckAndPing() {
 }
 
 // A connection is considered a backup connection if the channel state
-// is completed, the connection is not the best connection and it is active.
+// is completed, the connection is not the selected connection and it is active.
 bool P2PTransportChannel::IsBackupConnection(Connection* conn) const {
-  return state_ == STATE_COMPLETED && conn != best_connection_ &&
+  return state_ == STATE_COMPLETED && conn != selected_connection_ &&
          conn->active();
 }
 
@@ -1387,18 +1404,18 @@ bool P2PTransportChannel::IsPingable(Connection* conn, int64_t now) {
 // Returns the next pingable connection to ping.  This will be the oldest
 // pingable connection unless we have a connected, writable connection that is
 // past the maximum acceptable ping interval. When reconnecting a TCP
-// connection, the best connection is disconnected, although still WRITABLE
+// connection, the selected connection is disconnected, although still WRITABLE
 // while reconnecting. The newly created connection should be selected as the
 // ping target to become writable instead. See the big comment in
 // CompareConnections.
 Connection* P2PTransportChannel::FindNextPingableConnection() {
   int64_t now = rtc::TimeMillis();
   Connection* conn_to_ping = nullptr;
-  if (best_connection_ && best_connection_->connected() &&
-      best_connection_->writable() &&
-      (best_connection_->last_ping_sent() + config_.max_strong_interval <=
+  if (selected_connection_ && selected_connection_->connected() &&
+      selected_connection_->writable() &&
+      (selected_connection_->last_ping_sent() + config_.max_strong_interval <=
        now)) {
-    conn_to_ping = best_connection_;
+    conn_to_ping = selected_connection_;
   } else {
     conn_to_ping = FindConnectionToPing(now);
   }
@@ -1416,21 +1433,22 @@ void P2PTransportChannel::MarkConnectionPinged(Connection* conn) {
 // explained below.
 // Set USE-CANDIDATE if doing ICE AND this channel is in CONTROLLING AND
 //    a) Channel is in FULL ICE AND
-//      a.1) |conn| is the best connection OR
-//      a.2) there is no best connection OR
-//      a.3) the best connection is unwritable OR
-//      a.4) |conn| has higher priority than best_connection.
+//      a.1) |conn| is the selected connection OR
+//      a.2) there is no selected connection OR
+//      a.3) the selected connection is unwritable OR
+//      a.4) |conn| has higher priority than selected_connection.
 //    b) we're doing LITE ICE AND
-//      b.1) |conn| is the best_connection AND
+//      b.1) |conn| is the selected_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()) ||
-                    (CompareConnectionCandidates(best_connection_, conn) < 0);
-  } else if (remote_ice_mode_ == ICEMODE_LITE && conn == best_connection_) {
-    use_candidate = best_connection_->writable();
+    use_candidate =
+        (conn == selected_connection_) || (selected_connection_ == NULL) ||
+        (!selected_connection_->writable()) ||
+        (CompareConnectionCandidates(selected_connection_, conn) < 0);
+  } else if (remote_ice_mode_ == ICEMODE_LITE && conn == selected_connection_) {
+    use_candidate = selected_connection_->writable();
   }
   conn->set_use_candidate_attr(use_candidate);
   last_ping_sent_ms_ = rtc::TimeMillis();
@@ -1438,21 +1456,11 @@ void P2PTransportChannel::PingConnection(Connection* conn) {
 }
 
 // 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.
+// the selected connection again.  It could have become usable, or become
+// unusable.
 void P2PTransportChannel::OnConnectionStateChange(Connection* connection) {
   ASSERT(worker_thread_ == rtc::Thread::Current());
 
-  // Update the best connection if the state change is from pending best
-  // connection and role is controlled.
-  if (ice_role_ == ICEROLE_CONTROLLED) {
-    if (connection == pending_best_connection_ && connection->writable()) {
-      pending_best_connection_ = NULL;
-      LOG(LS_INFO) << "Switching best connection on controlled side"
-                   << " because it's now writable: " << connection->ToString();
-      SwitchBestConnectionTo(connection);
-    }
-  }
-
   // May stop the allocator session when at least one connection becomes
   // strongly connected after starting to get ports and the local candidate of
   // the connection is at the latest generation. It is not enough to check
@@ -1475,7 +1483,7 @@ void P2PTransportChannel::OnConnectionStateChange(Connection* connection) {
 void P2PTransportChannel::OnConnectionDestroyed(Connection* connection) {
   ASSERT(worker_thread_ == rtc::Thread::Current());
 
-  // Note: the previous best_connection_ may be destroyed by now, so don't
+  // Note: the previous selected_connection_ may be destroyed by now, so don't
   // use it.
 
   // Remove this connection from the list.
@@ -1489,18 +1497,15 @@ void P2PTransportChannel::OnConnectionDestroyed(Connection* connection) {
   LOG_J(LS_INFO, this) << "Removed connection ("
     << static_cast<int>(connections_.size()) << " remaining)";
 
-  if (pending_best_connection_ == connection) {
-    pending_best_connection_ = NULL;
-  }
-
-  // If this is currently the best connection, then we need to pick a new one.
-  // The call to SortConnections will pick a new one.  It looks at the current
-  // best connection in order to avoid switching between fairly similar ones.
-  // Since this connection is no longer an option, we can just set best to NULL
-  // and re-choose a best assuming that there was no best connection.
-  if (best_connection_ == connection) {
-    LOG(LS_INFO) << "Best connection destroyed.  Will choose a new one.";
-    SwitchBestConnectionTo(NULL);
+  // If this is currently the selected connection, then we need to pick a new
+  // one. The call to SortConnections will pick a new one.  It looks at the
+  // current selected connection in order to avoid switching between fairly
+  // similar ones. Since this connection is no longer an option, we can just
+  // set selected to nullptr and re-choose a best assuming that there was no
+  // selected connection.
+  if (selected_connection_ == connection) {
+    LOG(LS_INFO) << "selected connection destroyed.  Will choose a new one.";
+    SwitchSelectedConnection(nullptr);
     RequestSort();
   }
 
@@ -1559,9 +1564,15 @@ void P2PTransportChannel::OnReadPacket(Connection* connection,
 
   // May need to switch the sending connection based on the receiving media path
   // if this is the controlled side.
-  if (ice_role_ == ICEROLE_CONTROLLED && !best_nominated_connection() &&
-      connection->writable() && best_connection_ != connection) {
-    SwitchBestConnectionTo(connection);
+  if (ice_role_ != ICEROLE_CONTROLLED || selected_connection_ == connection) {
+    return;
+  }
+
+  if (ShouldSwitchConnection(connection)) {

[pthatcher1, 2016/06/21 07:16:53]

Can you move this line into ShouldSwitchConnection?

if (selected_connection_ == connection) {
  return false;
}

And then this would simplify to:

if (ice_role_ == ICEROLE_CONTROLLED &&
ShouldSwitchSelectedConnection(connection)) {
  ...
}

[honghaiz3, 2016/06/22 08:03:16]

Done.

+    LOG(LS_INFO)
+        << "Switching selected connection on controlled side due to receiving: "
+        << connection->ToString();
+    SwitchSelectedConnection(connection);
   }
 }
 
@@ -1572,7 +1583,7 @@ void P2PTransportChannel::OnSentPacket(const rtc::SentPacket& sent_packet) {
 }
 
 void P2PTransportChannel::OnReadyToSend(Connection* connection) {
-  if (connection == best_connection_ && writable()) {
+  if (connection == selected_connection_ && writable()) {
     SignalReadyToSend(this);
   }
 }