search

xiph / opus

Modern audio compression for the internet.
https://opus-codec.org/
Other
2.27k stars 604 forks source link

CMake project relies on available headers and compiler options for instruction set availability detection #198

Open davidebeatrici opened 4 years ago

davidebeatrici commented 4 years ago

https://github.com/xiph/opus/blob/034c1b61a250457649d788bbf983b3f0fb63f02e/cmake/OpusFunctions.cmake#L44-L132 https://github.com/xiph/opus/blob/b83dd52868326a401c8578041e3dbea439d53f11/CMakeLists.txt#L127-L206 https://github.com/xiph/opus/blob/b83dd52868326a401c8578041e3dbea439d53f11/CMakeLists.txt#L324-L450

I believe that, instead, it should check whether the instruction set is supported by the CPU.

xnorpx commented 4 years ago

There is for sure improvements that can be done here no doubt.

But it would be good to understand how it happened by providing the CMake options set and what OS?

xnorpx commented 4 years ago

Also what version of Opus (what is the latest commit)

ghost commented 4 years ago

Using default CMake configure options with Ninja generator (i.e. cmake -G "Ninja" "-DBUILD_SHARED_LIBS=ON") Then cmake --build . Windows 10 x64 2004 We have opus version 1.3.1 as a submodule.

xnorpx commented 4 years ago

@ZeroAbility

There was bugs in regarding to avx flags in the first iteration of cmake, there is fixes here: https://github.com/xiph/opus/commit/927de8453c502586c03e25c169ec08f2a93ebc02

Can you upgrade to latest Opus master and see if you can reproduce?

ghost commented 4 years ago

I just did a clone and build of master and the dll appears to work.

davidebeatrici commented 4 years ago

I just realized /arch:AVX is only specified when both AVX_SUPPORTED and OPUS_X86_PRESUME_AVX are true, sorry about that.

In 1.3.1 only AVX_SUPPORTED is taken into account: https://github.com/xiph/opus/blob/e85ed7726db5d677c9c0677298ea0cb9c65bdd23/opus_functions.cmake#L104-L185

I updated my initial message.

xnorpx commented 4 years ago

So the PRESUME was added here mostly for when the target machine was known to support the intrinsic version but is not the same as the buildmachine. Think separate buildsystem and deployed on Servers with known hardware. Or compiling for your own machine that you know support AVX.

MAY_HAVE define will use runtime check to ensure that CPU supports intrinsics.

xnorpx commented 4 years ago

@davidebeatrici just to clarify what do want added to CMake here?

davidebeatrici commented 4 years ago

I would use a COMPILER_ prefix for those definitions, to make it clear that the project only checks whether the compiler supports the flag/option.

xnorpx commented 4 years ago

opened https://gitlab.xiph.org/xiph/opus/-/issues/2340 to track this

davidebeatrici commented 4 years ago

Thank you very much!