RTCMediaStreamTrackStats.ssrcIds collected by RTCStatsCollector.

RTCMediaStreamTrackStats.ssrcIds collected by RTCStatsCollector.

This also maps tracks to [Voice/Video][Sender/Receiver]Info, meaning
follow-up CLs can add stats to tracks that are located in the "info"
structs.

There is an assumption about a track only belonging to a single group of
SSRCs (a single [Voice/Video][Sender/Receiver]Info). If we are to
support multiple SSRC groups we need to decide what to do with the
per-track stats coming from multiple "info" structs. This needs to be
addressed in a follow-up CL.

A bug was revealed and fixed where local and remote tracks could have
the same label yielding the same ID for multiple tracks. Fix:
distinguish between local and remote cases, not just label.

BUG=webrtc:6757, chromium:659137, chromium:627816

[hbos, 2016-12-28 13:38:20]

Patchset #1 (id:1) has been deleted

[hbos, 2016-12-28 13:55:32]

Patchset #1 (id:20001) has been deleted

[hbos, 2016-12-28 15:42:33]

Patchset #1 (id:40001) has been deleted

[hbos, 2016-12-28 15:49:20]

Patchset #1 (id:60001) has been deleted

[hbos, 2016-12-28 15:49:29]

The CQ bit was checked by hbos@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-28 15:49:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[hbos, 2016-12-28 15:50:40]

Patchset #1 (id:80001) has been deleted

[hbos, 2016-12-28 15:52:51]

The CQ bit was checked by hbos@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-28 15:52:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[hbos, 2016-12-28 15:56:21]

Description was changed from

==========
RTCMediaStreamTrackStats.ssrcIds collected by RTCStatsCollector.

This also maps tracks to [Voice/Video][Sender/Receiver]Info, meaning
follow-up CLs can add stats to tracks that are located in the "info"
structs.

BUG=webrtc:6757, chromium:659137, chromium:627816
==========

to

==========
RTCMediaStreamTrackStats.ssrcIds collected by RTCStatsCollector.

This also maps tracks to [Voice/Video][Sender/Receiver]Info, meaning
follow-up CLs can add stats to tracks that are located in the "info"
structs.

A bug was revealed and fixed where local and remote tracks could have
the same label yielding the same ID for multiple tracks. Fix:
distinguish between local and remote cases, not just label.

BUG=webrtc:6757, chromium:659137, chromium:627816
==========

[hbos, 2016-12-28 15:59:44]

Description was changed from

==========
RTCMediaStreamTrackStats.ssrcIds collected by RTCStatsCollector.

This also maps tracks to [Voice/Video][Sender/Receiver]Info, meaning
follow-up CLs can add stats to tracks that are located in the "info"
structs.

A bug was revealed and fixed where local and remote tracks could have
the same label yielding the same ID for multiple tracks. Fix:
distinguish between local and remote cases, not just label.

BUG=webrtc:6757, chromium:659137, chromium:627816
==========

to

==========
RTCMediaStreamTrackStats.ssrcIds collected by RTCStatsCollector.

This also maps tracks to [Voice/Video][Sender/Receiver]Info, meaning
follow-up CLs can add stats to tracks that are located in the "info"
structs.

There is an assumption about a track only belonging to a single group of
SSRCs (a single [Voice/Video][Sender/Receiver]Info). If we are to
support multiple SSRC groups we need to decide what to do with the
per-track stats coming from "info" structs. This needs to be addressed
in a follow-up CL.

A bug was revealed and fixed where local and remote tracks could have
the same label yielding the same ID for multiple tracks. Fix:
distinguish between local and remote cases, not just label.

BUG=webrtc:6757, chromium:659137, chromium:627816
==========

[hbos, 2016-12-28 16:01:01]

Description was changed from

==========
RTCMediaStreamTrackStats.ssrcIds collected by RTCStatsCollector.

This also maps tracks to [Voice/Video][Sender/Receiver]Info, meaning
follow-up CLs can add stats to tracks that are located in the "info"
structs.

There is an assumption about a track only belonging to a single group of
SSRCs (a single [Voice/Video][Sender/Receiver]Info). If we are to
support multiple SSRC groups we need to decide what to do with the
per-track stats coming from "info" structs. This needs to be addressed
in a follow-up CL.

A bug was revealed and fixed where local and remote tracks could have
the same label yielding the same ID for multiple tracks. Fix:
distinguish between local and remote cases, not just label.

BUG=webrtc:6757, chromium:659137, chromium:627816
==========

to

==========
RTCMediaStreamTrackStats.ssrcIds collected by RTCStatsCollector.

This also maps tracks to [Voice/Video][Sender/Receiver]Info, meaning
follow-up CLs can add stats to tracks that are located in the "info"
structs.

There is an assumption about a track only belonging to a single group of
SSRCs (a single [Voice/Video][Sender/Receiver]Info). If we are to
support multiple SSRC groups we need to decide what to do with the
per-track stats coming from multiple "info" structs. This needs to be
addressed in a follow-up CL.

A bug was revealed and fixed where local and remote tracks could have
the same label yielding the same ID for multiple tracks. Fix:
distinguish between local and remote cases, not just label.

BUG=webrtc:6757, chromium:659137, chromium:627816
==========

[hbos, 2016-12-28 16:12:29]

hbos@webrtc.org changed reviewers:
+ deadbeef@webrtc.org, hta@webrtc.org, pthatcher@webrtc.org

[hbos, 2016-12-28 16:12:30]

Please take a look, pthatcher, deadbeef and hta.

https://codereview.webrtc.org/2605033002/diff/100001/webrtc/api/rtcstatscolle...
File webrtc/api/rtcstatscollector.cc (right):

https://codereview.webrtc.org/2605033002/diff/100001/webrtc/api/rtcstatscolle...
webrtc/api/rtcstatscollector.cc:35: typedef std::map<MediaStreamTrackInterface*,
uint32_t> TracksToSsrcs;
There is also std::vector<SsrcGroup> ssrc_groups of Video[Sender/Receiver]Info
(but not Voice[...]), is this something else? Should I refer to multiple SSRCs
as something other than "SSRC groups" in comments like this?

https://codereview.webrtc.org/2605033002/diff/100001/webrtc/api/rtcstatscolle...
webrtc/api/rtcstatscollector.cc:300: // multiple SSRC groups per sender?
crbug.com/659137
Looking for input about what to do in either of these cases, whether we address
them here or in follow-up CLs.

https://codereview.webrtc.org/2605033002/diff/100001/webrtc/api/rtcstatscolle...
webrtc/api/rtcstatscollector.cc:310: // for |GetParameters().encodings[0].ssrc|.
(Discovered by debugging rtcstats_integrationtest.cc when sender case also used
GetParameters)

[commit-bot: I haz the power, 2016-12-28 16:56:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-28 16:56:51]

Dry run: This issue passed the CQ dry run.

[hbos, 2016-12-29 09:53:55]

https://codereview.webrtc.org/2605033002/diff/100001/webrtc/api/rtcstatscolle...
File webrtc/api/rtcstatscollector.cc (right):

https://codereview.webrtc.org/2605033002/diff/100001/webrtc/api/rtcstatscolle...
webrtc/api/rtcstatscollector.cc:389: if (is_local) {
So I'm assuming it's either [Voice/Video]SenderInfo or
[Voice/Video]ReceiverInfo. But could a track both receive and send? E.g. we're
retransmitting what we're receiving from some external source. Or would a setup
like that use one local and one remote track to achieve this?

[hta-webrtc, 2017-01-02 14:30:12]

On 2016/12/29 09:53:55, hbos wrote:
>
https://codereview.webrtc.org/2605033002/diff/100001/webrtc/api/rtcstatscolle...
> File webrtc/api/rtcstatscollector.cc (right):
> 
>
https://codereview.webrtc.org/2605033002/diff/100001/webrtc/api/rtcstatscolle...
> webrtc/api/rtcstatscollector.cc:389: if (is_local) {
> So I'm assuming it's either [Voice/Video]SenderInfo or
> [Voice/Video]ReceiverInfo. But could a track both receive and send? E.g. we're
> retransmitting what we're receiving from some external source. Or would a
setup
> like that use one local and one remote track to achieve this?

A MediaStreamTrack is unidirectional by definition. So is an SSRC.

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

Not completely happy. When you have bugs that say "I don't know if this can
happen, and I'm not sure what should happen if it does", the right thing to put
in is DCHECK(this-didn't-happen).

The fake completely defined in a .h file is ugly.

https://codereview.webrtc.org/2605033002/diff/120001/webrtc/api/rtcstatscolle...
File webrtc/api/rtcstatscollector.cc (right):

https://codereview.webrtc.org/2605033002/diff/120001/webrtc/api/rtcstatscolle...
webrtc/api/rtcstatscollector.cc:298: // currently be used. crbug.com/659137
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.

https://codereview.webrtc.org/2605033002/diff/120001/webrtc/api/rtcstatscolle...
webrtc/api/rtcstatscollector.cc:301: tracks_to_ssrcs[sender->track().get()] =
sender->ssrc();
If you DCHECK(!tracks_to_ssrcs.is_member(sender->track().get()) before this
line, you at least know that you have a problem.

https://codereview.webrtc.org/2605033002/diff/120001/webrtc/api/rtcstatscolle...
webrtc/api/rtcstatscollector.cc:538: // Prepare |channel_names_| and
|media_info_|. These are read in
Do you mean channel_name_pairs_?

https://codereview.webrtc.org/2605033002/diff/120001/webrtc/api/rtcstatscolle...
File webrtc/api/rtcstatscollector.h (right):

https://codereview.webrtc.org/2605033002/diff/120001/webrtc/api/rtcstatscolle...
webrtc/api/rtcstatscollector.h:164: // ... Also read in
|ProducePartialResultsOnSignalingThread|.
It's easier to read these comment-above-declaration comments if you start off
with saying what it is, like this:

// Information about the media.
// Read in |ProducePartialResultsOnSignalingThread|.

This comment isn't clear about which parts of the previous comment (if any)
apply to this variable.

https://codereview.webrtc.org/2605033002/diff/120001/webrtc/api/test/fakertps...
File webrtc/api/test/fakertpsenderreceiver.h (right):

https://codereview.webrtc.org/2605033002/diff/120001/webrtc/api/test/fakertps...
webrtc/api/test/fakertpsenderreceiver.h:25: RtpParameters
CreateRtpParametersWithSsrc(uint32_t ssrc) {
I don't like having all this stuff in a .h file. It's a bother, but can you do a
.cc for it?

I'm surprised the style linter didn't yell at you for this.

[hbos, 2017-01-02 17:42:03]

I'll take another look at mapping info -> track and track -> info(s) (multiple
infos per track possible for sender), possibly as a standalone CL.

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

https://codereview.webrtc.org/2605033002/diff/120001/webrtc/api/rtcstatscolle...
File webrtc/api/rtcstatscollector.cc (left):

https://codereview.webrtc.org/2605033002/diff/120001/webrtc/api/rtcstatscolle...
webrtc/api/rtcstatscollector.cc:313: }
So why don't we skip this any more?

https://codereview.webrtc.org/2605033002/diff/120001/webrtc/api/rtcstatscolle...
File webrtc/api/rtcstatscollector.cc (right):

https://codereview.webrtc.org/2605033002/diff/120001/webrtc/api/rtcstatscolle...
webrtc/api/rtcstatscollector.cc:35: typedef std::map<MediaStreamTrackInterface*,
uint32_t> TracksToSsrcs;
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.

https://codereview.webrtc.org/2605033002/diff/120001/webrtc/api/rtcstatscolle...
webrtc/api/rtcstatscollector.cc:291: TracksToSsrcs tracks_to_ssrcs;
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.

https://codereview.webrtc.org/2605033002/diff/120001/webrtc/api/rtcstatscolle...
webrtc/api/rtcstatscollector.cc:294: continue;
{}s please

https://codereview.webrtc.org/2605033002/diff/120001/webrtc/api/rtcstatscolle...
webrtc/api/rtcstatscollector.cc:295: if (sender->ssrc() != 0) {
Why not do another early continue?

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

https://codereview.webrtc.org/2605033002/diff/120001/webrtc/api/rtcstatscolle...
webrtc/api/rtcstatscollector.cc:316: continue;
{}s please

https://codereview.webrtc.org/2605033002/diff/120001/webrtc/api/rtcstatscolle...
webrtc/api/rtcstatscollector.cc:321: continue;
{}s please

https://codereview.webrtc.org/2605033002/diff/120001/webrtc/api/rtcstatscolle...
webrtc/api/rtcstatscollector.cc:322: RTC_DCHECK_EQ(1u,
parameters.encodings.size());
This is going to explode as soon as we implement GetParameters() for simulcast. 
Could you make it RTC_DCHECK_LE?

https://codereview.webrtc.org/2605033002/diff/120001/webrtc/api/rtcstatscolle...
webrtc/api/rtcstatscollector.cc:325: tracks_to_ssrcs[receiver->track().get()] =
*ssrc;
{}s please

https://codereview.webrtc.org/2605033002/diff/120001/webrtc/api/rtcstatscolle...
webrtc/api/rtcstatscollector.cc:332: const T* GetMediaInfoFromSsrc(const
std::vector<T>& infos, uint32_t ssrc) {
Sounds more like "BySsrc" than "FromSsrc"

Or why not just "FindBySsrc"?

https://codereview.webrtc.org/2605033002/diff/120001/webrtc/api/rtcstatscolle...
webrtc/api/rtcstatscollector.cc:335: return &info;
{}s please

https://codereview.webrtc.org/2605033002/diff/120001/webrtc/api/rtcstatscolle...
webrtc/api/rtcstatscollector.cc:341: const std::vector<uint32_t>& ssrcs) {
Why not just call this ToStrings?  You could even templatize it.

https://codereview.webrtc.org/2605033002/diff/120001/webrtc/api/rtcstatscolle...
webrtc/api/rtcstatscollector.cc:561:
ProducePartialResultsOnSignalingThread(timestamp_us);
Should you DCHECK that you're on the signaling thread?

https://codereview.webrtc.org/2605033002/diff/120001/webrtc/api/test/fakertps...
File webrtc/api/test/fakertpsenderreceiver.h (right):

https://codereview.webrtc.org/2605033002/diff/120001/webrtc/api/test/fakertps...
webrtc/api/test/fakertpsenderreceiver.h:25: RtpParameters
CreateRtpParametersWithSsrc(uint32_t ssrc) {
I'd prefer a name like CreateRtpParametersWithOneSsrc.

And I'm not entirely sure it's worth it to have it exposed via the header file.

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

This CL correctly lays the groundwork for later stats, but I think there's still
some uncertainty about what "ssrcIds" is supposed to be, or whether it's
necessary in the first place. Currently the options being weighed are "rename it
to streamStatsIds" or "remove it": https://github.com/w3c/webrtc-stats/issues/85

So we should resolve the issue with the spec first before we land this.

https://codereview.webrtc.org/2605033002/diff/120001/webrtc/api/peerconnection.h
File webrtc/api/peerconnection.h (right):

https://codereview.webrtc.org/2605033002/diff/120001/webrtc/api/peerconnectio...
webrtc/api/peerconnection.h:97: virtual
std::vector<rtc::scoped_refptr<RtpSenderInterface>> GetSenders()
These are already virtual (hence "override" at the end).

https://codereview.webrtc.org/2605033002/diff/120001/webrtc/api/rtcstatscolle...
File webrtc/api/rtcstatscollector.cc (right):

https://codereview.webrtc.org/2605033002/diff/120001/webrtc/api/rtcstatscolle...
webrtc/api/rtcstatscollector.cc:315: if (!receiver->track())
A receiver should always have a track; you can DCHECK here.

https://codereview.webrtc.org/2605033002/diff/120001/webrtc/api/rtcstatscolle...
webrtc/api/rtcstatscollector.cc:322: RTC_DCHECK_EQ(1u,
parameters.encodings.size());
On 2017/01/03 19:29:00, pthatcher1 wrote:
> This is going to explode as soon as we implement GetParameters() for
simulcast. 
> Could you make it RTC_DCHECK_LE?

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

https://codereview.webrtc.org/2605033002/diff/120001/webrtc/api/rtcstatscolle...
webrtc/api/rtcstatscollector.cc:561:
ProducePartialResultsOnSignalingThread(timestamp_us);
On 2017/01/03 19:28:59, pthatcher1 wrote:
> Should you DCHECK that you're on the signaling thread?

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

https://codereview.webrtc.org/2605033002/diff/120001/webrtc/api/rtcstatscolle...
File webrtc/api/rtcstatscollector_unittest.cc (right):

https://codereview.webrtc.org/2605033002/diff/120001/webrtc/api/rtcstatscolle...
webrtc/api/rtcstatscollector_unittest.cc:1260: CreateRtpParametersWithSsrc(4));
May be overkill, but you could also test what happens if an RtpReceiver's
SSRC(s) are unknown (which happens if SSRCs aren't signaled and no packets have
been received yet).

[hbos, 2017-01-10 13:42:08]

Conclusion of
https://github.com/w3c/webrtc-stats/issues/85#issuecomment-271545059 is that
ssrcIds will be removed.
The groundwork wiring of tracks <-> infos is done here instead:
https://codereview.webrtc.org/2611983002/.

With that, I'm closing this CL without landing. If the above CL lands the CLs
depending on this one will have to rebase to that one instead.