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

Here is the modified patch. I added a flag to configure.ac which is set to
0 to disable ambisonics, and 1 to enable it. Right now the implementation
simply creates a surround encoder with N uncoupled streams.


Thanks,
Michael Graczyk
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.xiph.org/pipermail/opus/attachments/20160506/5ba6a372/attachment.html>
-------------- next part --------------

Does anyone have more thoughts on this patch? I would like to get this in
so I can send more involved patches related to ambisonics.

Also, do you guys prefer to do code review using pull requests on github?
It seems that both are used for Opus.

On Fri, May 6, 2016 at 4:46 PM, Michael Graczyk <mgraczyk at google.com>
wrote:
> Here is the modified patch. I added a flag to configure.ac which is set
> to 0 to disable ambisonics, and 1 to enable it. Right now the
> implementation simply creates a surround encoder with N uncoupled streams.
>
>
> Thanks,
> Michael Graczyk
>


-- 

Thanks,
Michael Graczyk
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.xiph.org/pipermail/opus/attachments/20160516/06f8269b/attachment.html>

Hi Michael,

Email is fine. I've not yet had time to review your patch. It looked
about right, but I need to have a closer look.

Cheers,

	Jean-Marc

On 05/16/2016 05:06 AM, Michael Graczyk wrote:
> Does anyone have more thoughts on this patch? I would like to get this
> in so I can send more involved patches related to ambisonics.
> 
> Also, do you guys prefer to do code review using pull requests on
> github? It seems that both are used for Opus. 
> 
> On Fri, May 6, 2016 at 4:46 PM, Michael Graczyk <mgraczyk at google.com
> <mailto:mgraczyk at google.com>> wrote:
> 
>     Here is the modified patch. I added a flag to configure.ac
>     <http://configure.ac> which is set to 0 to disable ambisonics,
and 1
>     to enable it. Right now the implementation simply creates a surround
>     encoder with N uncoupled streams.
> 
> 
>     Thanks,
>     Michael Graczyk
> 
> 
> 
> 
> -- 
> 
> Thanks,
> Michael Graczyk

On 05/16/2016 05:06 AM, Michael Graczyk wrote:
> Does anyone have more thoughts on this patch? I would like to get this
> in so I can send more involved patches related to ambisonics.
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.
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. 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?
> Also, do you guys prefer to do code review using pull requests on
> github? It seems that both are used for Opus. 
I actually prefer patches by email on this list.

Cheers,

	Jean-Marc
> On Fri, May 6, 2016 at 4:46 PM, Michael Graczyk <mgraczyk at google.com
> <mailto:mgraczyk at google.com>> wrote:
> 
>     Here is the modified patch. I added a flag to configure.ac
>     <http://configure.ac> which is set to 0 to disable ambisonics,
and 1
>     to enable it. Right now the implementation simply creates a surround
>     encoder with N uncoupled streams.
> 
> 
>     Thanks,
>     Michael Graczyk
> 
> 
> 
> 
> -- 
> 
> Thanks,
> Michael Graczyk

On May 20, 2016 18:32, "Jean-Marc Valin" <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?
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.xiph.org/pipermail/opus/attachments/20160520/571e72e2/attachment.html>

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