Windows: Meson build fails with nnet_avx2.c compiled without AVX2 about opus HOT 23 OPEN

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.

from opus.

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 auto and if the requirements aren't met, the build will just disable that feature and continue.

If you strongly recommend a feature but also want to give people the option to disable it, you can set the default value to enabled.

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 enable- prefix though.

from opus.

@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 ? )

from opus.

@xnorpx So I had a look at your MR again and I think almost everything is already included except for this part:
https://gitlab.xiph.org/xiph/opus/-/merge_requests/82/diffs#0cc1139e3347f573ae1feee5b73dbc8a8a21fcfa_442_446
Can you explain a bit what that does and whether it's related to the current issue?

from opus.

@jmvalin I take a look tonight and remind myself.

from opus.

@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' ],
[ 'enable-dred', 'ENABLE_DRED' ],
[ 'enable-osce', 'ENABLE_OSCE' ],

  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' ],
[ 'enable-dred', 'ENABLE_DRED' ],
[ 'enable-osce', 'ENABLE_OSCE' ],

should be off by default and are they independent options?

from opus.

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?

from opus.

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.

@xnorpx So I had a look at your MR again and I think almost everything is already included except for this part: https://gitlab.xiph.org/xiph/opus/-/merge_requests/82/diffs#0cc1139e3347f573ae1feee5b73dbc8a8a21fcfa_442_446 Can you explain a bit what that does and whether it's related to the current issue?

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.

from opus.

(I am a little overloaded at work but maybe the Meson folks can take a look at it. @tp-m ? )

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.

from opus.

(I am a little overloaded at work but maybe the Meson folks can take a look at it. @tp-m ? )

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!

from opus.

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

from opus.

@jmvalin It does fix the issue. Nits:

• I had to disable that new DNN component altogether, I guess it's designed to be used from the tarball only?
• Opus is also falling into the Meson trap of c_std=gnu99 under MSVC, which triggers the standards-non-compliant C89 mode (mesonbuild/meson#11641)

For the latter, would it be possible (after fixing this particular issue) to use c_std=c11 and use the _GNU_SOURCE etc. macros, as I did here: https://gitlab.freedesktop.org/gstreamer/meson-ports/x264/-/commit/41af23e8743913ac8cdf1f6d37e5e391ea79a12d ?

from opus.

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.

from opus.

@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)?

from opus.

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.

Is it only for VLA C99 is needed?

from opus.

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.

from opus.

@xnorpx and others, can you have a look at https://gitlab.xiph.org/xiph/opus/-/merge_requests/115 and see if that makes sense?

from opus.

@jmvalin Looks good to me 👍

from opus.

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.

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.

from opus.

Enabling dred or osce should automatically enable Deep PLC.

For those cases I usually prefer something like get_option('deep-plc').enable_auto_if(<check if the other two are enabled>).require(dred or osce, error_message: 'Deep PLC must not be disabled if DRED or OSCE are enabled') which is a bit verboseful, but conveys the chain of requirements.

from opus.

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.

from opus.

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.
 ...

from opus.

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.

from opus.