Skip to content
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

RTCCodecStats - add docs #32452

Merged
merged 18 commits into from
Mar 14, 2024
Merged

Conversation

hamishwillee
Copy link
Collaborator

@hamishwillee hamishwillee commented Feb 27, 2024

This adds docs for RTCCodecStats, a "codec statistics" object you can get by iterating a RTCStatsReport.

This is part of the ongoing work to finished RTC stats, which was started in #27028

As with the other ones, this is not "exciting work". Many of these stats are just reflections of data from other webRTC objects. I've tried to properly cross link, but not to provide examples etc.
The value in this is largely around exposing the BCD table.

@github-actions github-actions bot added Content:WebAPI Web API docs size/m [PR only] 51-500 LoC changed labels Feb 27, 2024

{{APIRef("WebRTC")}}

The **`clockRate`** property of the {{domxref("RTCCodecStats")}} dictionary is a positive number containing the media sampling rate in Hz.
Copy link
Collaborator Author

@hamishwillee hamishwillee Feb 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewer, note that this shouldn't affect your review, as it is what the spec says.


Specifically, "media sampling rate" is what the spec says. However this does not match RTCRtpCodecParameters.clockRate - which is usually where I'd expect the data to come from.

Note that RTCCodecStats.clockRate is the media sampling rate, and RTCRtpCodecParameters.clockRate is the clock rate is the rate at which the codec's RTP timestamp advances (according to docs).
Most codecs have specific values or ranges of values they permit; see the IANA payload format media type registry for details.

This need not be the same as the codecs sampling rate - according to wikipedia the video codecs typically use a clock rate of 90000 so their frames can be more precisely aligned with the RTCP NTP timestamp, even though video sampling rates are typically in the range of 1–60 samples per second.

EDITED - confirming in w3c/webrtc-stats#785

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, so we could fix this in a follow-up if it turns out to be wrong.

@hamishwillee hamishwillee marked this pull request as ready for review March 1, 2024 04:18
@hamishwillee hamishwillee requested a review from a team as a code owner March 1, 2024 04:18
@hamishwillee hamishwillee requested review from wbamberg and removed request for a team March 1, 2024 04:18
@hamishwillee hamishwillee requested a review from a team as a code owner March 1, 2024 04:54
@hamishwillee hamishwillee requested review from dipikabh and removed request for a team March 1, 2024 04:54
@dipikabh dipikabh self-assigned this Mar 6, 2024
Copy link
Contributor

@dipikabh dipikabh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @hamishwillee, thanks for this work!
I've only done a language review. I understand some of the sentences are as they appear on other similar pages. I've commented/asked questions in case there is scope for improving clarity.

files/en-us/web/api/rtccodecstats/index.md Outdated Show resolved Hide resolved
files/en-us/web/api/rtccodecstats/index.md Outdated Show resolved Hide resolved
files/en-us/web/api/rtccodecstats/index.md Outdated Show resolved Hide resolved
files/en-us/web/api/rtccodecstats/index.md Outdated Show resolved Hide resolved
files/en-us/web/api/rtccodecstats/index.md Outdated Show resolved Hide resolved
files/en-us/web/api/rtccodecstats/sdpfmtpline/index.md Outdated Show resolved Hide resolved
files/en-us/web/api/rtccodecstats/sdpfmtpline/index.md Outdated Show resolved Hide resolved

## Value

A {{domxref("DOMHighResTimeStamp")}} value indicating the time at which the activity described by the statistics in this object was recorded, in milliseconds elapsed since the beginning of January 1, 1970, UTC.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
A {{domxref("DOMHighResTimeStamp")}} value indicating the time at which the activity described by the statistics in this object was recorded, in milliseconds elapsed since the beginning of January 1, 1970, UTC.
A {{domxref("DOMHighResTimeStamp")}} value indicating the time when the activity described by the statistics in this object was recorded. It is measured in milliseconds since January 1, 1970, UTC.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm good with this but have added to my notes to change globally once this and my other ones merge.


A {{domxref("DOMHighResTimeStamp")}} value indicating the time at which the activity described by the statistics in this object was recorded, in milliseconds elapsed since the beginning of January 1, 1970, UTC.

The value should be accurate to within a few milliseconds but may not be entirely precise, either because of hardware or operating system limitations or because of [fingerprinting](/en-US/docs/Glossary/Fingerprinting) protection in the form of reduced clock precision or accuracy.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this what we intend to say here:

Suggested change
The value should be accurate to within a few milliseconds but may not be entirely precise, either because of hardware or operating system limitations or because of [fingerprinting](/en-US/docs/Glossary/Fingerprinting) protection in the form of reduced clock precision or accuracy.
The value is accurate to within a few milliseconds, though it might not be entirely precise because of hardware or operating system limitations. Additionally, [fingerprinting](/en-US/docs/Glossary/Fingerprinting) protection could reduce the precision or accuracy of the time reported.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. My versions says "should be" within a few ms, whereas your version must be accurate to within a few ms irrespective of all factors. I actually don't know which it is, so would rather say "should be".

The bigger problem is that this text is many years old - all I've done is propagated the original info in a dictionary. I can't be sure from the spec that this is still true. I've asked here w3c/webrtc-stats#786

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it - "is" vs "should be", makes sense

Good move to to seek clarification 👍

files/en-us/web/api/rtccodecstats/transportid/index.md Outdated Show resolved Hide resolved
Co-authored-by: Dipika Bhattacharya <[email protected]>
@hamishwillee hamishwillee requested a review from dipikabh March 8, 2024 04:06
@hamishwillee
Copy link
Collaborator Author

@dipikabh Thanks for the review. I've accepted most of it. For the remainder I'll do as a post process against all the duplicates "at some point" (have taken a copy of actions)

Copy link
Contributor

@dipikabh dipikabh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @hamishwillee. The changes and your clarifications/edits look good.

Only one nit in the RTCCodecStats/clockRate page (#32452 (comment)), but leaving my +1

@hamishwillee
Copy link
Collaborator Author

thanks @dipikabh - appreciate your patience.

@hamishwillee
Copy link
Collaborator Author

@wbamberg Can you please give this a look since you reviewed my other WebRTC stats PRs? It's had editorial/language review from @dipikabh already - some of those are outstanding because I'll do them as a job-lot later across all the docs.

There is a technical question about timestamps in #32452 (comment) - still hoping to get some info to be "certain" on that. But if you know about this already ...

Copy link
Collaborator

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Hamish! It's been a while since I saw one of these, it was hard to remember what they are supposed to look like! I had one super-nit, so approving anyway.

files/en-us/web/api/rtccodecstats/index.md Outdated Show resolved Hide resolved
@hamishwillee
Copy link
Collaborator Author

Thanks very much @wbamberg (and @dipikabh ). I appreciate these are not fun to review!

@hamishwillee hamishwillee merged commit 667d3fc into mdn:main Mar 14, 2024
10 checks passed
@hamishwillee hamishwillee deleted the RTCCodecStats_docs branch March 14, 2024 22:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:WebAPI Web API docs size/m [PR only] 51-500 LoC changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants