RTCMediaStreamTrackStats.ssrcIds collected by RTCStatsCollector.

webrtc/api/rtcstatscollector.cc

Index: webrtc/api/rtcstatscollector.cc
diff --git a/webrtc/api/rtcstatscollector.cc b/webrtc/api/rtcstatscollector.cc
index cf97057c1b6884595618b3083beeacad5727f93d..c54619ef5d189e77723c0f540c79a73977668bd3 100644
--- a/webrtc/api/rtcstatscollector.cc
+++ b/webrtc/api/rtcstatscollector.cc
@@ -29,6 +29,11 @@ namespace webrtc {
 
 namespace {
 
+// If the track is associated with multiple SSRCs, the |uint32_t| is the first
+// one in the group, used as an identifier for that group. The SSRCs are listed
+// in ||Voice/Video][Sender/Receiver]Info::ssrcs()|.
+typedef std::map<MediaStreamTrackInterface*, uint32_t> TracksToSsrcs;

[pthatcher1, 2017/01/03 19:29:00]

I'd prefer you just use std::map<MediaStreamTrackInterface*, uint32_t> and not
have this typedef.  That's more consistent with the style guide and our more
recent code.

+
 std::string RTCCertificateIDFromFingerprint(const std::string& fingerprint) {
   return "RTCCertificate_" + fingerprint;
 }
@@ -54,8 +59,9 @@ std::string RTCIceCandidatePairStatsIDFromConnectionInfo(
 }
 
 std::string RTCMediaStreamTrackStatsIDFromMediaStreamTrackInterface(
-    const MediaStreamTrackInterface& track) {
-  return "RTCMediaStreamTrack_" + track.id();
+    const MediaStreamTrackInterface& track, bool is_local) {
+  return (is_local ? "RTCMediaStreamTrack_local_" :
+                     "RTCMediaStreamTrack_remote_") + track.id();
 }
 
 std::string RTCTransportStatsIDFromTransportChannel(
@@ -280,10 +286,73 @@ const std::string& ProduceIceCandidateStats(
   return stats->id();
 }
 
+TracksToSsrcs GetTracksToSsrcs(
+    const std::vector<rtc::scoped_refptr<RtpSenderInterface>> senders) {
+  TracksToSsrcs tracks_to_ssrcs;

[pthatcher1, 2017/01/03 19:28:59]

I think a better name would be "track_to_ssrc".   Making them plural leaves it
ambiguous whether the key or value is plural.  But both are singular in the map.

+  for (const rtc::scoped_refptr<RtpSenderInterface>& sender : senders) {
+    if (!sender->track())
+      continue;

[pthatcher1, 2017/01/03 19:28:59]

{}s please

+    if (sender->ssrc() != 0) {

[pthatcher1, 2017/01/03 19:28:59]

Why not do another early continue?

if (!sender->ssrc()) {
  continue;
}

+      // TODO(hbos): What if multiple |RtpSenderInterface| have the same track
+      // attached yielding multiple SSRCs groups? Only one of them would
+      // currently be used. crbug.com/659137

[hta-webrtc, 2017/01/02 15:29:46]

This is supposed to be allowed, and the track is then sent twice, per list
discussion. We need an integration test that checks what happens in this case.

+      // TODO(hbos): What if |GetParameters().encodings.size() > 1| yields
+      // multiple SSRC groups per sender? crbug.com/659137
+      tracks_to_ssrcs[sender->track().get()] = sender->ssrc();

[hta-webrtc, 2017/01/02 15:29:46]

If you DCHECK(!tracks_to_ssrcs.is_member(sender->track().get()) before this
line, you at least know that you have a problem.

+    }
+  }
+  return tracks_to_ssrcs;
+}
+// TODO(hbos,deadbeef): With template argument |T| for both |RtpSenderInterface|
+// and |RtpReceiverInterface| this function should be able to cover both sender
+// and receiver cases. But the |RtpSenderInterface| needs to be updated first,
+// when this comment was written it could have an |ssrc()| and still not a value
+// for |GetParameters().encodings[0].ssrc|.
+TracksToSsrcs GetTracksToSsrcs(
+    const std::vector<rtc::scoped_refptr<RtpReceiverInterface>> receivers) {
+  TracksToSsrcs tracks_to_ssrcs;
+  for (const rtc::scoped_refptr<RtpReceiverInterface>& receiver : receivers) {
+    if (!receiver->track())

[Taylor Brandstetter, 2017/01/04 00:36:56]

A receiver should always have a track; you can DCHECK here.

+      continue;

[pthatcher1, 2017/01/03 19:29:00]

{}s please

+    RTC_DCHECK(
+        tracks_to_ssrcs.find(receiver->track().get()) == tracks_to_ssrcs.end());
+    RtpParameters parameters = receiver->GetParameters();
+    if (parameters.encodings.empty())
+      continue;

[pthatcher1, 2017/01/03 19:28:59]

{}s please

+    RTC_DCHECK_EQ(1u, parameters.encodings.size());

[pthatcher1, 2017/01/03 19:29:00]

This is going to explode as soon as we implement GetParameters() for simulcast. 
Could you make it RTC_DCHECK_LE?

[Taylor Brandstetter, 2017/01/04 00:36:56]

In that case the DCHECK would be redundant since "parameters.encodings.empty()"
is already checked above.

+    rtc::Optional<uint32_t> ssrc = parameters.encodings[0].ssrc;
+    if (ssrc)
+      tracks_to_ssrcs[receiver->track().get()] = *ssrc;

[pthatcher1, 2017/01/03 19:28:59]

{}s please

+  }
+  return tracks_to_ssrcs;
+}
+
+// |T| can be any |MediaSenderInfo| or |MediaReceiverInfo|.
+template<typename T>
+const T* GetMediaInfoFromSsrc(const std::vector<T>& infos, uint32_t ssrc) {

[pthatcher1, 2017/01/03 19:28:59]

Sounds more like "BySsrc" than "FromSsrc"

Or why not just "FindBySsrc"?

+  for (const T& info : infos) {
+    if (info.ssrc() == ssrc)
+      return &info;

[pthatcher1, 2017/01/03 19:28:59]

{}s please

+  }
+  return nullptr;
+}
+
+std::vector<std::string> StringSsrcsFromSsrcs(
+    const std::vector<uint32_t>& ssrcs) {

[pthatcher1, 2017/01/03 19:28:59]

Why not just call this ToStrings?  You could even templatize it.

+  std::vector<std::string> string_ssrcs;
+  for (uint32_t ssrc : ssrcs) {
+    string_ssrcs.push_back(rtc::ToString<>(ssrc));
+  }
+  return string_ssrcs;
+}
+
 void ProduceMediaStreamAndTrackStats(
     int64_t timestamp_us,
     rtc::scoped_refptr<StreamCollectionInterface> streams,
     bool is_local,
+    const TracksToSsrcs& tracks_to_ssrcs,
+    const rtc::Optional<cricket::VoiceMediaInfo>& voice_info,
+    const rtc::Optional<cricket::VideoMediaInfo>& video_info,
     RTCStatsReport* report) {
   // TODO(hbos): When "AddTrack" is implemented we should iterate tracks to
   // find which streams exist, not iterate streams to find tracks.
@@ -306,17 +375,33 @@ void ProduceMediaStreamAndTrackStats(
     for (const rtc::scoped_refptr<AudioTrackInterface>& audio_track :
          stream->GetAudioTracks()) {
       std::string id = RTCMediaStreamTrackStatsIDFromMediaStreamTrackInterface(
-        *audio_track.get());
-      if (report->Get(id)) {
-        // Skip track, stats already exist for it.
-        continue;
-      }

[pthatcher1, 2017/01/03 19:29:00]

So why don't we skip this any more?

+        *audio_track.get(), is_local);
       std::unique_ptr<RTCMediaStreamTrackStats> audio_track_stats(
           new RTCMediaStreamTrackStats(id, timestamp_us));
       stream_stats->track_ids->push_back(audio_track_stats->id());
       SetMediaStreamTrackStatsFromMediaStreamTrackInterface(
           *audio_track.get(),
           audio_track_stats.get());
+      TracksToSsrcs::const_iterator it = tracks_to_ssrcs.find(
+          audio_track.get());
+      if (it != tracks_to_ssrcs.end() && voice_info) {
+        uint32_t ssrc = it->second;
+        if (is_local) {
+          const cricket::VoiceSenderInfo* sender_info =
+              GetMediaInfoFromSsrc(voice_info->senders, ssrc);
+          if (sender_info) {
+            audio_track_stats->ssrc_ids = StringSsrcsFromSsrcs(
+                sender_info->ssrcs());
+          }
+        } else {
+          const cricket::VoiceReceiverInfo* receiver_info =
+              GetMediaInfoFromSsrc(voice_info->receivers, ssrc);
+          if (receiver_info) {
+            audio_track_stats->ssrc_ids = StringSsrcsFromSsrcs(
+                receiver_info->ssrcs());
+          }
+        }
+      }
       audio_track_stats->remote_source = !is_local;
       audio_track_stats->detached = false;
       int signal_level;
@@ -344,17 +429,33 @@ void ProduceMediaStreamAndTrackStats(
     for (const rtc::scoped_refptr<VideoTrackInterface>& video_track :
          stream->GetVideoTracks()) {
       std::string id = RTCMediaStreamTrackStatsIDFromMediaStreamTrackInterface(
-          *video_track.get());
-      if (report->Get(id)) {
-        // Skip track, stats already exist for it.
-        continue;
-      }
+          *video_track.get(), is_local);
       std::unique_ptr<RTCMediaStreamTrackStats> video_track_stats(
           new RTCMediaStreamTrackStats(id, timestamp_us));
       stream_stats->track_ids->push_back(video_track_stats->id());
       SetMediaStreamTrackStatsFromMediaStreamTrackInterface(
           *video_track.get(),
           video_track_stats.get());
+      TracksToSsrcs::const_iterator it = tracks_to_ssrcs.find(
+          video_track.get());
+      if (it != tracks_to_ssrcs.end() && video_info) {
+        uint32_t ssrc = it->second;
+        if (is_local) {
+          const cricket::VideoSenderInfo* sender_info =
+              GetMediaInfoFromSsrc(video_info->senders, ssrc);
+          if (sender_info) {
+            video_track_stats->ssrc_ids = StringSsrcsFromSsrcs(
+                sender_info->ssrcs());
+          }
+        } else {
+          const cricket::VideoReceiverInfo* receiver_info =
+              GetMediaInfoFromSsrc(video_info->receivers, ssrc);
+          if (receiver_info) {
+            video_track_stats->ssrc_ids = StringSsrcsFromSsrcs(
+                receiver_info->ssrcs());
+          }
+        }
+      }
       video_track_stats->remote_source = !is_local;
       video_track_stats->detached = false;
       if (video_track->GetSource()) {
@@ -427,9 +528,6 @@ void RTCStatsCollector::GetStatsReport(
 
     num_pending_partial_reports_ = 3;
     partial_report_timestamp_us_ = cache_now_us;
-    invoker_.AsyncInvoke<void>(RTC_FROM_HERE, signaling_thread_,
-        rtc::Bind(&RTCStatsCollector::ProducePartialResultsOnSignalingThread,
-            rtc::scoped_refptr<RTCStatsCollector>(this), timestamp_us));
 
     // TODO(hbos): No stats are gathered by
     // |ProducePartialResultsOnWorkerThread|, remove it.
@@ -437,8 +535,9 @@ void RTCStatsCollector::GetStatsReport(
         rtc::Bind(&RTCStatsCollector::ProducePartialResultsOnWorkerThread,
             rtc::scoped_refptr<RTCStatsCollector>(this), timestamp_us));
 
-    // Prepare |channel_names_| and |media_info_| for use in
-    // |ProducePartialResultsOnNetworkThread|.
+    // Prepare |channel_names_| and |media_info_|. These are read in

[hta-webrtc, 2017/01/02 15:29:46]

Do you mean channel_name_pairs_?

+    // |ProducePartialResultsOnNetworkThread|, and |media_info_| is also used in
+    // |ProducePartialResultsOnSignalingThread|.
     channel_name_pairs_.reset(new ChannelNamePairs());
     if (pc_->session()->voice_channel()) {
       channel_name_pairs_->voice = rtc::Optional<ChannelNamePair>(
@@ -459,6 +558,7 @@ void RTCStatsCollector::GetStatsReport(
     invoker_.AsyncInvoke<void>(RTC_FROM_HERE, network_thread_,
         rtc::Bind(&RTCStatsCollector::ProducePartialResultsOnNetworkThread,
             rtc::scoped_refptr<RTCStatsCollector>(this), timestamp_us));
+    ProducePartialResultsOnSignalingThread(timestamp_us);

[pthatcher1, 2017/01/03 19:28:59]

Should you DCHECK that you're on the signaling thread?

[Taylor Brandstetter, 2017/01/04 00:36:56]

ProducePartialResultsOnSignalingThread does this. The start of this method does
too.

   }
 }
 
@@ -702,9 +802,21 @@ void RTCStatsCollector::ProduceMediaStreamAndTrackStats_s(
     int64_t timestamp_us, RTCStatsReport* report) const {
   RTC_DCHECK(signaling_thread_->IsCurrent());
   ProduceMediaStreamAndTrackStats(
-      timestamp_us, pc_->local_streams(), true, report);
+      timestamp_us,
+      pc_->local_streams(),
+      true,
+      GetTracksToSsrcs(pc_->GetSenders()),
+      media_info_->voice,
+      media_info_->video,
+      report);
   ProduceMediaStreamAndTrackStats(
-      timestamp_us, pc_->remote_streams(), false, report);
+      timestamp_us,
+      pc_->remote_streams(),
+      false,
+      GetTracksToSsrcs(pc_->GetReceivers()),
+      media_info_->voice,
+      media_info_->video,
+      report);
 }
 
 void RTCStatsCollector::ProducePeerConnectionStats_s(