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

Windows and CMake support #88

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

almoghamdani
Copy link

@almoghamdani almoghamdani commented Jul 27, 2019

This PR includes the following changes:

  • Removed the use of VLA in order to support MSVC that doesn't have implementation for VLA and fixed more issues that occurred during the compile on Windows.
  • Add CMakeLists.txt to support compile using CMake.
  • Add an option to compile without opus functions if linking against Opus

@almoghamdani almoghamdani changed the title Windows, CMake, C++ support Windows, CMake and C++ support Jul 27, 2019
@davidebeatrici
Copy link

Regarding eb45832: I would do something like mumble-voip/rnnoise@b30f2bb, instead.

@witaly-iwanow
Copy link

Very welcome change! Autotools, VLAs and overlap with Opus source code are really annoying. The code does high quality denoising, but once you start deploying it on different platforms you realize it needs a lot of TLC around compilation/build process

@j-schultz j-schultz mentioned this pull request Sep 10, 2020
@tahnik
Copy link

tahnik commented Nov 8, 2020

Is there a reason why this was never merged?

@petterreinholdtsen
Copy link
Contributor

I'm not heavily involved in the development, but took the chance to merge the C++ header fix in #131, causing this change to no longer merge cleanly. Perhaps you can rebase it without the header change?

@almoghamdani
Copy link
Author

@petterreinholdtsen I rebased and removed the header changes.

@almoghamdani almoghamdani changed the title Windows, CMake and C++ support Windows and CMake support Jan 18, 2021
@j-schultz
Copy link
Contributor

@almogh52 maybe you also want to have a look at my changes based on your branch which can be found here: alfatraining@7d6f71f

These changes are mostly carried over from community-contributed CMake code for libspeex.

@petterreinholdtsen
Copy link
Contributor

This pull request also remove two symbols from the list of exported symbols in the library.

Anyway, I suspect the different parts of this pull request should be handled and evaluated separately. At least I believe the introduction of a parallel build system should be decided independently of moving several local variables to the heap. Perhaps better to make separate pull reqeusts for each set of changes?

@davidebeatrici
Copy link

Once #164 is merged you can drop the VLA-related changes and just add to the CMake project the following:

if(MSVC)
	target_compile_definitions(rnnoise PRIVATE "USE_MALLOC")
endif()

@wnpllrzodiac
Copy link

@almoghamdani
static win32 lib built.
But I failed to integrated the lib into a win32 console project.

LNK2019 unreferenced symbol _opus_fft_c, _forward_transform refered from testRNNoise xxxxxxxx\testRNNoise\rnnoise.lib(denoise.obj)

@RytoEX RytoEX mentioned this pull request Mar 27, 2024
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.

7 participants