Dampening connection switch.

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 62 matching lines @@
 const int WEAK_PING_INTERVAL = 1000 * PING_PACKET_SIZE / 10000;
 
 // Writable connections are pinged at a faster rate while stabilizing.
 const int STABILIZING_WRITABLE_CONNECTION_PING_INTERVAL = 900;  // ms
 
 // Writable connections are pinged at a slower rate once stabilized.
 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;
 static constexpr int a_is_better = 1;
 static constexpr int b_is_better = -1;
 
 P2PTransportChannel::P2PTransportChannel(const std::string& transport_name,
                                          int component,
                                          P2PTransport* transport,
                                          PortAllocator* allocator)
@@ skipping 12 matching lines @@
       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 */,
               GATHER_ONCE /* continual_gathering_policy */,
               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);
   if (weak_ping_interval) {
     weak_ping_interval_ = static_cast<int>(weak_ping_interval);
   }
 }
 
 P2PTransportChannel::~P2PTransportChannel() {
   ASSERT(worker_thread_ == rtc::Thread::Current());
@@ skipping 51 matching lines @@
 // criteria is as follows:
 // i) write/receiving/connected states
 // ii) For controlled side,
 //        a) nomination state,
 //        b) last data received time.
 // iii) Lower cost / higher priority.
 // iv) rtt.
 // 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;
   }
 
   if (selected_connection_ == nullptr) {
     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;
   }
 
   // If everything else is the same, switch only if rtt has improved by
   // a margin.
   return new_connection->rtt() <= selected_connection_->rtt() - kMinImprovement;
 }
 
 void P2PTransportChannel::SetIceRole(IceRole ice_role) {
@@ skipping 155 matching lines @@
                    << config_.presume_writable_when_fully_relayed;
     }
   }
 
   if (config.regather_on_failed_networks_interval) {
     config_.regather_on_failed_networks_interval =
         config.regather_on_failed_networks_interval;
     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 {
   return config_;
 }
 
 void P2PTransportChannel::MaybeStartGathering() {
   if (ice_ufrag_.empty() || ice_pwd_.empty()) {
     return;
   }
@@ skipping 246 matching lines @@
   }
 
   if (!ShouldSwitchSelectedConnection(conn)) {
     LOG(LS_INFO)
         << "Not switching the selected connection on controlled side yet: "
         << conn->ToString();
     return;
   }
 
   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.
   RequestSortAndStateUpdate();
 }
 
 void P2PTransportChannel::AddRemoteCandidate(const Candidate& candidate) {
   ASSERT(worker_thread_ == rtc::Thread::Current());
 
   uint32_t generation = GetRemoteCandidateGeneration(candidate);
@@ skipping 299 matching lines @@
 
 // Monitor connection states.
 void P2PTransportChannel::UpdateConnectionStates() {
   int64_t now = rtc::TimeMillis();
 
   // We need to copy the list of connections since some may delete themselves
   // when we call UpdateState.
   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.
 void P2PTransportChannel::RequestSortAndStateUpdate() {
   if (!sort_dirty_) {
     worker_thread_->Post(RTC_FROM_HERE, this, MSG_SORT_AND_UPDATE_STATE);
     sort_dirty_ = true;
   }
 }
 
@@ skipping 11 matching lines @@
     thread()->Post(RTC_FROM_HERE, this, MSG_CHECK_AND_PING);
     thread()->PostDelayed(RTC_FROM_HERE,
                           *config_.regather_on_failed_networks_interval, this,
                           MSG_REGATHER_ON_FAILED_NETWORKS);
     started_pinging_ = true;
   }
 }
 
 // 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);
   bool b_writable = b->writable() || PresumedWritable(b);
   if (a_writable && !b_writable) {
     return a_is_better;
   }
   if (!a_writable && b_writable) {
     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.
   // When a TCP connection fails because of a TCP socket disconnecting, the
   // active side of the connection will attempt to reconnect for 5 seconds while
   // pretending to be writable (the connection is not set to the unwritable
   // state).  On the passive side, the connection also remains writable even
   // though it is disconnected, and a new connection is created when the active
   // side connects.  At that point, there are two TCP connections on the passive
   // side: 1. the old, disconnected one that is pretending to be writable, and
@@ skipping 47 matching lines @@
     return b_is_better;
   }
 
   // If we're still tied at this point, prefer a younger generation.
   // (Younger generation means a larger generation number).
   return (a->remote_candidate().generation() + a->port()->generation()) -
          (b->remote_candidate().generation() + b->port()->generation());
 }
 
 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;
   }
 
   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.
     if (a->nominated() && !b->nominated()) {
       return a_is_better;
     }
@@ skipping 32 matching lines @@
 
   // Any changes after this point will require a re-sort.
   sort_dirty_ = false;
 
   // Find the best alternative connection by sorting.  It is important to note
   // that amongst equal preference, writable connections, this will choose the
   // one whose estimated latency is lowest.  So it is the only one that we
   // 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();
                    });
 
   LOG(LS_VERBOSE) << "Sorting " << connections_.size()
                   << " available connections:";
   for (size_t i = 0; i < connections_.size(); ++i) {
     LOG(LS_VERBOSE) << connections_[i]->ToString();
   }
 
   Connection* top_connection =
       (connections_.size() > 0) ? connections_[0] : nullptr;
 
   // If necessary, switch to the new choice. Note that |top_connection| doesn't
   // 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);
   }
 
   // The controlled side can prune only if the selected connection has been
   // nominated because otherwise it may prune the connection that will be
   // selected by the controlling side.
   // TODO(honghaiz): This is not enough to prevent a connection from being
   // pruned too early because with aggressive nomination, the controlling side
   // will nominate every connection until it becomes writable.
   if (ice_role_ == ICEROLE_CONTROLLING ||
@@ skipping 228 matching lines @@
       ASSERT(false);
       break;
   }
 }
 
 // Handle queued up check-and-ping request
 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.
   bool need_more_pings_at_weak_interval = std::any_of(
       connections_.begin(), connections_.end(), [](Connection* conn) {
         return conn->active() &&
                conn->num_pings_sent() < MIN_PINGS_AT_WEAK_PING_INTERVAL;
       });
   int ping_interval = (weak() || need_more_pings_at_weak_interval)
                           ? weak_ping_interval_
@@ skipping 187 matching lines @@
   LOG_J(LS_INFO, this) << "Removed connection ("
     << static_cast<int>(connections_.size()) << " remaining)";
 
   // If this is currently the selected connection, then we need to pick a new
   // one. The call to SortConnectionsAndUpdateState will pick a new one. It
   // looks at the current selected connection in order to avoid switching
   // between fairly similar ones. Since this connection is no longer an option,
   // 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 {
     // If a non-selected connection was destroyed, we don't need to re-sort but
     // we do need to update state, because we could be switching to "failed" or
     // "completed".
     UpdateState();
   }
 }
 
@@ skipping 86 matching lines @@
     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 &&
       ShouldSwitchSelectedConnection(connection)) {
     LOG(LS_INFO) << "Switching selected connection on controlled side due to "
-                 << "data received: " << connection->ToString();
+                 << "data received";
     SwitchSelectedConnection(connection);
   }
 }
 
 void P2PTransportChannel::OnSentPacket(const rtc::SentPacket& sent_packet) {
   ASSERT(worker_thread_ == rtc::Thread::Current());
 
   SignalSentPacket(this, sent_packet);
 }
 
@@ skipping 114 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