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

Ship speexdsp's jitter buffer as part of local AudioBridge dependencies #3348

Merged
merged 5 commits into from Apr 2, 2024

Conversation

lminiero
Copy link
Member

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

  1. get rid of the speexdsp dependency (we only needed the jitter buffer, not everything else);
  2. fix the memory leak directly in the library.

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 to 0.x after merging.

@lminiero lminiero added the multistream Related to Janus 1.x label Mar 26, 2024
@tmatth
Copy link
Contributor

tmatth commented Mar 27, 2024

FWIW you could probably drop a lot/all of the os_support/arch macro/wrapper stuff given that:

  • the jitterbuffer isn't doing DSP per se so you don't have any float/fixed-point equivalents being called AFAIK
  • Janus isn't targetting the same range (or age) of platforms as speexdsp was
  • Janus doesn't support custom memory allocators (so you could either use the standard memmove et al or glib wrappers)

@lminiero
Copy link
Member Author

@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:

  1. merge the PR as it is, to address the main problem
  2. adapt Add optional RNNoise support to AudioBridge #3185 so that we bring in resampler.c too
  3. fix a few things @atoppi wanted to fix (mainly some internal queue sizes, IIRC)
  4. possibly strip the code as you suggested

For 4. later on, help from you would indeed be invaluable, considering your expertise with the library!

@atoppi
Copy link
Member

atoppi commented Mar 28, 2024

@tmatth maybe consider adding the fix related to memory leaks in upstream too.

@tmatth
Copy link
Contributor

tmatth commented Mar 29, 2024

@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.

@lminiero
Copy link
Member Author

lminiero commented Apr 2, 2024

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.

@lminiero
Copy link
Member Author

lminiero commented Apr 2, 2024

Merging, I'll backport to 0.x too shortly.

@lminiero lminiero merged commit 4be743e into master Apr 2, 2024
8 checks passed
@lminiero lminiero deleted the speexdsp-external branch April 2, 2024 10:17
RSATom added a commit to RSATom/janus-gateway-snap that referenced this pull request Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
multistream Related to Janus 1.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants