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
>