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

[pthatcher1, 2015/09/17 05:58:42]

Why did you combine these?  I think it made sense to be separate.

[honghaiz3, 2015/09/17 19:47:56]

There are two reasons. 
1. When you do ping, you also check the connections states, which makes it sort
of redundant for checking (except for the frequency).
2. When we check and fing the connection not receiving, we want to switch to a
short ping period without waiting another ping coming. 

 
 // 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
+static const uint32 LONG_PING_DELAY = 1000 * PING_PACKET_SIZE / 1000;  // 480ms
+static const uint32 SHORT_PING_DELAY = 1000 * PING_PACKET_SIZE / 10000;  // 50ms

[pthatcher1, 2015/09/17 05:58:41]

I think a good name for these would be WEAK_CONNECTION_PING_DELAY and
STRONG_CONNECTION_PING_DELAY,  and then comment that STRONG == writable &&
receiving, and WEAK == !writable || !receiving.

[honghaiz3, 2015/09/17 19:47:55]

Done.

 
 // 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
+// 2 * LONG_PING_DELAY - bit
+static const uint32 MAX_CURRENT_WRITABLE_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 12 matching lines @@
   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) {
+int CompareConnectionStates(cricket::Connection* a, cricket::Connection* b) {
+  // When the current best connection is not receiving but a backup connection
+  // is receiving, the backup connection has a higher priority regardless
+  // its writable state.

[pthatcher1, 2015/09/17 05:58:42]

It doesn't need to just be "backup".  We could simply say that a receiving
connection is better than a non-receiving one.

[honghaiz3, 2015/09/17 19:47:55]

Done.

+  if (a->receiving() && !b->receiving())
+    return 1;
+  if (!a->receiving() && b->receiving())
+    return -1;
+
   // 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;

[pthatcher1, 2015/09/17 05:58:42]

So what's better?  receiving-but-non-writable or writable-but-not-receiving?  
What you have is that receiving-but-non-writable is better.  I think
writable-but-not-receiving should be better because then if there are 2 seconds
of no packets, we wouldn't switch to a non-writable connection.

If I'm write, the receiving comparison should come after the write state
comparison.

What do you think?

 
   // 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
@@ skipping 12 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 18 matching lines @@
     // 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) {
+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 nominate state. Otherwise, do not switch if |a_conn| is

[pthatcher1, 2015/09/17 05:58:42]

nominate => nominated

[honghaiz3, 2015/09/17 19:47:56]

Done.

+  // nominated but |b_conn| is not.
+  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() > b_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),
+      next_connection_to_ping_(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),
+      ping_delay_(SHORT_PING_DELAY),
+      last_ping_sent_(0) {}
 
 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 10 matching lines @@
   session->SignalCandidatesReady.connect(
       this, &P2PTransportChannel::OnCandidatesReady);
   session->SignalCandidatesAllocationDone.connect(
       this, &P2PTransportChannel::OnCandidatesAllocationDone);
   session->StartGettingPorts();
 }
 
 void P2PTransportChannel::AddConnection(Connection* connection) {
   connections_.push_back(connection);
   connection->set_remote_ice_mode(remote_ice_mode_);
+  connection->set_receiving_timeout(receiving_timeout_);
   connection->SignalReadPacket.connect(
       this, &P2PTransportChannel::OnReadPacket);
   connection->SignalReadyToSend.connect(
       this, &P2PTransportChannel::OnReadyToSend);
   connection->SignalStateChange.connect(
       this, &P2PTransportChannel::OnConnectionStateChange);
   connection->SignalDestroyed.connect(
       this, &P2PTransportChannel::OnConnectionDestroyed);
   connection->SignalNominated.connect(this, &P2PTransportChannel::OnNominated);
 }
@@ skipping 101 matching lines @@
 
 void P2PTransportChannel::SetReceivingTimeout(int receiving_timeout_ms) {
   if (receiving_timeout_ms < 0) {
     return;
   }
   receiving_timeout_ = receiving_timeout_ms;
   check_receiving_delay_ =
       std::max(MIN_CHECK_RECEIVING_DELAY, receiving_timeout_ / 10);
   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
 void P2PTransportChannel::Connect() {
   ASSERT(worker_thread_ == rtc::Thread::Current());
   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);
+  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 18 matching lines @@
   port->SignalDestroyed.connect(this, &P2PTransportChannel::OnPortDestroyed);
   port->SignalRoleConflict.connect(
       this, &P2PTransportChannel::OnRoleConflict);
 
   // Attempt to create a connection from this new port to all of the remote
   // candidates that we were given so far.
 
   std::vector<RemoteCandidate>::iterator iter;
   for (iter = remote_candidates_.begin(); iter != remote_candidates_.end();
        ++iter) {
-    CreateConnection(port, *iter, iter->origin_port(), false);
+    CreateConnection(port, *iter, iter->origin_port());
   }
 
   SortConnections();
 }
 
 // A new candidate is available, let listeners know
 void P2PTransportChannel::OnCandidatesReady(
     PortAllocatorSession *session, const std::vector<Candidate>& candidates) {
   ASSERT(worker_thread_ == rtc::Thread::Current());
   for (size_t i = 0; i < candidates.size(); ++i) {
@@ skipping 195 matching lines @@
   // remote candidate with an older generation arrives, drop it.
   if (generation != 0 && generation < remote_candidate_generation_) {
     LOG(LS_WARNING) << "Dropping a remote candidate because its generation "
                     << generation
                     << " is lower than the current remote generation "
                     << remote_candidate_generation_;
     return;
   }
 
   // Create connections to this remote candidate.
-  CreateConnections(candidate, NULL, false);
+  CreateConnections(candidate, NULL);
 
   // Resort the connections list, which may have new elements.
   SortConnections();
 }
 
 // Creates connections from all of the ports that we care about to the given
 // remote candidate.  The return value is true if we created a connection from
 // the origin port.
 bool P2PTransportChannel::CreateConnections(const Candidate& remote_candidate,
-                                            PortInterface* origin_port,
-                                            bool readable) {
+                                            PortInterface* origin_port) {
   ASSERT(worker_thread_ == rtc::Thread::Current());
 
   Candidate new_remote_candidate(remote_candidate);
   new_remote_candidate.set_generation(
       GetRemoteCandidateGeneration(remote_candidate));
   // ICE candidates don't need to have username and password set, but
   // the code below this (specifically, ConnectionRequest::Prepare in
   // port.cc) uses the remote candidates's username.  So, we set it
   // here.
   if (remote_candidate.username().empty()) {
@@ skipping 17 matching lines @@
   }
 
   // Add a new connection for this candidate to every port that allows such a
   // connection (i.e., if they have compatible protocols) and that does not
   // already have a connection to an equivalent candidate.  We must be careful
   // to make sure that the origin port is included, even if it was pruned,
   // since that may be the only port that can create this connection.
   bool created = false;
   std::vector<PortInterface *>::reverse_iterator it;
   for (it = ports_.rbegin(); it != ports_.rend(); ++it) {
-    if (CreateConnection(*it, new_remote_candidate, origin_port, readable)) {
+    if (CreateConnection(*it, new_remote_candidate, origin_port)) {
       if (*it == origin_port)
         created = true;
     }
   }
 
   if ((origin_port != NULL) &&
       std::find(ports_.begin(), ports_.end(), origin_port) == ports_.end()) {
-    if (CreateConnection(
-        origin_port, new_remote_candidate, origin_port, readable))
+    if (CreateConnection(origin_port, new_remote_candidate, origin_port))
       created = true;
   }
 
   // Remember this remote candidate so that we can add it to future ports.
   RememberRemoteCandidate(new_remote_candidate, origin_port);
 
   return created;
 }
 
 // Setup a connection object for the local and remote candidate combination.
 // And then listen to connection object for changes.
 bool P2PTransportChannel::CreateConnection(PortInterface* port,
                                            const Candidate& remote_candidate,
-                                           PortInterface* origin_port,
-                                           bool readable) {
+                                           PortInterface* origin_port) {
   // Look for an existing connection with this remote address.  If one is not
   // found, then we can create a new connection for this address.
   Connection* connection = port->GetConnection(remote_candidate.address());
   if (connection != NULL) {
     connection->MaybeUpdatePeerReflexiveCandidate(remote_candidate);
 
     // It is not legal to try to change any of the parameters of an existing
     // connection; however, the other side can send a duplicate candidate.
     if (!remote_candidate.IsEquivalent(connection->remote_candidate())) {
       LOG(INFO) << "Attempt to change a remote candidate."
@@ skipping 14 matching lines @@
     connection = port->CreateConnection(remote_candidate, origin);
     if (!connection)
       return false;
 
     AddConnection(connection);
 
     LOG_J(LS_INFO, this) << "Created connection with origin=" << origin << ", ("
                          << connections_.size() << " total)";
   }
 
-  // If we are readable, it is because we are creating this in response to a
-  // ping from the other side.  This will cause the state to become readable.
-  if (readable)
-    connection->ReceivedPing();
-
   return true;
 }
 
 bool P2PTransportChannel::FindConnection(
     cricket::Connection* connection) const {
   std::vector<Connection*>::const_iterator citer =
       std::find(connections_.begin(), connections_.end(), connection);
   return citer != connections_.end();
 }
 
@@ skipping 104 matching lines @@
 bool P2PTransportChannel::GetStats(ConnectionInfos *infos) {
   ASSERT(worker_thread_ == rtc::Thread::Current());
   // Gather connection infos.
   infos->clear();
 
   std::vector<Connection *>::const_iterator it;
   for (it = connections_.begin(); it != connections_.end(); ++it) {
     Connection *connection = *it;
     ConnectionInfo info;
     info.best_connection = (best_connection_ == connection);
-    info.readable =
-        (connection->read_state() == Connection::STATE_READABLE);
+    info.receiving = connection->receiving();
     info.writable =
         (connection->write_state() == Connection::STATE_WRITABLE);
     info.timeout =
         (connection->write_state() == Connection::STATE_WRITE_TIMEOUT);
     info.new_connection = !connection->reported();
     connection->set_reported(true);
     info.rtt = connection->rtt();
     info.sent_total_bytes = connection->sent_total_bytes();
     info.sent_bytes_second = connection->sent_bytes_second();
     info.sent_discarded_packets = connection->sent_discarded_packets();
@@ skipping 20 matching lines @@
 // Begin allocate (or immediately re-allocate, if MSG_ALLOCATE pending)
 void P2PTransportChannel::Allocate() {
   // Time for a new allocator, lets make sure we have a signalling channel
   // to communicate candidates through first.
   waiting_for_signaling_ = true;
   SignalRequestSignaling(this);
 }
 
 // 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);
 }
 
 // Prepare for best candidate sorting.
 void P2PTransportChannel::RequestSort() {
   if (!sort_dirty_) {
     worker_thread_->Post(this, MSG_SORT);
     sort_dirty_ = true;
   }
@@ skipping 18 matching lines @@
   ConnectionCompare cmp;
   std::stable_sort(connections_.begin(), connections_.end(), cmp);
   LOG(LS_VERBOSE) << "Sorting 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 (top_connection != best_connection_ && best_connection_->writable() &&
+      !top_connection->writable()) {
+    // This is a very special case where the best_connection becomes
+    // not receiving but the top connection is not writable.

[pthatcher1, 2015/09/17 05:58:41]

It seems like, instead, that we should always go into "high speed pinging"
whenever we are "weak" in our connectivity, rather than wait until we have a top
connection to ping.  I think that would be a lot more simple.

[honghaiz3, 2015/09/17 19:47:56]

Yes. That is what is done. 
Combining the comments, I am removing this extra part and sort connections based
on write state first and then receiving. This is also safer.  

+    // We don't switch but check the writability of the |top_connection|.
+    // If we are in the long ping delay, reschedule it so that it will be
+    // executed much sooner. If we are in the short ping delay, just make
+    // it the next one to be pinged, so that we do not overflow the channel
+    // with ping messages.
+    if (ping_delay_ == LONG_PING_DELAY) {
+      thread()->Clear(this, MSG_CHECK_AND_PING);
+      thread()->Post(this, MSG_CHECK_AND_PING);
+    }
+    next_connection_to_ping_ = top_connection;

[pthatcher1, 2015/09/17 05:58:42]

Shouldn't the faster pinging work fine without storing next_connection_to_ping_?

[honghaiz3, 2015/09/17 19:47:56]

Yes. We don't need the extra speedup of the ping request.  

[pthatcher1, 2015/09/17 22:01:17]

So are you going to remove next_connection_to_ping_?

+  } else if (ShouldSwitch(best_connection_, top_connection, ice_role_)) {
+    // 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.
     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 29 matching lines @@
 
 void P2PTransportChannel::PruneConnections() {
   // We can prune any connection for which there is a connected, writable
   // connection on the same network with better or equal priority.  We leave
   // those with better priority just in case they become writable later (at
   // 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
   // reconnecting a TCP connection and temporarily do not prune connections in
-  // this network. See the big comment in CompareConnections.
+  // this network. See the big comment in CompareConnections. If the |primier|
+  // connection is not receiving, we do not prune the connections in the network
+  // because either some connection will become receiving or all connections
+  // in the network will time out and be deleted.

[pthatcher1, 2015/09/17 05:58:42]

I think this would be more clear as "once on connection is receiving, we begin
pruning other connections on the same network".

[honghaiz3, 2015/09/17 19:47:55]

Removed the comments as we change back to check writable.

 
   // Get a list of the networks that we are using.
   std::set<rtc::Network*> networks;
   for (const Connection* conn : connections_) {
     networks.insert(conn->port()->Network());
   }
   for (rtc::Network* network : networks) {
     Connection* primier = GetBestConnectionOnNetwork(network);
-    if (!(primier && primier->writable() && primier->connected())) {
+    if (!(primier && primier->receiving() && primier->connected())) {

[pthatcher1, 2015/09/17 05:58:41]

Why don't we want to keep it as writable()?  Shouldn't we wait until a
connection is writable before pruning other connections?

[honghaiz3, 2015/09/17 19:47:56]

Yes. especially now that we put writable with higher priority then receiving

       continue;
     }
 
     for (Connection* conn : connections_) {
       if ((conn != primier) && (conn->port()->Network() == network) &&
+          (conn != pending_best_connection_) &&
           (CompareConnectionCandidates(primier, conn) >= 0)) {
         conn->Prune();
       }
     }
   }
 }
 
 // Track the best connection, and let listeners know
 void P2PTransportChannel::SwitchBestConnectionTo(Connection* conn) {
   // Note: if conn is NULL, the previous best_connection_ has been destroyed,
@@ skipping 11 matching lines @@
     // When it just switched to a best connection, set receiving to true.
     set_receiving(true);
   } 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";
 
-  bool readable = false;
-  for (uint32 i = 0; i < connections_.size(); ++i) {
-    if (connections_[i]->read_state() == Connection::STATE_READABLE) {
-      readable = true;
-      break;
-    }
-  }
-  set_readable(readable);
+  bool receiving = best_connection_ && best_connection_->receiving();

[pthatcher1, 2015/09/17 05:58:42]

Shouldn't the logic here be the same as the previous logic:  if *any* connection
is receiving, the channel is?

[honghaiz3, 2015/09/17 19:47:56]

Done. 
I was thinking that we should use the best_connection's receiving to decide
certain actions (notify upper layer and switch to the fast ping mode) but I also
see the reasons for checking any connection.

+  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 35 matching lines @@
 
   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() {
-  // Make sure the states of the connections are up-to-date (since this affects
-  // which ones are pingable).
+void P2PTransportChannel::OnCheckAndPing() {
   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() {
-  // Check receiving only if the best connection has received data packets
-  // because we want to detect not receiving any packets only after the media
-  // have started flowing.
-  if (best_connection_ && best_connection_->recv_total_bytes() > 0) {
-    bool receiving = rtc::Time() <=
-        best_connection_->last_received() + receiving_timeout_;
-    set_receiving(receiving);
+  // When the best connection is not receiving or writable, switch to short ping
+  // delay.

[pthatcher1, 2015/09/17 05:58:41]

not receiving or writable => not receiving or not writable

[honghaiz3, 2015/09/17 19:47:56]

Done with neither nor. 

+  ping_delay_ = writable() && receiving() ? LONG_PING_DELAY : SHORT_PING_DELAY;

[pthatcher1, 2015/09/17 05:58:41]

We might even consider making a method 

bool weak() { return !writable() && !receiving(); }

Then you'd have delay = weak() ? WEAK_CONNECTION_PING_DELAY :
STRONG_CONNECTION_PING_DELAY;

[honghaiz3, 2015/09/17 19:47:56]

Done.

+  uint32 now = rtc::Time();
+  if (now >= last_ping_sent_ + ping_delay_) {

[pthatcher1, 2015/09/17 05:58:41]

Might be more readable as:

uint32 time_since_last_ping = rtc::Time() - last_ping_sent_;
if (time_since_last_ping > delay) {
  ...
}

[honghaiz3, 2015/09/17 19:47:55]

Similar to the other comments. I try to avoid subtraction on two unsigned
number.  

+    Connection* conn = FindNextPingableConnection();
+    if (conn) {
+      PingConnection(conn);
+    }
   }
-
-  thread()->PostDelayed(check_receiving_delay_, this, MSG_CHECK_RECEIVING);
+  uint32 check_delay = std::min(ping_delay_, check_receiving_delay_);
+  thread()->PostDelayed(check_delay, this, MSG_CHECK_AND_PING);

[pthatcher1, 2015/09/17 05:58:41]

I think this was more clear before with two "loops" running.   Was there a good
reason to combine them?

[honghaiz3, 2015/09/17 19:47:55]

For the two reasons I mentioned at the top. 
If you still feel better to use two loops, I will change it back. 

 }
 
 // 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->write_state() != Connection::STATE_WRITABLE) {
     return false;
   }
 

[pthatcher1, 2015/09/17 05:58:41]

Might be more readable as:

// If the channel is weak, then ping pruned candidates.  Otherwise, don't.
return weak() || !conn->pruned();

[honghaiz3, 2015/09/17 19:47:56]

Done.

-  if (writable()) {
-    // If we are writable, then we only want to ping connections that could be
-    // better than this one, i.e., the ones that were not pruned.
-    return (conn->write_state() != Connection::STATE_WRITE_TIMEOUT);
-  } else {
-    // If we are not writable, then we need to try everything that might work.
-    // This includes both connections that do not have write timeout as well as
-    // ones that do not have read timeout.  A connection could be readable but
-    // be in write-timeout if we pruned it before.  Since the other side is
-    // still pinging it, it very well might still work.
-    return (conn->write_state() != Connection::STATE_WRITE_TIMEOUT) ||
-           (conn->read_state() != Connection::STATE_READ_TIMEOUT);
+  if (writable() && receiving()) {
+    // If we are writable and also receiving, then we only want to ping
+    // connections that could be better than this one, i.e., the ones that
+    // were not pruned.
+    return !conn->pruned();
   }
+  // If we are not writable, then we need to try everything that might work.
+  return true;
 }
 
 // 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() {
+  if (next_connection_to_ping_ && !next_connection_to_ping_->writable()) {
+    Connection* conn = next_connection_to_ping_;
+    next_connection_to_ping_ = nullptr;
+    return conn;
+  }
   uint32 now = rtc::Time();
   if (best_connection_ && best_connection_->connected() &&
-      (best_connection_->write_state() == Connection::STATE_WRITABLE) &&
+      best_connection_->writable() && best_connection_->receiving() &&
       (best_connection_->last_ping_sent() + MAX_CURRENT_WRITABLE_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;
@@ skipping 40 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_ = rtc::Time();
+  conn->Ping(last_ping_sent_);
 }
 
 // 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