-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Use speexdsp's jitter buffer in the AudioBridge #3214
Conversation
Pinging @tmatth explicitly since he seems to be an active contributor to speexdsp, and so may point out things we might be doing incredibly wrong 🤭 |
@@ -8495,6 +8355,7 @@ static void *janus_audiobridge_participant_thread(void *data) { | |||
g_free(mixedpkt->data); | |||
g_free(mixedpkt); | |||
} | |||
g_usleep(2500); |
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.
Was this added to avoid busy waiting?
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.
Yep: before, janus_audiobridge_participant_thread
was only used for outgoing packets, and so we had a g_async_queue_timeout_pop
that would wait 100ms at max. Since now there's two different queues to probe, we peek both and return right away if they're empty (to check if we need to need anything with the other). This way we give the thread some idle time.
if(!first && participant->codec == JANUS_AUDIOCODEC_OPUS && participant->fec) { | ||
if(ntohs(rtp->seq_number) != participant->expected_seq) { | ||
/* Lost a packet here? Use FEC to recover */ | ||
use_fec = TRUE; |
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.
N.B. Opus FEC only works for single packet loss (not on a burst) IIUC, I'm not sure if this is implicit in your handling here but maybe you want to refine this check?
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.
It's supposed to be implicit, in the sense that this check is done on an out of order packet, so the packet that follows a loss (we detect a gap). In that case, even if there was a loss higher than 1, we only inject a single FEC decoded packet anyway, since previous losses if any are ignored.
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.
Just some questions but LGTM.
Thanks for the review! |
Merging. |
The audio buffer in the AudioBridge has historically been a bit of its Achilles heel: while it worked fine in most situations, it definitely struggled more with problematic connections (e.g., high jitter or packet loss), which often resulted in broken or spotty audio for specific participants that could only be solved by having them reconnect (until the problem happened again).
This PR tries to fix this by using a proper jitter buffer, specifically the one made available as part of the Speex DSP library. In our tests in different circumstances where we could replicate problems before it seems to perform much better, so we thought we'd make this available as a PR so that you can test it before we merge. As a result of this change, we refactored a bit of related code too, which means that for instance the
prebuffer
property you used before is gone from configuration and API: in fact, that was only meaningful to our own old buffer, and doesn't make much sense using this new jitter buffer instead.If you use the AudioBridge for anything in your application, you most definitely want to test this. Notice that this will be a breaking change for many: in fact, the fact we now rely on Speex DSP for its jitter buffer also means that the AudioBridge will not compile unless the library is available when rebuilding Janus. In most deployments currently relying on the AudioBridge this may not be the case, meaning they'd get stuck with an older version of the plugin. As such, make sure you install the library before compiling this PR, or when updating Janus after it's merged.
Also notice this PR is for the
0.x
branch: when it will come the time to merge this, I'll port all the same changes tomaster
as well.Looking forward to your feedback!