opus - Apr 2017 - [Patch] Non-diegetic support for channel mapping 254

Hi Jean-Marc et all,

Thanks for the quick feedback, responses to your questions are below:

I've attached a revised patch. PTAL, thanks!

On Fri, Apr 7, 2017 at 1:22 PM Jean-Marc Valin <jmvalin at jmvalin.ca>
wrote:

Hi Drew,

Thakns for the patch. Here's some comments for now (not done reviewing):

1) You want to use isqrt32() rather than celt_sqrt(), since celt_sqrt()
changes behaviour for fixed-point and provides no guarantee about rounding.


 Thanks! I'll make that change.

2) About these two lines:
+      if (nondiegetic_channels && 2 != nondiegetic_channels)
+         return OPUS_UNIMPLEMENTED;

Why not just have the condition be:
+      if (nondiegetic_channels != 2)


We should allow either 0 or 2 nondiegetic channels.


As a minor detail, I prefer N != 2 than 2 != N.


Acknowledged.


Also, unless you think it may one day make sense to have
nondiegetic_channels != 2, I think you should just return OPUS_BAD_ARG.
OPUS_UNIMPLEMENTED is used for things that may one day be implemented.


Done.


Also, many sure you catch all non-sensical values. For example, what if
channels is set to 1000?


Done.


3) From the draft, it seems like you can only have two nondiegetic
channels, so instead of:
+      nb_streams = acn_channels + ((nondiegetic_channels / 2) % 2);
+      nb_coupled_streams = nondiegetic_channels / 2;

Why not just have:
+      nb_streams = acn_channels + nondiegetic_channels;
+      nb_coupled_streams = nondiegetic_channels != 0;

Done


4) About this change:
-   if (!validate_layout(&st->layout) ||
!validate_encoder_layout(&st->layout))
+   if (!validate_layout(&st->layout) ||
+       (mapping_type == MAPPING_TYPE_SURROUND &&
+       !validate_encoder_layout(&st->layout)))

Is there nothing to validate for ambisonics?

We do this when determine the (n+1)^2 + 2j listed above, but we can
validate that here as well.


5) For opus_multistream_surround_encoder_init(), the same comments as
for opus_multistream_surround_encoder_get_size() maybe some code can be
shared (if possible)?

6) ambisonics_rate_allocation() again duplicates some of the code from
above. That should be avoided if possible.


Agreed, this is smelly. I will make a new function and call it from all
these places.

I haven't gone through the details of the rate allocation yet, but I
thought I'd give you these comments already.

Cheers,

        Jean-Marc


On 07/04/17 11:26 AM, Drew Allen wrote:
> Hello all,
>
> Attached is a proposed patch for Opus that allows support for encoding
> non-diegetic stereo audio as a coupled stream for use with channel
> mapping 254. It also rejects channel counts that are not (n+1)^2 + 2j,
> where n is 0 to 14 and j is either 0 or 1 (See IETF public draft doc
> attached for clarification).
>
> Please let me know any suggestions / concerns / comments.
>
> Thank you for your time,
>
> Cheers,
> Drew Allen (from the Chrome Media Audio team)
> IRC (#opus): llama_of_bits
>
>
> _______________________________________________
> opus mailing list
> opus at xiph.org
> http://lists.xiph.org/mailman/listinfo/opus
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.xiph.org/pipermail/opus/attachments/20170410/a050051d/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: nondiegetic_support_rev01.patch
Type: text/x-patch
Size: 5883 bytes
Desc: not available
URL:
<http://lists.xiph.org/pipermail/opus/attachments/20170410/a050051d/attachment.bin>

Hi Drew,

I think your revised patch looks good overall. Just two comments:

1) In opus_multistream_encoder_init_impl(), rather than use a huge
condition with two ORs for the return OPUS_BAD_ARG, maybe you can just
multiple three separate if()s, each with its own condition (one of which
will be entirely in an #ifdef). That should make the code easier to read.

2) In opus_multistream_surround_encoder_init(), I believe the mapping[]
array is set wrong. When you have "mapping[a] = b", then "a"
is the
channel position in the API, whereas "b" is the channel position in
the
file. Is it possible you got the order reversed?

As for the patch itself, can you use "git format-patch" so that it
includes all the information, including commit message and author.

Cheers,

	Jean-Marc

On 10/04/17 12:41 PM, Drew Allen wrote:
> Hi Jean-Marc et all,
> 
> Thanks for the quick feedback, responses to your questions are below:
> 
> I've attached a revised patch. PTAL, thanks!
> 
> On Fri, Apr 7, 2017 at 1:22 PM Jean-Marc Valin <jmvalin at jmvalin.ca
> <mailto:jmvalin at jmvalin.ca>> wrote:
> 
>     Hi Drew,
> 
>     Thakns for the patch. Here's some comments for now (not done
reviewing):
> 
>     1) You want to use isqrt32() rather than celt_sqrt(), since celt_sqrt()
>     changes behaviour for fixed-point and provides no guarantee about
>     rounding.
> 
> 
>  Thanks! I'll make that change.
> 
>     2) About these two lines:
>     +      if (nondiegetic_channels && 2 != nondiegetic_channels)
>     +         return OPUS_UNIMPLEMENTED;
> 
>     Why not just have the condition be:
>     +      if (nondiegetic_channels != 2)
> 
> 
> We should allow either 0 or 2 nondiegetic channels.
>  
> 
>     As a minor detail, I prefer N != 2 than 2 != N.
> 
> 
> Acknowledged.
>  
> 
>     Also, unless you think it may one day make sense to have
>     nondiegetic_channels != 2, I think you should just return OPUS_BAD_ARG.
>     OPUS_UNIMPLEMENTED is used for things that may one day be implemented.
> 
>  
> Done.
>  
> 
>     Also, many sure you catch all non-sensical values. For example, what if
>     channels is set to 1000?
> 
> 
> Done.
>  
> 
>     3) From the draft, it seems like you can only have two nondiegetic
>     channels, so instead of:
>     +      nb_streams = acn_channels + ((nondiegetic_channels / 2) % 2);
>     +      nb_coupled_streams = nondiegetic_channels / 2;
> 
>     Why not just have:
>     +      nb_streams = acn_channels + nondiegetic_channels;
>     +      nb_coupled_streams = nondiegetic_channels != 0;
> 
> Done
>  
> 
>     4) About this change:
>     -   if (!validate_layout(&st->layout) ||
>     !validate_encoder_layout(&st->layout))
>     +   if (!validate_layout(&st->layout) ||
>     +       (mapping_type == MAPPING_TYPE_SURROUND &&
>     +       !validate_encoder_layout(&st->layout)))
> 
>     Is there nothing to validate for ambisonics?
> 
> We do this when determine the (n+1)^2 + 2j listed above, but we can
> validate that here as well.
>  
> 
>     5) For opus_multistream_surround_encoder_init(), the same comments as
>     for opus_multistream_surround_encoder_get_size() maybe some code can be
>     shared (if possible)?
> 
>     6) ambisonics_rate_allocation() again duplicates some of the code from
>     above. That should be avoided if possible.
> 
> 
> Agreed, this is smelly. I will make a new function and call it from all
> these places.
> 
>     I haven't gone through the details of the rate allocation yet, but
I
>     thought I'd give you these comments already.
> 
>     Cheers,
> 
>             Jean-Marc
> 
> 
>     On 07/04/17 11:26 AM, Drew Allen wrote:
>     > Hello all,
>     >
>     > Attached is a proposed patch for Opus that allows support for
encoding
>     > non-diegetic stereo audio as a coupled stream for use with channel
>     > mapping 254. It also rejects channel counts that are not (n+1)^2 +
2j,
>     > where n is 0 to 14 and j is either 0 or 1 (See IETF public draft
doc
>     > attached for clarification).
>     >
>     > Please let me know any suggestions / concerns / comments.
>     >
>     > Thank you for your time,
>     >
>     > Cheers,
>     > Drew Allen (from the Chrome Media Audio team)
>     > IRC (#opus): llama_of_bits
>     >
>     >
>     > _______________________________________________
>     > opus mailing list
>     > opus at xiph.org <mailto:opus at xiph.org>
>     > http://lists.xiph.org/mailman/listinfo/opus
>     >
>

Hi Jean-Marc,

Thanks again for your comments.
Attached is a revised patch.

On Mon, Apr 24, 2017 at 1:08 PM Jean-Marc Valin <jmvalin at jmvalin.ca>
wrote:
> Hi Drew,
>
> I think your revised patch looks good overall. Just two comments:
>
> 1) In opus_multistream_encoder_init_impl(), rather than use a huge
> condition with two ORs for the return OPUS_BAD_ARG, maybe you can just
> multiple three separate if()s, each with its own condition (one of which
> will be entirely in an #ifdef). That should make the code easier to read.
>
Done.

> 2) In opus_multistream_surround_encoder_init(), I believe the mapping[]
> array is set wrong. When you have "mapping[a] = b", then
"a" is the
> channel position in the API, whereas "b" is the channel position
in the
> file. Is it possible you got the order reversed?
>
>
We assume that the input file is ordered first by ACN ambisonic channels
followed by a (possible) stereo track, and we want to swap the order for
the API in order to couple the stereo for coding. The mapping code appears
to be working on files I've tested it on so far.

> As for the patch itself, can you use "git format-patch" so that
it
> includes all the information, including commit message and author.
>
>
Done.

> Cheers,
>
>         Jean-Marc
>
> On 10/04/17 12:41 PM, Drew Allen wrote:
> > Hi Jean-Marc et all,
> >
> > Thanks for the quick feedback, responses to your questions are below:
> >
> > I've attached a revised patch. PTAL, thanks!
> >
> > On Fri, Apr 7, 2017 at 1:22 PM Jean-Marc Valin <jmvalin at
jmvalin.ca
> > <mailto:jmvalin at jmvalin.ca>> wrote:
> >
> >     Hi Drew,
> >
> >     Thakns for the patch. Here's some comments for now (not done
> reviewing):
> >
> >     1) You want to use isqrt32() rather than celt_sqrt(), since
> celt_sqrt()
> >     changes behaviour for fixed-point and provides no guarantee about
> >     rounding.
> >
> >
> >  Thanks! I'll make that change.
> >
> >     2) About these two lines:
> >     +      if (nondiegetic_channels && 2 !=
nondiegetic_channels)
> >     +         return OPUS_UNIMPLEMENTED;
> >
> >     Why not just have the condition be:
> >     +      if (nondiegetic_channels != 2)
> >
> >
> > We should allow either 0 or 2 nondiegetic channels.
> >
> >
> >     As a minor detail, I prefer N != 2 than 2 != N.
> >
> >
> > Acknowledged.
> >
> >
> >     Also, unless you think it may one day make sense to have
> >     nondiegetic_channels != 2, I think you should just return
> OPUS_BAD_ARG.
> >     OPUS_UNIMPLEMENTED is used for things that may one day be
> implemented.
> >
> >
> > Done.
> >
> >
> >     Also, many sure you catch all non-sensical values. For example,
what
> if
> >     channels is set to 1000?
> >
> >
> > Done.
> >
> >
> >     3) From the draft, it seems like you can only have two nondiegetic
> >     channels, so instead of:
> >     +      nb_streams = acn_channels + ((nondiegetic_channels / 2) %
2);
> >     +      nb_coupled_streams = nondiegetic_channels / 2;
> >
> >     Why not just have:
> >     +      nb_streams = acn_channels + nondiegetic_channels;
> >     +      nb_coupled_streams = nondiegetic_channels != 0;
> >
> > Done
> >
> >
> >     4) About this change:
> >     -   if (!validate_layout(&st->layout) ||
> >     !validate_encoder_layout(&st->layout))
> >     +   if (!validate_layout(&st->layout) ||
> >     +       (mapping_type == MAPPING_TYPE_SURROUND &&
> >     +       !validate_encoder_layout(&st->layout)))
> >
> >     Is there nothing to validate for ambisonics?
> >
> > We do this when determine the (n+1)^2 + 2j listed above, but we can
> > validate that here as well.
> >
> >
> >     5) For opus_multistream_surround_encoder_init(), the same comments
as
> >     for opus_multistream_surround_encoder_get_size() maybe some code
can
> be
> >     shared (if possible)?
> >
> >     6) ambisonics_rate_allocation() again duplicates some of the code
> from
> >     above. That should be avoided if possible.
> >
> >
> > Agreed, this is smelly. I will make a new function and call it from
all
> > these places.
> >
> >     I haven't gone through the details of the rate allocation yet,
but I
> >     thought I'd give you these comments already.
> >
> >     Cheers,
> >
> >             Jean-Marc
> >
> >
> >     On 07/04/17 11:26 AM, Drew Allen wrote:
> >     > Hello all,
> >     >
> >     > Attached is a proposed patch for Opus that allows support for
> encoding
> >     > non-diegetic stereo audio as a coupled stream for use with
channel
> >     > mapping 254. It also rejects channel counts that are not
(n+1)^2 +
> 2j,
> >     > where n is 0 to 14 and j is either 0 or 1 (See IETF public
draft
> doc
> >     > attached for clarification).
> >     >
> >     > Please let me know any suggestions / concerns / comments.
> >     >
> >     > Thank you for your time,
> >     >
> >     > Cheers,
> >     > Drew Allen (from the Chrome Media Audio team)
> >     > IRC (#opus): llama_of_bits
> >     >
> >     >
> >     > _______________________________________________
> >     > opus mailing list
> >     > opus at xiph.org <mailto:opus at xiph.org>
> >     > http://lists.xiph.org/mailman/listinfo/opus
> >     >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.xiph.org/pipermail/opus/attachments/20170425/ba97f67c/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Non-diegetic-support-for-Ambisonics-Mapping-254.patch
Type: text/x-patch
Size: 6342 bytes
Desc: not available
URL:
<http://lists.xiph.org/pipermail/opus/attachments/20170425/ba97f67c/attachment.bin>