Better determination of Symmetric NAT.

If we're using shared socket mode, Symmetric NAT can be correctly determined by having more than 1 srflx per Requester (i.e. socket).

Design Doc updated at https://docs.google.com/document/d/11zruojoNqZgZYDrSVk0O3JHqFz2cFldWYQbroZdZRBg/edit#heading=h.t7npdccia97t

BUG=4576
R=pthatcher@webrtc.org

Review URL: https://codereview.webrtc.org/1166013002

Cr-Commit-Position: refs/heads/master@{#9396}

webrtc/p2p/stunprober/main.cc

@@ -106,16 +106,19 @@ class HostNameResolver : public HostNameResolverInterface,
   rtc::AsyncResolver* resolver_;
 };
 
-std::string HistogramName(bool behind_nat,
+const char* PrintNatType(stunprober::NatType type) {
-                          bool is_src_port_shared,
+  switch (type) {
-                          int interval_ms,
+    case stunprober::NATTYPE_NONE:
-                          std::string suffix) {
+      return "Not behind a NAT";
-  char output[1000];
+    case stunprober::NATTYPE_UNKNOWN:
-  rtc::sprintfn(output, sizeof(output), "NetConnectivity6.%s.%s.%dms.%s",
+      return "Unknown NAT type";
-                behind_nat ? "NAT" : "NoNAT",
+    case stunprober::NATTYPE_SYMMETRIC:
-                is_src_port_shared ? "SrcPortShared" : "SrcPortUnique",
+      return "Symmetric NAT";
-                interval_ms, suffix.c_str());
+    case stunprober::NATTYPE_NON_SYMMETRIC:
-  return std::string(output);
+      return "Non-Symmetric NAT";
+    default:
+      return "Invalid";
+  }
 }
 
 void PrintStats(StunProber* prober) {
@@ -130,27 +133,15 @@ void PrintStats(StunProber* prober) {
   LOG(LS_INFO) << "Responses received: " << stats.num_response_received;
   LOG(LS_INFO) << "Target interval (ns): " << stats.target_request_interval_ns;
   LOG(LS_INFO) << "Actual interval (ns): " << stats.actual_request_interval_ns;
-  LOG(LS_INFO) << "Behind NAT: " << stats.behind_nat;
+  LOG(LS_INFO) << "NAT Type: " << PrintNatType(stats.nat_type);
-  if (stats.behind_nat) {
-    LOG(LS_INFO) << "NAT is symmetrical: " << (stats.srflx_addrs.size() > 1);
-  }
   LOG(LS_INFO) << "Host IP: " << stats.host_ip;
   LOG(LS_INFO) << "Server-reflexive ips: ";
   for (auto& ip : stats.srflx_addrs) {
     LOG(LS_INFO) << "\t" << ip;
   }
 
-  std::string histogram_name = HistogramName(
+  LOG(LS_INFO) << "Success Precent: " << stats.success_percent;
-      stats.behind_nat, FLAG_shared_socket, FLAG_interval, "SuccessPercent");
+  LOG(LS_INFO) << "Response Latency:" << stats.average_rtt_ms;
-
-  LOG(LS_INFO) << "Histogram '" << histogram_name.c_str()
-               << "' = " << stats.success_percent;
-
-  histogram_name = HistogramName(stats.behind_nat, FLAG_shared_socket,
-                                 FLAG_interval, "ResponseLatency");
-
-  LOG(LS_INFO) << "Histogram '" << histogram_name.c_str()
-               << "' = " << stats.average_rtt_ms << " ms";
 }
 
 void StopTrial(rtc::Thread* thread, StunProber* prober, int result) {

webrtc/p2p/stunprober/stunprober.cc

@@ -24,11 +24,15 @@ namespace stunprober {
 
 namespace {
 
-void IncrementCounterByAddress(std::map<rtc::IPAddress, int>* counter_per_ip,
+template <typename T>
-                               const rtc::IPAddress& ip) {
+void IncrementCounterByAddress(std::map<T, int>* counter_per_ip, const T& ip) {
   counter_per_ip->insert(std::make_pair(ip, 0)).first->second++;
 }
 
+bool behind_nat(NatType nat_type) {
+  return nat_type > stunprober::NATTYPE_NONE;
+}
+
 }  // namespace
 
 // A requester tracks the requests and responses from a single socket to many
@@ -418,7 +422,7 @@ void StunProber::MaybeScheduleStunRequests() {
       rtc::Bind(&StunProber::MaybeScheduleStunRequests, this), 1 /* ms */);
 }
 
-bool StunProber::GetStats(StunProber::Stats* prob_stats) {
+bool StunProber::GetStats(StunProber::Stats* prob_stats) const {
   // No need to be on the same thread.
   if (!prob_stats) {
     return false;
@@ -427,25 +431,26 @@ bool StunProber::GetStats(StunProber::Stats* prob_stats) {
   StunProber::Stats stats;
 
   int rtt_sum = 0;
-  bool behind_nat_set = false;
   int64 first_sent_time = 0;
   int64 last_sent_time = 0;
+  NatType nat_type = NATTYPE_INVALID;
 
   // Track of how many srflx IP that we have seen.
   std::set<rtc::IPAddress> srflx_ips;
 
   // If we're not receiving any response on a given IP, all requests sent to
   // that IP should be ignored as this could just be an DNS error.
-  std::map<rtc::IPAddress, int> num_response_per_ip;
+  std::map<rtc::IPAddress, int> num_response_per_server;
-  std::map<rtc::IPAddress, int> num_request_per_ip;
+  std::map<rtc::IPAddress, int> num_request_per_server;
 
   for (auto* requester : requesters_) {
+    std::map<rtc::SocketAddress, int> num_response_per_srflx_addr;
     for (auto request : requester->requests()) {
       if (request->sent_time_ms <= 0) {
         continue;
       }
 
-      IncrementCounterByAddress(&num_request_per_ip, request->server_addr);
+      IncrementCounterByAddress(&num_request_per_server, request->server_addr);
 
       if (!first_sent_time) {
         first_sent_time = request->sent_time_ms;
@@ -456,19 +461,26 @@ bool StunProber::GetStats(StunProber::Stats* prob_stats) {
         continue;
       }
 
-      IncrementCounterByAddress(&num_response_per_ip, request->server_addr);
+      IncrementCounterByAddress(&num_response_per_server, request->server_addr);
+      IncrementCounterByAddress(&num_response_per_srflx_addr,
+                                request->srflx_addr);
 
       rtt_sum += request->rtt();
-      if (!behind_nat_set) {
+      if (nat_type == NATTYPE_INVALID) {
-        stats.behind_nat = request->behind_nat;
+        nat_type = request->behind_nat ? NATTYPE_UNKNOWN : NATTYPE_NONE;
-        behind_nat_set = true;
+      } else if (behind_nat(nat_type) != request->behind_nat) {
-      } else if (stats.behind_nat != request->behind_nat) {
         // Detect the inconsistency in NAT presence.
         return false;
       }
       stats.srflx_addrs.insert(request->srflx_addr.ToString());
       srflx_ips.insert(request->srflx_addr.ipaddr());
     }
+
+    // If we're using shared mode and seeing >1 srflx addresses for a single
+    // requester, it's symmetric NAT.
+    if (shared_socket_mode_ && num_response_per_srflx_addr.size() > 1) {
+      nat_type = NATTYPE_SYMMETRIC;
+    }
   }
 
   // We're probably not behind a regular NAT. We have more than 1 distinct
@@ -481,11 +493,11 @@ bool StunProber::GetStats(StunProber::Stats* prob_stats) {
   int num_received = 0;
   int num_server_ip_with_response = 0;
 
-  for (const auto& kv : num_response_per_ip) {
+  for (const auto& kv : num_response_per_server) {
     DCHECK_GT(kv.second, 0);
     num_server_ip_with_response++;
     num_received += kv.second;
-    num_sent += num_request_per_ip[kv.first];
+    num_sent += num_request_per_server[kv.first];
   }
 
   // Not receiving any response, the trial is inconclusive.
@@ -493,17 +505,21 @@ bool StunProber::GetStats(StunProber::Stats* prob_stats) {
     return false;
   }
 
+  stats.nat_type = nat_type;
+
   // Shared mode is only true if we use the shared socket and there are more
   // than 1 responding servers.
   stats.shared_socket_mode =
       shared_socket_mode_ && (num_server_ip_with_response > 1);
 
+  if (stats.shared_socket_mode && nat_type == NATTYPE_UNKNOWN) {
+    stats.nat_type = NATTYPE_NON_SYMMETRIC;
+  }
+
   stats.host_ip = local_addr_.ToString();
   stats.num_request_sent = num_sent;
   stats.num_response_received = num_received;
   stats.target_request_interval_ns = interval_ms_ * 1000;
-  stats.symmetric_nat =
-      stats.srflx_addrs.size() > static_cast<size_t>(GetTotalServerSockets());
 
   if (num_sent) {
     stats.success_percent = static_cast<int>(100 * num_received / num_sent);

webrtc/p2p/stunprober/stunprober.h

@@ -30,6 +30,14 @@ static const int kMaxUdpBufferSize = 1200;
 
 typedef rtc::Callback1<void, int> AsyncCallback;
 
+enum NatType {
+  NATTYPE_INVALID,
+  NATTYPE_NONE,          // Not behind a NAT.
+  NATTYPE_UNKNOWN,       // Behind a NAT but type can't be determine.
+  NATTYPE_SYMMETRIC,     // Behind a symmetric NAT.
+  NATTYPE_NON_SYMMETRIC  // Behind a non-symmetric NAT.
+};
+
 class HostNameResolverInterface {
  public:
   HostNameResolverInterface() {}
@@ -139,10 +147,10 @@ class StunProber {
 
   struct Stats {
     Stats() {}
+
     int num_request_sent = 0;
     int num_response_received = 0;
-    bool behind_nat = false;
+    NatType nat_type = NATTYPE_INVALID;
-    bool symmetric_nat = false;
     int average_rtt_ms = -1;
     int success_percent = 0;
     int target_request_interval_ns = 0;
@@ -188,7 +196,7 @@ class StunProber {
   // Method to retrieve the Stats once |finish_callback| is invoked. Returning
   // false when the result is inconclusive, for example, whether it's behind a
   // NAT or not.
-  bool GetStats(Stats* stats);
+  bool GetStats(Stats* stats) const;
 
  private:
   // A requester tracks the requests and responses from a single socket to many

webrtc/p2p/stunprober/stunprober_unittest.cc

@@ -168,7 +168,7 @@ class StunProberTest : public testing::Test {
     EXPECT_EQ(ss_->num_socket(), total_sockets);
     EXPECT_TRUE(prober->GetStats(&stats));
     EXPECT_EQ(stats.success_percent, 100);
-    EXPECT_TRUE(stats.behind_nat);
+    EXPECT_TRUE(stats.nat_type > stunprober::NATTYPE_NONE);
     EXPECT_EQ(stats.host_ip, kLocalAddr.ipaddr().ToString());
     EXPECT_EQ(stats.srflx_addrs, srflx_addresses);
     EXPECT_EQ(static_cast<uint32>(stats.num_request_sent),