-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RTCStatsReport - add stats-type features and stats #18910
RTCStatsReport - add stats-type features and stats #18910
Conversation
@queengooborg I have updated the FF results using searchfox and bugzill - it turns out the IDL is quite accurate at working out what is supported. Also added a few values from various Chrome notes around the place. We can guess for some of them using data from here: https://webrtc-stats.callstats.io/verify/ i.e. use the tested versions as introduced versions. For Chrome at least that is probably reasonable, because most of this came in on the reported version M79/8 as far as I can tell. Advicde appreciated. |
Actually, maybe I can reasonably bisect using a combination of https://wpt.live/webrtc-stats/supported-stats.https.html and https://webrtc-stats.callstats.io/verify/ |
6dc54d0
to
6777c45
Compare
api/RTCPeerConnection.json
Outdated
"inbound_rtp_frameHeight": { | ||
"__compat": { | ||
"description": "<code>frameHeight</code> in 'inbound-rtp' stats", | ||
"spec_url": "https://w3c.github.io/webrtc-stats/#dom-rtcinboundrtpstreamstats-frameheight", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@queengooborg This is the start of what is going to replace RTCInbountRtpStreamStats
. My current plan is to do all the values like this - so all inbounds will be inbound_rtp_Xxxx
and there will be keys for outbound_rtp_Xxxx
and so on that will be peers.
Do you think this is OK? Or should we have deeper grouping - e.g. a feature inbound_rtp
and fields below it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, we can always make changes to the structure in the future if we need to!
@queengooborg Not as much success as I'd hoped:
I think we are going to have little choice but to mark things as either "true" or better ">=Version" for the unknown cases and add an exception. That is not great, but it is better than what we have by a mile, and will get progressively better as time goes on. Thoughts? If not, I need direction. |
I'm going to mark this as blocked on #19014. We're going to need to spend a lot of time researching this to determine the proper version numbers, and it doesn't align with the 2020+ data goal so I'm not sure we can justify the research time right now (even though it's a part of our dictionary removals goal). |
@queengooborg PS, on further thought, all this data should be in RTCStatsReport - this is the object used to access the data - which is returned by getStats - make sense? |
Yes, we should put all the data into While we're at it, maybe we should structure the data into a tree, with subfeatures for each stats type and subfeatures under those for each property? Example: {
"api": {
"RTCStatsReport": {
"__compat": { ... },
"candidate-pair_type": {
"__compat": {
"description": "<code>candidate-pair</code> stats type",
...
},
"availableIncomingBitrate": {
"__compat": { ... }
}
}
}
}
} WDYT? |
Yes :-). I didn't think it mattered originally, but we may well want to link different parts of these subfeatures into different docs. Easier if we split it this way. |
a43b09d
to
d83fa5a
Compare
@queengooborg Done - now all in RTCStatsReport and renamed. I dropped the old commits from the history. |
Oh no need to worry about dropping commits and such! BCD strictly squash merges all PRs so they're all compressed into one anyways! |
Thanks for merging those @queengooborg . I wish we could just merge this - we'd be no worse off in any way, and significantly better for end users. I am assuming that this is basically on hold while you try make better tests and until decisions on ranged values is made. |
I see this duplicates some fields which I suppose is to be expected based on mdn/mdn#384 (comment)? |
Thanks so much for jumping in here @jan-ivar - @queengooborg, Jan-Ivar is the person I mentioned yesterday (WebRTC Media Senior Staff Software Engineer). |
@jan-ivar Yes. They are duplicated as sub features of the different selectors, and that is both desirable and expected. I added a response to your response in the linked topic. |
Thank you @jan-ivar for joining in! As mentioned in the issue on mdn/mdn, our biggest issue is working out what browser versions each type and property was introduced. While http://wpt.live/webrtc/RTCPeerConnection-mandatory-getStats.https.html is helpful, it doesn't work on all browser versions, nor does it cover all of the properties. I've attempted to reverse engineer the code for use in our own tooling (see openwebdocs/mdn-bcd-collector@fcd51bc), but I'm running into some issues with it, namely:
|
It adds support for RTCMediaSourceStats (track and kind). Also the default ones like timestamp and ID. Not the ones spcific to video and audio states. Tested with BCD collector.
Added in https://bugzilla.mozilla.org/show_bug.cgi?id=1531087 Tested with https://mdn-bcd-collhttps://github.com/mdn/content/issues/26146ector.gooborg.com/tests/api/RTCStatsReport/type_peer-connection other docs work in
3e2ccc0
to
e1cfe16
Compare
e1cfe16
to
38e2b49
Compare
@queengooborg YEEE HAAARRR. All done. Ready for you to look at. Last pass was to get a few cases which had "true" against them. Just FYI:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all your hard work on this, @hamishwillee! Let's go ahead and get this landed right away!
@queengooborg YOU TOO!!!! This is such a good step. FYI, once I have the docs I'll come back and link to the MDN pages. |
This PR updates
RTCStatsReport
to include returned stats, sorted bytype_
key. It combines data from:The BCD collector resulted in a large number of ranged values. In order to get this in:
Chrome >= 80
were modified to be preciseChrome = 80
.type_
range to the minimum for contained subfeatures.The ranged value commit was done separately so should be easy to review.
FF Data
framesPerSecond, jitterBufferDelay, jitterBufferEmittedCount, totalSamplesReceived, concealedSamples, silentConcealedSamples
Fixes #6879.