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

[Drew Allen]

Hi Jean-Marc,

Thanks for the feedback. Attached are my comments and an updated patch.

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


*That was a typo. Fixed.*

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.

*My understanding is that it's impossible to pass ellipsis to another
function.*


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


*Not sure how that happened. Reverted.*

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


*Done.*

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

*I'm confused... How else would people be able to encode a packet of audio
using the projection encoder? *

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

*My mistake. Is there a good resource I can read about fixed point float
operations?*

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

*Done.*

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.

*Done. I've shifted >>4 to matrix_data and >>4 to input inside the loop and
>>= 7 to the output outside the loop. Let me know your thoughts.*

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);

*Can you show how to go from short -> char as well? I'm alittle confused on
how this can be done without knowing the endianness first.*

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.

*Done.*

On Wed, Oct 4, 2017 at 10:30 AM Jean-Marc Valin <jmvalin at jmvalin.ca> wrote:

> 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
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.xiph.org/pipermail/opus/attachments/20171010/4874c983/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Support-for-Channel-Mapping-253.patch
Type: text/x-patch
Size: 88130 bytes
Desc: not available
URL: <http://lists.xiph.org/pipermail/opus/attachments/20171010/4874c983/attachment-0001.bin>