search for: enable_experimental_ambison

...> I'd rather have them be different, since they're not really the same > thing. The idea is that we may have more than one family map to the same > type (e.g. currently both families 0 and 255 map to TYPE_NONE). > > One last thing I just noticed, please don't rely on > ENABLE_EXPERIMENTAL_AMBISONICS actually being defined to zero when your > patch is disabled. Some people don't use the autotools build (e.g. they > use the plain Makefile or Visual Studio, or custom scripts), so your > patch would break their build. Instead, please rely on #ifdef's. > > Cheers, > &gt...

...nce they're not really the same > > thing. The idea is that we may have more than one family map to the > same > > type (e.g. currently both families 0 and 255 map to TYPE_NONE). > > > > One last thing I just noticed, please don't rely on > > ENABLE_EXPERIMENTAL_AMBISONICS actually being defined to zero when > your > > patch is disabled. Some people don't use the autotools build (e.g. > they > > use the plain Makefile or Visual Studio, or custom scripts), so your > > patch would break their build. Instead, please rely on #if...

Here's another one. On Thu, Nov 2, 2017 at 9:54 AM Jean-Marc Valin <jmvalin at jmvalin.ca> wrote: > Hi Drew, > > We're getting there... Some minor comments: > > 1) The public header file should not have an > #ifdef ENABLE_EXPERIMENTAL_AMBISONICS > since that would require the user code to define it. > > Done > 2) Why do you have #define MAPPING_MATRIX_C ? > > No idea. Fixed. > 3) Looks like MAPPING_MATRIX_MAX_SIZE is not longer useful, right? > > Yup. Removed. > 4) Even though it's not strictly nec...

Jean-Marc, > Is there any reason you can't just use the generic multi-stream API, > i.e. opus_multistream_encoder_init() and give it the mapping you need? I would like the encoder to make decisions based on the fact that the audio is Ambisonics. For example, the bitrate allocation should not be the same for every channel. The original multistream API does not provide a way to pass in

On 05/05/2016 06:29 PM, Michael Graczyk wrote: > Alright, that is simpler. I can modify the patch to simply add > "mapping_family == 2" to the allowed values in the surround encoder > functions. Sounds reasonable. I would also suggest putting ambisonics behind a #define ENABLE_EXPERIMENTAL_AMBISONICS (or similar) until the spec is final to avoid accidental deployment of non-compliant implementations. Cheers, Jean-Marc

...> explicit assignments. I'd rather have them be different, since they're not really the same thing. The idea is that we may have more than one family map to the same type (e.g. currently both families 0 and 255 map to TYPE_NONE). One last thing I just noticed, please don't rely on ENABLE_EXPERIMENTAL_AMBISONICS actually being defined to zero when your patch is disabled. Some people don't use the autotools build (e.g. they use the plain Makefile or Visual Studio, or custom scripts), so your patch would break their build. Instead, please rely on #ifdef's. Cheers, Jean-Marc

...er have them be different, since they're not really the same > thing. The idea is that we may have more than one family map to the same > type (e.g. currently both families 0 and 255 map to TYPE_NONE). > > One last thing I just noticed, please don't rely on > ENABLE_EXPERIMENTAL_AMBISONICS actually being defined to zero when your > patch is disabled. Some people don't use the autotools build (e.g. they > use the plain Makefile or Visual Studio, or custom scripts), so your > patch would break their build. Instead, please rely on #ifdef's. > >...

This fixes the build error here: https://mf4.xiph.org/jenkins/job/opus-pedantic-makefile/886/

Since they correspond to mapping family values, I figured it would be wise to make them match so that the correspondence would be clear. If you would rather that correspondence not be explicit I will remove the explicit assignments. -------------- next part -------------- An HTML attachment was scrubbed... URL:

Hi Jean-Marc, Thanks so much for your review. Attached are my comments and an updated patch. On Mon, Oct 30, 2017 at 5:48 PM Jean-Marc Valin <jmvalin at jmvalin.ca> wrote: > Hi Drew, > > I've had some time to dig more deeply into your patch. Here's some more > in-depth comments: > > 1) I note that your OpusMSEncoder struct in private.h adds a > subframe_mem[]

Hi Michael, Any particular reason you assign explicit values to your MappingType enum? If not, I'd rather not depend on the int values. Other than that, I think your patch is good to go. Cheers, Jean-Marc On 05/23/2016 01:27 PM, Michael Graczyk wrote: > Hi Jean-Marc, > > On Sat, May 21, 2016 at 1:58 PM, Jean-Marc Valin <jmvalin at jmvalin.ca> wrote: >> 1) I think

Hi Drew, We're getting there... Some minor comments: 1) The public header file should not have an #ifdef ENABLE_EXPERIMENTAL_AMBISONICS since that would require the user code to define it. 2) Why do you have #define MAPPING_MATRIX_C ? 3) Looks like MAPPING_MATRIX_MAX_SIZE is not longer useful, right? 4) Even though it's not strictly necessary here, please add parentheses to the definition of MATRIX_INDEX() to avoid nasty...

...;mailto:jmvalin at jmvalin.ca>>> wrote: > > > >     Hi Drew, > > > >     We're getting there... Some minor comments: > > > >     1) The public header file should not have an > >     #ifdef ENABLE_EXPERIMENTAL_AMBISONICS > >     since that would require the user code to define it. > > > > Done  > > > >     2) Why do you have #define MAPPING_MATRIX_C ? > > > > No idea. Fixed. > >   > > &gt...

...i Drew, > > > > > > > > We're getting there... Some minor comments: > > > > > > > > 1) The public header file should not have an > > > > #ifdef ENABLE_EXPERIMENTAL_AMBISONICS > > > > since that would require the user code to define > it. > > > > > > > > Done > > > > > > > > 2) Why do you have #define MAPPING_MATRIX_C ?...

...>         >     Hi Drew, > >         > > >         >     We're getting there... Some minor comments: > >         > > >         >     1) The public header file should not have an > >         >     #ifdef ENABLE_EXPERIMENTAL_AMBISONICS > >         >     since that would require the user code to define it. > >         > > >         > Done  > >         > > >         >     2) Why do you have #define MAPPING_MATRIX_C ? > >         &g...

...; > > >> > > > We're getting there... Some minor comments: >> > > > >> > > > 1) The public header file should not have an >> > > > #ifdef ENABLE_EXPERIMENTAL_AMBISONICS >> > > > since that would require the user code to >> define it. >> > > > >> > > > Done >> > > > >> > > > 2) Why do you have #d...

...gt;         >         >     We're getting there... Some minor > comments: > >         >         > > >         >         >     1) The public header file should not > have an > >         >         >     #ifdef ENABLE_EXPERIMENTAL_AMBISONICS > >         >         >     since that would require the user code > to define it. > >         >         > > >         >         > Done  > >         >         > > >         >         > ...