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..b3a1cc8650c23b1f568d3910d1a496a79625738b 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_DAMPENING_INTERVAL = 500;  // ms

[honghaiz3, 2016/07/13 19:38:08]

I think with at least three STUN pings on the selected connection, if the backup
becomes receiving and the selected still is not receiving after 500ms, it is a
pretty strong sign that the backup connection should be selected. 
Choosing 1000ms may adversely affecting normal connection switches that should
be made.  

+
 // 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_DAMPENING_INTERVAL) {
   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,19 @@ bool P2PTransportChannel::ShouldSwitchSelectedConnection(
     return true;
   }
 
-  int cmp = CompareConnections(selected_connection_, new_connection);
+  bool connection_dampened = false;
+  int cmp = CompareConnectionStatesWithDampening(
+      selected_connection_, new_connection, &connection_dampened);
+  if (cmp != 0) {
+    return cmp < 0;
+  }
+  // A connection is dampened indicates that it is in a better receiving state,
+  // so we need to remember it and check it again periodically.
+  if (connection_dampened) {
+    pending_selected_connection_ = new_connection;
+  }
+
+  cmp = CompareConnectionsWithoutStates(selected_connection_, new_connection);
   if (cmp != 0) {
     return cmp < 0;
   }
@@ -369,6 +384,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_dampening_interval) {
+    config_.receiving_dampening_interval = config.receiving_dampening_interval;
+    LOG(LS_INFO) << "Set receiving_dampening_interval to"
+                 << *config_.receiving_dampening_interval;
+  }
 }
 
 const IceConfig& P2PTransportChannel::config() const {
@@ -635,8 +655,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 +975,22 @@ 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 number of
+    // pings sent on the selected connection so that it will be pinged at least
+    // a few times.
+    selected_connection_->reset_num_pings_sent();
+    for (Connection* c : connections_) {
+      c->reset_first_received_since_channel_weak();
+    }
+  }
+  was_strong_ = is_strong;
 }
 
 // Prepare for best candidate sorting.
@@ -985,10 +1020,9 @@ 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::CompareConnectionWriteAndConnectedStates(
+    const Connection* a,
+    const Connection* b) 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 +1034,11 @@ 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;
-  }
-
-  // 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 a_is_better;
-  }
-  if (!a->receiving() && b->receiving()) {
-    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;
   }
 
   // WARNING: Some complexity here about TCP reconnecting.
@@ -1052,6 +1075,54 @@ int P2PTransportChannel::CompareConnectionStates(const Connection* a,
   return 0;
 }
 
+// Compare two connections based on their writing, connected, and receiving
+// states.
+int P2PTransportChannel::CompareConnectionStates(const Connection* a,
+                                                 const Connection* b) const {
+  int cmp = CompareConnectionWriteAndConnectedStates(a, b);
+  if (cmp != 0) {
+    return cmp;
+  }
+
+  // We prefer a receiving connection to a non-receiving, higher-priority
+  // connection when sorting connections.
+  if (a->receiving() && !b->receiving()) {
+    return a_is_better;
+  }
+  if (!a->receiving() && b->receiving()) {
+    return b_is_better;
+  }
+
+  return 0;
+}
+
+int P2PTransportChannel::CompareConnectionStatesWithDampening(
+    const Connection* selected_conn,
+    const Connection* new_conn,
+    bool* new_conn_dampened) const {
+  int cmp = CompareConnectionWriteAndConnectedStates(selected_conn, new_conn);
+  if (cmp != 0) {
+    return cmp;
+  }
+
+  if (selected_conn->receiving() && !new_conn->receiving()) {
+    return a_is_better;
+  }
+
+  if (!selected_conn->receiving() && new_conn->receiving()) {
+    // If |selected_conn| is not receiving and |new_conn| is receiving, we wait
+    // for a short while before switching to the new connection.
+    if (new_conn->first_received_since_channel_weak() != 0 &&
+        rtc::TimeMillis() - new_conn->first_received_since_channel_weak() >=
+            *config_.receiving_dampening_interval) {
+      return b_is_better;
+    }
+    *new_conn_dampened = true;
+  }
+
+  return 0;
+}
+
 // Compares two connections based only on the candidate and network information.
 // Returns positive if |a| is better than |b|.
 int P2PTransportChannel::CompareConnectionCandidates(
@@ -1082,19 +1153,12 @@ int P2PTransportChannel::CompareConnectionCandidates(
          (b->remote_candidate().generation() + b->port()->generation());
 }
 
-int P2PTransportChannel::CompareConnections(const Connection* a,
-                                            const Connection* b) const {
+int P2PTransportChannel::CompareConnectionsWithoutStates(
+    const Connection* a,
+    const 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;
-  }
-
   if (ice_role_ == ICEROLE_CONTROLLED) {
     // Compare the connections based on the nomination states and the last data
     // received time if this is on the controlled side.
@@ -1117,6 +1181,24 @@ int P2PTransportChannel::CompareConnections(const Connection* a,
   return CompareConnectionCandidates(a, b);
 }
 
+int P2PTransportChannel::CompareConnections(const Connection* a,
+                                            const Connection* b) const {
+  // 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 cmp = CompareConnectionStates(a, b);
+  if (cmp != 0) {
+    return cmp;
+  }
+  cmp = CompareConnectionsWithoutStates(a, b);
+  if (cmp != 0) {
+    return cmp;
+  }
+  // Otherwise, compare the latency estimate. Note that a negative return value
+  // indicates |b| is better.
+  return b->rtt() - a->rtt();
+}
+
 bool P2PTransportChannel::PresumedWritable(const Connection* conn) const {
   return (conn->write_state() == Connection::STATE_WRITE_INIT &&
           config_.presume_writable_when_fully_relayed &&
@@ -1143,13 +1225,8 @@ 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);
-                     if (cmp != 0) {
-                       return cmp > 0;
-                     }
+                     return CompareConnections(a, b) > 0;
 
-                     // Otherwise, sort based on latency estimate.
-                     return a->rtt() < b->rtt();
                    });
 
   LOG(LS_VERBOSE) << "Sorting " << connections_.size()
@@ -1165,8 +1242,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);
   }
 
@@ -1247,6 +1323,9 @@ void P2PTransportChannel::SwitchSelectedConnection(Connection* conn) {
   Connection* old_selected_connection = selected_connection_;
   selected_connection_ = conn;
   if (selected_connection_) {
+    if (pending_selected_connection_ == selected_connection_) {
+      pending_selected_connection_ = nullptr;
+    }
     if (old_selected_connection) {
       LOG_J(LS_INFO, this) << "Previous selected connection: "
                            << old_selected_connection->ToString();
@@ -1415,6 +1494,15 @@ void P2PTransportChannel::OnCheckAndPing() {
   // Make sure the states of the connections are up-to-date (since this affects
   // which ones are pingable).
   UpdateConnectionStates();
+  if (pending_selected_connection_) {
+    if (ShouldSwitchSelectedConnection(pending_selected_connection_)) {
+      LOG(LS_INFO) << "Switching selected connection to a pending one";
+      SwitchSelectedConnection(pending_selected_connection_);
+    } else if (!weak()) {
+      pending_selected_connection_ = nullptr;
+    }
+  }
+
   // 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 +1710,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 +1817,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);
   }
 }