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

[pthatcher1, 2016/07/13 21:17:23]

I think we should be conservative with the value for now and stick with 1000ms. 
If we feel confident that we can lower it later, we can.  But I think it's
better to stick with a bigger value and then test going down rather than risk
being too low.

[honghaiz3, 2016/07/14 04:54:20]

There is actually some risk of choosing the same dampening value and the
receiving timeout value. 
Because by the time we re-check it again the backup connection may become not
receiving if only one stun-ping response message was received on the backup
connection. This may cause some switching not happening while it should happen. 
I set it to 900ms for now. 

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

[pthatcher1, 2016/07/13 21:17:23]

I don't like the idea of ShouldSwitchSelectedConnection being non-const and
changing state.

And I don't think we need to store the pending_selected_connection_.  I think
there are alternatives that are more simple.  

For example, if we know we need to wait X ms to see if we're past the dampening
period, we could just schedule a MSG_SORT_AND_UPDATE_STATE that will take care
of switching, if it should.

[honghaiz3, 2016/07/14 04:54:20]

Done for other parts except that
ShouldSwitchSelectedConnection needs to schedule a event, which access this,
which is not a const argument. 
So there is no easy way to keep the method const. 

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

Can't the *caller* of ShouldSwitchSelectedConnection schedule the re-comparison?
 Then it can remain const.

+
+  cmp = CompareConnectionsWithoutStates(selected_connection_, new_connection);

[pthatcher1, 2016/07/13 21:17:23]

I also think it would be more simple to keep it as one CompareConnections and
not break it up into two (WithDampening and WithoutStates).

[honghaiz3, 2016/07/14 04:54:21]

Done.

   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;

[pthatcher1, 2016/07/13 21:17:23]

I think we can calculate the "has it been receiving long enough" logic
independently for each Connection and avoid having to reach into the connection
with things like reset_num_pings_sent() and
reset_first_received_since_channel_weak().

For example, we can simply have a "continuously_receiving_since()" that give the
timestamp of the time we went from non receiving -> receiving.  

[honghaiz3, 2016/07/14 04:54:21]

We can do that. But a potential problem is that if the backup connection was
receiving (for example due to the periodic ping message on the backup
connection), it will switch right away. 
So I just reset the continuously_receiving_since to |now| if the selected
connection becomes weak. 

I realize reset_num_pings_sent is not needed any more because if the switching
is dampened, it will be in weak state and so it will be pinged in fast rate. 

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

[pthatcher1, 2016/07/13 21:17:22]

Doesn't this change the order so that connected vs not takes precedence over
receiving?

[honghaiz3, 2016/07/14 04:54:20]

I did not think that should have a major impact although we don't have to change
that in this CL. 

+
+  return 0;
+}
+
+int P2PTransportChannel::CompareConnectionStatesWithDampening(
+    const Connection* selected_conn,
+    const Connection* new_conn,

[pthatcher1, 2016/07/13 21:17:23]

CompareConnectionX should know about "selected" and "new" connections.  I think
it would make more sense to pass call them "a" and "b" and then have a
"b_receiving_delay" passed in.  Or better yet, combine "now" with
"b_receiving_delay" into a "b_receiving_threshold".

[honghaiz3, 2016/07/14 04:54:20]

Done

+    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) {

[pthatcher1, 2016/07/13 21:17:22]

I think it would make more sense to have the Connection have a method with
something like continuously_receiving_since() that returns a timestamp of the
earliest time of the currently continuous time span that it's been receiving. 
Then this could be:

if (!a->receiving() && b->receiving() &&
    b->continuously_receiving_since() < b_receiving_threshold) {
  return b_is_better;
}
return 0;

[honghaiz3, 2016/07/14 04:54:20]

Done, somewhat different though. 

+      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;

[pthatcher1, 2016/07/13 21:17:22]

I think it would be less risky to leave the CompareConnections methods as-is
(with the exception off adding the b_receiving_threhsold).

[honghaiz3, 2016/07/14 04:54:20]

Done.

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

[pthatcher1, 2016/07/13 21:17:23]

This code, or the equivalent, really should be inside of UpdateConnectionStates.

[honghaiz3, 2016/07/14 04:54:20]

This serves the purpose of periodical checking the dampened connection. 
Now that is not needed, just remove this. 

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