Do not switch best connection on the controlled side too frequently

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 35 matching lines @@
   if (!origin_port)
     return cricket::PortInterface::ORIGIN_MESSAGE;
   else if (port == origin_port)
     return cricket::PortInterface::ORIGIN_THIS_PORT;
   else
     return cricket::PortInterface::ORIGIN_OTHER_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.
   if (a_cost < b_cost) {
     return 1;
   }
   if (a_cost > b_cost) {
     return -1;
   }
 
   // Compare connection priority. Lower values get sorted last.
   if (a->priority() > b->priority())
     return 1;
   if (a->priority() < b->priority())
     return -1;
 
   // If we're still tied at this point, prefer a younger generation.
   return (a->remote_candidate().generation() + a->port()->generation()) -
          (b->remote_candidate().generation() + b->port()->generation());
 }
 
 // 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;
   if (a->write_state() > b->write_state())
     return -1;
 
   // We prefer a receiving connection to a non-receiving, higher-priority
   // connection when sorting connections and choosing which connection to
   // switch to.
   if (a->receiving() && !b->receiving())
     return 1;
   if (!a->receiving() && b->receiving())
     return -1;
 
   // WARNING: Some complexity here about TCP reconnecting.
   // When a TCP connection fails because of a TCP socket disconnecting, the
   // active side of the connection will attempt to reconnect for 5 seconds while
   // pretending to be writable (the connection is not set to the unwritable
   // state).  On the passive side, the connection also remains writable even
   // though it is disconnected, and a new connection is created when the active
   // 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.
   // So, in code throughout this file, we'll check whether the connection is
   // connected() or not, and if it is not, treat it as "worse" than a connected
   // one, even though it's writable.  In the code below, we're doing so to make
   // sure we treat a new writable connection as better than an old disconnected
   // connection.
 
   // In the case where we reconnect TCP connections, the original best
   // connection is disconnected without changing to WRITE_TIMEOUT. In this case,
   // the new connection, when it becomes writable, should have higher priority.
   if (a->write_state() == cricket::Connection::STATE_WRITABLE &&
       b->write_state() == cricket::Connection::STATE_WRITABLE) {
     if (a->connected() && !b->connected()) {
       return 1;
     }
     if (!a->connected() && b->connected()) {
       return -1;
     }
   }
   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 {
 
 // When the socket is unwritable, we will use 10 Kbps (ignoring IP+UDP headers)
 // for pinging.  When the socket is writable, we will use only 1 Kbps because
 // we don't want to degrade the quality on a modem.  These numbers should work
 // 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;
 
 // Writable connections are pinged at a faster rate while stabilizing.
 const int STABILIZING_WRITABLE_CONNECTION_PING_INTERVAL = 900;  // ms
 
 // Writable connections are pinged at a slower rate once stabilized.
 const int STABLE_WRITABLE_CONNECTION_PING_INTERVAL = 2500;  // ms
 
 static const int MIN_CHECK_RECEIVING_INTERVAL = 50;  // ms
 
 P2PTransportChannel::P2PTransportChannel(const std::string& transport_name,
                                          int component,
                                          P2PTransport* transport,
                                          PortAllocator* allocator)
     : P2PTransportChannel(transport_name, component, allocator) {}
 
 P2PTransportChannel::P2PTransportChannel(const std::string& transport_name,
                                          int component,
                                          PortAllocator* allocator)
     : TransportChannelImpl(transport_name, component),
       allocator_(allocator),
       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),
       tiebreaker_(0),
       gathering_state_(kIceGatheringNew),
       check_receiving_interval_(MIN_CHECK_RECEIVING_INTERVAL * 5),
       config_(MIN_CHECK_RECEIVING_INTERVAL * 50 /* receiving_timeout */,
               0 /* backup_connection_ping_interval */,
               false /* gather_continually */,
               false /* prioritize_most_likely_candidate_pairs */,
@@ skipping 42 matching lines @@
   connection->SignalReadyToSend.connect(
       this, &P2PTransportChannel::OnReadyToSend);
   connection->SignalStateChange.connect(
       this, &P2PTransportChannel::OnConnectionStateChange);
   connection->SignalDestroyed.connect(
       this, &P2PTransportChannel::OnConnectionDestroyed);
   connection->SignalNominated.connect(this, &P2PTransportChannel::OnNominated);
   had_connection_ = true;
 }
 
+int P2PTransportChannel::CompareConnections(
+    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;
+  }
+
+  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;
+    }

[pthatcher1, 2016/06/22 19:18:37]

Can you make this match the style Taylor had in his CL, like this?

if (a->nominated() && !b->nominated()) {
  return a_is_better;
}
if (!a->nominated() && b->nominated()) {
  return b_is_better;
}

[honghaiz3, 2016/06/22 20:01:23]

We should do this when we merge the two CLs, so we don't have to duplicate the
work now. If it is not fixed after merging, will do this in a separate CL. 

+
+    if (a->last_data_received() > b->last_data_received()) {
+      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::ShouldSwitchSelectedConnection(
+    cricket::Connection* new_connection) const {
+  if (!new_connection || selected_connection_ == new_connection) {
+    return false;
+  }
+
+  if (selected_connection_ == nullptr) {
+    return true;
+  }
+
+  int cmp = CompareConnections(selected_connection_, new_connection);
+  if (cmp != 0) {
+    return cmp < 0;
+  }
+
+  // If everything else is the same, switch only if rtt has improved by
+  // a margin.
+  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) {
     ice_role_ = ice_role;
     for (PortInterface* port : ports_) {
       port->SetIceRole(ice_role);
     }
     // Update role on removed ports as well, because they may still have
     // connections alive that should be using the correct role.
     for (PortInterface* port : removed_ports_) {
@@ skipping 414 matching lines @@
     return nullptr;
   }
   *generation = params.rend() - it - 1;
   return &(*it);
 }
 
 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 (!ShouldSwitchSelectedConnection(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 that we have selected a connection, it is time to prune other
+  // connections and update the read/write state of the channel.
+  RequestSort();
 }
 
 void P2PTransportChannel::AddRemoteCandidate(const Candidate& candidate) {
   ASSERT(worker_thread_ == rtc::Thread::Current());
 
   uint32_t generation = GetRemoteCandidateGeneration(candidate);
   // If a remote candidate with a previous generation arrives, drop it.
   if (generation < remote_ice_generation()) {
     LOG(LS_WARNING) << "Dropping a remote candidate because its ufrag "
                     << candidate.username()
@@ skipping 229 matching lines @@
   ASSERT(worker_thread_ == rtc::Thread::Current());
 
   const auto& found = options_.find(opt);
   if (found == options_.end()) {
     return false;
   }
   *value = found->second;
   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) {
   ASSERT(worker_thread_ == rtc::Thread::Current());
   if (flags != 0) {
     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;
 }
 
 bool P2PTransportChannel::GetStats(ConnectionInfos *infos) {
   ASSERT(worker_thread_ == rtc::Thread::Current());
   // Gather connection infos.
   infos->clear();
 
   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 =
         (connection->write_state() == Connection::STATE_WRITE_TIMEOUT);
     info.new_connection = !connection->reported();
     connection->set_reported(true);
     info.rtt = connection->rtt();
     info.local_candidate = connection->local_candidate();
     info.remote_candidate = connection->remote_candidate();
     info.key = connection;
@@ skipping 38 matching lines @@
   // will be sorted.
   UpdateConnectionStates();
 
   // Any changes after this point will require a re-sort.
   sort_dirty_ = false;
 
   // 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);
+  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) {
     LOG(LS_VERBOSE) << connections_[i]->ToString();
   }
 
   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);
+  // 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 (ShouldSwitchSelectedConnection(top_connection)) {
+    LOG(LS_INFO) << "Switching selected connection after sorting: "
+                 << top_connection->ToString();
+    SwitchSelectedConnection(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()) {
+  // The controlled side can prune only if the selected connection has been
+  // 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_connection_ && selected_connection_->nominated())) {
     PruneConnections();
   }
 
   // Check if all connections are timedout.
   bool all_connections_timedout = true;
   for (size_t 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 (all_connections_timedout) {
     HandleAllTimedOut();
   }
 
   // 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.
   UpdateState();
 }
 
-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
+  // 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
   // reconnecting a TCP connection and temporarily do not prune connections in
-  // this network. See the big comment in CompareConnections.
+  // this network. See the big comment in CompareConnectionStates.
 
   // Get a list of the networks that we are using.
   std::set<rtc::Network*> networks;
   for (const Connection* conn : connections_) {
     networks.insert(conn->port()->Network());
   }
   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)) {
         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();
+// Change the selected connection, and let listeners know.
+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
-  // selected_candidate pair_.
-  SignalSelectedCandidatePairChanged(this, best_connection_,
+  SignalSelectedCandidatePairChanged(this, selected_connection_,
                                      last_sent_packet_id_);
 }
 
 // Warning: UpdateState should eventually be called whenever a connection
 // is added, deleted, or the write state of any connection changes so that the
 // transport controller will get the up-to-date channel state. However it
 // should not be called too often; in the case that multiple connection states
 // change, it should be called after all the connection states have changed. For
 // example, we call this at the end of SortConnections.
 void P2PTransportChannel::UpdateState() {
@@ skipping 26 matching lines @@
         RTC_DCHECK(state == STATE_CONNECTING || state == STATE_COMPLETED);
         break;
       default:
         RTC_DCHECK(false);
         break;
     }
     state_ = state;
     SignalStateChanged(this);
   }
 
-  bool writable = best_connection_ && best_connection_->writable();
+  bool writable = selected_connection_ && selected_connection_->writable();
   set_writable(writable);
 
   bool receiving = false;
   for (const Connection* connection : connections_) {
     if (connection->receiving()) {
       receiving = true;
       break;
     }
   }
   set_receiving(receiving);
@@ skipping 19 matching lines @@
 }
 
 // If all connections timed out, delete them all.
 void P2PTransportChannel::HandleAllTimedOut() {
   for (Connection* connection : connections_) {
     connection->Destroy();
   }
 }
 
 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)) {
+    return selected_connection_;
+  }
 
   // Otherwise, we return the top-most in sorted order.
   for (size_t i = 0; i < connections_.size(); ++i) {
-    if (connections_[i]->port()->Network() == network)
+    if (connections_[i]->port()->Network() == network) {
       return connections_[i];
+    }
   }
 
   return NULL;
 }
 
 // Handle any queued up requests
 void P2PTransportChannel::OnMessage(rtc::Message *pmsg) {
   switch (pmsg->message_id) {
     case MSG_SORT:
       OnSort();
@@ skipping 11 matching lines @@
 void P2PTransportChannel::OnSort() {
   // Resort the connections based on the new statistics.
   SortConnections();
 }
 
 // Handle queued up check-and-ping request
 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() &&
                conn->num_pings_sent() < MIN_PINGS_AT_WEAK_PING_INTERVAL;
       });
   int ping_interval = (weak() || need_more_pings_at_weak_interval)
                           ? weak_ping_interval_
                           : STRONG_PING_INTERVAL;
   if (rtc::TimeMillis() >= last_ping_sent_ms_ + ping_interval) {
     Connection* conn = FindNextPingableConnection();
     if (conn) {
       PingConnection(conn);
       MarkConnectionPinged(conn);
     }
   }
   int delay = std::min(ping_interval, check_receiving_interval_);
   thread()->PostDelayed(RTC_FROM_HERE, delay, this, MSG_CHECK_AND_PING);
 }
 
 // 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();
 }
 
 // Is the connection in a state for us to even consider pinging the other side?
 // We consider a connection pingable even if it's not connected because that's
 // how a TCP connection is kicked into reconnecting on the active side.
 bool P2PTransportChannel::IsPingable(Connection* conn, int64_t now) {
   const Candidate& remote = conn->remote_candidate();
   // We should never get this far with an empty remote ufrag.
   ASSERT(!remote.username().empty());
@@ skipping 29 matching lines @@
   if (!conn->writable()) {
     return true;
   }
 
   // Ping writable, active connections if it's been long enough since the last
   // ping.
   int ping_interval = CalculateActiveWritablePingInterval(conn, now);
   return (now >= conn->last_ping_sent() + ping_interval);
 }
 
-bool P2PTransportChannel::IsBestConnectionPingable(int64_t now) {
-  if (!best_connection_ || !best_connection_->connected() ||
-      !best_connection_->writable()) {
+bool P2PTransportChannel::IsSelectedConnectionPingable(int64_t now) {
+  if (!selected_connection_ || !selected_connection_->connected() ||
+      !selected_connection_->writable()) {
     return false;
   }
 
-  int interval = CalculateActiveWritablePingInterval(best_connection_, now);
-  return best_connection_->last_ping_sent() + interval <= now;
+  int interval = CalculateActiveWritablePingInterval(selected_connection_, now);
+  return selected_connection_->last_ping_sent() + interval <= now;
 }
 
 int P2PTransportChannel::CalculateActiveWritablePingInterval(Connection* conn,
                                                              int64_t now) {
   // Ping each connection at a higher rate at least
   // MIN_PINGS_AT_WEAK_PING_INTERVAL times.
   if (conn->num_pings_sent() < MIN_PINGS_AT_WEAK_PING_INTERVAL) {
     return weak_ping_interval_;
   }
 
   int stable_interval = config_.stable_writable_connection_ping_interval;
   int stablizing_interval =
       std::min(stable_interval, STABILIZING_WRITABLE_CONNECTION_PING_INTERVAL);
 
   return conn->stable(now) ? stable_interval : stablizing_interval;
 }
 
 // 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 writable 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.
+// CompareConnectionStates.
 Connection* P2PTransportChannel::FindNextPingableConnection() {
   int64_t now = rtc::TimeMillis();
   Connection* conn_to_ping = nullptr;
-  if (IsBestConnectionPingable(now)) {
-    conn_to_ping = best_connection_;
+  if (IsSelectedConnectionPingable(now)) {
+    conn_to_ping = selected_connection_;
   } else {
     conn_to_ping = FindConnectionToPing(now);
   }
   return conn_to_ping;
 }
 
 void P2PTransportChannel::MarkConnectionPinged(Connection* conn) {
   if (conn && pinged_connections_.insert(conn).second) {
     unpinged_connections_.erase(conn);
   }
 }
 
 // Apart from sending ping from |conn| this method also updates
 // |use_candidate_attr| flag. The criteria to update this flag is
 // 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();
   conn->Ping(last_ping_sent_ms_);
 }
 
 // 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
   // that the connection becomes weakly connected because the connection may be
   // changing from (writable, receiving) to (writable, not receiving).
   bool strongly_connected = !connection->weak();
   bool latest_generation = connection->local_candidate().generation() >=
                            allocator_session()->generation();
   if (strongly_connected && latest_generation) {
     MaybeStopPortAllocatorSessions();
   }
 
   // We have to unroll the stack before doing this because we may be changing
   // the state of connections while sorting.
   RequestSort();
 }
 
 // When a connection is removed, edit it out, and then update our best
 // 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.
   std::vector<Connection*>::iterator iter =
       std::find(connections_.begin(), connections_.end(), connection);
   ASSERT(iter != connections_.end());
   pinged_connections_.erase(*iter);
   unpinged_connections_.erase(*iter);
   connections_.erase(iter);
 
   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();
   }
 
   UpdateState();
 }
 
 // When a port is destroyed remove it from our list of ports to use for
 // connection attempts.
 void P2PTransportChannel::OnPortDestroyed(PortInterface* port) {
   ASSERT(worker_thread_ == rtc::Thread::Current());
@@ skipping 39 matching lines @@
 
   // 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 (ice_role_ == ICEROLE_CONTROLLED && !best_nominated_connection() &&
-      connection->writable() && best_connection_ != connection) {
-    SwitchBestConnectionTo(connection);
+  if (ice_role_ == ICEROLE_CONTROLLED &&
+      ShouldSwitchSelectedConnection(connection)) {
+    LOG(LS_INFO) << "Switching selected connection on controlled side due to "
+                 << "data received: " << connection->ToString();
+    SwitchSelectedConnection(connection);
   }
 }
 
 void P2PTransportChannel::OnSentPacket(const rtc::SentPacket& sent_packet) {
   ASSERT(worker_thread_ == rtc::Thread::Current());
 
   SignalSentPacket(this, sent_packet);
 }
 
 void P2PTransportChannel::OnReadyToSend(Connection* connection) {
-  if (connection == best_connection_ && writable()) {
+  if (connection == selected_connection_ && writable()) {
     SignalReadyToSend(this);
   }
 }
 
 // Find "triggered checks".  We ping first those connections that have
 // received a ping but have not sent a ping since receiving it
 // (last_received_ping > last_sent_ping).  But we shouldn't do
 // triggered checks if the connection is already writable.
 Connection* P2PTransportChannel::FindOldestConnectionNeedingTriggeredCheck(
     int64_t now) {
@@ skipping 102 matching lines @@
 
   // During the initial state when nothing has been pinged yet, return the first
   // one in the ordered |connections_|.
   return *(std::find_if(connections_.begin(), connections_.end(),
                         [conn1, conn2](Connection* conn) {
                           return conn == conn1 || conn == conn2;
                         }));
 }
 
 }  // namespace cricket