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

[Jean-Marc Valin]

Hi Drew,

Here's some comments on your patch (in no particular order):

1) I see that it's adding an #include of stdarg.h to opus_multistream.h
Is that left over from the previous version?

2) Someone on this list might know better than I do on that one, but for
the new _ctl_va_list() calls, I believe the convention is for va_start()
and va_end() to appear in the caller rather than in in the va_list()
function itself.

3) I'm not sure I understand why the opus_copy_channel_out*() functions
had to be moved.

4) OpusPEncoder: I'd prefer a more explicit name: OpusProjectionEncoder

5) Do you need to expose the "generic" opus_projection_encode() and
opus_projection_encode_float() in the API?

6) In mapping_matrix_multiply_float(), for the !FIXED_POINT case you're
multiplying by (1.f / 32768.f), but I think in the FIXED_POINT case, you
should instead >>15

7) In mapping_matrix_multiply_short(), I would prefer >>15 instead of
/32768 even though most compilers are smart enough by now.

8) For both matrix multiply functions, I'm a bit concerned with the
fixed-point accuracy. Maybe there's a way to keep the intermediate
results in 32 bits and rounding at the end of the sum. For example,
shifting only by 8 in the loop and doing a final shift by 7 outside the
loop.

9) For endianness conversion, you shouldn't have to even detect it. I
think something like this (untested) should work for both little-endian
and big endian:
for (i=0;i<...;i++)
   short_ptr[i] = char_ptr[2*i] + (char_ptr[2*i] << 8);

10) Please don't store pointers (MappingMatrix, OpusMSEncoder, ...) in
the OpusPEncoder/OpusPDecoder structs. All the data should be in a
single struct with no pointers (except to constant data) so that they
can be shallow-copied.

Cheers,

	Jean-Marc

On 27/09/17 03:19 PM, Drew Allen wrote:
> Hello all,
> 
> Here is an updated patch for channel mapping 253 support using the
> proposed "Projection" API. Matrix operations support and tests are
> included. Looking forward to your feedback.
> 
> Cheers,
> Drew
> 
> On Tue, Sep 19, 2017 at 9:04 AM Drew Allen <bitllama at google.com
> <mailto:bitllama at google.com>> wrote:
> 
>     Hello all,
> 
>     Attached is an up-to-date patch for supporting channel mapping 253.
>     Please advise and Thank you for your time. 
> 
>     Cheers,
>     Drew
> 
> 
> 
> 
> _______________________________________________
> opus mailing list
> opus at xiph.org
> http://lists.xiph.org/mailman/listinfo/opus
>