-
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
Ship speexdsp's jitter buffer as part of local AudioBridge dependencies #3348
Conversation
FWIW you could probably drop a lot/all of the
|
@tmatth thanks for the suggestions! Actually, since in the #3185 PR we do use the speexdsp resampler, we may indeed some of that stuff, which is why I kept it in for now. Besides, I'd rather not touch the code too much, since I'm not very familiar with it and don't want to risk breaking stuff. The plan for now is, in following efforts:
For 4. later on, help from you would indeed be invaluable, considering your expertise with the library! |
@tmatth maybe consider adding the fix related to memory leaks in upstream too. |
I'd be happy to look at an MR to https://gitlab.xiph.org/xiph/speexdsp or if someone has a reproducer for the leak I can work on one myself. |
The patch would be as simple as the tiny change we made on our own copy. I'll see if creating a simple reproducer is easy, since the leak can be triggered by adding a very late packet, which I guess means passing a big timestamp gap. |
Merging, I'll backport to |
Some time ago, in #3214 we modified the AudioBridge plugin to use the jitter buffer provided by SpeexDSP.
This was quite successful and greatly improved the audio experience, but we soon found out that the library comes with a nasty memory leak under some circumstances, specifically when dealing with very late packets. Some projects addressed this by forking the library and applying a fix themselves (e.g., like here), while we initially chose to fix this externally instead, by mimicking the same check SpeexDSP performs internally that leads to the leak (see a15e3fd and ad326e0). We found out that under very specific circumstances that's not enough, though, and so we decided to just include the relevant jitter buffer files as part of the repo, specifically as part of local AudioBridge dependencies.
This allowed us to
speexdsp
dependency (we only needed the jitter buffer, not everything else);We'll probably take advantage of this to also fix a few other slightly problematic behaviours, but my main priority at the moment is fixing the leak in a consistent and reliable manner.
Not sure if this will cause problems to existing deployments, e.g., due to symbol clashes if you don't do a
make clean
. Please make sure you test this, to ensure it works as expected and that it doesn't introduce regressions (which I doubt it will, but that's the point of testing). Should this all work nicely, I'll backport to0.x
after merging.