Michael Graczyk
2016-May-24 05:14 UTC
[opus] [PATCH] Add Functions to Create Ambisonic Multistream Encoder
Sounds good, I'll make those changes. Thanks for pointing out the make differences. Do you mind if I add an #ifndef, #define to the top of the file for the experiment flag? The code becomes significantly more nasty with #ifdefs everywhere and it would only get worse in subsequent patches. On May 23, 2016 21:28, "Jean-Marc Valin" <jmvalin at jmvalin.ca> wrote:> 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 >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.xiph.org/pipermail/opus/attachments/20160524/f58bab89/attachment-0001.html>
Jean-Marc Valin
2016-May-24 15:26 UTC
[opus] [PATCH] Add Functions to Create Ambisonic Multistream Encoder
Hi Michael, On 05/24/2016 01:14 AM, Michael Graczyk wrote:> Thanks for pointing out the make differences. Do you mind if I add an > #ifndef, #define to the top of the file for the experiment flag? The > code becomes significantly more nasty with #ifdefs everywhere and it > would only get worse in subsequent patches.Unless it makes the code really bad, I'd rather use #ifdefs directly since we'll want to use it to disable blocks of code and mostly because everything else already uses that (it would be confusing). Thanks, Jean-Marc> On May 23, 2016 21:28, "Jean-Marc Valin" <jmvalin at jmvalin.ca > <mailto:jmvalin at jmvalin.ca>> wrote: > > 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 >
Michael Graczyk
2016-May-25 02:33 UTC
[opus] [PATCH] Add Functions to Create Ambisonic Multistream Encoder
Alright, I've made the changes to the enum and flag usage. I'm don't see the difference with respect to "disabling blocks of code", since the compiler should completely elide the blocks that are conditioned on a constant 0. If there were compiler errors in the experimental code there could be a difference, but I'm hoping that is not the case on any platform! Either way I changed them to #ifdef everywhere. On Tue, May 24, 2016 at 8:26 AM, Jean-Marc Valin <jmvalin at jmvalin.ca> wrote:> Hi Michael, > > On 05/24/2016 01:14 AM, Michael Graczyk wrote: > > Thanks for pointing out the make differences. Do you mind if I add an > > #ifndef, #define to the top of the file for the experiment flag? The > > code becomes significantly more nasty with #ifdefs everywhere and it > > would only get worse in subsequent patches. > > Unless it makes the code really bad, I'd rather use #ifdefs directly > since we'll want to use it to disable blocks of code and mostly because > everything else already uses that (it would be confusing). > > Thanks, > > Jean-Marc > > > > On May 23, 2016 21:28, "Jean-Marc Valin" <jmvalin at jmvalin.ca > > <mailto:jmvalin at jmvalin.ca>> wrote: > > > > 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 > > >-- Thanks, Michael Graczyk -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.xiph.org/pipermail/opus/attachments/20160524/a8328429/attachment.html> -------------- next part --------------
Reasonably Related 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