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

handle non-vla compilers #219

Closed
wants to merge 1 commit into from
Closed

handle non-vla compilers #219

wants to merge 1 commit into from

Conversation

marler8997
Copy link

@marler8997 marler8997 commented Nov 22, 2023

Leverages the C std version and the standard __STDC_NO_VLA__ macro to detect whether VLA is supported, and if not falls back to using alloca.

This is needed to support the MSVC compiler. With this (and defining M_PI) I was able to compile rnnoise with MSVC.

Leverages the C std version and the standard __STDC_NO_VLA__ macro
to detect whether VLA is supported, and if not falls back to using alloca.
@jmvalin
Copy link
Member

jmvalin commented Mar 26, 2024

Best solution in this case is to just use fixed sizes. See the upgrade1 branch.

@marler8997
Copy link
Author

Having troubles building the upgrade1 branch. Did you forget to commit src/rnnoise_data.h?

@jmvalin
Copy link
Member

jmvalin commented Mar 27, 2024

rnnoise_data.[ch] are supposed to be automatically downloaded when you run autogen.sh.

@marler8997
Copy link
Author

I'm building on Windows so no autogen.sh, but I was able to manually download and extract them and got further. Looks like are still some type definitions missing, after adding this to rnn_data.h I was able to build:

typedef int rnn_weight;
typedef struct {
    const rnn_weight *bias;
    const rnn_weight *input_weights;
    int nb_inputs;
    int nb_neurons;
    float activation;
} DenseLayer;
typedef struct {
    const rnn_weight *bias;
    const rnn_weight *input_weights;
    const rnn_weight *recurrent_weights;
    int nb_inputs;
    int nb_neurons;
    float activation;
} GRULayer;

This was what I inferred from how these types were being used, but they could be wrong (i.e. I could have nb_inputs and nb_neorons swapped. Do you know why I'm missing these types? Also is upgrade1 expected to be merged to master or is it a permanent branch that we should start releasing from?

@jmvalin
Copy link
Member

jmvalin commented Mar 27, 2024

Actually, rnn_data.[ch] are no longer useful at all. See the Makefile.am file for the new list of files in the build.

@jmvalin
Copy link
Member

jmvalin commented Mar 27, 2024

...and I just pushed some cleanup changes to remove (hopefully) all useless files

@marler8997
Copy link
Author

Cool thanks for the fixes and the help. Looking forward to integrating the recent changes into our project. Would love official support for windows, there's a few PRs around this (one of the CMake PRs would do).

@marler8997 marler8997 closed this Mar 27, 2024
@marler8997 marler8997 deleted the novla branch March 27, 2024 04:14
@RytoEX
Copy link

RytoEX commented Mar 27, 2024

Cool thanks for the fixes and the help. Looking forward to integrating the recent changes into our project. Would love official support for windows, there's a few PRs around this (one of the CMake PRs would do).

Echoing this sentiment for CMake support. We at OBS Studio have been building RNNoise on Windows with #88 for years now. If updates are coming to the master branch, that PR would either need updated or replaced. I haven't looked at #188 to see if that's comparable or if it suits our needs.

@jmvalin
Copy link
Member

jmvalin commented Mar 28, 2024

Well, I suggest you give the upgrade1 a try. It's still experimental but it should already have better quality than master. It uses a larger model (hence the download), but if you compile with with AVX2 support (or even just with SSE 4.1) you shouldn't really see a slowdown compared to master. As for CMake support, I don't have time to maintain that, but if someone is willing to spend the time keeping it up-to-date, I'm not against. Could work similarly to what's in Opus where the list of files are kept separately from the autotools/cmake/meson build systems.

Hartmnt pushed a commit to mumble-voip/ReNameNoise that referenced this pull request Apr 3, 2024
Leverages the C std version and the standard __STDC_NO_VLA__ macro
to detect whether VLA is supported, and if not falls back to using alloca.

Original merge request: xiph/rnnoise#219
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants