Implement Turn/Turn first logic for connection selection.

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 180 matching lines @@
 // STRONG_PING_DELAY (480ms) is applied when the best connection is both
 // writable and receiving.
 static const uint32_t 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.
 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
 
 P2PTransportChannel::P2PTransportChannel(const std::string& transport_name,
                                          int component,
                                          P2PTransport* transport,
                                          PortAllocator* allocator)
     : TransportChannelImpl(transport_name, component),
       transport_(transport),
       allocator_(allocator),
       worker_thread_(rtc::Thread::Current()),
       incoming_only_(false),
       error_(0),
+      connections_(this),
       best_connection_(NULL),
       pending_best_connection_(NULL),
       sort_dirty_(false),
       remote_ice_mode_(ICEMODE_FULL),
       ice_role_(ICEROLE_UNKNOWN),
       tiebreaker_(0),
       gathering_state_(kIceGatheringNew),
       check_receiving_delay_(MIN_CHECK_RECEIVING_DELAY * 5),
       receiving_timeout_(MIN_CHECK_RECEIVING_DELAY * 50),
       backup_connection_ping_interval_(0) {
@@ skipping 162 matching lines @@
     receiving_timeout_ = config.receiving_timeout_ms;
     check_receiving_delay_ =
         std::max(MIN_CHECK_RECEIVING_DELAY, receiving_timeout_ / 10);
 
     for (Connection* connection : connections_) {
       connection->set_receiving_timeout(receiving_timeout_);
     }
     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
 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;
@@ skipping 194 matching lines @@
       return;
     } else {
       ASSERT(false);
       port->SendBindingErrorResponse(stun_msg, address,
                                      STUN_ERROR_SERVER_ERROR,
                                      STUN_ERROR_REASON_SERVER_ERROR);
       return;
     }
   }
 
-  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,
                                    STUN_ERROR_REASON_SERVER_ERROR);
     return;
   }
 
   LOG(LS_INFO) << "Adding connection from "
                << (remote_candidate_is_new ? "peer reflexive" : "resurrected")
                << " candidate: " << remote_candidate.ToString();
@@ skipping 156 matching lines @@
                 << connection->remote_candidate().ToString()
                 << "New remote candidate: "
                 << remote_candidate.ToString();
       return false;
     }
   } else {
     PortInterface::CandidateOrigin origin = GetOrigin(port, origin_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);
     if (!connection)
       return false;
 
     AddConnection(connection);
 
     LOG_J(LS_INFO, this) << "Created connection with origin=" << origin << ", ("
                          << connections_.size() << " total)";
   }
 
   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();
 }
 
 uint32_t P2PTransportChannel::GetRemoteCandidateGeneration(
     const Candidate& candidate) {
   // If the candidate has a ufrag, use it to find the generation.
   if (!candidate.username().empty()) {
     uint32_t generation = 0;
@@ skipping 431 matching lines @@
   return conn->active();
 }
 
 // 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 (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
 // |use_candidate_attr| flag. The criteria to update this flag is
 // explained below.
 // Set USE-CANDIDATE if doing ICE AND this channel is in CONTROLLING AND
 //    a) Channel is in FULL ICE AND
 //      a.1) |conn| is the best connection OR
 //      a.2) there is no best connection OR
 //      a.3) the best connection is unwritable OR
@@ skipping 125 matching lines @@
 
   SignalSentPacket(this, sent_packet);
 }
 
 void P2PTransportChannel::OnReadyToSend(Connection* connection) {
   if (connection == best_connection_ && writable()) {
     SignalReadyToSend(this);
   }
 }
 
+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