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

RNNoise #3427

Merged
merged 5 commits into from Jul 7, 2018

Conversation

@main--
Copy link
Contributor

commented Jun 26, 2018

RNNoise is machine learning based noise suppression. It is used in WebRTC (chromium and firefox afaik).

It compiles, runs and works (on my machine). I'm not sure if I integrated the build system correctly though. I'm building a shared library because rnnoise has symbol clashes with opus. I also didn't work with windows at all.

@main-- main-- force-pushed the main--:rnnoise branch from 64a441f to 052b3eb Jun 26, 2018

@main--

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2018

So about the linker problems: librnnoise apparently uses a few functions copied (?) from opus. Building a static library causes these to conflict. Building a shared library works, but requires the user/packager to install librnnoise.so.0 to /usr/lib (the mumble subfolder doesn't work for some reason?? I have no idea how any of this works) . This is clearly awful, if you have any ideas please help (ffs it can't be this hard just to hide a few symbols).

@davidebeatrici

This comment has been minimized.

Copy link
Member

commented Jun 26, 2018

The CELT library is built as shared, in order not to conflict with Opus.

CONFIG(static) {
CONFIG -= static
CONFIG += shared
}

CONFIG(sbcelt) {
TARGET = celt
CONFIG += static
} else {
CONFIG(static) {
CONFIG -= static
CONFIG += shared
}
}

In the Debian package, it's located in /usr/lib/mumble: https://packages.debian.org/stable/amd64/mumble/filelist

@main--

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2018

Ah I worked it out. The glue code in CELTCodec.cpp manually loads the library from /usr/lib/mumble.

Not sure what to do here. This patch is tiny, adding custom library loading for rnnoise would probably more than double the code. Dropping a librnnoise.so.0 into the system library directory isn't perfect but I haven't seen the library being released (or even used) anywhere outside browsers (which presumably do their own thing anyways). Even then, this compiles the standard RNNoise library without any modifications so even if someone were to accidentally use it they definitely wouldn't run into any nasty surprises.

The only missing piece then is Windows. Thus the AppVeyor build fails. The travis build is mostly green, except for, again, the Windows build. Can you help me with that? Is there any way I can get a working Windows build without a Windows development environment?


win32 {
DEFINES += WIN32 _WIN32
INCLUDEPATH += ../$$BUILDDIR/win32

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Jun 27, 2018

Member

config.h is in $$BUILDDIR, not in $$BUILDDIR/win32.

Remove this line.

DEFINES += FLOAT_APPROX
}

INCLUDEPATH += ../$$BUILDDIR

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Jun 27, 2018

Member

Move this out of the unix block, so that the path is included for Windows too.

@davidebeatrici

This comment has been minimized.

Copy link
Member

commented Jun 27, 2018

Ah I worked it out. The glue code in CELTCodec.cpp manually loads the library from /usr/lib/mumble.

Not sure what to do here. This patch is tiny, adding custom library loading for rnnoise would probably more than double the code. Dropping a librnnoise.so.0 into the system library directory isn't perfect but I haven't seen the library being released (or even used) anywhere outside browsers (which presumably do their own thing anyways). Even then, this compiles the standard RNNoise library without any modifications so even if someone were to accidentally use it they definitely wouldn't run into any nasty surprises.

Since RNNoise is not released yet, what do you think about building it as static library and Opus as shared library instead?

The only missing piece then is Windows. Thus the AppVeyor build fails. The travis build is mostly green, except for, again, the Windows build. Can you help me with that? Is there any way I can get a working Windows build without a Windows development environment?

See my comments above.

@main--

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2018

I just copy-pasted the build structure from some other submodule (opus I think). I thought perhaps windows needs a different config.h so things don't break? No idea.

Since RNNoise is not released yet, what do you think about building it as static library and Opus as shared library instead?

Makes sense to me. Honestly at this point I think it would be best if you make the necessary changes to this patch yourself (you have push access) as most of what I would do is just guesswork - I'm not familiar enough with the build system to make these changes.

All I can say is that RNNoise works amazingly well in Mumble, it almost perfectly removes common annoyances with voice activation like typing and breathing noises. The only downside is slightly reduced quality but this is a price I would think many are willing to pay (me and all of my friends that tested this included).

@davidebeatrici

This comment has been minimized.

Copy link
Member

commented Jun 27, 2018

I just copy-pasted the build structure from some other submodule (opus I think). I thought perhaps windows needs a different config.h so things don't break? No idea.

Yes, but you didn't put a config.h in win32, that's why the compiler didn't find it.

Makes sense to me. Honestly at this point I think it would be best if you make the necessary changes to this patch yourself (you have push access) as most of what I would do is just guesswork - I'm not familiar enough with the build system to make these changes.

All I can say is that RNNoise works amazingly well in Mumble, it almost perfectly removes common annoyances with voice activation like typing and breathing noises. The only downside is slightly reduced quality but this is a price I would think many are willing to pay (me and all of my friends that tested this included).

No problem, thank you very much for your contribution!

I'm sure RNNoise will be much appreciated, especially on Linux, where echo cancellation is not supported.

@@ -23,6 +23,10 @@
#include "opus.h"
#endif

#ifdef USE_RNNOISE
#include "rnnoise.h"

This comment has been minimized.

Copy link
@main--

main-- Jun 27, 2018

Author Contributor

fyi the linker error is caused by the missing extern "C" here.

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Jun 27, 2018

Member

Fixed, thank you.

@davidebeatrici davidebeatrici force-pushed the main--:rnnoise branch 4 times, most recently from b1b00cf to 031fcba Jun 27, 2018

@davidebeatrici davidebeatrici force-pushed the main--:rnnoise branch 2 times, most recently from d4e23cb to 5d4872b Jul 6, 2018

@davidebeatrici davidebeatrici force-pushed the main--:rnnoise branch from 5d4872b to 37718eb Jul 6, 2018

Translation update
Updating 'mumble_en.ts'...
    Found 1709 source text(s) (3 new and 1706 already existing)

@davidebeatrici davidebeatrici merged commit e54f60f into mumble-voip:master Jul 7, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@davidebeatrici

This comment has been minimized.

Copy link
Member

commented Jul 7, 2018

Thank you again for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.