[opus] [PATCH] Support for Channel Mapping 253.
Jean-Marc Valin
jmvalin at jmvalin.ca
Tue Oct 10 19:19:11 UTC 2017
Hi Drew,
On 10/10/17 02:29 PM, Drew Allen wrote:
> 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.*
Basically, what I mean is that I *think* you should remove the calls to
va_end() from _ctl_va_list() calls and instead have the regular _ctl()
functions call va_end(). That appears to be how functions like vprintf()
are called.
> 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? *
Sorry, I got confused when reviewing. Forget about that point!
> 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?*
I'm not aware of any unfortunately.
> 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.*
I'm not sure I understand what you mean here (and I can't see where that
change is in the code).
> 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.*
Well, the endianness of a machine is only visible when you attempt to
cast multi-byte values to char pointers (input/output them). Unless you
do that, you have no way to know how your "int" value is represented. In
this case, we've decided that the byte ordering
"on the wire" is little endian, so all you need is write out the least
significant byte,then the most significant byte. If you cast to char,
then you need the endianness, but if you do (my_short>>8), you always
get the most significant byte. Similarly, (my_short&0xff) is always the
least significant byte. No need for an endianness check.
> 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.*
Well, I still see pointers in your OpusProjectionDecoder.
Cheers,
Jean-Marc
> On Wed, Oct 4, 2017 at 10:30 AM Jean-Marc Valin <jmvalin at jmvalin.ca
> <mailto: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>
> > <mailto: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 <mailto:opus at xiph.org>
> > http://lists.xiph.org/mailman/listinfo/opus
> >
>
More information about the opus
mailing list