A few updates on connection states

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.
  */
 
 #include "webrtc/p2p/base/p2ptransportchannel.h"
 
+#include <algorithm>
 #include <set>
 #include "webrtc/p2p/base/common.h"
 #include "webrtc/p2p/base/relayport.h"  // For RELAY_PORT_TYPE.
 #include "webrtc/p2p/base/stunport.h"  // For STUN_PORT_TYPE.
 #include "webrtc/base/common.h"
 #include "webrtc/base/crc32.h"
 #include "webrtc/base/logging.h"
 #include "webrtc/base/stringencode.h"
 
 namespace {
 
 // messages for queuing up work for ourselves
-enum {
-  MSG_SORT = 1,
-  MSG_PING,
-  MSG_CHECK_RECEIVING
-};
+enum { MSG_SORT = 1, MSG_CHECK_AND_PING };
 
 // 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 uint32 PING_PACKET_SIZE = 60 * 8;
-static const uint32 WRITABLE_DELAY = 1000 * PING_PACKET_SIZE / 1000;  // 480ms
-static const uint32 UNWRITABLE_DELAY = 1000 * PING_PACKET_SIZE / 10000;  // 50ms
+// STRONG_PING_DELAY (480ms) is applied when the best connection is both
+// writable and receiving.
+static const uint32 STRONG_PING_DELAY = 1000 * PING_PACKET_SIZE / 1000;
+// WEAK_PING_DELAY (48ms) is applied when the best connection is either not
+// writable or not receiving.
+static const uint32 WEAK_PING_DELAY = 1000 * PING_PACKET_SIZE / 10000;
 
-// If there is a current writable connection, then we will also try hard to
-// make sure it is pinged at this rate.
-static const uint32 MAX_CURRENT_WRITABLE_DELAY = 900;  // 2*WRITABLE_DELAY - bit
+// If the current best connection is both writable and receiving, then we will
+// also try hard to make sure it is pinged at this rate (a little less than
+// 2 * STRONG_PING_DELAY).
+static const uint32 MAX_CURRENT_STRONG_DELAY = 900;
 
 static const int MIN_CHECK_RECEIVING_DELAY = 50;  // ms
 
 // The minimum improvement in RTT that justifies a switch.
 static const double kMinImprovement = 10;
 
 cricket::PortInterface::CandidateOrigin GetOrigin(cricket::PortInterface* port,
                                          cricket::PortInterface* origin_port) {
   if (!origin_port)
     return cricket::PortInterface::ORIGIN_MESSAGE;
@@ skipping 10 matching lines @@
   if (a->priority() > b->priority())
     return 1;
   if (a->priority() < b->priority())
     return -1;
 
   // If we're still tied at this point, prefer a younger generation.
   return (a->remote_candidate().generation() + a->port()->generation()) -
          (b->remote_candidate().generation() + b->port()->generation());
 }
 
-// Compare two connections based on their connected state, writability and
-// static preferences.
-int CompareConnections(cricket::Connection *a, cricket::Connection *b) {
+// Compare two connections based on their writing, receiving, and connected
+// states.
+int CompareConnectionStates(cricket::Connection* a, cricket::Connection* b) {
   // Sort based on write-state.  Better states have lower values.
   if (a->write_state() < b->write_state())
     return 1;
   if (a->write_state() > b->write_state())
     return -1;
 
+  if (a->receiving() && !b->receiving())
+    return 1;
+  if (!a->receiving() && b->receiving())
+    return -1;

[pthatcher1, 2015/09/22 23:41:46]

Can you put a comment along the lines of "We prefer a receiving connection over
a higher-priority non-receiving connection when choosing which connection to
ping and which to switch to"?

[honghaiz3, 2015/09/23 03:16:10]

Done. I do not see that the pinging order is related to the connection sorting
based on this, so I did not include that part. 

+
   // 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
   // 2.  the new, connected one that is maybe not yet writable.  For purposes of
   // pruning, pinging, and selecting the best connection, we want to treat the
@@ skipping 11 matching lines @@
   // the new connection, when it becomes writable, should have higher priority.
   if (a->write_state() == cricket::Connection::STATE_WRITABLE &&
       b->write_state() == cricket::Connection::STATE_WRITABLE) {
     if (a->connected() && !b->connected()) {
       return 1;
     }
     if (!a->connected() && b->connected()) {
       return -1;
     }
   }
+  return 0;
+}
 
+int CompareConnections(cricket::Connection* a, cricket::Connection* b) {
+  int state_cmp = CompareConnectionStates(a, b);
+  if (state_cmp != 0) {
+    return state_cmp;
+  }
   // Compare the candidate information.
   return CompareConnectionCandidates(a, b);
 }
 
 // Wraps the comparison connection into a less than operator that puts higher
 // priority writable connections first.
 class ConnectionCompare {
  public:
   bool operator()(const cricket::Connection *ca,
                   const cricket::Connection *cb) {
@@ skipping 17 matching lines @@
     // TODO: Yes we should definitely do this.  The TCP protocol gains
     // efficiency by being used bidirectionally, as opposed to two separate
     // unidirectional streams.  This test should probably occur before
     // comparison of local prefs (assuming combined prefs are the same).  We
     // need to be careful though, not to bounce back and forth with both sides
     // trying to rendevous with the other.
   }
 };
 
 // Determines whether we should switch between two connections, based first on
-// static preferences and then (if those are equal) on latency estimates.
-bool ShouldSwitch(cricket::Connection* a_conn, cricket::Connection* b_conn) {
+// connection states, static preferences, and then (if those are equal) on
+// latency estimates.
+bool ShouldSwitch(cricket::Connection* a_conn,
+                  cricket::Connection* b_conn,
+                  cricket::IceRole ice_role) {
   if (a_conn == b_conn)
     return false;
 
   if (!a_conn || !b_conn)  // don't think the latter should happen
     return true;
 
-  int prefs_cmp = CompareConnections(a_conn, b_conn);
-  if (prefs_cmp < 0)
-    return true;
-  if (prefs_cmp > 0)
+  // If the connection states (WRITE/RECEIVING/CONNECT) are different, we should
+  // switch regardless of the nominated state. Otherwise, do not switch if
+  // |a_conn| is nominated on the controlled side.

[pthatcher1, 2015/09/22 23:41:46]

Can you reword this as something like "We prefer to switch to a writable and
receiving connection over a non-writable or non-receiving connection, even if
the non-writable or non-receiving connection has been nominated by the
controlling side"?

[honghaiz3, 2015/09/23 03:16:10]

Done.

+  int state_cmp = CompareConnectionStates(a_conn, b_conn);
+  if (state_cmp != 0) {
+    return state_cmp < 0;
+  }
+  if (ice_role == cricket::ICEROLE_CONTROLLED && a_conn->nominated()) {
+    LOG(LS_VERBOSE) << "Controlled side did not switch due to nominated status";
     return false;
+  }
+
+  int prefs_cmp = CompareConnectionCandidates(a_conn, b_conn);
+  if (prefs_cmp != 0) {
+    return prefs_cmp < 0;
+  }
 
   return b_conn->rtt() <= a_conn->rtt() + kMinImprovement;
 }
 
 }  // unnamed namespace
 
 namespace cricket {
 
 P2PTransportChannel::P2PTransportChannel(const std::string& content_name,
                                          int component,
                                          P2PTransport* transport,
-                                         PortAllocator *allocator) :
-    TransportChannelImpl(content_name, component),
-    transport_(transport),
-    allocator_(allocator),
-    worker_thread_(rtc::Thread::Current()),
-    incoming_only_(false),
-    waiting_for_signaling_(false),
-    error_(0),
-    best_connection_(NULL),
-    pending_best_connection_(NULL),
-    sort_dirty_(false),
-    was_writable_(false),
-    remote_ice_mode_(ICEMODE_FULL),
-    ice_role_(ICEROLE_UNKNOWN),
-    tiebreaker_(0),
-    remote_candidate_generation_(0),
-    check_receiving_delay_(MIN_CHECK_RECEIVING_DELAY * 5),
-    receiving_timeout_(MIN_CHECK_RECEIVING_DELAY * 50) {
-}
+                                         PortAllocator* allocator)
+    : TransportChannelImpl(content_name, component),
+      transport_(transport),
+      allocator_(allocator),
+      worker_thread_(rtc::Thread::Current()),
+      incoming_only_(false),
+      waiting_for_signaling_(false),
+      error_(0),
+      best_connection_(nullptr),
+      pending_best_connection_(nullptr),
+      sort_dirty_(false),
+      was_writable_(false),
+      remote_ice_mode_(ICEMODE_FULL),
+      ice_role_(ICEROLE_UNKNOWN),
+      tiebreaker_(0),
+      remote_candidate_generation_(0),
+      check_receiving_delay_(MIN_CHECK_RECEIVING_DELAY * 5),
+      receiving_timeout_(MIN_CHECK_RECEIVING_DELAY * 50) {}
 
 P2PTransportChannel::~P2PTransportChannel() {
   ASSERT(worker_thread_ == rtc::Thread::Current());
 
   for (uint32 i = 0; i < allocator_sessions_.size(); ++i)
     delete allocator_sessions_[i];
 }
 
 // Add the allocator session to our list so that we know which sessions
 // are still active.
@@ skipping 151 matching lines @@
   if (ice_ufrag_.empty() || ice_pwd_.empty()) {
     ASSERT(false);
     LOG(LS_ERROR) << "P2PTransportChannel::Connect: The ice_ufrag_ and the "
                   << "ice_pwd_ are not set.";
     return;
   }
 
   // Kick off an allocator session
   Allocate();
 
-  // Start pinging as the ports come in.
-  thread()->Post(this, MSG_PING);
-
-  thread()->PostDelayed(
-      check_receiving_delay_, this, MSG_CHECK_RECEIVING);
+  // Start checking and pinging as the ports come in.
+  thread()->Post(this, MSG_CHECK_AND_PING);
 }
 
 // A new port is available, attempt to make connections for it
 void P2PTransportChannel::OnPortReady(PortAllocatorSession *session,
                                       PortInterface* port) {
   ASSERT(worker_thread_ == rtc::Thread::Current());
 
   // Set in-effect options on the new port
   for (OptionMap::const_iterator it = options_.begin();
        it != options_.end();
@@ skipping 538 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.
   ConnectionCompare cmp;
   std::stable_sort(connections_.begin(), connections_.end(), cmp);
-  LOG(LS_VERBOSE) << "Sorting available connections:";
+  LOG(LS_VERBOSE) << "Sorting " << connections_.size()
+                  << " available connections:";
   for (uint32 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 best
   // connection although it will have higher priority if it is writable.
-  // The controlled side can switch the best connection only if the current
-  // |best connection_| has not been nominated by the controlling side yet.
-  if ((ice_role_ == ICEROLE_CONTROLLING || !best_nominated_connection()) &&
-      ShouldSwitch(best_connection_, top_connection)) {
+  if (ShouldSwitch(best_connection_, top_connection, ice_role_)) {
     LOG(LS_INFO) << "Switching best connection: " << top_connection->ToString();
     SwitchBestConnectionTo(top_connection);
   }
 
   // Controlled side can prune only if the best connection has been nominated.
   // because otherwise it may delete the connection that will be selected by
   // the controlling side.
   if (ice_role_ == ICEROLE_CONTROLLING || best_nominated_connection()) {
     PruneConnections();
   }
@@ skipping 65 matching lines @@
   Connection* old_best_connection = best_connection_;
   best_connection_ = conn;
   if (best_connection_) {
     if (old_best_connection) {
       LOG_J(LS_INFO, this) << "Previous best connection: "
                            << old_best_connection->ToString();
     }
     LOG_J(LS_INFO, this) << "New best connection: "
                          << best_connection_->ToString();
     SignalRouteChange(this, best_connection_->remote_candidate());
-    set_receiving(best_connection_->receiving());
   } else {
     LOG_J(LS_INFO, this) << "No best connection";
   }
 }
 
 void P2PTransportChannel::UpdateChannelState() {
   // The Handle* functions already set the writable state.  We'll just double-
   // check it here.
-  bool writable = ((best_connection_ != NULL)  &&
-      (best_connection_->write_state() ==
-      Connection::STATE_WRITABLE));
+  bool writable = best_connection_ && best_connection_->writable();
   ASSERT(writable == this->writable());
   if (writable != this->writable())
     LOG(LS_ERROR) << "UpdateChannelState: writable state mismatch";
 
-  // TODO(honghaiz): The channel receiving state is set in OnCheckReceiving.
-  // Will revisit in a subsequent code change.
+  bool receiving = false;
+  for (const Connection* connection : connections_) {
+    if (connection->receiving()) {
+      receiving = true;
+      break;
+    }
+  }

[pthatcher1, 2015/09/22 23:41:46]

I thought this was more readable using std::any_of.

[honghaiz3, 2015/09/23 03:16:10]

Using that caused the compiling to break on some Mac platform. 
So I switched it back. See here
http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/9633/ste...

+  set_receiving(receiving);
 }
 
 // We checked the status of our connections and we had at least one that
 // was writable, go into the writable state.
 void P2PTransportChannel::HandleWritable() {
   ASSERT(worker_thread_ == rtc::Thread::Current());
   if (!writable()) {
     for (uint32 i = 0; i < allocator_sessions_.size(); ++i) {
       if (allocator_sessions_[i]->IsGettingPorts()) {
         allocator_sessions_[i]->StopGettingPorts();
@@ skipping 12 matching lines @@
     was_writable_ = false;
     set_writable(false);
   }
 }
 
 void P2PTransportChannel::HandleAllTimedOut() {
   // Currently we are treating this as channel not writable.
   HandleNotWritable();
 }
 
+bool P2PTransportChannel::Weak() const {
+  return !(best_connection_ && best_connection_->receiving() && writable());

[pthatcher1, 2015/09/22 23:41:46]

Isn't the same as "!(best_connection_ && best_connection_->receiving() &&
best_connection_->writable());"?  If so, it's more readable.

[honghaiz3, 2015/09/23 03:16:10]

Done.

+}
+
 // If we have a best connection, return it, otherwise return top one in the
 // list (later we will mark it best).
 Connection* P2PTransportChannel::GetBestConnectionOnNetwork(
     rtc::Network* network) const {
   // If the best connection is on this network, then it wins.
   if (best_connection_ && (best_connection_->port()->Network() == network))
     return best_connection_;
 
   // Otherwise, we return the top-most in sorted order.
   for (uint32 i = 0; i < connections_.size(); ++i) {
     if (connections_[i]->port()->Network() == network)
       return connections_[i];
   }
 
   return NULL;
 }
 
 // Handle any queued up requests
 void P2PTransportChannel::OnMessage(rtc::Message *pmsg) {
   switch (pmsg->message_id) {
     case MSG_SORT:
       OnSort();
       break;
-    case MSG_PING:
-      OnPing();
-      break;
-    case MSG_CHECK_RECEIVING:
-      OnCheckReceiving();
+    case MSG_CHECK_AND_PING:
+      OnCheckAndPing();
       break;
     default:
       ASSERT(false);
       break;
   }
 }
 
 // Handle queued up sort request
 void P2PTransportChannel::OnSort() {
   // Resort the connections based on the new statistics.
   SortConnections();
 }
 
-// Handle queued up ping request
-void P2PTransportChannel::OnPing() {
+// 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();
-
-  // Find the oldest pingable connection and have it do a ping.
-  Connection* conn = FindNextPingableConnection();
-  if (conn)
-    PingConnection(conn);
-
-  // Post ourselves a message to perform the next ping.
-  uint32 delay = writable() ? WRITABLE_DELAY : UNWRITABLE_DELAY;
-  thread()->PostDelayed(delay, this, MSG_PING);
-}
-
-void P2PTransportChannel::OnCheckReceiving() {
-  if (best_connection_) {
-    bool receiving = rtc::Time() <=
-        best_connection_->last_received() + receiving_timeout_;
-    set_receiving(receiving);
+  // When the best connection is either not receiving or not writable,
+  // switch to weak ping delay.
+  int ping_delay = Weak() ? WEAK_PING_DELAY : STRONG_PING_DELAY;
+  if (rtc::Time() >= last_ping_sent_ms_ + ping_delay) {
+    Connection* conn = FindNextPingableConnection();
+    if (conn) {
+      PingConnection(conn);
+    }
   }
-
-  thread()->PostDelayed(check_receiving_delay_, this, MSG_CHECK_RECEIVING);
+  int check_delay = std::min(ping_delay, check_receiving_delay_);
+  thread()->PostDelayed(check_delay, this, MSG_CHECK_AND_PING);
 }

[pthatcher1, 2015/09/22 23:41:46]

So we only check for receiving() every 500ms?  Doesn't that make the receive
timeout less precise?  Shouldn't we check receiving() more often than that so
that we know right after the timeout period that we're not receiving?

[honghaiz3, 2015/09/23 03:16:10]

We took the minimum of the ping_delay and check_receiving_delay. 
So it is smaller than that. 

 
 // Is the connection in a state for us to even consider pinging the other side?
 // We consider a connection pingable even if it's not connected because that's
 // how a TCP connection is kicked into reconnecting on the active side.
 bool P2PTransportChannel::IsPingable(Connection* conn) {
   const Candidate& remote = conn->remote_candidate();
   // We should never get this far with an empty remote ufrag.
   ASSERT(!remote.username().empty());
   if (remote.username().empty() || remote.password().empty()) {
     // If we don't have an ICE ufrag and pwd, there's no way we can ping.
     return false;
   }
 
   // An never connected connection cannot be written to at all, so pinging is
   // out of the question. However, if it has become WRITABLE, it is in the
   // reconnecting state so ping is needed.
   if (!conn->connected() && !conn->writable()) {
     return false;
   }
 
-  // If the channel is not writable, ping all candidates. Otherwise, we only
+  // If the channel is weak, ping all candidates. Otherwise, we only
   // want to ping connections that have not timed out on writing.
-  return !writable() || conn->write_state() != Connection::STATE_WRITE_TIMEOUT;
+  return Weak() || conn->write_state() != Connection::STATE_WRITE_TIMEOUT;
 }
 
 // Returns the next pingable connection to ping.  This will be the oldest
 // pingable connection unless we have a connected, writable connection that is
 // past the maximum acceptable ping delay. When reconnecting a TCP connection,
 // the best connection is disconnected, although still WRITABLE while
 // reconnecting. The newly created connection should be selected as the ping
 // target to become writable instead. See the big comment in CompareConnections.
 Connection* P2PTransportChannel::FindNextPingableConnection() {
   uint32 now = rtc::Time();
   if (best_connection_ && best_connection_->connected() &&
-      (best_connection_->write_state() == Connection::STATE_WRITABLE) &&
-      (best_connection_->last_ping_sent() + MAX_CURRENT_WRITABLE_DELAY <=
-       now)) {
+      best_connection_->writable() &&
+      (best_connection_->last_ping_sent() + MAX_CURRENT_STRONG_DELAY <= now)) {
     return best_connection_;
   }
 
   // First, find "triggered checks".  We ping first those connections
   // that have received a ping but have not sent a ping since receiving
   // it (last_received_ping > last_sent_ping).  But we shouldn't do
   // triggered checks if the connection is already writable.
   Connection* oldest_needing_triggered_check = nullptr;
   Connection* oldest = nullptr;
   for (Connection* conn : connections_) {
@@ skipping 38 matching lines @@
   bool use_candidate = false;
   if (remote_ice_mode_ == ICEMODE_FULL && ice_role_ == ICEROLE_CONTROLLING) {
     use_candidate = (conn == best_connection_) ||
                     (best_connection_ == NULL) ||
                     (!best_connection_->writable()) ||
                     (conn->priority() > best_connection_->priority());
   } else if (remote_ice_mode_ == ICEMODE_LITE && conn == best_connection_) {
     use_candidate = best_connection_->writable();
   }
   conn->set_use_candidate_attr(use_candidate);
-  conn->Ping(rtc::Time());
+  last_ping_sent_ms_ = rtc::Time();
+  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) {
@@ skipping 81 matching lines @@
   }
 }
 
 void P2PTransportChannel::OnReadyToSend(Connection* connection) {
   if (connection == best_connection_ && writable()) {
     SignalReadyToSend(this);
   }
 }
 
 }  // namespace cricket