The current RTCStatsReport implementation ambitiously deviates from http://dev.w3.org/2011/webrtc/edi...brtc.html#rtcstatsreport-object by using a still-in-development http://heycam.github.io/webidl/#MapClass pattern instead of a getter. This was done partly because of advice and partly because our webidl compiler doesn't support getters yet (Bug 851282). However, a lot of the problems that plague getters don't manifest when the set of supported property names is immutable, as is the case with getStats(). Also, the simpler syntax of stats[id] over stats.get(id) is desirable to the WG and for compatibility with Chrome at this point. Bz showed me we can get the behavior of getters by creating properties manually on the .wrappedJSObject, which I've taken a stab at, without even removing the MapClass behavior. By working both ways for now we get compatibility with the spec, with Chrome and older Firefox, and we can have the MapClass discussion later.

Supports both MapClass and getter pattern of enumeration.

Comment on attachment 8348792 [details] [diff] [review] Spec-compliant enumeration of RTCStatsReport (2) >+ props[k] = { enumerable:true, configurable:false, writable:false, >+ value:obj[k] }; So this sets up non-configurable readonly properties... >+ Cu.makeObjectPropsNormal(pubobj); Then how can this possibly work? Seems to me like this either throws or does nothing for the properties in question (looks like the latter, based on looking at makeObjectPropsNormal). That might be OK if the "stat" objects in appendStats only have primitive-valued properties. Do they?

Comment on attachment 8348792 [details] [diff] [review] Spec-compliant enumeration of RTCStatsReport (2) Also, why is the "for (let k in cobj) {" bit needed? That needs some documentation, at the very least, because that whole block is not making sense to me. >+ for (var key in this.__DOM_IMPL__.wrappedJSObject) { This seems wrong, unless you're trying to include properties the page has defined on the object. Since you're checking for "this" having the property anyway on the next line, why are you looking at property names from the wrappedJSObject? In general, every use of .wrappedJSObject should have a comment explaining why it's needed. And every use of this publify() stuff should too, since it's all security-sensitive stuff you don't want people messing with...

(In reply to Boris Zbarsky [:bz] from comment #3) > Comment on attachment 8348792 [details] [diff] [review] > Spec-compliant enumeration of RTCStatsReport (2) > > >+ props[k] = { enumerable:true, configurable:false, writable:false, > >+ value:obj[k] }; > > So this sets up non-configurable readonly properties... Yes, the spec doesn't say, but I thought it made some sense for stats to be read-only? > >+ Cu.makeObjectPropsNormal(pubobj); > > Then how can this possibly work? > > Seems to me like this either throws or does nothing for the properties in > question (looks like the latter, based on looking at makeObjectPropsNormal). > That might be OK if the "stat" objects in appendStats only have > primitive-valued properties. Do they? They do. This publify() is only called from appendStats() which is only called with internal stats dictionaries as 'stats' argument. I should probably unroll this single-use function so its context is clearer. I'll remove the makeObjectPropsNormal() call since I didn't know what it did until now (cut'n'paste error on my part). > Also, why is the "for (let k in cobj) {" bit needed? That needs some > documentation, at the very least, because that whole block is not making > sense to me. Sure. The first publify() inside appendStats() is for the individual stats records (objects) themselves, associated with each property name, so users are able to see into each one of them. The "for (let k in cobj)" part is for the RTCStatsObject itself (i.e. nesting-level 0), so users can enumerate the different properties. They're both needed, or visibility is obstructed. > >+ for (var key in this.__DOM_IMPL__.wrappedJSObject) { > > This seems wrong, unless you're trying to include properties the page has > defined on the object. This code is the .forEach() MapClass support. Since I'm already storing the stats in the public object, I thought it made sense to read them from there instead of keeping them in two places. I can keep an internal copy if you think this is unsafe. > Since you're checking for "this" having the property > anyway on the next line, why are you looking at property names from the > wrappedJSObject? Ah, thanks for finding this bug! I couldn't figure out how I broke the forEach() case. I meant wrappedJSObject in place of this here. > In general, every use of .wrappedJSObject should have a comment explaining > why it's needed. And every use of this publify() stuff should too, since > it's all security-sensitive stuff you don't want people messing with... I'll add comments and resubmit. Thanks!

> Yes, the spec doesn't say, It needs to, if it specs the properties existing. But my real issue was with calling a function which is supposed to change the values of the properties after you've just made sure their values can't be changed. > They're both needed I don't understand why... appendStats sets properties on this.__DOM_IMPL__.wrappedJSObject and it sets them to content objects afaict. Why do they then need to be redefined again? Bobby? > Since I'm already storing the stats in the public object, I thought it made sense to > read them from there I don't understand. You do: >+ for (var key in this.__DOM_IMPL__.wrappedJSObject) { >+ if (this.hasOwnProperty(key) && key != "mozPcid") { which ends up looking for keys on the wrappedJSObject whose name is not "mozPcid" (should that even be on the content object?) which are also own properties of "this". Why do you need to look at the wrappedJSObject here at all? And if as you say that part is wrong and the hasOwnProperty should be on the content object, you will get a superset of the right set of propety names here, no? Getting the value from the content object is maybe ok, once you have the right property names, since you define these props as readonly nonconfigurable....

(In reply to Boris Zbarsky [:bz] (Vacation Dec 19 to Jan 1) from comment #6) > > Yes, the spec doesn't say, > > It needs to, if it specs the properties existing. Actually, the spec specifies a getter, which I interpret to mean that reading should be possible, but not writing. This seemed the most equivalent. > > They're both needed > > I don't understand why... appendStats sets properties on > this.__DOM_IMPL__.wrappedJSObject and it sets them to content objects > afaict. Why do they then need to be redefined again? Bobby? > > > Since I'm already storing the stats in the public object, I thought it made sense to > > read them from there > > I don't understand. You do: > > >+ for (var key in this.__DOM_IMPL__.wrappedJSObject) { > >+ if (this.hasOwnProperty(key) && key != "mozPcid") { > > which ends up looking for keys on the wrappedJSObject whose name is not > "mozPcid" (should that even be on the content object?) Yes, mozPcid is a field used by our about:webrtc page unfortunately. I'll try using a get method instead to avoid tripping on it. > which are also own > properties of "this". Why do you need to look at the wrappedJSObject here > at all? Because that's where the stats are stored. > And if as you say that part is wrong and the hasOwnProperty should > be on the content object, you will get a superset of the right set of > propety names here, no? > > Getting the value from the content object is maybe ok, once you have the > right property names, since you define these props as readonly > nonconfigurable.... I have a new patch that cleans this up quite a bit.

> Actually, the spec specifies a getter That fully specifies the behavior, actually. For a named getter, http://heycam.github.io/webidl/#getownproperty applies and the properties should be enumerable, configurable, readonly. Of course trying to configure them actually needs to be prevented, per spec; see http://heycam.github.io/webidl/#defineownproperty and note that in your case you'd fall into step 2, substep 2, subsubstep 1. Basically, the getter behavior can only be implemented fully faithfully by a proxy. Yet another reason the spec shouldn't be using one here. > Yes, mozPcid is a field used by our about:webrtc page unfortunately. Can you just make it an actual webidl attribute and annotate it as only being available on that page? That seems ideal... > Because that's where the stats are stored. That object is under the control of the page. You shouldn't store anything chrome code might care about on it.

(In reply to Boris Zbarsky [:bz] (Vacation Dec 19 to Jan 1) from comment #9) > > Yes, mozPcid is a field used by our about:webrtc page unfortunately. > > Can you just make it an actual webidl attribute and annotate it as only > being available on that page? That seems ideal... How do I do that?

You have to write a bit of C++ code, unfortunately. Specifically, you use https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#.5BFunc.3D.22funcname.22.5D and use some C++ function that you put somewhere (ideally in some webrtc-related header file). The implementation of that function can do something like what Navigator::GetWindowFromGlobal does to get the DOM window and then get its document and check the URI. But the end result will be that the property will only be exposed on pages that have that URI.

Minor update: - Forgot to update has() function to match the other code changes. Otherwise unchanged. (In reply to Boris Zbarsky [:bz] (Vacation Dec 19 to Jan 1) from comment #9) > That fully specifies the behavior, actually. For a named getter, > http://heycam.github.io/webidl/#getownproperty applies and the properties > should be enumerable, configurable, readonly. > > Of course trying to configure them actually needs to be prevented, per spec; > see http://heycam.github.io/webidl/#defineownproperty and note that in your > case you'd fall into step 2, substep 2, subsubstep 1. So enumerable, non-configurable, readonly is what I have. Is that good? > Basically, the getter behavior can only be implemented fully faithfully by a > proxy. Yet another reason the spec shouldn't be using one here. OK, but this patch aims to adhere to the present spec behavior, unlike the existing code which deviates quite a bit, so I'd love to see this land before championing any changes to the spec. > > Yes, mozPcid is a field used by our about:webrtc page unfortunately. > > Can you just make it an actual webidl attribute and annotate it as only > being available on that page? That seems ideal... The mozPcid attribute is already public and the latest patch gives it a getter which keeps it out of enumeration, so my hope is that making it more private, if need be, can be a separate task. > > Because that's where the stats are stored. > > That object is under the control of the page. You shouldn't store anything > chrome code might care about on it. The new patch only stores a copy there and never references it.

Comment on attachment 8349204 [details] [diff] [review] Spec-compliant enumeration of RTCStatsReport (4) > So enumerable, non-configurable, readonly is what I have. Is that good? It's the best you can do for the moment. It's not quite what the spec says, but getOwnPropertyDescriptor is the only way to tell them apart. > which keeps it out of enumeration, getOwnPropertyNames would still return it, note. > so my hope is that making it more private, if need be, can be a separate task. I'm ok with a followup for that part; I just think we should do it. Will read through the details here tomorrow.

Comment on attachment 8349204 [details] [diff] [review] Spec-compliant enumeration of RTCStatsReport (4) >+ // TODO: Change to use webidl getters once available (Bug 851282) Should file a bug on this, depending on bug 851282, so once that gets fixed we'll know to fix this too. r=me

Updated comment to mention filed bug. Carrying forward r+ from bz.

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

Comment on attachment 8350346 [details] [diff] [review] Basic getStats mochitest Enough to prove stats are working on all builds. We can always add more.

Should work better (c++ programmer writing JS). Try - https://tbpl.mozilla.org/?tree=Try&rev=192b1bd45a49

Boris, can you clarify the input you still need, if anything?

I think we're good now, since we're no longer doing all that makeObjectPropsNormal stuff.

Comment on attachment 8350468 [details] [diff] [review] Basic getStats mochitest (2) Review of attachment 8350468 [details] [diff] [review]: ----------------------------------------------------------------- transferring review to whimboo (jsmith would be good too, either can review this)

Comment on attachment 8350468 [details] [diff] [review] Basic getStats mochitest (2) Review of attachment 8350468 [details] [diff] [review]: ----------------------------------------------------------------- Left some questions in the patch. Clearing review for now to get the questions answered, as I can't conclude yet if this is a r- or r+. ::: dom/media/tests/mochitest/pc.js @@ +1376,5 @@ > + * @param {object} stats > + * The stats to check from this PeerConnectionWrapper > + */ > + checkStats : function PCW_checkStats(stats) { > + function toNum(obj) { Nit - I'd name this toCounter instead of toNum to signify that you are counting something. @@ +1379,5 @@ > + checkStats : function PCW_checkStats(stats) { > + function toNum(obj) { > + return obj? obj : 0; > + } > + function numTracks(streams) { Nit - this might be better to have in head.js as a helper function @@ +1390,5 @@ > + > + // Use spec way of enumerating stats > + var counters = {}; > + for (var key in stats) { > + if (stats.hasOwnProperty(key)) { Confused - why is this needed? If you iterating over a dictionary, wouldn't this always be true? @@ +1407,5 @@ > + var nout = numTracks(this._pc.getLocalStreams()); > + is(toNum(counters["inboundrtp"]), nin, "Have " + nin + " inboundrtp stat(s)"); > + is(toNum(counters["outboundrtp"]), nout, "Have " + nout + " outboundrtp stat(s)"); > + ok(toNum(counters["localcandidate"]), "Have localcandidate stat(s)"); > + ok(toNum(counters["remotecandidate"]), "Have remotecandidate stat(s)"); Aren't there other stats possible here to analyze? ::: dom/media/tests/mochitest/templates.js @@ +165,5 @@ > test.next(); > }); > } > + ], > + [ Curious - why only test this as part of the Peer Connection tests? Why not include this in the Data Channel tests? @@ +182,5 @@ > + test.pcRemote.checkStats(stats); > + test.next(); > + }); > + } > + ], Nit - extra comma

> If you iterating over a dictionary, wouldn't this always be true? for..in loops in JS will see proto properties too.

Comment on attachment 8350468 [details] [diff] [review] Basic getStats mochitest (2) Review of attachment 8350468 [details] [diff] [review]: ----------------------------------------------------------------- Good starting point. I would like to have a test comparing the received and send packets etc. from both sides of the call, but we can add that later. ::: dom/media/tests/mochitest/pc.js @@ +1364,5 @@ > + var self = this; > + > + this._pc.getStats(selector, function(stats) { > + info(self + ": Got stats: " + JSON.stringify(stats)); > + self._last_stats = stats; Nit - is the storing of the result to enable comparising of old and current values in future tests? @@ +1366,5 @@ > + this._pc.getStats(selector, function(stats) { > + info(self + ": Got stats: " + JSON.stringify(stats)); > + self._last_stats = stats; > + onSuccess(stats); > + }, unexpectedCallbackAndFinish()); Curious - why is this call covered with an unexpectedCallbackAndFinish(), but the PCW_checkStats() not? @@ +1382,5 @@ > + } > + function numTracks(streams) { > + var n = 0; > + streams.forEach(function(stream) { > + n += stream.getAudioTracks().length + stream.getVideoTracks().length; I hope the stats code doesn't rely on the same code under the hood to determine the number of tracks. Curious - do we actually have any test case where the getAudioTracks() or the getVideoTracks() would return more then just one track? @@ +1392,5 @@ > + var counters = {}; > + for (var key in stats) { > + if (stats.hasOwnProperty(key)) { > + var res = stats[key]; > + counters[res.type] = toNum(counters[res.type]) + 1; Confused - does res.type give you the name of the stat or just something like "string" or "int"? What is the reason for having the counters always +1 (instead of e.g. just 0)? @@ +1401,5 @@ > + stats.forEach(function(res) { > + counters2[res.type] = toNum(counters2[res.type]) + 1; > + }); > + is(JSON.stringify(counters), JSON.stringify(counters2), > + "Spec and MapClass variant of RTCStatsReport enumeration agree"); The Getter and MapClass are taken at different points in time. How can you guarantee that they will be equal? Is the assumption that the streams ended already? ::: dom/media/tests/mochitest/templates.js @@ +168,5 @@ > + ], > + [ > + 'PC_LOCAL_CHECK_STATS', > + function (test) { > + test.pcLocal.getStats(null, function(stats) { What is the purpose of exposing the selector all way to here if it only gets null input in both cases? Can't we simply put null directly into the this._pc.getStats() call in PCW_getStats() function instead?

> ::: dom/media/tests/mochitest/templates.js > @@ +165,5 @@ > > test.next(); > > }); > > } > > + ], > > + [ > > Curious - why only test this as part of the Peer Connection tests? Why not > include this in the Data Channel tests? The Data Channel does not use RTP and therefore we don't get RTCP reports for them. So these stats don't apply to data channels.

Comment on attachment 8350468 [details] [diff] [review] Basic getStats mochitest (2) Review of attachment 8350468 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/tests/mochitest/pc.js @@ +1364,5 @@ > + var self = this; > + > + this._pc.getStats(selector, function(stats) { > + info(self + ": Got stats: " + JSON.stringify(stats)); > + self._last_stats = stats; FWIW - That's what I thought the intention of this variable was. @@ +1366,5 @@ > + this._pc.getStats(selector, function(stats) { > + info(self + ": Got stats: " + JSON.stringify(stats)); > + self._last_stats = stats; > + onSuccess(stats); > + }, unexpectedCallbackAndFinish()); Probably because there isn't a need for an error callback on PCW_getStats, as if unexpectedCallbackAndFinish does get called, we'll end the test with an error. @@ +1382,5 @@ > + } > + function numTracks(streams) { > + var n = 0; > + streams.forEach(function(stream) { > + n += stream.getAudioTracks().length + stream.getVideoTracks().length; To my understanding, I don't believe there's a mochitest exercising this behavior yet. Is this even possible yet in Peer Connections? @@ +1392,5 @@ > + var counters = {}; > + for (var key in stats) { > + if (stats.hasOwnProperty(key)) { > + var res = stats[key]; > + counters[res.type] = toNum(counters[res.type]) + 1; FWIW - I think what the code is doing here is counting the number of something per each type seen. So the +1 is just incrementing a counter for a particular type. @@ +1401,5 @@ > + stats.forEach(function(res) { > + counters2[res.type] = toNum(counters2[res.type]) + 1; > + }); > + is(JSON.stringify(counters), JSON.stringify(counters2), > + "Spec and MapClass variant of RTCStatsReport enumeration agree"); FWIW - at this point in the test, the streams would not be ended. ::: dom/media/tests/mochitest/templates.js @@ +168,5 @@ > + ], > + [ > + 'PC_LOCAL_CHECK_STATS', > + function (test) { > + test.pcLocal.getStats(null, function(stats) { Probably for extensibility in case we want to add tests for getting stats off of a media stream track.

(In reply to Nils Ohlmeier [:drno] from comment #25) > > ::: dom/media/tests/mochitest/templates.js > > @@ +165,5 @@ > > > test.next(); > > > }); > > > } > > > + ], > > > + [ > > > > Curious - why only test this as part of the Peer Connection tests? Why not > > include this in the Data Channel tests? > > The Data Channel does not use RTP and therefore we don't get RTCP reports > for them. So these stats don't apply to data channels. Okay - so two followup points to that then: 1. Some of the tests in the data channels exercise having an audio, video, or audio & video P2P call active with a data channel active (e.g. test_dataChannel_basicAudio.html). Shouldn't we have relevant getStats tests for these? 2. For the cases where no streams involved, we should then have negative tests using getStats on a P2P call only using data channels to ensure that the right behavior is executed.

(In reply to Jason Smith [:jsmith] from comment #22) > ::: dom/media/tests/mochitest/pc.js > @@ +1376,5 @@ > > + * @param {object} stats > > + * The stats to check from this PeerConnectionWrapper > > + */ > > + checkStats : function PCW_checkStats(stats) { > > + function toNum(obj) { > > Nit - I'd name this toCounter instead of toNum to signify that you are > counting something. Well, it converts undefined to Number. Also, hoping to avoid wrapping the is() and ok() lines. :-) > @@ +1379,5 @@ > > + checkStats : function PCW_checkStats(stats) { > > + function toNum(obj) { > > + return obj? obj : 0; > > + } > > + function numTracks(streams) { > > Nit - this might be better to have in head.js as a helper function Lets move it once multiple uses materialize. > @@ +1390,5 @@ > > + > > + // Use spec way of enumerating stats > > + var counters = {}; > > + for (var key in stats) { > > + if (stats.hasOwnProperty(key)) { > > Confused - why is this needed? If you iterating over a dictionary, wouldn't > this always be true? What bz said. If I take it out then 'forEach' shows up as a stat (because I'm also using a MapClass pattern in the returned object). That said, I'm wondering if it would be truer to the spec to try to solve this by making the functions non-enumerable... > @@ +1407,5 @@ > > + var nout = numTracks(this._pc.getLocalStreams()); > > + is(toNum(counters["inboundrtp"]), nin, "Have " + nin + " inboundrtp stat(s)"); > > + is(toNum(counters["outboundrtp"]), nout, "Have " + nout + " outboundrtp stat(s)"); > > + ok(toNum(counters["localcandidate"]), "Have localcandidate stat(s)"); > > + ok(toNum(counters["remotecandidate"]), "Have remotecandidate stat(s)"); > > Aren't there other stats possible here to analyze? Soon. You can see a dump of the stats in the try logs. I could check the structure of each one here however. > Curious - why only test this as part of the Peer Connection tests? Why not > include this in the Data Channel tests? Because I think it would test the same codepaths. This test is merely testing the stats mechanism itself. Stats on DataChannel are still in the spec stage, but they're coming.

(In reply to Nils Ohlmeier [:drno] from comment #24) > Good starting point. > I would like to have a test comparing the received and send packets etc. > from both sides of the call, but we can add that later. Thanks. This test is only of the stats-system itself. Yes, future use is why getStats() and checkStats() are separate. Go nuts. ;-) > > + this._pc.getStats(selector, function(stats) { > > + info(self + ": Got stats: " + JSON.stringify(stats)); > > + self._last_stats = stats; > > Nit - is the storing of the result to enable comparising of old and current > values in future tests? Yes. Patterned on other methods here. > > + this._pc.getStats(selector, function(stats) { > > + info(self + ": Got stats: " + JSON.stringify(stats)); > > + self._last_stats = stats; > > + onSuccess(stats); > > + }, unexpectedCallbackAndFinish()); > > Curious - why is this call covered with an unexpectedCallbackAndFinish(), > but the PCW_checkStats() not? this._pc.getStats() is an asynchronous call that takes a selector, a success callback and a failure callback. If it fails, we want to know. > > + function numTracks(streams) { > > + var n = 0; > > + streams.forEach(function(stream) { > > + n += stream.getAudioTracks().length + stream.getVideoTracks().length; > > I hope the stats code doesn't rely on the same code under the hood to > determine the number of tracks. It does not. It iterates internal live objects like MediaPipelines etc. > Curious - do we actually have any test case where the getAudioTracks() or > the getVideoTracks() would return more then just one track? Right now we hardcode one for each of audio, video, data, but that may change. > > + var counters = {}; > > + for (var key in stats) { > > + if (stats.hasOwnProperty(key)) { > > + var res = stats[key]; > > + counters[res.type] = toNum(counters[res.type]) + 1; > > Confused - does res.type give you the name of the stat or just something > like "string" or "int"? It is RTCStatsType in http://www.w3.org/2011/04/webrtc/wiki/Stats For a current data-dump, search for "stats" in https://tbpl.mozilla.org/php/getParsedLog.php?id=32265992&tree=Try&full=1 > What is the reason for having the counters always +1 (instead of e.g. just > 0)? I don't understand your question. It's an increment. The for-loop tallies up the number of each type of stats encountered. The toNum() makes each counter start at zero. > > + stats.forEach(function(res) { > > + counters2[res.type] = toNum(counters2[res.type]) + 1; > > + }); > > + is(JSON.stringify(counters), JSON.stringify(counters2), > > + "Spec and MapClass variant of RTCStatsReport enumeration agree"); > > The Getter and MapClass are taken at different points in time. How can you > guarantee that they will be equal? No, 'stats' here is an RTCStatsReport object, a singular blob of data returned from _pc.getStats() - http://dev.w3.org/2011/webrtc/editor/webrtc.html#rtcstatsreport-object - The two for-loops interrogate the same data. > ::: dom/media/tests/mochitest/templates.js > @@ +168,5 @@ > > + ], > > + [ > > + 'PC_LOCAL_CHECK_STATS', > > + function (test) { > > + test.pcLocal.getStats(null, function(stats) { > > What is the purpose of exposing the selector all way to here if it only gets > null input in both cases? For future use. You may select a MediaStreamTrack to filter by. null == give me everything.

(In reply to Jason Smith [:jsmith] from comment #26) > > + function numTracks(streams) { > > + var n = 0; > > + streams.forEach(function(stream) { > > + n += stream.getAudioTracks().length + stream.getVideoTracks().length; > > To my understanding, I don't believe there's a mochitest exercising this > behavior yet. Then we can count this one. This test compares the number of tracks (which varies among tests) against the number of stats from the MediaPipelines, which indicate activity, so it is a pretty good test of both. > Is this even possible yet in Peer Connections? Yes it works. Search for "Have 0 inboundrtp", "Have 1 inboundrtp" and "Have 2 inboundrtp" in https://tbpl.mozilla.org/php/getParsedLog.php?id=32265992&tree=Try&full=1 > > + [ > > + 'PC_LOCAL_CHECK_STATS', > > + function (test) { > > + test.pcLocal.getStats(null, function(stats) { > > Probably for extensibility in case we want to add tests for getting stats > off of a media stream track. Yes.

(In reply to Jason Smith [:jsmith] from comment #27) > (In reply to Nils Ohlmeier [:drno] from comment #25) > > The Data Channel does not use RTP and therefore we don't get RTCP reports > > for them. So these stats don't apply to data channels. Fyi, stats aren't limited to RTP, there's ICE stats in there too, though they're the same as for non-datachannel. More stats are coming. > 1. Some of the tests in the data channels exercise having an audio, video, > or audio & video P2P call active with a data channel active (e.g. > test_dataChannel_basicAudio.html). Shouldn't we have relevant getStats tests > for these? I consider this test to be of the stats-system itself, and I don't expect it to break until we add datachannel-specific stats, but how much redundancy we want is I guess up to you guys. I notice there is some overlap between commandsPeerConnection and commandsDataChannel already. I suppose they're separated for a reason? > 2. For the cases where no streams involved, we should then have negative > tests using getStats on a P2P call only using data channels to ensure that > the right behavior is executed. You still get ICE stats etc. I see some cases of "Have 0" already.

Comment on attachment 8350468 [details] [diff] [review] Basic getStats mochitest (2) Review of attachment 8350468 [details] [diff] [review]: ----------------------------------------------------------------- No further questions. Looks good to me.

Comment on attachment 8350468 [details] [diff] [review] Basic getStats mochitest (2) Review of attachment 8350468 [details] [diff] [review]: ----------------------------------------------------------------- Okay - thanks for clarification on the questions. This looks good to me then. I would suggest adding more coverage around the remaining stats that haven't been analyzed yet in the test, but I won't block landing on that.

Updated nit (removed extraneous commas). Carrying forward r+ from jsmith. (In reply to Jason Smith [:jsmith] from comment #33) > This looks good to me then. I would suggest adding more coverage around the > remaining stats that haven't been analyzed yet in the test, but I won't > block landing on that. I look forward to adding more detailed coverage to the existing stats as we add additional stats, but I'm happy to see this land with the other behavior change patch in this bug asap.

It seems like this causes ERROR TEST-UNEXPECTED-FAIL | /tests/dom/media/tests/mochitest/test_peerConnection_basicAudioVideo.html | Have 1 inboundrtp stat(s) - got 2, expected 1 on linux32/64 opt somewhat reliably. https://tbpl.mozilla.org/?tree=Try&rev=6ca845db3692

Thanks for catching that. My test appears victim to an existing intermittent anomaly that I cannot explain in your logs. Compare a good log https://tbpl.mozilla.org/php/getParsedLog.php?id=32482144&full=1&branch=try with a bad one https://tbpl.mozilla.org/php/getParsedLog.php?id=32481381&tree=Try#error0 (massaged here for readability): @@ -5,9 +5,6 @@ INFO TEST-INFO | test_pC_basicAudioVideo.html | Run step: PC_LOCAL_WAIT_FOR_ICE_CONNECTED INFO TEST-INFO | test_pC_basicAudioVideo.html | iceConnectionState: connected INFO TEST-PASS | test_pC_basicAudioVideo.html | pc_local: ICE is in connected state -INFO TEST-INFO | test_pC_basicAudioVideo.html | canplaythrough fired for media element pcRemote_remote_video -INFO TEST-INFO | test_pC_basicAudioVideo.html | timeupdate fired for media element pcRemote_remote_video -INFO TEST-INFO | test_pC_basicAudioVideo.html | time passed for media element pcRemote_remote_video INFO TEST-INFO | test_pC_basicAudioVideo.html | Run step: PC_REMOTE_WAIT_FOR_ICE_CONNECTED INFO TEST-INFO | test_pC_basicAudioVideo.html | iceConnectionState: connected INFO TEST-PASS | test_pC_basicAudioVideo.html | pc_remote: ICE is in connected state @@ -29,19 +26,17 @@ INFO TEST-PASS | test_pC_basicAudioVideo.html | Media flowing for pcRemote_local_audio INFO TEST-INFO | test_pC_basicAudioVideo.html | Analyzing element: pcRemote_local_video INFO TEST-PASS | test_pC_basicAudioVideo.html | Media flowing for pcRemote_local_video -INFO TEST-INFO | test_pC_basicAudioVideo.html | Analyzing element: pcRemote_remote_video -INFO TEST-PASS | test_pC_basicAudioVideo.html | Media flowing for pcRemote_remote_video INFO TEST-INFO | test_pC_basicAudioVideo.html | Run step: PC_LOCAL_CHECK_STATS INFO TEST-INFO | test_pC_basicAudioVideo.html | PeerConnectionWrapper (pcLocal): Got stats: XXXX INFO TEST-PASS | test_pC_basicAudioVideo.html | Spec and MapClass variant of RTCStatsReport enumeration agree INFO TEST-PASS | test_pC_basicAudioVideo.html | Have 2 inboundrtp stat(s) INFO TEST-PASS | test_pC_basicAudioVideo.html | Have 2 outboundrtp stat(s) INFO TEST-PASS | test_pC_basicAudioVideo.html | Have localcandidate stat(s) INFO TEST-PASS | test_pC_basicAudioVideo.html | Have remotecandidate stat(s) INFO TEST-INFO | test_pC_basicAudioVideo.html | Run step: PC_REMOTE_CHECK_STATS INFO TEST-INFO | test_pC_basicAudioVideo.html | PeerConnectionWrapper (pcRemote): Got stats: XXXX INFO TEST-PASS | test_pC_basicAudioVideo.html | Spec and MapClass variant of RTCStatsReport enumeration agree -INFO TEST-PASS | test_pC_basicAudioVideo.html | Have 2 inboundrtp stat(s) +ERROR TEST-UNEXPECTED-FAIL | test_pC_basicAudioVideo.html | Have 1 inboundrtp stat(s) - got 2, expected 1 INFO TEST-PASS | test_pC_basicAudioVideo.html | Have 2 outboundrtp stat(s) INFO TEST-PASS | test_pC_basicAudioVideo.html | Have localcandidate stat(s) INFO TEST-PASS | test_pC_basicAudioVideo.html | Have remotecandidate stat(s) The pcRemote_remote_video is missing in the bad log for some reason. Anyone have any idea why that might happen? test_peerConnection_basicAudioVideo.html happily validates all flows it finds, but does not detect missing ones apparently? This explains the lowered number from numTracks(this._pc.getRemoteStreams()) which my test uses as the "expected" inboundrtp stat(s) number. I naively expected this number to match the rtp stats returned (from the media pipelines internally). Is this a poor assumption? Not sure how to proceed. In the interest of landing my code-change, I'm tempted to weaken my test to ">=" for now (since stats always over-deliver here), and then file a new bug on this anomaly. Sound good?

I think this falls into the general area of the canplaythrough event being used for the media checks not being reliable (basically they fire even if now media ever gets exchanged). See bug 948249 as the over all ticket for tracking the changes to improve this weakness. I think it should be alright to change the test to ">=" for now and open a new ticket for restricting it to just "=" and link that ticket to 948249 and this ticket.

I'm in agreement with the proposed path forward in comment 37.

Weakened inboundrtp test for now and filed Bug 957145 reminder. Carrying forward r+ from jsmith. Try - https://tbpl.mozilla.org/?tree=Try&rev=b2570e11d47a

https://hg.mozilla.org/integration/mozilla-inbound/rev/f88ff4f85819 https://hg.mozilla.org/integration/mozilla-inbound/rev/9e34b5a089e6

https://hg.mozilla.org/mozilla-central/rev/f88ff4f85819 https://hg.mozilla.org/mozilla-central/rev/9e34b5a089e6