1055378 - Calling getStats with a track returns no stats or stats for wrong track

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[mroberts]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/36.0.1985.143 Safari/537.36

Steps to reproduce:

1. In Firefox, navigate to apprtc.appspot.com/?audio=true&video=false and allow microphone access.
2. In another Firefox tab or in Chrome, navigate to the same apprtc room, also passing audio=true&video=false, and allow microphone access.
3. In Firefox, run the following commands in the console:

    pc.getStats(pc.getRemoteStreams()[0].getAudioTracks()[0],console.log.bind(console),console.error.bind(console));

    pc.getStats(pc.getLocalStreams()[0].getAudioTracks()[0],console.log.bind(console),console.error.bind(console));

4. In Firefox, navigate to about:webrtc and compare the logged RTCStatsReports to those reported internally.


Actual results:

Calling getStats on the local audio track returned an empty RTCStatsReport. Calling getStats on the remote audio track returned an RTCStatsReport containing one item of type "inboundrtp", but no item of type "outboundrtp". Looking in about:webrtc, there is an unreported item of type "outboundrtp" (in my case, named "outbound_rtp_audio_0").


Expected results:

I expected in at least one of the RTCStatsReports to see an item of type "outboundrtp" from which I could read "bytesSent", "packetsSent", etc.

If you rerun my steps, but set audio=true&video=true, then the result is different:
- Calling getStats on the local audio track returns a non-empty RTCStatsReport.
- Calling getStats on the remote audio track returns an RTCStatsReport that contains an item of type "outboundrtp" from which I can read "bytesSent", "packetsSent", etc., matching that reported in about:webrtc.

And so it seems the RTCStatsReport isn't being set correctly in the audio-only case?

Comment 1

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to mroberts from comment #0)
> pc.getStats(pc.getRemoteStreams()[0].getAudioTracks()[0],console.log.
> bind(console),console.error.bind(console));

Thanks for the excellent reproduce steps and the console.log.bind(console) trick!

I can reproduce it and it seems we have an index error referring to tracks, so even when you do get info back, it may be for the wrong track unfortunately. Fix coming up.

Workaround:
===========

The track argument is a filter, and passing in null instead will return stats for all tracks:

  e.g. pc.getStats(null,console.log.bind(console),console.error.bind(console));

You should then be able to iterate and to tell audio from video tracks.

Please note though that it is not sufficient to check for type:"outboundrtp", you must also check for isRemote:false, otherwise you're looking at RTCP info from the other side (which is stored in the same structure).

Comment 2

[mroberts]

Thanks for taking a look, and thanks for the tip on isRemote:false! I can confirm that the workaround works for apprtc.

However, the workaround does not work for my own application. When I pass null, Firefox displays an empty error message (no text, no line numbers) in the console. Here's what it looks like: http://i.imgur.com/DdJQZrz.png

This error when passing null was actually the reason I explored using the individual tracks. Unfortunately, I do not yet have a small, reproducible test case for this, as my application relies on a media server rather than another WebRTC client.

Comment 3

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to mroberts from comment #2)
> However, the workaround does not work for my own application. When I pass
> null, Firefox displays an empty error message (no text, no line numbers) in
> the console. Here's what it looks like: http://i.imgur.com/DdJQZrz.png

Any errors in the Tools/Web Developer/Browser Console?

Comment 4

[mroberts]

Ah, totally my fault there! My code had an error where I was calling forEach on the RTCStatsReport. Thanks for pointing me to the Browser Console. I can confirm that the workaround works for my application.

Comment 5

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: getStats w/non-null track arg now returns stats for the right track

Now relies on MediaStream relationship and known implementation limitations rather than trackIds that were wrong.

I hope this lands before your patch. ;-)

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Adds RTCRTPStreamStats.mediaType + test of getStats w/track arg

- Adds test of the track-filtering in getStats. (drno)
  I added it to templates.js to piggyback on and test all the various
  audio/video combinations, since our trackId handling is a bit fragile,
  and because tests with callbacks are a pain (when can we use promises?)

- Adds RTCRTPStreamStats.mediaType stat, which is based on the latest
  agreement for checking media type [1], since I needed it for the test. (smaug)

Try - https://tbpl.mozilla.org/?tree=Try&rev=582107070d29

[1] http://lists.w3.org/Archives/Public/public-webrtc/2014May/0129.html

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8476926 [details] [diff] [review]
getStats w/non-null track arg now returns stats for the right track

Review of attachment 8476926 [details] [diff] [review]:
-----------------------------------------------------------------

r- -- makes too many assumptions about trackids that it doesn't need to make (and will break without warning), see replaceTrack patches

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +2224,5 @@
> +    if (aSelector) {
> +      if (mMedia->GetRemoteStream(i)->GetMediaStream()->HasTrack(*aSelector)) {
> +        // XXX This MUST be addressed when we add multiple tracks of a type!
> +        // XXX Remote trackIds are 1-based while Local ones are zero-based!!!
> +        TrackID id = aSelector->AsVideoStreamTrack() ? 2 : 1;

instead look at my ReplaceTrack patch; in HasTrackType() it finds the index into mPipelines for a given type (video or audio), and then you can use trackid() (receive side) or trackid_locked() (transmit side) to get the actual trackid

Comment 8

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: getStats w/non-null track arg now returns stats for the right track (2)

A small change ended up in the wrong patch. Re-uploading

Comment 9

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 2: Adds RTCRTPStreamStats.mediaType + test of getStats w/track arg (2)

A small change ended up in the wrong patch. Re-uploading

Comment 10

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 1: getStats w/non-null track arg now returns stats for the right track (3)

Incorporated feedback.

Try - https://tbpl.mozilla.org/?tree=Try&rev=50b8fb49da8a

Comment 11

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 3: Tightened test of getStats w/track arg

Checks that filter still filters.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8476959 [details] [diff] [review]
Part 2: Adds RTCRTPStreamStats.mediaType + test of getStats w/track arg (2)

http://dev.w3.org/2011/webrtc/editor/webrtc.html#idl-def-RTCRTPStreamStats
looks rather different.
So assuming http://lists.w3.org/Archives/Public/public-webrtc/2014May/0129.html holds, r+.

(we really should get the spec updated asap when we land changes to our webidl)

Comment 13

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Olli Pettay [:smaug] from comment #12)
> http://dev.w3.org/2011/webrtc/editor/webrtc.html#idl-def-RTCRTPStreamStats
> looks rather different.

This is more recent: https://www.w3.org/2011/04/webrtc/wiki/Stats

Comment 14

[Nils Ohlmeier [:drno]]

Comment on attachment 8476959 [details] [diff] [review]
Part 2: Adds RTCRTPStreamStats.mediaType + test of getStats w/track arg (2)

Review of attachment 8476959 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

::: dom/media/tests/mochitest/templates.js
@@ +418,5 @@
> +    'PC_LOCAL_CHECK_GETSTATS_AUDIOTRACK_OUTBOUND',
> +    function (test) {
> +      var pc = test.pcLocal;
> +      var stream = pc._pc.getLocalStreams()[0];
> +      var track = stream && stream.getAudioTracks()[0];

I'm wondering if we should iterate over streams and tracks here instead of assuming that the first track in the first stream is what we are looking for.

@@ +422,5 @@
> +      var track = stream && stream.getAudioTracks()[0];
> +      if (track) {
> +        pc.getStats(track, function(stats) {
> +          ok(pc.hasStat(stats,
> +                        { type:"outboundrtp", isRemote:false, mediaType:"audio" }),

Is isRemote ever going to be true?

Comment 15

[Nils Ohlmeier [:drno]]

Comment on attachment 8477476 [details] [diff] [review]
Part 3: Tightened test of getStats w/track arg

Review of attachment 8477476 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

Comment 16

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Nils Ohlmeier [:drno] from comment #14)
> > +      var stream = pc._pc.getLocalStreams()[0];
> > +      var track = stream && stream.getAudioTracks()[0];
> 
> I'm wondering if we should iterate over streams and tracks here instead of
> assuming that the first track in the first stream is what we are looking for.

It doesn't really matter which audio-track we test getStats track-filtering on (and besides we're limited to 1 audio track for now), so I think we're fine (and same for video).

One reason I did it this way is that calling async-calls like getStats once per test is hard enough - and partly why I hijacked templates.js for this - but calling them several times in a for-loop is a PITA. 

> > +        pc.getStats(track, function(stats) {
> > +          ok(pc.hasStat(stats,
> > +                        { type:"outboundrtp", isRemote:false, mediaType:"audio" }),
> 
> Is isRemote ever going to be true?

Yes, isRemote:true signifies RTCP data which is stored in an entry of the opposite type. For example: the RTCP info for the outbound/sender track shown above would in an type:"inboundrtp" entry with isRemote:true. These pairs link to each-other using remoteId.

Btw, the reason I don't test for the presence of RTCP data is that it often takes a few seconds to appear and it caused the tests to fail.

Comment 17

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Try https://tbpl.mozilla.org/?tree=Try&rev=f66b44be551e

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e98290b4d231
https://hg.mozilla.org/integration/mozilla-inbound/rev/edfbcca4ef08
https://hg.mozilla.org/integration/mozilla-inbound/rev/b337b2d0f24d

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e98290b4d231
https://hg.mozilla.org/mozilla-central/rev/edfbcca4ef08
https://hg.mozilla.org/mozilla-central/rev/b337b2d0f24d