search

RustAudio / rnnoise-c

Rust bindings to Xiph's rnnoise denoising library
Other
7 stars 3 forks source link

Rnnoise does not compile on Windows #7

Open Flakebi opened 4 years ago

Flakebi commented 4 years ago

Hi,

rnnoise has a few problems when it comes to compiling on Windows.

  1. libclang has to be installed and referenced by $env:LIBCLANG_PATH = "/path/to/clang/dlls/", not a big problem but if you know a way to make this more convenient that would be nice.
  2. msvc does not support variable size stack arrays. There are some open pull-requests which fix this issue, also Mumble uses its own fork with a fix: https://github.com/mumble-voip/rnnoise
  3. There is a line that crashes on Windows and probably gets optimized out on Linux. Removing it is not perfect but it works afterwards 🤷 https://github.com/Flakebi/rnnoise/commit/fdc7a18dca6f0303c3ba719c6e91b02c1a098f9c
  4. M_PI is missing by default on Windows.
  5. The Windows linker complains about symbols being defined multiple times when using audiopus and rnnoise simultaneously.

Here is my full list of changes: https://github.com/xiph/rnnoise/compare/master...Flakebi:master.

The upstream project doesn’t seem to be maintained unfortunately.

est31 commented 4 years ago

libclang has to be installed

That one is easily fixable. I can create a PR.

msvc does not support variable size stack arrays

VLAs are also bad for security and performance reasons. Mallocs aren't any better and the patch you link doesn't check the return value of malloc. That's an instant sign of code smell. We need something better.

There is a line that crashes on Windows and probably gets optimized out on Linux

It's meant to crash because I guess activation is supposed to be one of ACTIVATION_SIGMOID, ACTIVATION_TANH and ACTIVATION_RELU. Maybe there is a bug in the code where the else arm is reached, this needs more investigation.

M_PI is missing by default on Windows.

Solution is here: https://stackoverflow.com/a/26065595

The Windows linker complains about symbols being defined multiple times

Yeah I guess it's because both create celt functions. Not sure what a good solution is.

jneem commented 4 years ago

I had a look at the VLA business. It seems that celt_fir and celt_iir are unused, so they can just be deleted (probably that file was just copied over from opus). The other VLA is in _celt_autocorr, and it is only ever called with the constant value PITCH_BUF_SIZE.

So I can easily prepare a patch removing these VLAs; the question is what to do with it, given that the original project appears unresponsive. Would you consider vendoring rnnoise here?

est31 commented 4 years ago

@jneem the maintainer has been pinged. Could you make a proper PR to https://github.com/xiph/rnnoise and if there is no reaction within a week, we can vendor it here or fork it for RustAudio.

jneem commented 4 years ago

I happened to swing by https://github.com/xiph/rnnoise today and noticed that there was some activity a couple of days ago. Still no response on this PR, though...

jneem commented 4 years ago

Just a heads-up that I decided to fork my own rust version of rnnoise here. I wouldn't necessarily recommend it for anyone else's use, but it is 100% safe rust and it appears to provide identical results to the original, up to some small numerical errors that were caused by switching to a different FFT implementation.

est31 commented 4 years ago

@jneem that's awesome and absolutely the best resolution of this! I wonder, given nnnoiseless, is there any use in this crate? If no, I think I'll deprecate it.

jneem commented 4 years ago

I can think of two reasons that people might prefer this crate, at least for now:

Edit: I've fixed the second problem: nnnoiseless is now about 10-20% faster than rnnoise on my machine. Can't do much about the first point, though.

est31 commented 4 years ago

About the first point, it's better if it's maintained at all than not maintained :).

As the second point seems resolved, I'll deprecate the Rust bindings.