-
Notifications
You must be signed in to change notification settings - Fork 618
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: Meson build fails with nnet_avx2.c compiled without AVX2 #322
Comments
@jmvalin I think you need an updated version of this: https://gitlab.xiph.org/xiph/opus/-/merge_requests/82 (I am a little overloaded at work but maybe the Meson folks can take a look at it. @tp-m ? ) |
@xnorpx So I had a look at your MR again and I think almost everything is already included except for this part: |
@jmvalin I take a look tonight and remind myself. |
@jmvalin the error is caused by nnet_avx2.c is missing build flag: "/arch:AVX2" so that needs to be added in some meson way. Then it looks like the new options is not printed out [ 'enable-deep-plc', 'ENABLE_DEEP_PLC' ], General configuration
Custom modes : NO
Assertions : NO
Hardening : YES
Fuzzing : NO
Check ASM : NO
API documentation : NO
Extra programs : YES
Tests : NO Finally it looks like dnn dir is added by default not sure if that what you wanted and what is expected? Is the expectation that [ 'enable-deep-plc', 'ENABLE_DEEP_PLC' ], should be off by default and are they independent options? |
Yeah, I forgot to print the options. The options are meant to be off by default, but not completely independent. Enabling dred or osce should automatically enable Deep PLC. BTW, I realized that I named the options enable-* when all the other options don't have "enable-" in the name. Think I should change that or will it break too many things at this point? |
I would remove enable now from the options. Better break now than never fix it.
Yes, you need to change it similar to my code change to add /arch:AVX2, you can see that I extended the list a couple of lines above to specify /arch:AVX2 there. |
I'm traveling this week, but have asked my colleagues to take a look. We've run into this on the GStreamer CI as well when trying to upgrade to the latest opus. |
Awesome thanks! |
Hi all, coming here from GStreamer. I've already found and fixed a similar issue in flac, I'll be glad to have a look into it. |
OK, I think I managed to extract the relevant part of the previous MR. Can you see if this fixes the problem: https://gitlab.xiph.org/xiph/opus/-/commit/601f2722795dcd24a3d3c838e97b8555f84fa85a |
@jmvalin It does fix the issue. Nits:
For the latter, would it be possible (after fixing this particular issue) to use |
Regarding the DNN stuff, it should work in all cases, but you need to run the autogen script (or at least the download_model line) to get the model files that aren't in git. Regarding the second issue, I'm not sure I understand but Opus should normally compile with any revision of the standard, including C89. |
@xnorpx Looking at meson_options.txt, I see there's "boolean" and "feature" options and I haven't quite figured out when one should be used over the other. Any thoughts there (wrt dred, deep-plc, osce)? |
I would say that you want to use feature options for ... "features", and not for flags When you want to do automatic detection of when to enable a feature, without the "automagic" problems autotools has. For example, something is platform-specific, or requires external deps, or is an experimental feature. With these, you set the default value of the feature to If you strongly recommend a feature but also want to give people the option to disable it, you can set the default value to For configuration options that do not fit any of these, for example things that aren't really a "feature" like picking using fixed point instead of floating-point, fuzzing support, etc, a boolean option is best. This is all preference, of course. Some people prefer using boolean everywhere except where it's convenient to use feature options: such as with external deps. The options should definitely not have an |
Is it only for VLA C99 is needed? |
VLAs are the only C99 features we're using and their use is optional since they're not mandatory in C99. Alternatives include alloca() (defining USE_ALLOCA), or a separate buffer (defining NONTHREADSAFE_PSEUDOSTACK) through that last one is not recommended. |
@xnorpx and others, can you have a look at https://gitlab.xiph.org/xiph/opus/-/merge_requests/115 and see if that makes sense? |
@jmvalin Looks good to me 👍 |
Should this be noted in the config as well, i.e. if you enable dred or osce in meson it should enable deep plc as well automagicatlly? (or did I not understand that correctly) other than that it lgtm. |
For those cases I usually prefer something like |
The idea here would be to automatically enable deep-plc when either dred or osce are enabled, not prevent the latter unless deep-plc is enabled. That's the autoconf behaviour. |
unrelated to this thread but I took a look at CMake and there is no option for DEEP_PLC. Should also write out the abbreviations for dred and osce in the options section. ...
* OPUS_CHECK_ASM, enable bit-exactness checks between optimized and c implementations.
* OPUS_DNN_FLOAT_DEBUG, Run DNN computations as float for debugging purposes.
* OPUS_DRED, enable DRED.
* OPUS_OSCE, enable OSCE.
* OPUS_FIXED_POINT_DEBUG, debug fixed-point implementation.
... |
Yeah, there's a whole bunch of CMake and meson stuff that needs improvement. I know very little about them so I just did the bare minimum to get things working. Improvements are definitely welcome. |
With opus 1.5.1, building with MSVC on Windows is failing with
nnet_avx2.c is being compiled without AVX2 enabled
. This is x86_64 architecture computer. The CMake build is working fine.The text was updated successfully, but these errors were encountered: