-
Notifications
You must be signed in to change notification settings - Fork 14
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
Add MediaStreamTrackAudioStats interface #117
Conversation
History: Already covered in the April Virtual Interim and a PR like this "kind-of" already got approved in #97, but then that started the whole async vs sync debate so we never merged that |
Thought: In order to be consistent with video stats, where we don't have droppedFrames - instead we have totalFrames, we should consider changing this PR to have totalFrames and totalFramesDuration instead of "dropped". The app could simply calculate "droppedFramesDuration" as totalFramesDuration - deliveredFramesDuration. So the glitch% would be (totalFramesDuration - deliveredFramesDuration) / totalFramesDuration |
Based on the Virtual Interim, I should:
|
PR updated! |
There's a section in the webrtc stats spec (here) that motivates total counters versus instantaneous values or pre-computed averages. Part of the API shape discussions that the WG has had was to do totals in |
Spoke with Jan-Ivar, I think it makes sense to also have the latest latency sample for use cases like A/V sync, in addition to average latency over app defined interval |
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 LGTM but I'd like @padenot to sign off on it.
I also filed #119 to add latency of the latest audio frame for use of A/V sync. The avg latency is still useful for "what was the quality during interval X" |
I added the current latency in follow up PR #120 but because I can't figure out how to create PR chains, this PR needs to land first so I can rebase the next PR. In the meantime its diff contains both PRs |
Friendly ping |
Unless anyone objects, I will merge this on Monday. |
index.html
Outdated
<li> | ||
<p>The <dfn data-lt="current audio latency">current audio | ||
latency</dfn> is the input latency of the last audio frame that | ||
was delivered. If no audio frames have been delivered yet, this |
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.
what's the unit?
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.
Milliseconds like the other metrics. But this should be in #119, sorry.
index.html
Outdated
manner.</p> | ||
<p>The percentage of audio that was dropped can be calculated as | ||
[= dropped audio frames duration =] / ([= delivered audio frames | ||
duration =] + [= dropped audio frames duration =]).</p> |
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 is incorrect, this is a ratio, not a percentage.
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.
Rephrased as delivered/total to match the JS exposed attributes rather than the internal slots and moved the note. But if 9 out of 10 seconds of audio was delivered, that's a 9/10=90% percentage of the total duration delivered?
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.
@henbos "percentage" implies a number between 0 and 100, when the division produces a number between 0 and 1 i.e. "ratio".
Maybe do an editorial PR to s/percentage/ratio/?
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.
Lol, sorry I didn't catch that. Yes: #126
index.html
Outdated
the same time as [= delivered audio frames =] and is measured in | ||
milliseconds. Note that once a frame has been delivered to a sink, | ||
that sink may internally have its own latency (such as playout | ||
delay) which is not included in this measurement.</p> |
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.
Summing latencies of audio buffer (if that's what you mean) does not make any sense. You want a series of values here if you want to understand what's going on.
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.
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.
For the sake of progress I'll remove all latency metrics and follow up on them in #119.
It seems to me that the "controversial" part is the latency so let's merge the frame counters and follow up. The second part is about how to phrase the algorithm ("stable state" or otherwise) but that's #108 and editorial so let's do that for both audio and video when we have decided what phrase to use.
The remaining counters are very close to what we already have for video.
{{MediaStreamTrackAudioStats/[[DroppedFrames]]}} is set to | ||
[= dropped audio frames =] and | ||
{{MediaStreamTrackAudioStats/[[DroppedFramesDuration]]}} is set to | ||
[= dropped audio frames duration =].</p> |
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.
Please rewrite this in terms of an algorithm running in parallel and stable state: https://html.spec.whatwg.org/multipage/webappapis.html#await-a-stable-state so it's clear what happen. Alternatively you can post a task from your algorithm running in parallel, that's more or less equivalent here.
No need to invent something else.
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.
Note whatwg/html#2882.
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.
I already filed #108 but did not have a reference for the language requested. Now I have a reference, but I also have a bug that questions this wording (Jan-Ivar's link). So I'm not sure what to make of that, but let's follow up in that issue as a separate PR because we should fix both the audio and video algorithm at the same time, they use identical language and this is editorial.
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.
I also have a bug that questions this wording (Jan-Ivar's link)
To clarify, Paul said:
- "please rewrite this in terms of an algorithm running in parallel and stable state"
- "Alternatively you can post a task from your algorithm running in parallel, that's more or less equivalent here.
My reason for mentioning whatwg/html#2882 was to suggest going with 2.
They're just different ways to write algorithms in the spec that involve synchronous steps AND async steps.
To apply a JS analogy to spec writing: 1 is like async-await, and 2 is like promise-then. 1 is out of favor, because 2 is closer to how you would need to implement things e.g. in c++.
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.
Ack, let's continue over at #108 where I quoted this last response.
Only partially fixes #96, but is missing latency
The RTCAudioSourceStats contains capture related metrics that are better suited for a MediaStreamTrack method than RTCPeerConnection.getStats() since you may be capturing for a non-WebRTC context. Moving the metrics was covered by the April 18, 2023 Virtual Interim and the Working Group input was supportive of the move.
Since then I've updated the language to talk about audio frames rather than audio samples and to use the same language about "delivered" vs "dropped" that we used in the video stats case.
This is the new interface (edited to match latest patch set):
Edit: From the October interim minutes, conclusion: "Overall approach makes sense, flesh out the details in the editors meeting"
Preview | Diff