Implement Turn/Turn first logic for connection selection.

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 c58a235f2dbc80483bc1915eaf089c573e25dc93..dc7e3a8aab235258a0eb30ec42a16736c444d8e3 100644
--- a/webrtc/p2p/base/p2ptransportchannel.cc
+++ b/webrtc/p2p/base/p2ptransportchannel.cc
@@ -198,7 +198,7 @@ const uint32_t WEAK_PING_DELAY = 1000 * PING_PACKET_SIZE / 10000;
 // 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_t MAX_CURRENT_STRONG_DELAY = 900;
+const uint32_t MAX_CURRENT_STRONG_DELAY = 900;
 
 static const int MIN_CHECK_RECEIVING_DELAY = 50;  // ms
 
@@ -212,6 +212,7 @@ P2PTransportChannel::P2PTransportChannel(const std::string& transport_name,
       worker_thread_(rtc::Thread::Current()),
       incoming_only_(false),
       error_(0),
+      connections_(this),
       best_connection_(NULL),
       pending_best_connection_(NULL),
       sort_dirty_(false),
@@ -394,6 +395,20 @@ void P2PTransportChannel::SetIceConfig(const IceConfig& config) {
     LOG(LS_INFO) << "Set ICE receiving timeout to " << receiving_timeout_
                  << " milliseconds";
   }
+
+  connections_.enable_most_likely_first(
+      config.ping_most_likely_candidate_pair_first);
+  LOG(LS_INFO) << "Set ping most likely connection to "
+               << connections_.most_likely_first();

[pthatcher1, 2016/01/27 19:59:58]

This might be better named prioritize_most_likely_to_work

[guoweis_webrtc, 2016/02/29 18:03:00]

Done.

+}
+
+IceConfig P2PTransportChannel::GetIceConfig() const {
+  IceConfig config;
+  config.gather_continually = gather_continually_;
+  config.receiving_timeout_ms = receiving_timeout_;
+  config.ping_most_likely_candidate_pair_first =
+      connections_.most_likely_first();
+  return config;

[pthatcher1, 2016/01/27 19:59:58]

Instead of converting between the struct and the member variables, can we just
store a struct internally as the member variable?

[guoweis_webrtc, 2016/02/29 18:03:00]

Done.

 }
 
 // Go into the state of processing candidates, and running in general
@@ -608,8 +623,8 @@ void P2PTransportChannel::OnUnknownAddress(
     }
   }
 
-  Connection* connection = port->CreateConnection(
-      remote_candidate, cricket::PortInterface::ORIGIN_THIS_PORT);
+  Connection* connection =
+      port->CreateConnection(remote_candidate, PortInterface::ORIGIN_THIS_PORT);
   if (!connection) {
     ASSERT(false);
     port->SendBindingErrorResponse(stun_msg, address, STUN_ERROR_SERVER_ERROR,
@@ -786,7 +801,7 @@ bool P2PTransportChannel::CreateConnection(PortInterface* port,
 
     // Don't create connection if this is a candidate we received in a
     // message and we are not allowed to make outgoing connections.
-    if (origin == cricket::PortInterface::ORIGIN_MESSAGE && incoming_only_)
+    if (origin == PortInterface::ORIGIN_MESSAGE && incoming_only_)
       return false;
 
     connection = port->CreateConnection(remote_candidate, origin);
@@ -802,8 +817,7 @@ bool P2PTransportChannel::CreateConnection(PortInterface* port,
   return true;
 }
 
-bool P2PTransportChannel::FindConnection(
-    cricket::Connection* connection) const {
+bool P2PTransportChannel::FindConnection(Connection* connection) const {
   std::vector<Connection*>::const_iterator citer =
       std::find(connections_.begin(), connections_.end(), connection);
   return citer != connections_.end();
@@ -1255,43 +1269,19 @@ bool P2PTransportChannel::IsPingable(Connection* conn, uint32_t now) {
 // 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 (connections_.empty()) {
+    return nullptr;
+  }

[pthatcher1, 2016/01/27 19:59:58]

Why is this needed?  Can't connection_ just handle it?

[guoweis_webrtc, 2016/02/29 18:03:00]

Done.

+
   uint32_t now = rtc::Time();
   if (best_connection_ && best_connection_->connected() &&
       best_connection_->writable() &&
       (best_connection_->last_ping_sent() + MAX_CURRENT_STRONG_DELAY <= now)) {
+    connections_.MarkConnectionPinged(best_connection_);

[pthatcher1, 2016/01/27 19:59:58]

I don't think FindNextPingableConnection should do the logic of causing the
candidate pair to be marked pinged.  It should only be marked pinged when the
ping is actually sent.

[guoweis_webrtc, 2016/02/29 18:03:00]

Removed this function.

     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_) {
-    if (!IsPingable(conn, now)) {
-      continue;
-    }
-    bool needs_triggered_check =
-        (!conn->writable() &&
-         conn->last_ping_received() > conn->last_ping_sent());
-    if (needs_triggered_check &&
-        (!oldest_needing_triggered_check ||
-         (conn->last_ping_received() <
-          oldest_needing_triggered_check->last_ping_received()))) {
-      oldest_needing_triggered_check = conn;
-    }
-    if (!oldest || (conn->last_ping_sent() < oldest->last_ping_sent())) {
-      oldest = conn;
-    }
-  }
-
-  if (oldest_needing_triggered_check) {
-    LOG(LS_INFO) << "Selecting connection for triggered check: " <<
-        oldest_needing_triggered_check->ToString();
-    return oldest_needing_triggered_check;
-  }
-  return oldest;
+  return connections_.FindNextPingableConnection(now);
 }
 
 // Apart from sending ping from |conn| this method also updates
@@ -1437,4 +1427,131 @@ void P2PTransportChannel::OnReadyToSend(Connection* connection) {
   }
 }
 
+P2PTransportChannel::Connections::Connections(P2PTransportChannel* channel)
+    : channel_(channel) {}

[pthatcher1, 2016/01/27 19:59:58]

I don't like how the Connections class depends on the TC and the TC depends on
the Connections.  It just seems too complex.  I looked though how the
Connections class uses TC, and it looks like everything comes down to
best_connection_ (and maybe state_ == completed, but that's derived from
connections_ and best_connection_).

So, I think we should either pass in best_connection_ to
FindNextPingableConnection or make it possible to keep
FindNextPingableConnection in TC and expose whatever needs to be exposed from
Connections.  

Or, we could just have TC store both pinged_connections_ and
unpinged_connections_, and then have a connections() method that returns an
iterator that iterates over both.  That way, we just have to implement an
iterator that can cover both rather than a whole collection class.

[guoweis_webrtc, 2016/02/29 18:03:00]

Agree. Removed Connections and move these 2 to TC.

+
+void P2PTransportChannel::Connections::enable_most_likely_first(bool enable) {
+  ping_most_likely_first_ = enable;
+}
+
+P2PTransportChannel::Connections::iterator
+P2PTransportChannel::Connections::erase(
+    P2PTransportChannel::Connections::iterator pos) {
+  pinged_connections_.erase(*pos);
+  unpinged_connections_.erase(*pos);
+  return Base::erase(pos);
+}
+
+void P2PTransportChannel::Connections::push_back(Connection*& connection) {
+  Base::push_back(connection);
+  unpinged_connections_.insert(connection);
+}
+
+Connection* P2PTransportChannel::Connections::FindNextPingableConnection(
+    uint32_t now) {
+  RTC_CHECK(size() ==
+            pinged_connections_.size() + unpinged_connections_.size());
+
+  // If there is nothing pingable in the |unpinged_connections_|, copy over from
+  // |pinged_connections_|.
+  if (std::find_if(unpinged_connections_.begin(), unpinged_connections_.end(),
+                   [this, now](Connection* conn) {
+                     return channel_->IsPingable(conn, now);
+                   }) == unpinged_connections_.end()) {
+    unpinged_connections_.insert(pinged_connections_.begin(),
+                                 pinged_connections_.end());
+    pinged_connections_.clear();

[pthatcher1, 2016/01/27 19:59:58]

Shouldn't this happen after MarkConnectionPinged results in an empty
unpinged_connections_?

[guoweis_webrtc, 2016/02/29 18:03:00]

If the unpinged_connections has only unpingable connections, it's virtually
empty.

+  }
+
+  // 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;
+  for (auto conn : *this) {
+    if (!channel_->IsPingable(conn, now)) {
+      continue;
+    }
+    bool needs_triggered_check =
+        (!conn->writable() &&
+         conn->last_ping_received() > conn->last_ping_sent());
+    if (needs_triggered_check &&
+        (!oldest_needing_triggered_check ||
+         (conn->last_ping_received() <
+          oldest_needing_triggered_check->last_ping_received()))) {
+      oldest_needing_triggered_check = conn;
+    }
+  }

[pthatcher1, 2016/01/27 19:59:58]

I think having a separate FindOldestConnectionNeedingTriggeredCheck would be
nice.

[guoweis_webrtc, 2016/02/29 18:03:00]

Done.

+
+  if (oldest_needing_triggered_check) {
+    LOG(LS_INFO) << "Selecting connection for triggered check: "
+                 << oldest_needing_triggered_check->ToString();
+    MarkConnectionPinged(oldest_needing_triggered_check);

[pthatcher1, 2016/01/27 19:59:58]

Again, I don't like that just finding it marks it pinged. It should only be
marked pinged once it's actually pinged.

[guoweis_webrtc, 2016/02/29 18:03:00]

Removed.

+    return oldest_needing_triggered_check;
+  }
+
+  Connection* conn_to_ping = nullptr;
+  for (Connection* conn : unpinged_connections_) {
+    if (!channel_->IsPingable(conn, now)) {
+      continue;
+    }
+    if (!conn_to_ping || Compare(conn_to_ping, conn) == conn) {
+      conn_to_ping = conn;
+    }
+  }
+
+  if (conn_to_ping) {
+    MarkConnectionPinged(conn_to_ping);
+  }
+
+  return conn_to_ping;
+}
+
+void P2PTransportChannel::Connections::MarkConnectionPinged(
+    Connection* connection) {
+  if (pinged_connections_.insert(connection).second) {
+    unpinged_connections_.erase(connection);
+  }
+}
+
+Connection* P2PTransportChannel::Connections::Compare(Connection* conn1,
+                                                      Connection* conn2) {
+  RTC_DCHECK(conn1 != conn2);
+  if (ping_most_likely_first_) {
+    bool conn1_turn_turn = IsTurnTurn(conn1);

[pthatcher1, 2016/01/27 19:59:58]

rr1 would be nicer, shorter name.

[guoweis_webrtc, 2016/02/29 18:03:00]

Done.

+    bool conn2_turn_turn = IsTurnTurn(conn2);
+    if (conn1_turn_turn ^ conn2_turn_turn) {
+      return conn1_turn_turn ? conn1 : conn2;
+    }
+    if (conn1_turn_turn && conn2_turn_turn) {

[pthatcher1, 2016/01/27 19:59:58]

This would be more readable as:

bool rr1 == IsRelayRelay(conn1);
bool rr2 == IsRelayRelay(conn2);
if (rr1 && !rr2) {
  return conn1;
} else if (rr2 && !rr1) {
  return conn2;
} else if (rr1 && rr2) {
  bool udp1 = IsUdp(conn1);
  bool udp2 = IsUdp(conn2);
  if (udp1 && !udp2) {
    return conn1;
  } else if (udp2 && udp1) {
    return conn2;
  }
}

And, actually, it would be nice to put that in a separate MostLikelyToWork
method.

if (prioritize_most_likely_to_work_) {
  Connection* most_likely_to_work_conn = MostLikelyToWork(conn1, conn2);
  if (most_likely_to_work_conn) {
    return most_likely_to_work_conn;
  }
}

[guoweis_webrtc, 2016/02/29 18:03:00]

Done.

+      bool conn1_udp_relay =
+          conn1->local_candidate().relay_protocol() == UDP_PROTOCOL_NAME;
+      bool conn2_udp_relay =
+          conn2->local_candidate().relay_protocol() == UDP_PROTOCOL_NAME;
+      if (conn1_udp_relay ^ conn2_udp_relay) {
+        return conn1_udp_relay ? conn1 : conn2;
+      }
+    }
+  }
+
+  if (conn1->last_ping_sent() < conn2->last_ping_sent()) {
+    return conn1;
+  }
+  if (conn1->last_ping_sent() > conn2->last_ping_sent()) {
+    return conn2;
+  }

[pthatcher1, 2016/01/27 19:59:58]

And similarly, we could make a method for LeastRecentlyPinged:

Conneciton* conn = LeastRecentlyPinged(conn1, conn2);
if (conn) {
  return conn;
}

[guoweis_webrtc, 2016/02/29 18:03:00]

Done.

+
+  // During the initial state when nothing has been pinged yet, return the first
+  // one in the ordered |connections_|.
+  return *(std::find_if(Base::begin(), Base::end(),
+                        [conn1, conn2](Connection* conn) {
+                          return conn == conn1 || conn == conn2;
+                        }));
+}
+
+bool P2PTransportChannel::Connections::IsTurnTurn(Connection* conn) {

[pthatcher1, 2016/01/27 19:59:58]

IsRelayToRelay would be a more accurate name.

[guoweis_webrtc, 2016/02/29 18:03:00]

Done.

+  return conn->local_candidate().type() == RELAY_PORT_TYPE &&
+         conn->remote_candidate().type() == RELAY_PORT_TYPE;
+}
+
 }  // namespace cricket