Bug 4865: Enable connectivity when the remote peer is on public internet.

webrtc/p2p/client/basicportallocator.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 433 matching lines @@
   std::vector<Candidate> candidates;
   ProtocolType pvalue;
   bool candidate_allowed_to_send = CheckCandidateFilter(c);
   if (StringToProto(c.protocol().c_str(), &pvalue) &&
       data->sequence()->ProtocolEnabled(pvalue) &&
       candidate_allowed_to_send) {
     candidates.push_back(c);
   }
 
   if (!candidates.empty()) {
     SignalCandidatesReady(this, candidates);

[juberti1, 2015/08/06 00:35:26]

Will we push 0.0.0.0 to the application here? We want to avoid that.

[pthatcher1, 2015/08/06 01:06:19]

I believe candidate_allowed_to_send will be false because CheckCandidateFilter
filters it out.

[guoweis_webrtc, 2015/08/06 08:50:07]

Correct. It'll be filtered out

   }
 
-  // Moving to READY state as we have atleast one candidate from the port.
-  // Since this port has atleast one candidate we should forward this port
-  // to listners, to allow connections from this port.
-  // Also we should make sure that candidate gathered from this port is allowed
-  // to send outside.
-  if (!data->ready() && candidate_allowed_to_send) {
+  // Moving to READY state as we have at least one candidate from the port or

[juberti1, 2015/08/06 00:35:26]

This comment could be worded better. It's important to understand why
candidate_allowed_to_send is false for the any address.

[guoweis_webrtc, 2015/08/06 08:50:07]

Done.

+  // the candidate has the any address. When this port has at least one
+  // candidate we should forward this port to listeners, to allow connections
+  // from this port. Also we should make sure that candidate gathered from this
+  // port is allowed to send outside. When the candidate address is the any
+  // address, we need to move the port to READY such that connectivity is
+  // possible if the remote peer has a routable IP address on public Internet.

[pthatcher1, 2015/08/06 01:06:19]

I think we can re-word this in terms of "we're ready if we have a candidate that
can send or a candidate that can receive" (see my next comment).

[guoweis_webrtc, 2015/08/06 08:50:06]

Done.

+  if (!data->ready() && (candidate_allowed_to_send ||
+                         (c.address().IsAnyIP() && port->SharedSocket()))) {

[pthatcher1, 2015/08/06 01:06:19]

I think this would be a lot more readable if we broke up CheckCandidateFilter
and this logic into two predicate functions:

CandidateCanReceive (worth signalling)
CandidateCanSend (worth pinging from)


That way, much of this method can be somewhat like:

if (CandidateCanReceive(c)) {
  std::vector<Candidate> candidates;
  candidates.push_back(c);
  SignalCandidatesReady(this, candidates);
}

if ((CandidateCanReceive(c) || CandidateCanSend(c)) && !data->ready()) {
  data->set_ready();
  SignalPortReady(this, port);
}

[guoweis_webrtc, 2015/08/06 08:50:07]

I rephrase it a bit. Send and Receive is not very clear to me. TCP active
candidate is worth to signal but can't receive, for example.

     data->set_ready();
     SignalPortReady(this, port);
   }
 }
 
 void BasicPortAllocatorSession::OnPortComplete(Port* port) {
   ASSERT(rtc::Thread::Current() == network_thread_);
   PortData* data = FindPort(port);
   ASSERT(data != NULL);
 
@@ skipping 646 matching lines @@
   ServerAddresses servers;
   for (size_t i = 0; i < relays.size(); ++i) {
     if (relays[i].type == turn_type && SupportsProtocol(relays[i], type)) {
       servers.insert(relays[i].ports.front().address);
     }
   }
   return servers;
 }
 
 }  // namespace cricket