mangle getStats #124
mangle getStats #124
Conversation
| standardStats.priority = 'FIXME'; // unsigned long long | ||
| standardStats.nominated = 'FIXME'; // boolean | ||
| // FIXME: missing from spec | ||
| standardStats.selected = |
There was a problem hiding this comment.
"Selected" is tricky. "Nominated" is well defined in the spec.
There is a selectedCandidatePairId as part of the RTCTransportStats.
There was a problem hiding this comment.
selected seems well defined in https://tools.ietf.org/html/rfc5245#section-2.6 too?
I am assuming that the selectedCandidatePairId points to the candidatePair with selected === true. But that means having a selected attribute here does not add much value.
|
the more I am thinking about this (and writing code) the trickier this seems to get. Biggest issue is that there is no 1:1 mapping between the current chrome stats and the spec stats so some reports need to be split into two? |
|
I think of the whole snowball as one "report". Do you mean split up some of the dictionaries? |
|
@jan-ivar yes. The chrome type ssrc seems to contain both sender and receiver statistics which (IIUC) in the new model are in separate things. E.g. you have an inboundrtp report which has a remoteId that gives you the thing where you can query packet loss. |
|
Yes, I try to explain that squirrelly case here if it helps. Though, when you "query" it's really nothing more than an object property lookup between two dictionaries in the same object. Should be possible to reconstitute. |
|
just noticed that I never pushed https://github.com/fippo/adapter/commit/888d1cc7b0a8869783d8be84c2968afe0bf7b43f |
|
@alvestrand did you ever see https://codereview.webrtc.org/1307633007/ ? Not sure if it really helps since I might end up having to shim this by iterating over all local and remote tracks anyway until the potential change lands in all chrome versions |
generate additional track and rtp reports from ssrc report
|
@alvestrand I think this is ready for a detailed review. The output comes pretty close to what I think it should be. It seems to me that adding a RTCCodec dictionary to the native lib should be relatively straightforward. And it would avoid sdp 'parsing', I can take a look at that. |
|
(when you say you're done and you notice you forgot to split up local and remote measurements...) |
|
Fixed some more bugs and (experimentally) removed any goog* and ssrc reports. Also removed any attributes with goog* prefix (which discards some things I am interested in so this is too aggressive). This looks pretty sane actually. |
|
Ping |
|
build failure will be fixed by #161 -- but don't merge this before @alvestrand agrees this is what the stats should look like ,-) |
|
ping @alvestrand |
|
Is this still something we want to do? |
| }); | ||
| }; | ||
|
|
||
| driver.manage().timeouts().setScriptTimeout(3000); |
There was a problem hiding this comment.
This seems like a very high level test. I'd like to see some tests just on the fixChromeStats function, where you take a stats object (cut-down version of something that Chrome emits) and check that it turns into a standards-shaped stats object. This also serves as good documentation of how the two APIs look.
|
Yes, we want to do it. But it requires more review - I get confused with what this thing actually transforms. |
|
i'd rather wait for the new stuff to land :-) |
Not sure how far i'll get before giving up or if the end result will still be merge-able but I found a number of spec and implementation bugs within minutes so this seems worthwhile.
note that the approach might break if applications expect to find a report of type googCandidatePair -- hopefully applications where the stats get mangled don't do this.
@alvestrand