opus - May 2016 - [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>

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
>

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