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 182 matching lines @@
   }
 
   int prefs_cmp = CompareConnectionCandidates(a_conn, b_conn);
   if (prefs_cmp != 0) {
     return prefs_cmp < 0;
   }
 
   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 {
 
 // 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;
@@ skipping 18 matching lines @@
     : 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 469 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();
-    }
+  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()?

 }
 
 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 680 matching lines @@
   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.
 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;
+  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);
     }
   }

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

Can't this just be

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

?

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

Ah Yes. 

 
   // 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) {
@@ skipping 17 matching lines @@
   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 (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.
   // 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);
     RequestSort();
@@ skipping 47 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) {
+  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.

 }
 
 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()) {
@@ skipping 112 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