CHANGE(client): Exclude and discourage RNNoise

[Krzmbrzl]

Due to RNNoise being unmaintained and the library's code being in very poor shape, this commit excludes the RNNoise feature from Mumble by default and discourages its use.

The library contains Opus and CELT symbols (probably due to copy&paste of code) that can end up being called from Opus instead of its own versions of these functions. This can lead to completely unforeseen behavior, including crashes.
An example of this is as of writing this, enabling RNNoise on macOS leads to a crash of Mumble pretty much as soon as it starts up with an "invalid instruction" error. The reason being that part of RNNoise's implementation of one of Opus's symbols contains a code path that produces an invalid instruction in optimized builds (and a segfault in debug builds) and this code path is taken when Opus (wrongly) uses this function instead of its own.

Fixes #6041

Checks

• My commits follow the commit guidelines

[davidebeatrici]

Why don't we just modify RNNoise's CMake project so that it produces "unoptimized" code in order to fix the issue?

While I agree that the library seems abandoned, the technology itself proved to be useful in suppressing background noises such as clicky keyboard switches.

[Krzmbrzl]

Why don't we just modify RNNoise's CMake project so that it produces "unoptimized" code in order to fix the issue?

Because that's not a fix. If Opus ends up calling into RNNoise functions then God knows what will happen. This was just one particular instance of how this can bite us in our backsides. Just remember the PITA to find the root of the infamous Opus crash which ended up being caused by the bundled Opus lib calling into system Opus lib functions.
Something like this just not acceptable.

[davidebeatrici]

That issue was caused by RNNoise being linked, as opposed to Opus which was dynamically loaded.

Didn't we fix that permanently?

[Krzmbrzl]

No that issue had nothing to do with RNNoise, but it serves as an example of what can go wrong due to this kind of "symbol jumps".
We fixed the Opus issue by ensuring that we can't have duplicate symbols by no longer bundling Opus ourselves.

[jvbsl]

But RNNoise is one of the nicest features of mumble, I prefer it over any other noise filter in and outside of mumble.

I think ther'd need to be a better way. And invalid instruction sounds like it was optimized for a platform which has some optimized instructions(probably SIMD) which then where not available on the platform it was run on, which in itself is neither an error of rnnoise nor an error of the optimization itself, but an error of the compilation target used

[Krzmbrzl]

But RNNoise is one of the nicest features of mumble, I prefer it over any other noise filter in and outside of mumble.

That may be but I won't take on the burden of remotely debugging reports of all kinds of weird crashes. I don't have the time nor the motivation to do so.

However, you are free to compile Mumble yourself and enable RNNoise there. The option remains available, it's just not enabled by default.

And invalid instruction sounds like it was optimized for a platform which has some optimized instructions(probably SIMD) which then where not available on the platform it was run on, which in itself is neither an error of rnnoise nor an error of the optimization itself, but an error of the compilation target used

The error was 100% RNNoise's fault.

1. It exports symbols that belong to other libraries (Opus/Celt) which led to a wrong program flow during Opus processing.
2. The invalid instruction turns into a segfault when turning off optimization
3. The segfault is due to *((int *)0) = 0 which invokes undefined behavior and thus anything can happen with this code.

I would be willing to re-enable RNNoise if someone forks the repo and ensures that all duplicate symbols are removed or rather renamed to ensure that there won't ever be any symbol clashes.

But as things stand and given that I do like 99% of all issue handling here, I take the liberty of throwing RNNoise out.

Finally, according to some research I did there are deep learning noise suppressors that supposedly work a lot better than RNNoise. So maybe it would make more sense to focus any future efforts to exploring and implementing alternatives.

[Hartmnt]

So if I understand correctly, Mumble will ship in binary form without RNNoise. Upgrade path question: What will happen to users who have in the past explicitly enabled RNNoise in their settings after they run Mumble without the feature? Has that been tested yet?

[Krzmbrzl]

So if I understand correctly, Mumble will ship in binary form without RNNoise

Yes

Upgrade path question: What will happen to users who have in the past explicitly enabled RNNoise in their settings after they run Mumble without the feature? Has that been tested yet?

mumble/src/mumble/Settings.cpp

Lines 819 to 824 in ead6fc9

	#ifndef USE_RNNOISE
	if (noiseCancelMode == NoiseCancelRNN || noiseCancelMode == NoiseCancelBoth) {
	// Use Speex instead as this Mumble build was built without support for RNNoise
	noiseCancelMode = NoiseCancelSpeex;
	}
	#endif

and yes, this also has already been tested.

[DarkDefender]

Would there be any interest in replacing RNNoise with https://github.com/Rikorose/DeepFilterNet ?
There is already a LADSPA plugin for it that can be used with pipewire. I haven't tried it myself yet, but the demo files seems promising and even a upgrade quality wise from RNNoise:

https://rikorose.github.io/DeepFilterNet2-Samples/

[davidebeatrici]

Are there C or C++ bindings available for it?

[Krzmbrzl]

@DarkDefender I think we would generally be interested in testing out any kind of alternative noise suppression.

[DarkDefender]

Are there C or C++ bindings available for it?

LADSPA is a C API, so I think that is a yes?
However I'm unsure how easy it is to get working on mac and windows as this is a linux API first and foremost.
But I have seen some forum posts saying that people have been able to get LADSPA plugins to work on windows with Audacity, so perhaps there is hope.

Perhaps the way forward is to support some general API like LADSPA so users could if they wanted also hook up additional effects? And mumble could ship with some plugins like the DeepFilterNet plugin per default.

[Krzmbrzl]

Perhaps the way forward is to support some general API like LADSPA 

Yeah something like this would probably be good. We only have to ensure that it'll end up being cross platform.
There are ongoing efforts of outsourcing the audio processing into a standalone library at https://github.com/mumble-voip/libcrossaudio which should eventually be used in main Mumble as well. Maybe it'd be easier to integrate this into that library instead of into Mumble directly.

[davidebeatrici]

I strongly agree.

[DarkDefender]

I agree that this should probably end up in libcrossaudio. However it seems like it is still in very early stages, so maybe implementing a proof on concept in mumble first would be best?

Or is there any easy way to have mumble use libcrossaudio already? (Like a WIP branch or something)

[davidebeatrici]

No WIP branch yet, but the library could be tested in Mumble with PipeWire (Linux) and WASAPI (Windows). We're only missing device notifications (e.g. added/removed) and channel positioning (right now the default layout is used).

[Krzmbrzl]

Related: #6299

[skobkin]

That's bad. At least while there is no alternative in place.

This is the first time I'd recommend users to avoid updating to latest version.

I hope that some kind of replacement will be implemented in the final 1.5 release.

[Hartmnt]

The code is not removed, you can still enable RNNoise at compile time. I do not expect any replacement to ship with 1.5 stable.

[skobkin]

The code is not removed, you can still enable RNNoise at compile time

It doesn't help in cases when users do not know how to build app from the source. And setting up your own binary distribution channels is not an easy task too.

For a simple end-user it's a major regression.

[Krzmbrzl]

@skobkin there won't be a replacement. But we'll gladly accept PRs implementing something for 1.6

[Hartmnt]

Good news: I have spent the better part of a week worth of my free time to create a fork of RNNoise which should hopefully (testing on Mac pending) resolve the symbol and manual trap problems in upstream RNNoise.

If all goes well, 1.5 stable might ship with ReNameNoise instead of RNNoise #6364

[skobkin]

@Hartmnt What a wonderful news!
You're our hero 😅

[jmvalin]

(I'm the author of RNNoise) Maybe the symbol renames can just be upstreamed instead of changing all the API names?

[Hartmnt]

(I'm the author of RNNoise) Maybe the symbol renames can just be upstreamed instead of changing all the API names?

Hi, first off: Thank you for RNNoise, it is a great piece of software. I figured that RNNoise was unmaintained as the symbol clashing issue (and others) have been open and reported for years without any response xiph/rnnoise#165

Secondly, we want to release Mumble 1.5 stable as soon as possible, because we were stuck for over a year and RNNoise was one of the blockers. So for me it was either "fork RNNoise now, or live with the consequences of having to deal with users who are not able to compile Mumble (with RNNoise) themselves"

Thirdly, my C/C++ knowledge is ok, but not fantastic, so I went ahead and renamed every single symbol/define/name/whatever that I could find just to be extra sure there will not be a problem with this until we are ready to swap RNNoise for a plugin system in libcrossaudio. Because of that I decided to create a commit in my fork for every symbol rename (about 300 commits) so I could more easily find out, if I messed something up later. This would be a pain to back port to upstream now, because I did not expect upstream to be alive to be honest.

Anyway you can find the fork here: https://gitlab.com/Hartmnt/renamenoise until we pull the repo into the mumble-voip namespace here on GitHub. All community fixes aka pull requests I pulled in during the last two days are properly attributed in the commit history by cherry-picking and a link to the original PR

[Hartmnt]

The switch to ReNameNoise was merged. It will land in 1.5 stable 🎉