Re: stats api webidl implementers feedback

On 10/23/2013 05:28 PM, Jan-Ivar Bruaroey wrote:
> 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?

Dom pointed me to some advice to use double unless you really have to 
use float some time ago - I forget from where.

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

I think it's an RTP component - in Chrome, they're named "audio", 
"video", "audio-rtcp" and "video-rtcp" if I remember rightly. If BUNDLE 
and RTCP-multiplexing are both on, we only need one, but in many cases, 
we will need more.

>
>>
>>> 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:59:31 UTC