Our implementation of RTCIceCandidatePairStats is quite a bit behind the spec: https://w3c.github.io/webrtc-stats/#candidatepair-dict* And we received requests to at least include bytes send/received in our stats. It would be really helpful if the ICE candidate table on about:webrtc would also include bytes send/received, because then it would become obvious if the remote side has choosen a different candidate pair then local.

Michael this is the ICE candidates stats bug we talked about today. Would be great if you could take a look.

Comment on attachment 8875351 [details] Bug 1339906 - pt 1 - Add bytesSent and bytesReceived to RTCIceCandidatePairStats. https://reviewboard.mozilla.org/r/146764/#review151434

Comment on attachment 8875353 [details] Bug 1339906 - pt 3 - change componentId to transportId to match RTCIceCandidatePairStats spec. https://reviewboard.mozilla.org/r/146768/#review151436

Comment on attachment 8875355 [details] Bug 1339906 - pt 4 - add last sent and received timestamps to RTCIceCandidatePairStats. https://reviewboard.mozilla.org/r/146772/#review151440

Comment on attachment 8875352 [details] Bug 1339906 - pt 2 - add bytes sent and received to about:webrtc page. https://reviewboard.mozilla.org/r/146766/#review151442 Not sure I'm supposed to be on this review? I don't think anything here requires DOM peer review. Feel free to r? me again if I missed something though.

Comment on attachment 8875354 [details] Bug 1339906 - pt 5 - add writable field to webidl for RTCIceCandidatePairStats and implement readable and writeable fields. https://reviewboard.mozilla.org/r/146770/#review151446 Is there a followup bug to implement this, since it's still a field in the spec?

Comment on attachment 8875354 [details] Bug 1339906 - pt 5 - add writable field to webidl for RTCIceCandidatePairStats and implement readable and writeable fields. https://reviewboard.mozilla.org/r/146770/#review151446 Yes! I created it today: Bug 1371391

Comment on attachment 8875352 [details] Bug 1339906 - pt 2 - add bytes sent and received to about:webrtc page. https://reviewboard.mozilla.org/r/146766/#review151442 Nope! Sorry about that!

Comment on attachment 8875352 [details] Bug 1339906 - pt 2 - add bytes sent and received to about:webrtc page. https://reviewboard.mozilla.org/r/146766/#review151516

Comment on attachment 8875353 [details] Bug 1339906 - pt 3 - change componentId to transportId to match RTCIceCandidatePairStats spec. https://reviewboard.mozilla.org/r/146768/#review151520

Comment on attachment 8875354 [details] Bug 1339906 - pt 5 - add writable field to webidl for RTCIceCandidatePairStats and implement readable and writeable fields. https://reviewboard.mozilla.org/r/146770/#review151554 Besides leaving |readable| in the WebIDL I would actually ask you to add |writeable| which should be pretty easy to populate from the nr_ice_component.can_send value. That does not have to happen here. Maybe in a separate follow-up ticket. ::: dom/webidl/RTCStatsReport.webidl (Diff revision 1) > DOMString transportId; > DOMString localCandidateId; > DOMString remoteCandidateId; > RTCStatsIceCandidatePairState state; > unsigned long long priority; > - boolean readable; I don't have a problem taking out |readable| form the stats if we never populate it. But I think removing something from our webidl files which is still in the spec is not the right thing to do. I would rather leave it in here and not implement it.

Since there's a followup at bug 1371391, I'm in agreement with Comment 19. If you're going to fill in the code in the followup, you can drop patch 4 and leave the field as is for right now since it's already there, if broken.

Comment on attachment 8875351 [details] Bug 1339906 - pt 1 - Add bytesSent and bytesReceived to RTCIceCandidatePairStats. https://reviewboard.mozilla.org/r/146764/#review151560 ::: media/mtransport/third_party/nICEr/src/ice/ice_peer_ctx.c:828 (Diff revision 1) > if(!cand) > ABORT(R_REJECTED); > > + // accumulate the received bytes for the active candidate pair > + if (peer_comp->active) { > + peer_comp->active->bytes_recvd += len; I find it a little strange that we are attributing the received bytes here to a pair which appears to be not used at all when processing the incoming packet. Makes me wonder if we could attribute it to the wrong pair. But since it is "only" stats, its not going to affect the connection I guess.

Comment on attachment 8875355 [details] Bug 1339906 - pt 4 - add last sent and received timestamps to RTCIceCandidatePairStats. https://reviewboard.mozilla.org/r/146772/#review151578

Comment on attachment 8875356 [details] Bug 1339906 - pt 6 - add tests for RTCIceCandidatePairStats. https://reviewboard.mozilla.org/r/146774/#review151580 ::: dom/media/tests/mochitest/test_peerConnection_stats.html:50 (Diff revision 1) > + expected: ["id", "timestamp", "type", > + "transportId", "localCandidateId", "remoteCandidateId", "state", > + "priority", "nominated", "bytesSent", "bytesReceived", > + "lastPacketSentTimestamp", "lastPacketReceivedTimestamp",], > + optional: ["selected",], > + unimplemented: ["totalRoundTripTime", "currentRoundTripTime", Aren't you missing readable and writable in this list?

Comment on attachment 8875354 [details] Bug 1339906 - pt 5 - add writable field to webidl for RTCIceCandidatePairStats and implement readable and writeable fields. https://reviewboard.mozilla.org/r/146770/#review151554 The spec for writeable is "Has gotten ACK to an ICE request." I don't think this exactly matches up with nr_ice_component.can_send since we set can_send to true when setting up consent. At that point we don't know if we've had an ACE to an ICE request, correct? > I don't have a problem taking out |readable| form the stats if we never populate it. > > But I think removing something from our webidl files which is still in the spec is not the right thing to do. I would rather leave it in here and not implement it. Sorry, I misunderstood an earlier conversation with Nico. I thought he said take them out of the webidl file if we don't implement. What he really said was don't let them show up in the js if they aren't implemented. Based on this new understanding, I'll need to add the other non-implemented fields to the webidl as well: totalRoundTripTime, currentRoundTripTime, availableOutgoingBitrate, availableIncomingBitrate, requestsReceived, requestsSent, responsesReceived, responsesSent, retransmissionsReceived, retransmissionsSent, and consentRequestsSent.

Comment on attachment 8875356 [details] Bug 1339906 - pt 6 - add tests for RTCIceCandidatePairStats. https://reviewboard.mozilla.org/r/146774/#review151580 > Aren't you missing readable and writable in this list? Yes! Good catch - thank you.

Comment on attachment 8875351 [details] Bug 1339906 - pt 1 - Add bytesSent and bytesReceived to RTCIceCandidatePairStats. https://reviewboard.mozilla.org/r/146764/#review151560 > I find it a little strange that we are attributing the received bytes here to a pair which appears to be not used at all when processing the incoming packet. Makes me wonder if we could attribute it to the wrong pair. But since it is "only" stats, its not going to affect the connection I guess. The problem is on the receiving socket side's nr_ice_component, no active pair ever appears (based on my printf logging), so the only place to look for the real candidate pair is in the peer component's active pair.

My position on unimplemented stats fields being present in the WebIDL are that they are a liability, and dead code. By unimplemented I mean that they are never populated, and most importantly do not appear in stats reports. "readable", from what I understand, is implemented by that definition in that it appears in the JS stats report object, although its value is never properly updated. So I would argue that it should stay and its behavior should be fixed (in another bug). My general thought is that until the spec is set in stone, adding unimplemented fields to the WebIDL is of zero to negative value. If nothing else adding them makes it hard to determine what is implemented. I would recommend the removal of unimplemented fields, and the use of WebIDL as a source of implementation truth.

Based on code here: https://dxr.mozilla.org/mozilla-central/rev/e61060be36424240058f8bef4c5597f401bc8b7e/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp#3586 I don't believe that readable could ever have made an appearance in the JS stats report object. So based on that and Nico's unimplemented stats being a liability, I think it qualifies for the deletion I made earlier.

Comment on attachment 8875354 [details] Bug 1339906 - pt 5 - add writable field to webidl for RTCIceCandidatePairStats and implement readable and writeable fields. https://reviewboard.mozilla.org/r/146770/#review153532

Pushed by mfroman@nostrum.com: https://hg.mozilla.org/integration/autoland/rev/8e82aceb8611 pt 1 - Add bytesSent and bytesReceived to RTCIceCandidatePairStats. r=drno,qdot https://hg.mozilla.org/integration/autoland/rev/12e9594627f4 pt 2 - add bytes sent and received to about:webrtc page. r=drno https://hg.mozilla.org/integration/autoland/rev/3353c46f3f5f pt 3 - change componentId to transportId to match RTCIceCandidatePairStats spec. r=drno,qdot https://hg.mozilla.org/integration/autoland/rev/f4fc4777893c pt 4 - add last sent and received timestamps to RTCIceCandidatePairStats. r=drno,qdot https://hg.mozilla.org/integration/autoland/rev/4b6403bab1e4 pt 5 - add writable field to webidl for RTCIceCandidatePairStats and implement readable and writeable fields. r=drno,qdot https://hg.mozilla.org/integration/autoland/rev/2930e8e259a9 pt 6 - add tests for RTCIceCandidatePairStats. r=drno

This broke a tier-2 android test: https://treeherder.mozilla.org/logviewer.html#?job_id=107094824&repo=autoland Could you take a look at bug 1373015?

I'm guessing that due to mozreview weirdness I got readded to the patch 2 review, and also somehow ni?'d. I can't find where I was ni?'d, so if I'm missing something, please re-ni? me.

Oh, oops, tab reload problem. Re ni'ing mjf. Ignore me. :)

https://hg.mozilla.org/mozilla-central/rev/8e82aceb8611 https://hg.mozilla.org/mozilla-central/rev/12e9594627f4 https://hg.mozilla.org/mozilla-central/rev/3353c46f3f5f https://hg.mozilla.org/mozilla-central/rev/f4fc4777893c https://hg.mozilla.org/mozilla-central/rev/4b6403bab1e4 https://hg.mozilla.org/mozilla-central/rev/2930e8e259a9

(In reply to Wes Kocher (:KWierso) from comment #37) > This broke a tier-2 android test: > https://treeherder.mozilla.org/logviewer.html#?job_id=107094824&repo=autoland > > Could you take a look at bug 1373015? I'll take a look - this is likely a strange timing-related issue since I mostly added a couple fields. Off to get an android build in my Ubuntu VM! Thanks for the heads up.

We've released FF54. Mark 54 won't fix.