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

RESOLVED FIXED in mozilla34

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mroberts, Assigned: jib)

Tracking

31 Branch
mozilla34
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

4 years ago
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?
Assignee: nobody → jib
(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).
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: getStats doesn't return outboundrtp statistics for audio-only calls → Calling getStats with a track returns no stats or stats for wrong track
(Reporter)

Comment 2

4 years ago
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.
(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?
(Reporter)

Comment 4

4 years ago
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.
Created attachment 8476926 [details] [diff] [review]
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. ;-)
Attachment #8476926 - Flags: review?(rjesup)
Created attachment 8476943 [details] [diff] [review]
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
Attachment #8476943 - Flags: review?(drno)
Attachment #8476943 - Flags: review?(bugs)
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
Attachment #8476926 - Flags: review?(rjesup) → review-
Created attachment 8476957 [details] [diff] [review]
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
Attachment #8476926 - Attachment is obsolete: true
Attachment #8476957 - Flags: review?(rjesup)
Created attachment 8476959 [details] [diff] [review]
Part 2: Adds RTCRTPStreamStats.mediaType + test of getStats w/track arg (2)

A small change ended up in the wrong patch. Re-uploading
Attachment #8476943 - Attachment is obsolete: true
Attachment #8476943 - Flags: review?(drno)
Attachment #8476943 - Flags: review?(bugs)
Attachment #8476959 - Flags: review?(drno)
Attachment #8476959 - Flags: review?(bugs)
Attachment #8476957 - Flags: review?(rjesup)
Created attachment 8477180 [details] [diff] [review]
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
Attachment #8476957 - Attachment is obsolete: true
Attachment #8477180 - Flags: review?(rjesup)
Depends on: 1032839
Attachment #8476959 - Flags: review?(rjesup)
Created attachment 8477476 [details] [diff] [review]
Part 3: Tightened test of getStats w/track arg

Checks that filter still filters.
Attachment #8477476 - Flags: review?(drno)
Attachment #8477180 - Attachment description: getStats w/non-null track arg now returns stats for the right track (3) → Part 1: getStats w/non-null track arg now returns stats for the right track (3)
Attachment #8476959 - Attachment description: Adds RTCRTPStreamStats.mediaType + test of getStats w/track arg (2) → Part 2: Adds RTCRTPStreamStats.mediaType + test of getStats w/track arg (2)
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)
Attachment #8476959 - Flags: review?(bugs) → review+
(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
Attachment #8476959 - Flags: review?(rjesup) → review+
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?
Attachment #8476959 - Flags: review?(drno) → review+
Comment on attachment 8477476 [details] [diff] [review]
Part 3: Tightened test of getStats w/track arg

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

LGTM
Attachment #8477476 - Flags: review?(drno) → review+
(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.
Attachment #8477180 - Flags: review?(rjesup) → review+
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.