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;
 
-// 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;
 }
 
+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) {
     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;
   }
+
+  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) {
   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 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 (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 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

[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();
   }
 
   // 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 {

[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
   // 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)) {
+          (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_);
 }
 
 // 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))

[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) {
     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
-  // 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 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;
+  }
+
+  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);
   }
 }
 
 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