A few updates on connection states

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 ee9906053cd772766821e491b4a42c826a211a78..f06473a31dd9a07486d898e6724b1e2ad6463b40 100644
--- a/webrtc/p2p/base/p2ptransportchannel.cc
+++ b/webrtc/p2p/base/p2ptransportchannel.cc
@@ -22,11 +22,7 @@
 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
@@ -34,12 +30,17 @@ enum {
 // 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 not
+// writable or not receiving.
+static const uint32 STRONG_PING_DELAY = 1000 * PING_PACKET_SIZE / 1000;
+// WEAK_PING_DELAY (48ms) is applied when the best connection is both
+// writable and 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
+// make sure it is pinged at this rate (a little less than
+// 2 * STRONG_PING_DELAY).
+static const uint32 MAX_CURRENT_WRITABLE_DELAY = 900;
 
 static const int MIN_CHECK_RECEIVING_DELAY = 50;  // ms
 
@@ -70,15 +71,20 @@ int CompareConnectionCandidates(cricket::Connection* a,
          (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;
+
   // 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
@@ -110,7 +116,14 @@ int CompareConnections(cricket::Connection *a, cricket::Connection *b) {
       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);
 }
@@ -148,19 +161,33 @@ class ConnectionCompare {
 };
 
 // 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 RECEIVING/WRITE/CONNECT states are different, we should switch
+  // regardless of the nominated state. Otherwise, do not switch if |a_conn| is
+  // nominated.
+  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;
 }
@@ -172,25 +199,25 @@ 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),
+      last_ping_sent_(0) {}
 
 P2PTransportChannel::~P2PTransportChannel() {
   ASSERT(worker_thread_ == rtc::Thread::Current());
@@ -256,23 +283,18 @@ void P2PTransportChannel::SetIceTiebreaker(uint64 tiebreaker) {
 }
 
 // Currently a channel is considered ICE completed once there is no
-// more than one connection per Network. This works for a single NIC
-// with both IPv4 and IPv6 enabled. However, this condition won't
-// happen when there are multiple NICs and all of them have
-// connectivity.
-// TODO(guoweis): Change Completion to be driven by a channel level
-// timer.
+// more than one connection per Network that is not pruned.
+// If all connections are pruned, declare transport failure.
 TransportChannelState P2PTransportChannel::GetState() const {
-  std::set<rtc::Network*> networks;
-
-  if (connections_.size() == 0) {
-    return TransportChannelState::STATE_FAILED;
-  }
-
-  for (uint32 i = 0; i < connections_.size(); ++i) {
-    rtc::Network* network = connections_[i]->port()->Network();
-    if (networks.find(network) == networks.end()) {
-      networks.insert(network);
+  std::set<rtc::Network*> networks_with_unpruned_connections;
+  for (const Connection* connection : connections_) {
+    if (connection->pruned()) {
+      continue;
+    }
+    rtc::Network* network = connection->port()->Network();
+    if (networks_with_unpruned_connections.find(network) ==
+        networks_with_unpruned_connections.end()) {
+      networks_with_unpruned_connections.insert(network);
     } else {
       LOG_J(LS_VERBOSE, this) << "Ice not completed yet for this channel as "
                               << network->ToString()
@@ -280,8 +302,11 @@ TransportChannelState P2PTransportChannel::GetState() const {
       return TransportChannelState::STATE_CONNECTING;
     }
   }
-  LOG_J(LS_VERBOSE, this) << "Ice is completed for this channel.";
+  if (networks_with_unpruned_connections.size() == 0) {
+    return TransportChannelState::STATE_FAILED;
+  }
 
+  LOG_J(LS_VERBOSE, this) << "Ice is completed for this channel.";
   return TransportChannelState::STATE_COMPLETED;
 }
 
@@ -347,6 +372,9 @@ void P2PTransportChannel::SetReceivingTimeout(int receiving_timeout_ms) {
   }
   LOG(LS_VERBOSE) << "Set ICE receiving timeout to " << receiving_timeout_
                   << " milliseconds";
+  for (Connection* connection : connections_) {
+    connection->set_receiving_timeout(receiving_timeout_);
+  }
 }
 
 // Go into the state of processing candidates, and running in general
@@ -363,10 +391,7 @@ void P2PTransportChannel::Connect() {
   Allocate();
 
   // Start pinging as the ports come in.
-  thread()->Post(this, MSG_PING);
-
-  thread()->PostDelayed(
-      check_receiving_delay_, this, MSG_CHECK_RECEIVING);
+  thread()->Post(this, MSG_CHECK_AND_PING);
 }
 
 // A new port is available, attempt to make connections for it
@@ -848,6 +873,9 @@ bool P2PTransportChannel::GetStats(ConnectionInfos *infos) {
   std::vector<Connection *>::const_iterator it;
   for (it = connections_.begin(); it != connections_.end(); ++it) {
     Connection *connection = *it;
+    if (connection->pruned() && !connection->receiving()) {
+      continue;
+    }
     ConnectionInfo info;
     info.best_connection = (best_connection_ == connection);
     info.receiving = connection->receiving();
@@ -891,10 +919,9 @@ void P2PTransportChannel::Allocate() {
 
 // Monitor connection states.
 void P2PTransportChannel::UpdateConnectionStates() {
-  uint32 now = rtc::Time();
-
   // We need to copy the list of connections since some may delete themselves
   // when we call UpdateState.
+  uint32 now = rtc::Time();
   for (uint32 i = 0; i < connections_.size(); ++i)
     connections_[i]->UpdateState(now);
 }
@@ -925,7 +952,8 @@ void P2PTransportChannel::SortConnections() {
   // 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();
   }
@@ -936,10 +964,7 @@ void P2PTransportChannel::SortConnections() {
   // 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);
   }
@@ -987,7 +1012,7 @@ void P2PTransportChannel::PruneConnections() {
   // which point, we would prune out the current best connection).  We leave
   // connections on other networks because they may not be using the same
   // resources and they may represent very distinct paths over which we can
-  // switch. If the |primier| connection is not connected, we may be
+  // switch. If the |premier| connection is not connected, we may be
   // reconnecting a TCP connection and temporarily do not prune connections in
   // this network. See the big comment in CompareConnections.
 
@@ -997,14 +1022,14 @@ void P2PTransportChannel::PruneConnections() {
     networks.insert(conn->port()->Network());
   }
   for (rtc::Network* network : networks) {
-    Connection* primier = GetBestConnectionOnNetwork(network);
-    if (!(primier && primier->writable() && primier->connected())) {
+    Connection* premier = GetBestConnectionOnNetwork(network);
+    if (!(premier && premier->writable() && premier->connected())) {
       continue;
     }
-
+    premier->Unprune();  // Un-prune the premier connection.
     for (Connection* conn : connections_) {
-      if ((conn != primier) && (conn->port()->Network() == network) &&
-          (CompareConnectionCandidates(primier, conn) >= 0)) {
+      if ((conn != premier) && (conn->port()->Network() == network) &&
+          (CompareConnectionCandidates(premier, conn) >= 0)) {
         conn->Prune();
       }
     }
@@ -1034,15 +1059,15 @@ void P2PTransportChannel::SwitchBestConnectionTo(Connection* conn) {
 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 =
+      std::any_of(connections_.begin(), connections_.end(),
+                  [](const Connection* conn) { return conn->receiving(); });
+  set_receiving(receiving);
 }
 
 // We checked the status of our connections and we had at least one that
@@ -1075,6 +1100,10 @@ void P2PTransportChannel::HandleAllTimedOut() {
   HandleNotWritable();
 }
 
+bool P2PTransportChannel::Weak() const {
+  return !(best_connection_ && best_connection_->receiving() && writable());
+}
+
 // If we have a best connection, return it, otherwise return top one in the
 // list (later we will mark it best).
 Connection* P2PTransportChannel::GetBestConnectionOnNetwork(
@@ -1098,11 +1127,8 @@ void P2PTransportChannel::OnMessage(rtc::Message *pmsg) {
     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);
@@ -1116,30 +1142,22 @@ void P2PTransportChannel::OnSort() {
   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_ + 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);
 }
 
 // Is the connection in a state for us to even consider pinging the other side?
@@ -1161,9 +1179,9 @@ bool P2PTransportChannel::IsPingable(Connection* conn) {
     return false;
   }
 
-  // If the channel is not writable, 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;
+  // If the channel is weak, ping all candidates. Otherwise, ping only
+  // un-pruned and not-write-timed-out candidates.
+  return Weak() || (!conn->pruned() && !conn->write_timed_out());
 }
 
 // Returns the next pingable connection to ping.  This will be the oldest
@@ -1175,7 +1193,7 @@ bool P2PTransportChannel::IsPingable(Connection* conn) {
 Connection* P2PTransportChannel::FindNextPingableConnection() {
   uint32 now = rtc::Time();
   if (best_connection_ && best_connection_->connected() &&
-      (best_connection_->write_state() == Connection::STATE_WRITABLE) &&
+      best_connection_->writable() &&
       (best_connection_->last_ping_sent() + MAX_CURRENT_WRITABLE_DELAY <=
        now)) {
     return best_connection_;
@@ -1236,7 +1254,8 @@ void P2PTransportChannel::PingConnection(Connection* conn) {
     use_candidate = best_connection_->writable();
   }
   conn->set_use_candidate_attr(use_candidate);
-  conn->Ping(rtc::Time());
+  last_ping_sent_ = rtc::Time();
+  conn->Ping(last_ping_sent_);
 }
 
 // When a connection's state changes, we need to figure out who to use as
@@ -1323,10 +1342,13 @@ void P2PTransportChannel::OnReadPacket(
   // 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 && !best_nominated_connection() &&
-      connection->writable() && best_connection_ != connection) {
+  // If the receiving connection is different from the sending connection,
+  // may need to switch the sending connection based on the receiving media
+  // path if this is the controlled side. It will not switch if the sending
+  // connection is nominated but the receiving connection is not nominated.
+  if (best_connection_ != connection && ice_role_ == ICEROLE_CONTROLLED &&
+      (!best_nominated_connection() || connection->nominated()) &&
+      connection->writable()) {
     SwitchBestConnectionTo(connection);
   }
 }