Re: stats api webidl implementers feedback

On 10/23/13 2:35 AM, Harald Alvestrand wrote:
> 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!

Thank you for leading on defining this stuff!

> 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.

Thanks for the heads-up. I'll make the change right away.

> 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.

Yes, see my follow-up comment, which you beat me to. Will add.

>
>> 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.)

OK I figured. Wait, float or double?

>
>> + 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.

OK, thanks. Probably a misunderstanding. This is an ICE "component"?

>
>> 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?

Not sure, will check with the person who did this part. :-)

>
>> + 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.

Makes sense.

.: Jan-Ivar :.

>
>> // 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 15:29:21 UTC