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

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.xiph.org/pipermail/opus/attachments/20170407/49c09e69/attachment-0002.html>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.xiph.org/pipermail/opus/attachments/20170407/49c09e69/attachment-0003.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: non_diegetic_254.patch
Type: text/x-patch
Size: 6404 bytes
Desc: not available
URL:
<http://lists.xiph.org/pipermail/opus/attachments/20170407/49c09e69/attachment-0001.bin>

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.

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)

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

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.

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

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;

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?

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.

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
>

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>