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

[Jean-Marc Valin]

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