meetecho / janus-gateway

Janus WebRTC Server
https://janus.conf.meetecho.com
GNU General Public License v3.0
8.17k stars 2.47k forks source link

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

Closed lminiero closed 5 months ago

lminiero commented 6 months ago

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 a15e3fdfc6049927de9748d88e7fac3b19a3455d and ad326e0cd3f1debc725a3156cf20bcb20fda2323). 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.

tmatth commented 6 months ago

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

lminiero commented 6 months ago

@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 #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 commented 6 months ago

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

tmatth commented 6 months ago

@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 commented 5 months ago

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 commented 5 months ago

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