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

[Drew Allen]

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>