RTCIceCandidateStats added.

webrtc/api/rtcstatscollector.cc

Index: webrtc/api/rtcstatscollector.cc
diff --git a/webrtc/api/rtcstatscollector.cc b/webrtc/api/rtcstatscollector.cc
index 084eab177cc7e557df3c69579a9807463e481e2a..765a875c73ed203c47f0a134f27099b64243a788 100644
--- a/webrtc/api/rtcstatscollector.cc
+++ b/webrtc/api/rtcstatscollector.cc
@@ -18,9 +18,24 @@
 #include "webrtc/api/webrtcsession.h"
 #include "webrtc/base/checks.h"
 #include "webrtc/base/sslidentity.h"
+#include "webrtc/p2p/base/candidate.h"
+#include "webrtc/p2p/base/port.h"
 
 namespace webrtc {
 
+const char* CandidateTypeToRTCIceCandidateType(const std::string& type) {
+  if (type == cricket::LOCAL_PORT_TYPE)
+    return RTCIceCandidateType::kHost;
+  if (type == cricket::STUN_PORT_TYPE)
+    return RTCIceCandidateType::kSrflx;
+  if (type == cricket::PRFLX_PORT_TYPE)
+    return RTCIceCandidateType::kPrflx;
+  if (type == cricket::RELAY_PORT_TYPE)
+    return RTCIceCandidateType::kRelay;
+  RTC_NOTREACHED();
+  return nullptr;
+}
+
 rtc::scoped_refptr<RTCStatsCollector> RTCStatsCollector::Create(
     PeerConnection* pc, int64_t cache_lifetime_us) {
   return rtc::scoped_refptr<RTCStatsCollector>(
@@ -93,6 +108,8 @@ void RTCStatsCollector::ProducePartialResultsOnSignalingThread(
   SessionStats session_stats;
   if (pc_->session()->GetTransportStats(&session_stats)) {
     ProduceCertificateStats_s(timestamp_us, session_stats, report.get());
+    ProduceIceCandidateAndPairStats_s(timestamp_us, session_stats,
+                                      report.get());
   }
   ProducePeerConnectionStats_s(timestamp_us, report.get());
 
@@ -201,6 +218,53 @@ void RTCStatsCollector::ProduceCertificateStatsFromSSLCertificateAndChain_s(
   }
 }
 
+void RTCStatsCollector::ProduceIceCandidateAndPairStats_s(
+      int64_t timestamp_us, const SessionStats& session_stats,
+      RTCStatsReport* report) const {
+  RTC_DCHECK(signaling_thread_->IsCurrent());
+  for (const auto& transport : session_stats.transport_stats) {
+    for (const auto& channel : transport.second.channel_stats) {
+      for (const cricket::ConnectionInfo& info : channel.connection_infos) {
+        // TODO(hbos): RTCIceCandidatePairStats referencing the resulting pair.

[hta-webrtc, 2016/10/04 14:12:45]

Complete sentence: "TODO(hbos): Produce RTCIceCandidatePairStats..."

[hbos, 2016/10/05 10:16:30]

Done.

+        // crbug.com/633550
+        ProduceIceCandidateStats_s(
+            timestamp_us, info.local_candidate, true, report);
+        ProduceIceCandidateStats_s(
+            timestamp_us, info.remote_candidate, false, report);
+      }
+    }
+  }
+}
+
+const std::string& RTCStatsCollector::ProduceIceCandidateStats_s(
+    int64_t timestamp_us, const cricket::Candidate& candidate, bool is_local,
+    RTCStatsReport* report) const {
+  RTC_DCHECK(signaling_thread_->IsCurrent());
+  std::string id = "RTCIceCandidate_" + candidate.id();

[hta-webrtc, 2016/10/04 14:12:45]

This uses the same namespace for local and remote candidates. Is that a problem?
(If the id is locally assigned, it isn't.)

Note - it's by design that you can't tell whether a candidate is local or
remote; you need the candidate pair stats to tell you that.

[hbos, 2016/10/05 10:16:30]

Since the ID should be based on the object that produced it - a candidate - and
by design you can't tell whether a candidate is local or remote - I think it
makes sense that the ID is just "RTCIceCandidate_X".

If, however, a candidate can be both local and remote depending on the context,
then the ID needs to be different to avoid a name clash. This may very well be
the case, I don't know? I updated the code to handle this, but if a candidate
can't be both local and remote I'd rather DCHECK.

[hta-webrtc, 2016/10/05 11:17:14]

DCHECK seems good. What I'm not sure about is where the candidate ID comes from,
so I can't check that it's actually designed to be unique.

[hbos, 2016/10/06 08:07:27]

+deadbeef

+  const RTCStats* s = report->Get(id);

[hta-webrtc, 2016/10/04 14:12:45]

Bad variable name.

[hbos, 2016/10/05 10:16:30]

Done.

+  if (!s) {
+    std::unique_ptr<RTCIceCandidateStats> candidate_stats;
+    if (is_local) {
+      candidate_stats.reset(
+          new RTCLocalIceCandidateStats(std::move(id), timestamp_us));
+    } else {
+      candidate_stats.reset(
+          new RTCRemoteIceCandidateStats(std::move(id), timestamp_us));
+    }
+    candidate_stats->ip = candidate.address().ipaddr().ToString();
+    candidate_stats->port = static_cast<int32_t>(candidate.address().port());
+    candidate_stats->protocol = candidate.protocol();
+    candidate_stats->candidate_type = CandidateTypeToRTCIceCandidateType(
+        candidate.type());
+    candidate_stats->priority = static_cast<int32_t>(candidate.priority());
+    // TODO(hbos): Define candidate_stats->url. crbug.com/632723
+
+    s = candidate_stats.get();
+    report->AddStats(std::move(candidate_stats));
+  }

[hta-webrtc, 2016/10/04 14:12:45]

What's the "else"? Is it an error to have duplicate IDs, or did we just
encounter the same candidate in two different contexts?

[hbos, 2016/10/05 10:16:30]

It means we encountered the same candidate twice, no need to duplicate its stats
since it's identical.
NOTE: I'm assuming candidate.id() is a unique identifier.

Perhaps we should add a second reviewer, someone who knows about candidates,
their IDs and whether or not the same candidate can be encountered multiple
times and if they can be both local and remote at the same time in different
contexts.

If so, we should have another unittest for this.

[hta-webrtc, 2016/10/05 11:17:14]

Adding a reviewer who knows the cricket::Candidate structure seems like a good
idea. Please do. deadbeef, honghaiz and pthatcher seem to have been recent
editors to the .h file.

[hbos, 2016/10/06 08:07:27]

+deadbeef

[Taylor Brandstetter, 2016/10/06 19:09:07]

The same candidate can't be both a local and remote candidate. And the ID
*should* be unique. In practice it's just a random string with 48 bits of
entropy, so collisions are possible. But this isn't the stats collector's fault;
the stats collector should assume it's unique.

[hbos, 2016/10/07 08:37:52]

Acknowledged. Changing stats ID back to "RTCIceCandidate_..." and DCHECKing
localness.

+  return s->id();
+}
+
 void RTCStatsCollector::ProducePeerConnectionStats_s(
     int64_t timestamp_us, RTCStatsReport* report) const {
   RTC_DCHECK(signaling_thread_->IsCurrent());