Re: stats api webidl implementers feedback

On 10/23/2013 08:14 AM, Jan-Ivar Bruaroey wrote:
> On 10/18/13 5:54 PM, Göran Eriksson AP wrote:
>> Hi Harald,
>>
>> Great to see the updated proposal for stats parameters uploaded on 
>> the W3C WebRTC Wiki, http://www.w3.org/2011/04/webrtc/wiki/Stats- 
>> this makes it much easier to discuss it before it ends up in the spec.
>
> Thanks for that.
>
> We recently added the stats skeleton in Nightly, so I have some early 
> feedback from that - mostly webidl differences (this is just a 
> skeleton, it doesn't return much yet).
>
> Here's what we have at the moment: 
> http://mxr.mozilla.org/mozilla-central/source/dom/webidl/RTCStatsReport.webidl 
> - In addition, we have some changes for IceCandidatePairs in 
> https://bug906990.bugzilla.mozilla.org/attachment.cgi?id=820548 that 
> haven't landed yet, but work.

Thanks a LOT!

Some comments inserted - at the actual IDL level, we need nits to be 
picked.... where I don't comment, it's mostly "oops, bad me, you're right".

Our implementation actually doesn't represent this stuff as IDL yet; 
you'll find the actual names we
use in the file "libjingle/source/talk/app/webrtc/statscollector.cc" in 
the libjingle distro.



>
> Mostly these are small differences. Here they are (I'll post comments 
> in a follow-up):
>
> enum RTCStatsType {
> "inbound-rtp",
> "outbound-rtp",
> + "cand-pair",
> + "local-candidate",
> + "remote-candidate"
> +};

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.

In addition to the above types, we (google) have "session", "track", 
"candidate", and "transport" + the oddly named "VideoBwe", which 
probably should be renamed something else.

>
> dictionary RTCStats {
> - DOMHiResTimeStamp timestamp;
> + DOMHighResTimeStamp timestamp;
> RTCStatsType type;
> DOMString id;
> };
>
> All SSRCs:
>
> dictionary RTCRTPStreamStats : RTCStats {
> DOMString ssrc;
> DOMString remoteId;
> // New stuff:
> - boolean isRemote // default false

Not sure we are OK with deleting the isRemote flag. I don't think you 
can tell the difference between
the two ends if you do that.

> DOMString mediaTrackId;
> DOMString transportId;
> DOMString codecId;
> };
>
> Note the convention that names ending in “Id” are identifiers of other 
> stats objects.
> Directed SSRCs:
>
> dictionary RTCInboundRTPStreamStats : RTCRTPStreamStats {
> unsigned long packetsReceived;
> unsigned long bytesReceived;
> // In addition to current spec:
> float jitter;
> };
>
> @@ -31,63 +37,83 @@
> unsigned long bytesSent;
> };
>
> Representing MediaStreamTracks:
>
> dictionary RTCMediaStreamTrackStats : RTCStats {
> DOMString trackIdentifier; // track.id property
> boolean remoteSource;
> sequence<DOMString> ssrcIds;
> // Only makes sense for audio
> - unsigned int audioLevel;
> + unsigned long audioLevel;
> // Only makes sense for video
> - unsigned int frameWidth;
> - unsigned int frameHeight;
> - unsigned float framesPerSecond; // The nominal FPS value
> - unsigned int framesSent;
> - unsigned int framesReceived; // Only makes sense for remoteSource=true
> - unsigned int framesDecoded;
> - int firs;
> + unsigned long frameWidth;
> + unsigned long frameHeight;
> + unsigned long framesPerSecond; // The nominal FPS value

Using long in general is a good idea. I think framesPerSecond needs to 
be a double, due to the pesky NTSC rate of 29.997 frames per second. 
(the reason why video clock is always the otherwise ridiculous 90000 
ticks per second - it's the smallest integer that makes 29.997 turn into 
an integer.)

> + unsigned long framesSent;
> + unsigned long framesReceived; // Only for remoteSource=true
> + unsigned long framesDecoded;
> + long first;
> -}
> +};
>
> -dictionary MediaStream : RTCStats {
> +dictionary RTCMediaStreamStats : RTCStats {
> DOMString streamIdentifier; // stream.id property
> sequence<DOMString> trackIds; // Note: This is the id of the stats 
> object, not the track.id
> -}
> +};
>
> Representing transports:
>
> -dictionary RTCTransport: RTCStats {
> +dictionary RTCTransportStats: RTCStats {
> unsigned long bytesSent;
> unsigned long bytesReceived;
> -}
> +};
>
> dictionary RTCIceComponentStats : RTCStats {
> DOMString transportId;
> - int component;
> + long component;
> unsigned long bytesSent;
> unsigned long bytesReceived;
> - bool activeConnection;
> + boolean activeConnection;
> -}
> +};
>
> +enum RTCStatsIceCandidatePairState {
> + "frozen",
> + "waiting",
> + "in_progress",
> + "failed",
> + "succeeded",
> + "cancelled"
> +};
>
> dictionary RTCIceCandidatePairStats : RTCStats {
> - DOMString componentId;
> + DOMString candidatePairId;

Don't understand this change. componentId is needed to group all the 
candidate pairs that are
linked to the same component. The candidate pair already has an ID - the 
"id" field.

> DOMString localCandidateId;
> DOMString remoteCandidateId;
> + RTCStatsIceCandidatePairState state;
> - boolean writable; // Has gotten ACK to an ICE request
> - boolean readable; // Has gotten a valid incoming ICE request

Where will you keep the information about whether a candidate pair is 
working or not, if you
don't keep it here?

> + unsigned long long priority;
> + boolean nominated; // Internal?
> + boolean selected; // Internal?
> -}
> +};
>
> +enum RTCStatsIceCandidateType {
> + "host",
> + "server-reflexive",
> + "peer-reflexive",
> + "relayed"
> +};
>
> -dictionary RTCIceCandidate : RTCStats {
> +dictionary RTCIceCandidateStats : RTCStats {
> + DOMString candidateId;
> DOMString ipAddress;
> - int portNumber;
> + long portNumber;
> - DOMString transport; // "tcp" or "udp"

I think we need the info on whether it's tcp, udp or tls-over-tcp. An 
enum might be right.

> // More ICE-related info goes here.
> + RTCStatsIceCandidateType candidateType;
> -}
> +};
>
> -dictionary RTCCodec : RTCStats {
> +dictionary RTCCodecStats : RTCStats {
> - unsigned int payloadType; // As used in RTP encoding.
> + unsigned long payloadType; // As used in RTP encoding.
> DOMString codec; // video/vp8 or equivalent
> - unsigned int clockRate;
> - unsigned int channels; // 2 for stereo, missing for most other cases.
> + unsigned long clockRate;
> + unsigned long channels; // 2=stereo, missing for most other cases.
> DOMString parameters; // From SDP description line
> -}
> +};
>
> +callback RTCStatsReportCallback = void (object value, DOMString key, 
> RTCStatsReport obj);
>
> +// [MapClass(DOMString, object)] // TODO: Use once it's available 
> (Bug 928114)
> interface RTCStatsReport {
> - getter RTCStats (DOMString id);
> + void forEach(RTCStatsReportCallback callbackFn, optional any thisArg);
> + object get(DOMString key);
> + boolean has(DOMString key);
> };
>
> .: Jan-Ivar :.
>

Received on Wednesday, 23 October 2013 06:35:32 UTC