Do not switch best connection on the controlled side too frequently

webrtc/p2p/base/p2ptransportchannel.cc

Index: webrtc/p2p/base/p2ptransportchannel.cc
diff --git a/webrtc/p2p/base/p2ptransportchannel.cc b/webrtc/p2p/base/p2ptransportchannel.cc
index d83726eab5a5b53c1af9edbda62f93907ac5e22c..9c27bf64681ffce9253a296266b32271fc06b265 100644
--- a/webrtc/p2p/base/p2ptransportchannel.cc
+++ b/webrtc/p2p/base/p2ptransportchannel.cc
@@ -200,6 +200,39 @@ bool ShouldSwitch(cricket::Connection* a_conn,
   return b_conn->rtt() <= a_conn->rtt() + kMinImprovement;
 }
 
+// Determines whether we should switch the best 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 best connection too frequently when the controlling side is doing
+// aggressive nominations. With this implementation, the controlled side will
+// switch the best connection if the new nominated connection is writable, and
+// i) the best connection is nullptr, or
+// ii) the best connection was not nominated, or
+// iii) the new connection has been nominated twice consecutively, or

[honghaiz3, 2016/06/14 23:18:27]

This would be useful for connections not receiving data. 

[pthatcher1, 2016/06/15 21:59:53]

As I mentioned in the another comment, I think that would make the purpose of
this CL be lost, because an aggressively nominating peer will nominate every
candidate pair multiple times.

[honghaiz3, 2016/06/16 23:03:49]

Done.

+// iv) the new connection is nominated and is receiving data, or
+// v) 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 ShouldSwitchOnNominatedOrDataReceived(

[pthatcher1, 2016/06/15 21:59:53]

I think this should be called ShouldSwitchSelectedConnection and should be like
it is in Taylor's CL (a member method), but with modified logic.

[honghaiz3, 2016/06/16 23:03:48]

I don't think it should use the current ShouldSwitch because this is based on
nomination. If we do that, it may caused Connection keeping on switching if the
priority computation of two sides are ever different. 

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

That will only happen if the controlling side isn't sending media, in which case
there's only one candidate pair being used anyway.  

And I think this would simplify our code quite a bit: the logic for switching is
always the same regardless of when or why we decide to check if we should
switch.

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

Done.

+    cricket::Connection* best_connection,
+    cricket::Connection* new_connection,
+    cricket::Connection* last_nominated,
+    cricket::Connection* last_data_received) {
+  RTC_CHECK(new_connection != nullptr);
+  if (!new_connection->writable()) {
+    return false;
+  }

[pthatcher1, 2016/06/15 21:59:53]

Shouldn't this use the "comparison of writable state"?  For example, what if
both are not writable, or the new connection is only presumed writable?

[honghaiz3, 2016/06/16 01:13:08]

Right. The new connection is already writable. 
Plus, In the ShouldSwitch method, if the current connection is writable and is
nominated, we will not switch. ShouldSwitch is called when sorting connections. 

[honghaiz3, 2016/06/16 23:03:49]

If a nominated connection is not writable, it won't switch. Don't need to
compare the writable/receiving state. 

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

True.  "if (!wrtible()) { return false; }" is more simple.

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

Changed this logic to just compare the write/receiving state. 

+  if (best_connection == nullptr || !best_connection->nominated()) {
+    return true;
+  }

[pthatcher1, 2016/06/15 21:59:53]

But the new_connection might not be nominated either, so we should probably
compare nominated flags like we compare writable states.

[honghaiz3, 2016/06/16 23:03:49]

This method is primarily called when it is nominated. its state must be
nominated except when it is called with data receiving. In the latter case, we
shall switch anyway. 

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

If the controlling side isn't aggressive and none of the connections are
nominated, this logic becomes "select the last writable connection that calls
this method".   And we assume this will only be called when those things make
sense. 

That's pretty subtle.  It would prefer it it were more explicit.  Something like
this:

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

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

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

Done.

+  if (new_connection == last_nominated ||
+      new_connection == last_data_received) {
+    return true;
+  }

[pthatcher1, 2016/06/15 21:59:53]

Perhaps instead of recording the last conn that received data, we should record
the timestamp of the last time we received data in each connection (in fact,
don't we already do that?) and compare those.  

[honghaiz3, 2016/06/16 23:03:48]

using the timestamp is less effective as that only compared the receiving time
of two connections, not equivalent to the connection receiving data most
recently. 

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

I mean something like this:

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

Then we don't have to store the last connection with data received, and it's
more clear.

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

Done.

+  // Compare the network cost and priority.
+  return CompareConnectionCandidates(best_connection, new_connection) < 0;

[pthatcher1, 2016/06/15 21:59:53]

I think we should make all the logic very similar where it's comparison based
and the comparison have clear  precedence:

writable_state > receiving_timestamp > nomination_flag > network cost > priority

[honghaiz3, 2016/06/16 01:13:08]

As I mentioned in another comment, if the current connection is nominated, we
don't want to switch simply based on connection sorting. 

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

I'm mostly suggesting we make the logic clear.  Switching based on connection
sorting is basically what we already do if both are nominated (as it will be
with aggressive nomination).

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

Done.

+}
+
 }  // unnamed namespace
 
 namespace cricket {
@@ -238,8 +271,6 @@ P2PTransportChannel::P2PTransportChannel(const std::string& transport_name,
       worker_thread_(rtc::Thread::Current()),
       incoming_only_(false),
       error_(0),
-      best_connection_(NULL),
-      pending_best_connection_(NULL),
       sort_dirty_(false),
       remote_ice_mode_(ICEMODE_FULL),
       ice_role_(ICEROLE_UNKNOWN),
@@ -729,20 +760,25 @@ 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();
-    }
+  if (best_connection_ == conn) {
+    return;
+  }
+
+  if (ShouldSwitchOnNominatedOrDataReceived(best_connection_, conn,
+                                            last_nominated_conn_,
+                                            last_data_received_conn_)) {
+    last_nominated_conn_ = nullptr;
+    last_data_received_conn_ = nullptr;
+    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;
+    LOG(LS_INFO) << "Not switching the best connection on controlled side yet: "
+                 << conn->ToString();
+    last_nominated_conn_ = conn;
   }

[pthatcher1, 2016/06/15 21:59:53]

Can this just be this?

if (!ShouldSwitchSelectedConnection(best_connection, conn) {
  LOG(LS_INFO) << "Not switching the best connection on controlled side yet:";
  return;
}

SwitchBestConnectionTo(conn);

By the way, I think SwitchBestConnectionTo should be renamed
"SwitchSelectedConnection" and should log that the switch happened, and should
do RequestSort() rather than having it logged here and RequestSort here.

[honghaiz3, 2016/06/16 23:03:49]

Done. 
Regarding the logging, I think the benefit of logging here is that when you
debug, you know where/why the connection is switched.

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

What about RequestSort()?  Shouldn't we do that in SwitchSelectedConnection()?

 }
 
@@ -1444,9 +1480,18 @@ void P2PTransportChannel::OnConnectionStateChange(Connection* connection) {
 
   // 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;
+  if (ice_role_ == ICEROLE_CONTROLLED && connection == last_nominated_conn_) {
+    // This essentially requires that the connection is both last nominated
+    // and receiving data, or the best connection is nullptr, in order
+    // to switch to |connection|.
+    if (connection == best_connection_) {
+      // If it is already the best connection, reset the last_nominated_conn_
+      last_nominated_conn_ = nullptr;
+    } else if (ShouldSwitchOnNominatedOrDataReceived(
+                   best_connection_, connection, nullptr,
+                   last_data_received_conn_)) {
+      last_nominated_conn_ = nullptr;
+      last_data_received_conn_ = nullptr;
       LOG(LS_INFO) << "Switching best connection on controlled side"
                    << " because it's now writable: " << connection->ToString();
       SwitchBestConnectionTo(connection);
@@ -1489,8 +1534,8 @@ void P2PTransportChannel::OnConnectionDestroyed(Connection* connection) {
   LOG_J(LS_INFO, this) << "Removed connection ("
     << static_cast<int>(connections_.size()) << " remaining)";
 
-  if (pending_best_connection_ == connection) {
-    pending_best_connection_ = NULL;
+  if (last_nominated_conn_ == connection) {
+    last_nominated_conn_ = nullptr;
   }

[pthatcher1, 2016/06/15 21:59:53]

I think we should just remove these lines and have it be OK if
last_nominated_conn_ == best_connection_ instead of nulling-out
last_nominated_conn_.  Nulling it out no longer matches the name.

[honghaiz3, 2016/06/16 23:03:49]

Done.

 
   // If this is currently the best connection, then we need to pick a new one.
@@ -1559,9 +1604,17 @@ void P2PTransportChannel::OnReadPacket(Connection* connection,
 
   // May need to switch the sending connection based on the receiving media path
   // if this is the controlled side.
-  if (ice_role_ == ICEROLE_CONTROLLED && !best_nominated_connection() &&
-      connection->writable() && best_connection_ != connection) {
+  if (ice_role_ != ICEROLE_CONTROLLED || best_connection_ == connection) {
+    return;
+  }
+
+  if (ShouldSwitchOnNominatedOrDataReceived(best_connection_, connection,
+                                            last_nominated_conn_, nullptr)) {
     SwitchBestConnectionTo(connection);
+    last_data_received_conn_ = nullptr;

[pthatcher1, 2016/06/15 21:59:53]

Why would we set this to nullptr?

[honghaiz3, 2016/06/16 23:03:48]

Perhaps not necessary. Removed. 

+    last_nominated_conn_ = nullptr;
+  } else {
+    last_data_received_conn_ = connection;
   }

[pthatcher1, 2016/06/15 21:59:53]

Wouldn't it make more sense to do this?

last_data_received_conn_ = connection;
if (ShouldSwitchSelectedConnection(best_connection_, connection)) {
  SwitchBestConnectionTo(connection);
}

You can take ShouldSwitchSelectedConnection from Taylor's recent CL where he
makes it a member method.  Or you can wait until his CL lands.  It should
contain all of the logic for ice_role_, whether best_connection_ == connection,
etc.

[honghaiz3, 2016/06/16 23:03:49]

Done.

 }