904622 - Add RTP stats to "about:webrtc" page

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

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

Once the getStats API (Bug 902003) is farther along, make an "about:webrtc-internals" page akin to chrome://webrtc-internals , using it.

Note that chrome://webrtc-internals is not currently built on the getStats API.

Comment 1

[Maire Reavy [:mreavy]]

Moving this up on the schedule (and to a higher priority).  Re-targeting for Firefox 27.

Comment 2

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

Attachment: WIP - Basic JSON dump of RTP stats on about:webrtc

A starting point.

Comment 3

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

(In reply to Jan-Ivar Bruaroey [:jib] from comment #2)
> Created attachment 8344307 [details] [diff] [review]
> WIP - Basic JSON dump of RTP stats on about:webrtc
> 
> A starting point.

Here you can see what I mean.

Comment 4

[Byron Campen [:bwc]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #3)
> (In reply to Jan-Ivar Bruaroey [:jib] from comment #2)
> > Created attachment 8344307 [details] [diff] [review]
> > WIP - Basic JSON dump of RTP stats on about:webrtc
> > 
> > A starting point.
> 
> Here you can see what I mean.

It seems natural to break these down by track id, in a similar manner to component id for ICE stuff? Ultimately, it would be nice to make it clear what ICE components are being used by what media stream tracks; we almost can reconstruct this mapping using what is declared in webidl, but not quite. We can go from track id to transport id using RTCRTPStreamStats, and then from transport id to component id (as an integer) using RTCIceComponentStats, but this does not give us the DOMString component id. I think we need to discuss this a little further.

Comment 5

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

Attachment: Nicer dump of RTP stats on about:webrtc

Comment 6

[Byron Campen [:bwc]]

Comment on attachment 8357911 [details] [diff] [review]
Nicer dump of RTP stats on about:webrtc

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

Seems about right overall. There are some things I'd like to see fixed though, but these are at the review level of scrutiny, so giving f+.

::: toolkit/content/aboutWebrtc.xhtml
@@ +143,5 @@
>    candTable.appendChild(buildCandTableHeader(local));
>    return candTable;
>  }
>  
> +function round(num) {

"round" by itself implies rounding to the nearest integer, where here we are rounding to the nearest hundredth.

@@ +159,5 @@
>  
>    stats.forEach(function(stat) {
>      if (!stat.componentId) {
> +      var statDiv = document.createElement('div');
> +      statDiv.appendChild(document.createElement('b'))

It appears that <b> is one of those things that is frowned upon nowadays
(see https://developer.mozilla.org/en-US/docs/Web/HTML/Element/b); <strong> seems to be recommended instead. But, I'm not doctrinaire about html (yet), so I don't care that much either way. Someone else might though.

@@ +163,5 @@
> +      statDiv.appendChild(document.createElement('b'))
> +             .appendChild(document.createTextNode(stat.id));
> +
> +      var statsString = " " + new Date(stat.timestamp).toTimeString();
> +      if (stat.type) {

Shouldn't we be doing the same kind of checking for all three of id, timestamp, and type? (Any exception thrown will cause the entire stats page to fail to build, so I'm inclined to do the checking, but that may be overly aggressive)

@@ +171,5 @@
> +        statsString += " Received: " + stat.packetsReceived + " packets";
> +        if (stat.bytesReceived !== undefined) {
> +          statsString += " (" + round(stat.bytesReceived/1024) + " Kb)";
> +        }
> +      } else if (stat.packetsSent !== undefined) {

Should this be an "else"? Aren't both going to be in the same stat (according to RTCStatsReport.webidl)?

@@ +176,5 @@
> +        statsString += " Sent: " + stat.packetsSent + " packets";
> +        if (stat.bytesSent !== undefined) {
> +          statsString += " (" + round(stat.bytesSent/1024) + " Kb)";
> +        }
> +      }

It looks like this will result in an id, timestamp, and type (but nothing else) being put on the page for new stats without a componentId. Is this what we want?

Comment 7

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

Attachment: Dump of RTP stats on about:webrtc (2)

Updated with feedback.

> > +      if (stat.type) {
> 
> Shouldn't we be doing the same kind of checking for all three of id,
> timestamp, and type? (Any exception thrown will cause the entire stats page
> to fail to build, so I'm inclined to do the checking, but that may be overly
> aggressive)

All RTCStats must have id, timestamp and type, and I prefer no checks (that way we learn sooner if something is missing) so I should remove that check.

> > +      } else if (stat.packetsSent !== undefined) {
> 
> Should this be an "else"? Aren't both going to be in the same stat
> (according to RTCStatsReport.webidl)?

It's either an RTCInboundRTPStreamStats or an RTCOutboundRTPStreamStats, never both.

> > +      }
> 
> It looks like this will result in an id, timestamp, and type (but nothing
> else) being put on the page for new stats without a componentId. Is this
> what we want?

I would argue we should update this page as we add stats, so I'm not sure it matters.

Comment 8

[Byron Campen [:bwc]]

Comment on attachment 8357962 [details] [diff] [review]
Dump of RTP stats on about:webrtc (2)

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

Looks good to me.

Comment 9

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

Attachment: More RTP stats on about:webrtc - SSRC, jitter

Comment 10

[Byron Campen [:bwc]]

Comment on attachment 8360636 [details] [diff] [review]
More RTP stats on about:webrtc - SSRC, jitter

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

Looks good, with nits. Up to you.

::: toolkit/content/aboutWebrtc.xhtml
@@ +164,5 @@
>               .appendChild(document.createTextNode(stat.id));
>  
>        var statsString = " " + new Date(stat.timestamp).toTimeString() +
>                          " " + stat.type;
> +      if (stat.ssrc !== undefined) {

I guess I'd be inclined to not do the check, since I think a "SSRC: undefined" would be better at calling attention to a problem than the lack of an SSRC: string.

@@ +172,5 @@
>          statsString += " Received: " + stat.packetsReceived + " packets";
>          if (stat.bytesReceived !== undefined) {
>            statsString += " (" + round00(stat.bytesReceived/1024) + " Kb)";
>          }
> +        if (stat.jitter !== undefined) {

Same thing here.

Comment 11

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

Attachment: More RTP stats on about:webrtc: SSRC, jitter (2) r=bwc

Good point. Carrying forward r+ from Byron.

Comment 12

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

Attachment: Dump of RTP stats on about:webrtc (3) r=bwc

Updated commit message. Carrying forward r+ from Byron.

Comment 13

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

Attachment: RTP stats on about:webrtc, merged. r=bwc

Merged. Carrying forward r+ from Byron.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/434780f5d023

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/434780f5d023

Comment 16

[Karthikeyan R]

Is this fix included in beta release? or any plan to do so?

Comment 17

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

This is in mozilla29, which means it hits Beta on March 18th according to https://wiki.mozilla.org/RapidRelease/Calendar#Future_branch_dates

You can try it today on Firefox Aurora (aka Alpha) www.mozilla.org/en-US/firefox/aurora

Comment 18

[Karthikeyan R]

Thanks for the info. I'm able to test the about:webrtc page using Aurora build.

However, the stats provided currently are not useful for me. I'm expecting more detailed information as provided in "chrome://webrtc-internals".

Or at least, the kbps for each stream and the frameHeight and frameWidth details for the video stream.

Comment 19

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

Attachment: specstats.html - stats test page

(In reply to Karthikeyan R from comment #18)
> I'm expecting more detailed information as provided in "chrome://webrtc-internals".

We've got more stats to add for sure https://bugzilla.mozilla.org/showdependencytree.cgi?id=964161&hide_resolved=1

> Or at least, the kbps for each stream and the frameHeight and frameWidth
> details for the video stream.

See the attached page which updates and calculates kbps continuously. About:webrtc doesn't auto-update yet, so hit refresh to update the info. Note that RTCP information like jitter and packetloss (Nightly only) may not show initially depending on how recently the connection started, as it takes a couple of seconds for RTCP packets to come in.

frameWidth and frameHeight are hardcoded to 640 x 480 at the moment. Working on that now (Bug 907352).

Comment 20

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

I've opened Bug 976669 to add kbps to about:webrtc page.