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 84 matching lines @@
 
   // 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
@@ skipping 87 matching lines @@
 }  // 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;
 
-// 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
 
 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 41 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;
 }
 
+// Determines whether we should switch the selected connection to
+// |new_connection| based the writable state, the last nominated connection,
+// and the last connection receiving data. This is to prevent that the
+// controlled side may switch the selected connection too frequently when
+// the controlling side is doing aggressive nominations. With this
+// implementation, the controlled side will switch the selected connection
+// if the new nominated connection is writable, and
+// i) the selected connection is nullptr, or
+// ii) the selected connection was not nominated, or
+// iii) the new connection is nominated and is receiving data, or
+// iv) the new connection has lower cost or high priority.
+// TODO(honghaiz): Stop the aggressive nomination on the controlling side and
+// implement the ice-renomination option.
+bool P2PTransportChannel::ShouldSwitchOnNominatedOrDataReceived(
+    cricket::Connection* new_connection) const {
+  RTC_CHECK(new_connection != nullptr);
+  if (!new_connection->writable()) {
+    return false;
+  }
+  if (selected_connection_ == nullptr || !selected_connection_->nominated()) {
+    return true;
+  }

[pthatcher1, 2016/06/17 00:27:11]

As mentioned, I think this is more clear and correct:

if (best_connection == nullptr) {
  return true;
}

if (connection->nominated() != best_connection->nominated()) {
  return connection->nominated();
}

[honghaiz3, 2016/06/17 19:18:18]

Done.

+  if (new_connection == last_nominated_connection_ &&

[pthatcher1, 2016/06/17 00:27:11]

I think we shouldn't change the behavior based on which candidate pair was
nominated last, because that's essentially random and will make this behave like
it currently does with the bug (switches too much).

[honghaiz3, 2016/06/17 19:18:18]

Done.

+      new_connection == last_receiving_connection_) {
+    return true;
+  }

[pthatcher1, 2016/06/17 00:27:11]

As mentioned, I think this is more robust:

if (connection->last_data_received() != best_connection->last_data_received()) {
  return connection->last_data_received() >
best_connection->last_received_data();
}

[honghaiz3, 2016/06/17 19:18:18]

Done.

+  // Lastly, compare the network cost and priority.
+  return CompareConnectionCandidates(selected_connection_, new_connection) < 0;
+}
+
 void P2PTransportChannel::SetIceRole(IceRole ice_role) {
   ASSERT(worker_thread_ == rtc::Thread::Current());
   if (ice_role_ != ice_role) {
     ice_role_ = ice_role;
     for (std::vector<PortInterface *>::iterator it = ports_.begin();
          it != ports_.end(); ++it) {
       (*it)->SetIceRole(ice_role);
     }
   }
 }
@@ skipping 408 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;
   }
+
+  last_nominated_connection_ = conn;
+  if (!ShouldSwitchOnNominatedOrDataReceived(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
+  // 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 226 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 49 matching lines @@
   std::stable_sort(connections_.begin(), connections_.end(), cmp);
   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 (ShouldSwitch(selected_connection_, top_connection, ice_role_)) {
+    LOG(LS_INFO) << "Switching selected connection after sorting: "

[pthatcher1, 2016/06/17 00:27:11]

Can we please unify the two ShouldSwitch methods into one?

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

Done.

+                 << 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()) {
+  // 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 still problematic because with aggressive
+  // nomination, the controlling side will nominate every connection until it
+  // becomes writable.
+  if (ice_role_ == ICEROLE_CONTROLLING || selected_nominated_connection()) {
     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;
+Connection* P2PTransportChannel::selected_nominated_connection() const {
+  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
   // reconnecting a TCP connection and temporarily do not prune connections in
   // this network. See the big comment in CompareConnections.
 
   // 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();
+// Track 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
+  // TODO(honghaiz): rename selected_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)
       return connections_[i];
   }
 
   return NULL;
 }
 
@@ skipping 16 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 19 matching lines @@
   if (IsBackupConnection(conn)) {
     return (now >= conn->last_ping_response_received() +
                        config_.backup_connection_ping_interval);
   }
   return conn->active();
 }
 
 // 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);
   }
   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
+  // Update the selected connection if the state change is from last nominated
   // 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);
-    }
+  if (ice_role_ == ICEROLE_CONTROLLED &&
+      connection == last_nominated_connection_ &&
+      connection != selected_connection_ &&
+      ShouldSwitchOnNominatedOrDataReceived(connection)) {
+    LOG(LS_INFO) << "Switching selected connection on controlled side"
+                 << " because it's now writable: " << connection->ToString();
+    SwitchSelectedConnection(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 38 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 || selected_connection_ == connection) {
+    return;
+  }
+
+  last_receiving_connection_ = connection;
+  if (ShouldSwitchOnNominatedOrDataReceived(connection)) {
+    LOG(LS_INFO)
+        << "Switching selected connection on controlled side due to receiving: "
+        << 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