Dampening connection switch.

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 4ebd3a41c44612585ba4340a34d816e5e77fe167..e43853763e17a7269aa6f45d22aa69abe93cf9dd 100644
--- a/webrtc/p2p/base/p2ptransportchannel.cc
+++ b/webrtc/p2p/base/p2ptransportchannel.cc
@@ -80,6 +80,8 @@ const int STABLE_WRITABLE_CONNECTION_PING_INTERVAL = 2500;  // ms
 
 static const int MIN_CHECK_RECEIVING_INTERVAL = 50;  // ms
 
+static const int RECEIVING_SWITCHING_DELAY = 900;  // ms

[pthatcher1, 2016/07/14 18:01:44]

I don't quite follow the distinction between 900ms and 1000ms.  Perhaps you can
explain it in person.

+
 // We periodically check if any existing networks do not have any connection
 // and regather on those networks.
 static const int DEFAULT_REGATHER_ON_FAILED_NETWORKS_INTERVAL = 5 * 60 * 1000;
@@ -112,7 +114,8 @@ P2PTransportChannel::P2PTransportChannel(const std::string& transport_name,
               false /* prioritize_most_likely_candidate_pairs */,
               STABLE_WRITABLE_CONNECTION_PING_INTERVAL,
               true /* presume_writable_when_fully_relayed */,
-              DEFAULT_REGATHER_ON_FAILED_NETWORKS_INTERVAL) {
+              DEFAULT_REGATHER_ON_FAILED_NETWORKS_INTERVAL,
+              RECEIVING_SWITCHING_DELAY) {
   uint32_t weak_ping_interval = ::strtoul(
       webrtc::field_trial::FindFullName("WebRTC-StunInterPacketDelay").c_str(),
       nullptr, 10);
@@ -184,7 +187,7 @@ void P2PTransportChannel::AddConnection(Connection* connection) {
 // TODO(honghaiz): Stop the aggressive nomination on the controlling side and
 // implement the ice-renomination option.
 bool P2PTransportChannel::ShouldSwitchSelectedConnection(
-    Connection* new_connection) const {
+    Connection* new_connection) {
   if (!new_connection || selected_connection_ == new_connection) {
     return false;
   }
@@ -193,7 +196,20 @@ bool P2PTransportChannel::ShouldSwitchSelectedConnection(
     return true;
   }
 
-  int cmp = CompareConnections(selected_connection_, new_connection);
+  bool switching_dampened = false;
+  int64_t new_conn_receiving_threshold =
+      (*config_.receiving_switching_delay) +

[pthatcher1, 2016/07/14 18:01:44]

Don't we have to make sure config_.receiving_switching_delay is set?

[honghaiz3, 2016/07/14 23:15:01]

We set the default value at the beginning and only update it if the new value is
set. So it should be always set. 

+      new_connection->continuously_receiving_since();

[pthatcher1, 2016/07/14 18:01:44]

I was thinking of it the other way around:

int64_t now = rtc::TimeMillis();
int64_t new_connection_receiving_threshold = now;
if (config_.receiving_switching_delay) {
  // If the delay is set, the connection must have been receiving since longer
ago.
  new_connection_receiving_threshold -= *config_.receiving_switching_delay;
}

[honghaiz3, 2016/07/14 23:15:01]

Done. If it is not set, it will just get a default int value 0 if you try to
access it. 

+  int cmp =
+      CompareConnections(selected_connection_, new_connection,
+                         new_conn_receiving_threshold, &switching_dampened);
+  // If a connection switching is dampened, the new connection is in a better
+  // receiving state than the currently selected connection. So we need to
+  // re-check whether it needs to be switched at a later time.
+  if (switching_dampened && cmp >= 0) {
+    thread()->PostDelayed(RTC_FROM_HERE, *config_.receiving_switching_delay,
+                          this, MSG_SORT_AND_UPDATE_STATE);
+  }

[pthatcher1, 2016/07/14 18:01:44]

If ShoudlSwitchSelectedConnection also takes/returns a "bool*
switching_dampened", then the caller of ShouldSwitchSelectedConnection can
choose to schedule the re-sort.

[honghaiz3, 2016/07/14 23:15:01]

Done.

   if (cmp != 0) {
     return cmp < 0;
   }
@@ -369,6 +385,11 @@ void P2PTransportChannel::SetIceConfig(const IceConfig& config) {
     LOG(LS_INFO) << "Set regather_on_failed_networks_interval to "
                  << *config_.regather_on_failed_networks_interval;
   }
+  if (config.receiving_switching_delay) {
+    config_.receiving_switching_delay = config.receiving_switching_delay;
+    LOG(LS_INFO) << "Set receiving_switching_delay to"
+                 << *config_.receiving_switching_delay;
+  }
 }
 
 const IceConfig& P2PTransportChannel::config() const {
@@ -635,8 +656,7 @@ void P2PTransportChannel::OnNominated(Connection* conn) {
   }
 
   LOG(LS_INFO)
-      << "Switching selected connection on controlled side due to nomination: "
-      << conn->ToString();
+      << "Switching selected connection on controlled side due to nomination";
   SwitchSelectedConnection(conn);
   // Now that we have selected a connection, it is time to prune other
   // connections and update the read/write state of the channel.
@@ -956,6 +976,19 @@ void P2PTransportChannel::UpdateConnectionStates() {
   for (Connection* c : connections_) {
     c->UpdateState(now);
   }
+  if (!selected_connection_) {
+    was_strong_ = false;
+    return;
+  }
+  bool is_strong = !selected_connection_->weak();
+  if (was_strong_ && !is_strong) {
+    // If the selected connection just changes to weak,
+    // reset the continuously_receiving_since time for each connection.
+    for (Connection* c : connections_) {
+      c->reset_continuously_receiving_since(now);

[pthatcher1, 2016/07/14 18:01:44]

I don't think we need this.  Won't the dampening work without it?

+    }
+  }
+  was_strong_ = is_strong;
 }
 
 // Prepare for best candidate sorting.
@@ -987,8 +1020,11 @@ void P2PTransportChannel::MaybeStartPinging() {
 
 // Compare two connections based on their writing, receiving, and connected
 // states.
-int P2PTransportChannel::CompareConnectionStates(const Connection* a,
-                                                 const Connection* b) const {
+int P2PTransportChannel::CompareConnectionStates(
+    const Connection* a,
+    const Connection* b,
+    int64_t b_receiving_threshold,
+    bool* switching_dampened) const {
   // First, prefer a connection that's writable or presumed writable over
   // one that's not writable.
   bool a_writable = a->writable() || PresumedWritable(a);
@@ -1000,22 +1036,24 @@ int P2PTransportChannel::CompareConnectionStates(const Connection* a,
     return b_is_better;
   }
 
-  // Sort based on write-state. Better states have lower values.
-  if (a->write_state() < b->write_state()) {
-    return a_is_better;
-  }
-  if (b->write_state() < a->write_state()) {
-    return b_is_better;
+  // Better write-states have lower values, and a negative return value
+  // indicates b is better.
+  int cmp = b->write_state() - a->write_state();
+  if (cmp != 0) {
+    return cmp;
   }

[pthatcher1, 2016/07/14 18:01:44]

Might as well just leave this part unchanged, since it was more consistent with
the rest of the code and it makes the diff smaller.

[honghaiz3, 2016/07/14 23:15:01]

Done.

 
   // We prefer a receiving connection to a non-receiving, higher-priority
-  // connection when sorting connections and choosing which connection to
-  // switch to.
+  // connection when sorting connections.
   if (a->receiving() && !b->receiving()) {
     return a_is_better;
   }
   if (!a->receiving() && b->receiving()) {
-    return b_is_better;
+    if (b_receiving_threshold == 0 ||
+        rtc::TimeMillis() >= b_receiving_threshold) {

[pthatcher1, 2016/07/14 18:01:44]

With my alternate definition of "threshold", this would be:

if (b->receiving_continuously_since() < b_receiving_threshold) {
  return b_is_better;
} else {
  *switching_dampened = true;
}

Which I think is more clear.

[honghaiz3, 2016/07/14 23:15:01]

Done. Use an optional now. 
We still need to check if b_receiving_threshold is set. 

+      return b_is_better;
+    }
+    *switching_dampened = true;
   }
 
   // WARNING: Some complexity here about TCP reconnecting.
@@ -1083,14 +1121,17 @@ int P2PTransportChannel::CompareConnectionCandidates(
 }
 
 int P2PTransportChannel::CompareConnections(const Connection* a,
-                                            const Connection* b) const {
+                                            const Connection* b,
+                                            int64_t b_receiving_threshold,
+                                            bool* switching_dampened) 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);
+  int state_cmp =
+      CompareConnectionStates(a, b, b_receiving_threshold, switching_dampened);
   if (state_cmp != 0) {
     return state_cmp;
   }
@@ -1143,11 +1184,10 @@ void P2PTransportChannel::SortConnectionsAndUpdateState() {
   // need to consider switching to.
   std::stable_sort(connections_.begin(), connections_.end(),
                    [this](const Connection* a, const Connection* b) {
-                     int cmp = CompareConnections(a, b);
+                     int cmp = CompareConnections(a, b, 0, nullptr);
                      if (cmp != 0) {
                        return cmp > 0;
                      }
-
                      // Otherwise, sort based on latency estimate.
                      return a->rtt() < b->rtt();
                    });
@@ -1165,8 +1205,7 @@ void P2PTransportChannel::SortConnectionsAndUpdateState() {
   // have to be writable to become the selected connection although it will
   // have higher priority if it is writable.
   if (ShouldSwitchSelectedConnection(top_connection)) {
-    LOG(LS_INFO) << "Switching selected connection after sorting: "
-                 << top_connection->ToString();
+    LOG(LS_INFO) << "Switching selected connection after sorting";
     SwitchSelectedConnection(top_connection);
   }
 
@@ -1415,6 +1454,7 @@ void P2PTransportChannel::OnCheckAndPing() {
   // Make sure the states of the connections are up-to-date (since this affects
   // which ones are pingable).
   UpdateConnectionStates();
+
   // 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.
@@ -1622,7 +1662,7 @@ void P2PTransportChannel::OnConnectionDestroyed(Connection* connection) {
   // 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.";
+    LOG(LS_INFO) << "Selected connection destroyed. Will choose a new one.";
     SwitchSelectedConnection(nullptr);
     RequestSortAndStateUpdate();
   } else {
@@ -1729,7 +1769,7 @@ void P2PTransportChannel::OnReadPacket(Connection* connection,
   if (ice_role_ == ICEROLE_CONTROLLED &&
       ShouldSwitchSelectedConnection(connection)) {
     LOG(LS_INFO) << "Switching selected connection on controlled side due to "
-                 << "data received: " << connection->ToString();
+                 << "data received";
     SwitchSelectedConnection(connection);
   }
 }