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

[Jean-Marc Valin]

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?

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.

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

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

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?

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.

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.

8) I see there's a "gain" field in the matrix, but I can't see it used
anywhere. Did I miss something?

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;

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.

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

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


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
>