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

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>

On 25/04/17 10:12 AM, Drew Allen wrote:
> 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.
Well, if you look at section 5.1.1 of RFC7845:

       The 'coupled stream
       count' field indicates that the decoders for the first M Opus
       streams are to be initialized for stereo (two-channel) output,
       and the remaining (N - M) decoders are to be initialized for mono
       (a single channel) only.

In other words, it says that in the file itself (regardless of what you
do in the API), you need to have the stereo channels first, followed by
the mono channels. Where the channels go in the API itself is another
question and you're free to put the non-diegetic stereo at the end for that.
> The mapping code
> appears to be working on files I've tested it on so far.
Well, even if you get the mapping wrong, it's still going to work. It's
just that your non-diegetic stream will be made from two mono streams
and two of your directional channels will be coupled. That's one of the
limitations of simple testing, since it's not going to catch these kinds
of errors.

	Jean-Marc

My apologies for the confusion, I think I have the mapping layout correct
in this patch.

Cheers!



On Tue, Apr 25, 2017 at 10:07 AM Jean-Marc Valin <jmvalin at jmvalin.ca>
wrote:
> On 25/04/17 10:12 AM, Drew Allen wrote:
> > 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.
>
> Well, if you look at section 5.1.1 of RFC7845:
>
>        The 'coupled stream
>        count' field indicates that the decoders for the first M Opus
>        streams are to be initialized for stereo (two-channel) output,
>        and the remaining (N - M) decoders are to be initialized for mono
>        (a single channel) only.
>
> In other words, it says that in the file itself (regardless of what you
> do in the API), you need to have the stereo channels first, followed by
> the mono channels. Where the channels go in the API itself is another
> question and you're free to put the non-diegetic stereo at the end for
> that.
>
> > The mapping code
> > appears to be working on files I've tested it on so far.
>
> Well, even if you get the mapping wrong, it's still going to work.
It's
> just that your non-diegetic stream will be made from two mono streams
> and two of your directional channels will be coupled. That's one of the
> limitations of simple testing, since it's not going to catch these
kinds
> of errors.
>
>         Jean-Marc
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.xiph.org/pipermail/opus/attachments/20170428/d22b9a54/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Non-diegetic-support-for-Ambisonics-Mapping-254.patch
Type: application/octet-stream
Size: 6323 bytes
Desc: not available
URL:
<http://lists.xiph.org/pipermail/opus/attachments/20170428/d22b9a54/attachment.obj>