Jean-Marc Valin
2016-May-24 01:31 UTC
[opus] [PATCH] Add Functions to Create Ambisonic Multistream Encoder
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 "allocation mode" should be renamed to "mapping type" or >> something similar that to make it less confusing. > Done > >> 2) After discussing with Tim, the conclusion is that we should use >> mapping family 254 in the short term. We can switch to mapping family 2 >> when standardization is a bit more advanced. > Sounds good, done. > >> 3) Unless it causes problems in the existing code, it seems like family >> 254 should already set mapping_type=MAPPING_TYPE_AMBISONICS. From my >> understanding, it would not change the code behaviour, but would make it >> clearer what the new type is for (or did I miss something?). > Done, I think. The patch still doesn't change behavior, but it now > sets the member OpusMSEncoder.mapping_type to MAPPING_TYPE_AMBISONICS. >
Michael Graczyk
2016-May-24 02:57 UTC
[opus] [PATCH] Add Functions to Create Ambisonic Multistream Encoder
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: <http://lists.xiph.org/pipermail/opus/attachments/20160523/6781000b/attachment.html>
Jean-Marc Valin
2016-May-24 04:28 UTC
[opus] [PATCH] Add Functions to Create Ambisonic Multistream Encoder
On 05/23/2016 10:57 PM, Michael Graczyk wrote:> 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.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
Ulrich Windl
2016-May-24 06:00 UTC
[opus] Antw: Re: [PATCH] Add Functions to Create Ambisonic Multistream Encoder
>>> Jean-Marc Valin <jmvalin at jmvalin.ca> schrieb am 24.05.2016 um 03:31 inNachricht <5743AEDF.3070702 at jmvalin.ca>:> 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.Hi! I think once you specify a binary format, the bits are important; if you only look at the C source you shouldn't care about the actual bits when using enums. Ulrich> > 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 "allocation mode" should be renamed to "mapping type" or >>> something similar that to make it less confusing. >> Done >> >>> 2) After discussing with Tim, the conclusion is that we should use >>> mapping family 254 in the short term. We can switch to mapping family 2 >>> when standardization is a bit more advanced. >> Sounds good, done. >> >>> 3) Unless it causes problems in the existing code, it seems like family >>> 254 should already set mapping_type=MAPPING_TYPE_AMBISONICS. From my >>> understanding, it would not change the code behaviour, but would make it >>> clearer what the new type is for (or did I miss something?). >> Done, I think. The patch still doesn't change behavior, but it now >> sets the member OpusMSEncoder.mapping_type to MAPPING_TYPE_AMBISONICS. >> > _______________________________________________ > opus mailing list > opus at xiph.org > http://lists.xiph.org/mailman/listinfo/opus
Seemingly Similar Threads
- [PATCH] Add Functions to Create Ambisonic Multistream Encoder
- [PATCH] Add Functions to Create Ambisonic Multistream Encoder
- [PATCH] Add Functions to Create Ambisonic Multistream Encoder
- [PATCH] Add Functions to Create Ambisonic Multistream Encoder
- [PATCH] Add Functions to Create Ambisonic Multistream Encoder