-
Notifications
You must be signed in to change notification settings - Fork 128
Enabling PLC for opus decoder #992
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
base: main
Are you sure you want to change the base?
Conversation
boks1971
left a comment
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.
nice! once again, I understand only 25% :-) , but lgtm!
pkg/pipeline/builder/audio.go
Outdated
| if lastDur > 0 { | ||
| dur = lastDur | ||
| } else { | ||
| dur = 20 * time.Millisecond |
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.
:nit: maybe make a const for default opus duration?
pkg/pipeline/builder/audio_stats.go
Outdated
| func parseStatsString(s string) (OpusDecStats, error) { | ||
| var st OpusDecStats | ||
|
|
||
| getU64 := func(k string) (uint64, error) { |
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.
are these useful as some util in a utils package?
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.
interesting idea - a generic set of functions for finding an integer in a given regex might be interesting to have somewhere available - is there already a utils package you could recommend for this or you maybe referred to creating a utils package in this project?
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.
We put a lot of utils in protocol repo. Would be good to put it there.
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 tried to extract it here: livekit/protocol#1195
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.
updated
|
|
||
| opusPlcMaxFrames = 5 | ||
| opusDecStatsPollInterval = time.Second * 5 | ||
| opusDecPlcMaxJitter = 3 * time.Millisecond |
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.
why 3ms?
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.
semi arbitrary chosen value :)
The case I had in mind was - if the gap is almost one frame (e.g., 18–19 ms when the true frame is 20 ms) still count it as a loss. That probably won't happen as long as we lose full packets. Added for the cases when wall-clock PTS adjustment kicks in (even then it's going to be extremely rare).
Adding opuseparse element before the opus decoder in order to be able to determine buffer durations before the decoder and have ability to signal gaps should it result in smaller (up to 5 frames) gaps. In order to make the PLC logic effective a gap event needs to be signaled to the decoder in addition to enabling its
plcproperty.A periodic poll of opus decoder stats is also added and exposed as an element message used to update audio qos metrics.