FEAT(client): Add ReNameNoise as a replacement for RNNoise

[Hartmnt]

Replaces RNNoise with "ReNameNoise" - a fork I made in an attempt to save it for Mumble 1.5 stable

Note: #6299 and a plugin system in libcrossaudio is the future of noise suppression in Mumble, this is merely a hotfix because I myself do not want to live in a world where Mumble loses RNNoise support. As of yet, it is hard to estimate how much development time the long-term solution will need.

The fork renames every exported symbol (and non-exported for good measure) to make sure no compile or runtime problems exist. It will also include some community fixes for RNNoise that have been floating around the internet for some time.

Reverts #6292

[jmvalin]

(RNNoise author here) What's the exact purpose of ReNameNoise? Is it just to avoid the clashes with libopus when linking statically or is it also trying to have different names from the original RNNoise? And if so why?

[skobkin]

@jmvalin Check out #6292.
There is some kind of explanation from Mumble maintainers 🤷

[Krzmbrzl]

I wonder whether we shouldn't leave all user-facing text unchanged. The changed name will likely confuse users and I think it is not really necessary (because there is likely nothing to be gained from making the average user aware of the fork) 🤔

[Hartmnt]

I wonder whether we shouldn't leave all user-facing text unchanged. The changed name will likely confuse users and I think it is not really necessary (because there is likely nothing to be gained from making the average user aware of the fork)

Sure, but since he is here now we should ask @jmvalin if that is ok. It is a fork which is technically incompatible with upstream rnnoise as the API calls have been renamed. If users install RNNoise as system library, that will not work with Mumble after this PR

[Krzmbrzl]

It is a fork which is technically incompatible with upstream rnnoise as the API calls have been renamed. If users install RNNoise as system library, that will not work with Mumble after this PR

Only from the developer's perspective. From a user's perspective they are still identical

[skobkin]

@Krzmbrzl I'm not sure if it's legally and ethically correct.

I'm not implying anything, but suppose we'll have some problem with new implementation. Someone may come to @jmvalin for explanation for the code he doesn't maintain.

I think that if checkbox label changes a bit, it wouldn't be a big problem for user, but it'll be more fair to the original library author.

[Hartmnt]

Let's wait for input first and then decide. It will take a few days for me to complete this anyway

[jmvalin]

I mean if the idea is just to solve the symbols clash then maybe it should just be fixed upstream. Is there an actual list of symbols that are clashing with Opus? I assume there aren't that many, right?

[Hartmnt]

I mean if the idea is just to solve the symbols clash then maybe it should just be fixed upstream. Is there an actual list of symbols that are clashing with Opus? I assume there aren't that many, right?

Feel free to use any or all of the changes made to the fork upstream

[jmvalin]

Can you try again with the latest master and see any any symbol clashes remain?

[Hartmnt]

@jmvalin This issue in your repo seems to list all the exported symbols https://gitlab.xiph.org/xiph/rnnoise/-/issues/2
But please note that I will not try again to use upstream rrnoise. I have spent way too many hours on this already and also pulled in other changes.

The only open question remaining is, whether we should retain the RNNoise string in the UI or swap it with ReNameNoise. I do not mind either way, but I feel it would be nice if you would tell us your preference. Otherwise, we make the call.

[Krzmbrzl]

I'm not implying anything, but suppose we'll have some problem with new implementation. Someone may come to jmvalin for explanation for the code he doesn't maintain.

I don't see this much different from self-compiling instead of only using pre-compiled binaries from the maintainer.
Besides, I don't see it coming that any single user will have a problem with RNNoise in Mumble and go complain to the RNNoise folks. They'll complain here.

Anyway, I'm not opposed to changing the name in the UI. It's just unnecessary and potentially a little confusing in my POV.

[davidebeatrici]

@Hartmnt I'm pretty sure all of your commits can be upstreamed, aside from the clang-format one (changes the code style in the entire repository).

[Hartmnt]

@Hartmnt I'm pretty sure all of your commits can be upstreamed, aside from the clang-format one (changes the code style in the entire repository).

I agree, but I am not willing to put in the extra work. Whoever feels like it can squash the 300 commits and or cherry-pick single ones and help improve upstream with it.

[jmvalin]

Personally I think 300 commits is bit excessive to address a few clashing symbols (the gitlab master should now be free of clashes), but I'm not the one doing that. Now, how you name the resulting API probably won't affect me too much either way provided you make it clear it's an independent fork. The main question is whether you ultimately intend on back-porting future improvements like the (work-in-progress) ones I'm currently working on (https://gitlab.xiph.org/xiph/rnnoise/-/commits/upgrade1).

[Krzmbrzl]

💚 All backports created successfully

Status	Branch	Result
✅	1.5.x

Questions ?

Please refer to the Backport tool documentation and see the Github Action logs for details