Closed
Bug 1055378
Opened 10 years ago
Closed 10 years ago
Calling getStats with a track returns no stats or stats for wrong track
Categories
(Core :: WebRTC, defect)
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: mroberts, Assigned: jib)
References
Details
Attachments
(3 files, 3 obsolete files)
11.24 KB,
patch
|
smaug
:
review+
drno
:
review+
jesup
:
review+
|
Details | Diff | Splinter Review |
6.31 KB,
patch
|
jesup
:
review+
|
Details | Diff | Splinter Review |
6.65 KB,
patch
|
drno
:
review+
|
Details | Diff | Splinter Review |
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?
Updated•10 years ago
|
Assignee: nobody → jib
Assignee | ||
Comment 1•10 years ago
|
||
(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
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.
Assignee | ||
Comment 3•10 years ago
|
||
(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?
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.
Assignee | ||
Comment 5•10 years ago
|
||
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)
Assignee | ||
Comment 6•10 years ago
|
||
- 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 7•10 years ago
|
||
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-
Assignee | ||
Comment 8•10 years ago
|
||
A small change ended up in the wrong patch. Re-uploading
Attachment #8476926 -
Attachment is obsolete: true
Attachment #8476957 -
Flags: review?(rjesup)
Assignee | ||
Comment 9•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8476957 -
Flags: review?(rjesup)
Assignee | ||
Comment 10•10 years ago
|
||
Incorporated feedback. Try - https://tbpl.mozilla.org/?tree=Try&rev=50b8fb49da8a
Attachment #8476957 -
Attachment is obsolete: true
Attachment #8477180 -
Flags: review?(rjesup)
Assignee | ||
Updated•10 years ago
|
Attachment #8476959 -
Flags: review?(rjesup)
Assignee | ||
Comment 11•10 years ago
|
||
Checks that filter still filters.
Attachment #8477476 -
Flags: review?(drno)
Assignee | ||
Updated•10 years ago
|
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)
Assignee | ||
Updated•10 years ago
|
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 12•10 years ago
|
||
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+
Assignee | ||
Comment 13•10 years ago
|
||
(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
Updated•10 years ago
|
Attachment #8476959 -
Flags: review?(rjesup) → review+
Comment 14•10 years ago
|
||
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 15•10 years ago
|
||
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+
Assignee | ||
Comment 16•10 years ago
|
||
(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.
Updated•10 years ago
|
Attachment #8477180 -
Flags: review?(rjesup) → review+
Assignee | ||
Comment 17•10 years ago
|
||
Try https://tbpl.mozilla.org/?tree=Try&rev=f66b44be551e
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 18•10 years ago
|
||
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
Keywords: checkin-needed
Comment 19•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e98290b4d231 https://hg.mozilla.org/mozilla-central/rev/edfbcca4ef08 https://hg.mozilla.org/mozilla-central/rev/b337b2d0f24d
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in
before you can comment on or make changes to this bug.
Description
•