opus - May 2016 - [PATCH] Add Functions to Create Ambisonic Multistream Encoder

Hi Michael,

Sorry, I ended up looking at an older version and got confused. The new
one looks good and I only have a few nits:

1) I think "allocation mode" should be renamed to "mapping
type" or
something similar that to make it less confusing.

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.

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?).

Cheers,

	Jean-Marc

On 05/20/2016 07:36 PM, Michael Graczyk wrote:
> 
> On May 20, 2016 18:32, "Jean-Marc Valin" <jmvalin at
jmvalin.ca
> <mailto:jmvalin at jmvalin.ca>> wrote:
>>
>> Is it intended that this patch never sets ALLOCATION_MODE_AMBISONICS?
>> I'm having a hard time figuring out what it does in its current
state.
> That was intended. I wanted to "reserve" the number 2 in this
enum to
> make it clear that 2 would mean ambisonics in the future.
> 
>> Also, I think the ambisonics function should be completely disabled
>> (hidden behind EXPERIMENTAL_AMBISONICS/--enable-experimental-ambisonics
>> flags) until there's actually an approved spec to avoid problems in
the
>> future.
> I believe I did that already. It should be the case that without that
> flag, this patch is a noop. Did I miss something?
> 
>>As for the mapping family to use, I'm not yet sure whether it's
>> best to use 2 or 255 for now -- Tim, any opinion?
> Isn't 255 best reserved to mean "no mapping" as it does now?
>

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.
-------------- next part --------------

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