Comments (23)
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.
Related Issues (20)
- opus 1.5.1-1 seems to be incompatible with musescore 4.2.1 HOT 1
- Question about DRED HOT 4
- Undefined reference to opus_select_arch HOT 2
- opus_repacketizer_cat sometimes fails
- Limiting the stack allocation request HOT 2
- Please update opus-tools to support new features
- Deep PLC performance on Android HOT 16
- Compiling with CMake for Windows/MSVC only supports DLL runtime libraries HOT 9
- Added the OPUS_SET_INBAND_FEC(2) option HOT 1
- Please provide an option to not depend on downloading model data HOT 10
- v1.5.2 compile warnings
- Error: Range coder state mismatch between encoder and decoder in frame 1: 0x f3b1240 vs 0x40123b0f HOT 1
- Opus.lib: 1.3.1 version crashing in some windows machines in opus_encoder_create HOT 2
- Opus 1.5.x and main compilation failing with GCC 8.5.0 HOT 2
- Opus not building on Ubuntu 24.04
- No way to turn off the PLC/FEC?
- Floating Point libraries failing to build when targeting Fixed Point support only. MSVC 2017 v141 HOT 3
- Unexpected continuous noise when combining NoLACE with DTX HOT 9
- Bug: The decoded dred_offset < 0 HOT 1
- How can I verify if FEC is working? I am not hearing any changes with fec enabled vs disabled with 30% packet loss HOT 2
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from opus.