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

Add jitterBufferMinimumDelay metric #641

Merged
merged 2 commits into from Jun 29, 2022
Merged

Add jitterBufferMinimumDelay metric #641

merged 2 commits into from Jun 29, 2022

Conversation

ivocreusen
Copy link
Contributor

@ivocreusen ivocreusen commented Jun 15, 2022

@henbos
Copy link
Collaborator

henbos commented Jun 20, 2022

@alvestrand @dontcallmedom

@ivocreusen is a Googler (ivoc@), how do we make the bot recognize this?

@henbos henbos requested review from vr000m and alvestrand June 20, 2022 13:46
@henbos
Copy link
Collaborator

henbos commented Jun 20, 2022

@alvestrand @dontcallmedom

@ivocreusen is a Googler (ivoc@), how do we make the bot recognize this?

Never mind, I was able to re-run the IPR check and it is happy now

webrtc-stats.html Outdated Show resolved Hide resolved
as jitter and packet loss, and can be seen as the minimum obtainable jitter buffer delay if no
external factors would affect it.
</p>
</dd>
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we explicitly state that the metric is updated every time the jitterBufferEmittedCount is updated. This is the jitter buffer delay if there was no reason for holding the sample or frame for application related reasons.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would CPU issues cause this metric to be updated or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the description to mention it is updated whenever jitterBufferEmittedCount is updated. About CPU issues: I don't think CPU issues should affect the metric, but then again it is hard to predict exactly what happens when there is a CPU issue so I am not sure if we should give any explicit guarantees about that in the standards description.

@vr000m
Copy link
Contributor

vr000m commented Jun 28, 2022

left comments inline. Renaming the metric to jitterBufferMinimumDelay.

Copy link
Contributor

@vr000m vr000m left a comment

Choose a reason for hiding this comment

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

small changes, then go ahead and merge in this spec or provisional depending on implementation status

@henbos
Copy link
Collaborator

henbos commented Jun 28, 2022

I'll coordinate with ivoc to get this merged here or in provisional depending on implementation commitment and addressing comments

@henbos
Copy link
Collaborator

henbos commented Jun 28, 2022

He'll get to this and the implementation in a few weeks, putting the editors can integrate label in the meantime

…he description mentions it is updated at the same time as jitterBufferEmittedCount
Copy link
Collaborator

@henbos henbos left a comment

Choose a reason for hiding this comment

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

Looking good, let's merge!

@henbos henbos changed the title Add estimatedMinimumJitterBufferTargetDelay metric (#634) Add jitterBufferMinimumDelay metric Jun 29, 2022
@henbos henbos merged commit 299f870 into w3c:main Jun 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants