[opus] [PATCH] Support for Channel Mapping 253.

Tue Oct 31 20:10:46 UTC 2017

Hi Jean-Marc,

Thanks so much for your review. Attached are my comments and an updated
patch.

On Mon, Oct 30, 2017 at 5:48 PM Jean-Marc Valin <jmvalin at jmvalin.ca> wrote:

> Hi Drew,
>
> I've had some time to dig more deeply into your patch. Here's some more
> in-depth comments:
>
> 1) I note that your OpusMSEncoder struct in private.h adds a
> subframe_mem[] that's not in opus_multistream_encoder.c. I assume it's
> due to a merge problem (that field was removed some time ago), but can
> you confirm/fix the issue?
>
Done. Yes, this is a merge conflict.

>
> 2) I noticed your patch adds many OPUS_EXPORT declarations. OPUS_EXPORT
> is only meant for functions exposed to the API (and in the include/
> directory), but I see several of these in mapping_matrix.h, which I
> think is incorrect.
>
> Done.

> 3) opus_projection_ambisonics_encoder_init() (which should be
> OPUS_EXPORT) is missing from opus_projection.h
>
> Done.

> 4) I believe mapping_matrix_get_size() should be returning
> align(sizeof(MappingMatrix)) + rows * cols * sizeof(opus_int16)
> rather than:
> align(sizeof(MappingMatrix) + rows * cols * sizeof(opus_int16))
> to match what mapping_matrix_get_data() does
>
> Done.

> 5) I'm not sure I understand the purpose of mapping_matrix_validate().
> Can the user really cause the matrix not to be valid? If yes, then can't
> this cause problems even earlier in the code? If not, then maybe an
> assertion would be better. Also, is the size of the matrix the only
> thing you can validate?
>
> Removed it. You're right, it's functionally impossible for the user to
create an invalid matrix with the current API.

> 6) mapping_matrix_init() returns an error code, but the code that's
> calling it never checks it. Considering that it's an internal call, I
> would say it's probably better not to return anything, but instead to
> assert in the function.
>
> Done.

> 7) Same for mapping_matrix_multiply_short() and
> mapping_matrix_multiply_float(), the return value isn't checked. So it
> should either be replaced by an asssert or (if the user can cause a
> failure) actually checked.
>
> Done.

> 8) I see there's a "gain" field in the matrix, but I can't see it used
> anywhere. Did I miss something?
>
Gain should be pulled out by the user via
a  OPUS_PROJECTION_GET_DEMIXING_MATRIX_GAIN call and add it to the overall
output gain. We assume the mixing matrix gain is always zero and that this
only matters for the output gain from the demixing matrix.

>
> 9) In get_streams_from_channels(), I think it'd be simpler to just replace:
> *streams = channels / 2 + (channels % 2 == 1);
> with:
> *streams = (channels+1) / 2;
>
Done.

>
> 10) In opus_projection_ambisonics_encoder_init(), you to see if streams
> and coupled_streams are NULL, but unless I missed something I don't
> think there's any valid case where you wouldn't want to get those
> values. If you don't have them, then you have no way of knowing what
> mapping the encoder used, so no way of decoding. Instead, I would just
> return OPUS_BAD_ARG if they're NULL.
>
Done.

>
> 11) So one issue I just noticed is that opus_projection_encode() and
> opus_projection_encode_float() (same for the decoder) use arbitrarily
> large amounts of stack memory for "buf". In opus_multistream_encode()
> that's avoided by converting just two channels at a time, but here it's
> not quite clear how to do that without duplicating a lot of the
> multistream code. If we can't address the issue with this patch, the
> least would be to abort when trying to use these calls with
> NONTHREADSAFE_PSEUDOSTACK
>
Done. Let me know if we should try designing something around this.

>
> 12) I think opus_projection_decoder_ctl() is missing a va_end() call.
>

Done.

>
>
> Cheers,
>
>         Jean-Marc
>
>
>
> On 10/12/2017 05:44 PM, Drew Allen wrote:
> > thanks for all your feedback. here's the revised patch:
> >
> > On Wed, Oct 11, 2017 at 2:20 PM Timothy B. Terriberry <tterribe at xiph.org
> > <mailto:tterribe at xiph.org>> wrote:
> >
> >     Jean-Marc Valin wrote:
> >     > I think you'll want something like:
> >     > (opus_int16)((unsigned)demixing_matrix[2*i+1] << 8)
> >     > (though you might want to check it too)
> >
> >     FWIW, we use the construct
> >        int s = buf[2*i + 1] << 8 | buf[2*i];
> >        s = ((s & 0xFFFF) ^ 0x8000) - 0x8000;
> >
> >     to manually sign-extend the result in opus_compare (and also
> opus_demo).
> >     If you ultimately cast s to (opus_int16), then I'm pretty sure most
> >     compilers will turn the second line into a no-op and optimize it
> away.
> >     _______________________________________________
> >     opus mailing list
> >     opus at xiph.org <mailto:opus at xiph.org>
> >     http://lists.xiph.org/mailman/listinfo/opus
> >
> >
> >
> > _______________________________________________
> > 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/20171031/857c4d93/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Support-for-Channel-Mapping-253.patch
Type: text/x-patch
Size: 90378 bytes
Desc: not available
URL: <http://lists.xiph.org/pipermail/opus/attachments/20171031/857c4d93/attachment-0001.bin>