902003 - getStats method of RTCPeerConnection

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[zucca]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/28.0.1500.95 Safari/537.36

Steps to reproduce:

i'd like use getStats method for stream statistic data.

pc = new mozRTCPeerConnection();
pc.getStats (on peer attached ..)


Actual results:

pc.getStats is undefined


Expected results:

object of stream stats (as a Chrome)

Comment 1

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

Taking this one as I'm already working on it. Thanks for filing it!

> Expected results:
>
> object of stream stats (as a Chrome)

To clarify: The stream stats in Chrome right now are different from the spec, though I believe they are working on changing that. This bug is about building to the spec.

The spec is still very much in flux, so please check it out and make sure it fits your needs, as it is not too late to influence it. We welcome all feedback:

http://dev.w3.org/2011/webrtc/editor/webrtc.html#statistics-model

Comment 2

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

Attachment: WIP - working API with limited mock data

Comment 3

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

Attachment: mozstats.html - test page

Comment 4

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

Attachment: mozstats.html - test page (unblock mixed content to run)

Turned off audio on test page.

Comment 5

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

I'd like to close this bug once we have a getStats skeleton with a few baseline stats included like packetsSent/Received and bytesSent/Received. - I've opened Bug 908695 to address addition of more stats.

Comment 6

[zucca]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #5)
> I'd like to close this bug once we have a getStats skeleton with a few
> baseline stats included like packetsSent/Received and bytesSent/Received. -
> I've opened Bug 908695 to address addition of more stats.


ok, though i can't use your example, i get getUserMedia is not defined, very strange.

Comment 7

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

Attachment: WIP - working API with limited mock data - unbitrotted

(In reply to zucca from comment #6)
> ok, though i can't use your example, i get getUserMedia is not defined, very
> strange.

Firefox is blocking mixed content on the page. Click on the shield symbol at the head of the url in the url-bar and select "Disable Protection on this Page" (I used Google's adapter.js in the test to make it easy to attach here as a single file, is why).

I've also unbitrotted the patch now which you'll need to apply to your own build of Nightly to see anything, as no code here has landed yet.

If you get that working, note that the data you'll see returned is totally made up and probably not representative of the data you'll see when this is done. This is work in progress with still just the bare-bones API skeleton in place.

I had to push this off to work on a FF26 bug, but plan to come back for FF27.

Comment 8

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

Attachment: WIP - working API with limited mock data (3)

Unbitrotted again + added webidl based on struct NrIceCandidate from conversation with Byron.

Comment 9

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

Attachment: WIP - working API with limited mock data (4)

Rebased to work on top of Bug 917328.

- Added RTCStatsReportInternal to hold internal representation of data.
- C++ code now uses RTCStats webidl dictionaries.

Comment 10

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

Attachment: mozstats.html - test page (unblock mixed content to run) (2)

Works again.

- Made gUM constraints we don't support optional rather than mandatory
  (now that we finally fail on mandatory gUM constraints).

Again, don't forget to unblock mixed content.

Comment 11

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

Attachment: WIP - working API with limited mock data (5)

Last patch accidentally commented out deprecated readonly attributes localStreams and remoteStreams which mochitest framework relies on. Now fixed.

Comment 12

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

Attachment: WIP - working API with limited mock data (6)

Added RTCStatsReport to dom/tests/mochitest/general/test_interfaces.html to silence orange.

Comment 13

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

Attachment: WIP - working API with limited mock data (7)

Updated to work with Bug 817194.

Comment 14

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

Nice and green try: https://tbpl.mozilla.org/?tree=Try&rev=464852fd0b51

Comment 15

[Byron Campen [:bwc]]

Attachment: (WIP) Move mock data population to the same thread the real data will be populated in.

Comment 16

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

Attachment: getStats API skeleton

Removed mock data and refactored to support Bug 906990.

Comment 17

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

Attachment: Dispatch getStats to STS thread and back.

Refactored without mock data.

Comment 18

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

Here's a green try with Patch 5, 6 and 7 from Bug 906990 on-top of these patches: https://tbpl.mozilla.org/?tree=Try&rev=4af991055ac6

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 817001 [details] [diff] [review]
getStats API skeleton

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

Please file a bug for building tests for this

::: dom/media/PeerConnection.js
@@ +18,2 @@
>  
>  const PC_CID = Components.ID("{9878b414-afaa-4176-a887-1e02b3b047c2}");

Do we need to rev the PC_CID when we rev the API?  I don't think so, but just checking.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +1126,5 @@
> +NS_IMETHODIMP
> +PeerConnectionImpl::GetStats(mozilla::dom::MediaStreamTrack *aSelector) {
> +  PC_AUTO_ENTER_API_CALL(true);
> +
> +  // TODO: Insert dispatch to STS here.

Is there a bug for this?

Comment 20

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

Attachment: getStats API skeleton (2) r=jesup

Updated with feedback. Carrying forward r+ from jesup. Looking for DOM reviewer.

- Moved dangling RTCIceCandidatePairStats-reference over to second patch,
  as I discovered this patch didn't compile on its own.

(In reply to Randell Jesup [:jesup] from comment #19)
> Please file a bug for building tests for this

Will do, or maybe as a third patch here.

> >  const PC_CID = Components.ID("{9878b414-afaa-4176-a887-1e02b3b047c2}");
> 
> Do we need to rev the PC_CID when we rev the API?  I don't think so, but
> just checking.

True for XPIDL so probably true here too. Good catch, thanks!

> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
> @@ +1126,5 @@
> > +NS_IMETHODIMP
> > +PeerConnectionImpl::GetStats(mozilla::dom::MediaStreamTrack *aSelector) {
> > +  PC_AUTO_ENTER_API_CALL(true);
> > +
> > +  // TODO: Insert dispatch to STS here.
> 
> Is there a bug for this?

It's in the second patch.

Comment 21

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

Links again for webidl reference:
http://dev.w3.org/2011/webrtc/editor/webrtc.html#statistics-model
http://lists.w3.org/Archives/Public/public-webrtc/2013May/0053.html

Comment 22

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

(In reply to Jan-Ivar Bruaroey [:jib] from comment #20)
> - Moved dangling RTCIceCandidatePairStats-reference over to second patch,
>   as I discovered this patch didn't compile on its own.

I misspoke, I meant I moved it (or will move it) to the ice-pair patch (upcoming Part 7) in Bug 906990.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 817827 [details] [diff] [review]
getStats API skeleton (2) r=jesup

I'd like to see the ctor for RTCStatsReport to be removed.
It is not useable for web pages, and the spec doesn't have it.
RTCStatsReport interface itself is a big hacky, but since we don't have
support for MapClass, I don't see really other options for now.
MapClass approach is not what the spec has, but the spec isn't good either.

Comment 24

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 817002 [details] [diff] [review]
Dispatch getStats to STS thread and back.

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

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +1129,5 @@
>  PeerConnectionImpl::GetStats(mozilla::dom::MediaStreamTrack *aSelector) {
>    PC_AUTO_ENTER_API_CALL(true);
>  
> +#ifdef MOZILLA_INTERNAL_API
> +  uint32_t track = aSelector? aSelector->GetTrackID() : 0;

space before ?

Comment 25

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

Attachment: getStats API skeleton (3) r=jesup

Updated with comments.

- Removed RTCStatsReport constructor (using _create() instead)

Comment 26

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

Attachment: Dispatch getStats to STS thread and back (2) r=jesup

Updated with nits. Carrying forward r+ from jesup.

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 818090 [details] [diff] [review]
getStats API skeleton (3) r=jesup


>+enum RTCStatsIceCandidateType {
>+  "host",
>+  "server-reflexive",
>+  "peer-reflexive",
>+  "relayed"
>+};
>+
>+dictionary RTCIceCandidateStats : RTCStats {
>+  DOMString candidateId;
>+  DOMString ipAddress;
>+  long portNumber;
>+  RTCStatsIceCandidateType candidateType;
>+ }
Spec nor the proposal on the mailing list has this kind of dictionary,
but I was told this is needed and will be in the spec, in some form.



>+dictionary RTCStatsReportInternal {
>+  sequence<RTCRTPStreamStats>         RTPStream;
>+  sequence<RTCInboundRTPStreamStats>  InboundRTPStreamStats;
>+  sequence<RTCOutboundRTPStreamStats> OutboundRTPStreamStats;
>+  sequence<RTCMediaStreamTrackStats>  MediaStreamTrackStats;
>+  sequence<RTCMediaStreamStats>       RTCMediaStreamStats;
Should this be called MediaStreamStats for consistency

>+  sequence<RTCTransportStats>         TransportStats;
>+  sequence<RTCIceComponentStats>      IceComponentStats;
>+  sequence<RTCIceCandidateStats>      IceCandidateStats;
>+  sequence<RTCCodecStats>             CodecStats;
>+};
Odd to see uppercase property names. lowercase first letter would be the usual way.
Could you perhaps change that.


> #ifdef MOZILLA_INTERNAL_API
>   void virtualDestroyNSSReference() MOZ_FINAL;
>+  nsresult GetNow(DOMHighResTimeStamp *result);
DOMHighResTimeStamp* aResult same also in the method definition
And maybe rename GetNow to GetTimeSinceEpoc.



Could you make sure where have a bug open to implement MapClass for WebIDL, since the current API
looks a bit odd.

Comment 28

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

Attachment: getStats API skeleton (4) r=jesup, smaug

Updated with nits. Carrying forward r+ from smaug.

(In reply to Olli Pettay [:smaug] from comment #27)
> >+enum RTCStatsIceCandidateType {
> >+  "host",
> >+  "server-reflexive",
> >+  "peer-reflexive",
> >+  "relayed"
> >+};
> >+
> >+dictionary RTCIceCandidateStats : RTCStats {
> >+  DOMString candidateId;
> >+  DOMString ipAddress;
> >+  long portNumber;
> >+  RTCStatsIceCandidateType candidateType;
> >+ }
> Spec nor the proposal on the mailing list has this kind of dictionary,
> but I was told this is needed and will be in the spec, in some form.

It's in the mailing list message in Comment 21 FWIW as:

  dictionary RTCIceCandidate : RTCStats {
  DOMString ipAddress;
  int portNumber;
  // More ICE-related info goes here.
  }

But we renamed it for consistency with the others, and added some fields to it for debugging.

> >+dictionary RTCStatsReportInternal {
> >+  sequence<RTCRTPStreamStats>         RTPStream;
> >+  sequence<RTCInboundRTPStreamStats>  InboundRTPStreamStats;
> >+  sequence<RTCOutboundRTPStreamStats> OutboundRTPStreamStats;
> >+  sequence<RTCMediaStreamTrackStats>  MediaStreamTrackStats;
> >+  sequence<RTCMediaStreamStats>       RTCMediaStreamStats;
> Should this be called MediaStreamStats for consistency

Yes, thanks!

> >+  sequence<RTCTransportStats>         TransportStats;
> >+  sequence<RTCIceComponentStats>      IceComponentStats;
> >+  sequence<RTCIceCandidateStats>      IceCandidateStats;
> >+  sequence<RTCCodecStats>             CodecStats;
> >+};
> Odd to see uppercase property names. lowercase first letter would be the
> usual way. Could you perhaps change that.

Good point. Will do.

> > #ifdef MOZILLA_INTERNAL_API
> >   void virtualDestroyNSSReference() MOZ_FINAL;
> >+  nsresult GetNow(DOMHighResTimeStamp *result);
> DOMHighResTimeStamp* aResult same also in the method definition
> And maybe rename GetNow to GetTimeSinceEpoc.

Done.

> Could you make sure where have a bug open to implement MapClass for WebIDL,
> since the current API looks a bit odd.

Filed as Bug 928114.

Comment 29

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

Attachment: Dispatch getStats to STS thread and back (3) r=jesup

Rebased. Carrying forward r+ from jesup.

Comment 30

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

Please leave open as I plan to add some mochitests, thanks.

Comment 31

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5372aea57cdb
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a6d4fa91d5b

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5372aea57cdb
https://hg.mozilla.org/mozilla-central/rev/2a6d4fa91d5b

Comment 33

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

Attachment: Stats webidl tuning, rename stats enums to not contain dashes etc.stats_webidl_tuning.patch

Updated from feedback on list http://lists.w3.org/Archives/Public/public-webrtc/2013Oct/0090.html , mainly:
> We got advice last week to use lowercasewordsthatruntogether for string 
> constants, we should probably do that for all of these.
> 
> "cand-pair" -> "candidatepair" - we should be consistent in what we abbreviate or not.
> We called it "iceCandidate" in our code, but since we're being told 
> camelcase for string constants should be deprecated, "icecandidate" may be better.

and a few other nits.

Comment 34

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 821053 [details] [diff] [review]
Stats webidl tuning, rename stats enums to not contain dashes etc.stats_webidl_tuning.patch

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

::: dom/webidl/RTCStatsReport.webidl
@@ +44,5 @@
>    sequence<DOMString> ssrcIds;
>    unsigned long audioLevel;       // Only for audio, the rest are only for video
>    unsigned long frameWidth;
>    unsigned long frameHeight;
> +  float framesPerSecond; // The nominal FPS value

space out // to match the others

Comment 35

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

Attachment: Stats webidl tuning, enums wo/dashes, float to double, long long bytes etc.

Updated nit, but included more changes from just-updated http://www.w3.org/2011/04/webrtc/wiki/Stats - so asking for review again.

Comment 36

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

If someone could update commit message and land that's be great. thanks.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd9d47feac87

Comment 38

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/bd9d47feac87

Comment 39

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

(In reply to Jan-Ivar Bruaroey [:jib] from comment #5)
> I'd like to close this bug once we have a getStats skeleton with a few
> baseline stats included like packetsSent/Received and bytesSent/Received. -
> I've opened Bug 908695 to address addition of more stats.

Slight change - for bookkeeping, I think it makes sense to close this now and put upcoming patch for packets/bytes-sent/received on Bug 908695 instead, to reflect better what is and isn't in 27. I have Bug 930621 for unittests.