RTCStatsCollector: Utilize network thread to minimize thread hops.

webrtc/api/webrtcsession.cc

 /*
  *  Copyright 2012 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 988 matching lines @@
   }
   const TransportInfo* transport_info =
       description->GetTransportInfoByName(content_name);
   if (!transport_info) {
     return false;
   }
   *tdesc = transport_info->description;
   return true;
 }
 
-bool WebRtcSession::GetTransportStats(SessionStats* stats) {
+std::unique_ptr<SessionStats> WebRtcSession::GetSessionStats() {

[pthatcher1, 2016/12/15 23:18:59]

Why not just "GetStats()"?

[hbos, 2016/12/16 10:38:02]

Done.

   ASSERT(signaling_thread()->IsCurrent());
-  return (GetChannelTransportStats(voice_channel(), stats) &&
-          GetChannelTransportStats(video_channel(), stats) &&
-          GetChannelTransportStats(data_channel(), stats));
+  ChannelNamePairs channel_name_pairs;
+  if (voice_channel()) {
+    channel_name_pairs.voice = rtc::Optional<ChannelNamePair>(ChannelNamePair(
+        voice_channel()->content_name(), voice_channel()->transport_name()));
+  }
+  if (video_channel()) {
+    channel_name_pairs.video = rtc::Optional<ChannelNamePair>(ChannelNamePair(
+        video_channel()->content_name(), video_channel()->transport_name()));
+  }
+  if (data_channel()) {
+    channel_name_pairs.data = rtc::Optional<ChannelNamePair>(ChannelNamePair(
+        data_channel()->content_name(), data_channel()->transport_name()));
+  }
+  return GetSessionStats(channel_name_pairs);

[pthatcher1, 2016/12/15 23:18:59]

I don't understand.  Why not just put everything in this method and skip the
whole ChannelNamePair thing?

for (auto channel: [voice_channel(), video_channel(), data_channel()]) {
  if (!channel) {
    continue;
  }
  cricket::TransportStats transport_stats;
  if (!transport_controller_->GetStats(channel->transport_name(),
&transport_stats)) {
    continue;
  }
  std::unique_ptr<SessionStats> session_stats(new SessionStats());
  session_stats->proxy_to_transport[channel->content_name()] =
channel->transport_name();
  session_stats->transport_stats[channel->transport_name()] =
std::move(transport_stats);
}

[hbos, 2016/12/16 10:38:02]

Because what if there is a race between e.g. blah_channel_.release() on
signaling thread and blah_channel()->transport_name() on the network thread and
I'm wary of introducing locks to a model that is regularly doing blocking thread
invokes.

 }
 
-bool WebRtcSession::GetChannelTransportStats(cricket::BaseChannel* ch,
-                                             SessionStats* stats) {
-  ASSERT(signaling_thread()->IsCurrent());
-  if (!ch) {
-    // Not using this channel.
-    return true;
-  }
-
-  const std::string& content_name = ch->content_name();
-  const std::string& transport_name = ch->transport_name();
-  stats->proxy_to_transport[content_name] = transport_name;
-  if (stats->transport_stats.find(transport_name) !=
-      stats->transport_stats.end()) {
-    // Transport stats already done for this transport.
-    return true;
-  }
-
-  cricket::TransportStats tstats;
-  if (!transport_controller_->GetStats(transport_name, &tstats)) {
-    return false;
-  }
-
-  stats->transport_stats[transport_name] = tstats;
-  return true;
+std::unique_ptr<SessionStats> WebRtcSession::GetSessionStats(
+    const ChannelNamePairs& channel_name_pairs) {
+  if (network_thread()->IsCurrent())
+    return GetSessionStats_n(channel_name_pairs);

[pthatcher1, 2016/12/15 23:18:59]

{}s please

[hbos, 2016/12/16 10:38:02]

Done.

+  return network_thread()->Invoke<std::unique_ptr<SessionStats>>(
+      RTC_FROM_HERE,
+      rtc::Bind(&WebRtcSession::GetSessionStats_n, this, channel_name_pairs));
 }
 
 bool WebRtcSession::GetLocalCertificate(
     const std::string& transport_name,
     rtc::scoped_refptr<rtc::RTCCertificate>* certificate) {
-  ASSERT(signaling_thread()->IsCurrent());
   return transport_controller_->GetLocalCertificate(transport_name,
                                                     certificate);
 }
 
 std::unique_ptr<rtc::SSLCertificate> WebRtcSession::GetRemoteSSLCertificate(
     const std::string& transport_name) {
-  ASSERT(signaling_thread()->IsCurrent());
   return transport_controller_->GetRemoteSSLCertificate(transport_name);
 }
 
 bool WebRtcSession::EnableBundle(const cricket::ContentGroup& bundle) {
   const std::string* first_content_name = bundle.FirstContentName();
   if (!first_content_name) {
     LOG(LS_WARNING) << "Tried to BUNDLE with no contents.";
     return false;
   }
   const std::string& transport_name = *first_content_name;
@@ skipping 679 matching lines @@
   }
 
   data_channel_->SignalDtlsSetupFailure.connect(
       this, &WebRtcSession::OnDtlsSetupFailure);
 
   SignalDataChannelCreated();
   data_channel_->SignalSentPacket.connect(this, &WebRtcSession::OnSentPacket_w);
   return true;
 }
 
+std::unique_ptr<SessionStats> WebRtcSession::GetSessionStats_n(
+    const ChannelNamePairs& channel_name_pairs) {
+  ASSERT(network_thread()->IsCurrent());
+  std::unique_ptr<SessionStats> session_stats(new SessionStats());
+  if (channel_name_pairs.voice) {
+    cricket::TransportStats voice_transport_stats;
+    if (!transport_controller_->GetStats(
+        channel_name_pairs.voice->transport_name, &voice_transport_stats)) {
+      return nullptr;
+    }
+    session_stats->proxy_to_transport[channel_name_pairs.voice->content_name] =
+        channel_name_pairs.voice->transport_name;
+    session_stats->transport_stats[channel_name_pairs.voice->transport_name] =
+        std::move(voice_transport_stats);
+  }
+  if (channel_name_pairs.video) {
+    cricket::TransportStats video_transport_stats;
+    if (!transport_controller_->GetStats(
+        channel_name_pairs.video->transport_name, &video_transport_stats)) {
+      return nullptr;
+    }
+    session_stats->proxy_to_transport[channel_name_pairs.video->content_name] =
+        channel_name_pairs.video->transport_name;
+    session_stats->transport_stats[channel_name_pairs.video->transport_name] =
+        std::move(video_transport_stats);
+  }
+  if (channel_name_pairs.data) {
+    cricket::TransportStats data_transport_stats;
+    if (!transport_controller_->GetStats(
+        channel_name_pairs.data->transport_name, &data_transport_stats)) {
+      return nullptr;
+    }
+    session_stats->proxy_to_transport[channel_name_pairs.data->content_name] =
+        channel_name_pairs.data->transport_name;
+    session_stats->transport_stats[channel_name_pairs.data->transport_name] =
+        std::move(data_transport_stats);
+  }

[pthatcher1, 2016/12/15 23:18:59]

There's some duplication that could be removed and reduced to something like
this:.  Something like:

for (auto name_pair: [channel_name_pairs.voice, channel_name_pairs.video,
channel_name_pairs.data]) {
  cricket::TransportStats transport_stats;
  if (!transport_controller_->GetStats(name_pair->transport_name,
&transport_stats)) {
    return nullptr;
  }
  session_stats->proxy_to_transport[name_pair->content_name] =
name_pair->transport_name;
  session_stats->transport_stats[name_pair->transport_name] =
std::move(transport_stats);
}

[hbos, 2016/12/16 10:38:02]

Oh nice, good idea. Done.

+  return session_stats;
+}
+
 void WebRtcSession::OnDtlsSetupFailure(cricket::BaseChannel*, bool rtcp) {
   SetError(ERROR_TRANSPORT,
            rtcp ? kDtlsSetupFailureRtcp : kDtlsSetupFailureRtp);
 }
 
 void WebRtcSession::OnDataChannelMessageReceived(
     cricket::DataChannel* channel,
     const cricket::ReceiveDataParams& params,
     const rtc::CopyOnWriteBuffer& payload) {
   RTC_DCHECK(data_channel_type_ == cricket::DCT_SCTP);
@@ skipping 325 matching lines @@
 }
 
 void WebRtcSession::OnDtlsHandshakeError(rtc::SSLHandshakeError error) {
   if (metrics_observer_) {
     metrics_observer_->IncrementEnumCounter(
         webrtc::kEnumCounterDtlsHandshakeError, static_cast<int>(error),
         static_cast<int>(rtc::SSLHandshakeError::MAX_VALUE));
   }
 }
 }  // namespace webrtc