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

Jean-Marc Valin jmvalin at jmvalin.ca
Wed Oct 11 20:44:12 UTC 2017


Hi Drew,

Thanks for addressing these issues. A few remaining things:

1) In this line:
buf[i] = (opus_int16)demixing_matrix[2*i] +
((opus_int16)demixing_matrix[2*i+1] << 8);

you might want to test it with undefined-behaviour sanitizer, but I
believe the second term will be undefined for negative values (the value
would be positive after the cast and only be negative because of the
shift).

I think you'll want something like:
(opus_int16)((unsigned)demixing_matrix[2*i+1] << 8)
(though you might want to check it too)

2) In mapping_matrix_multiply_short(), I would recommend something along
these lines (untested) to improve accuracy:

      opus_int32 tmp = 0;
      for (col = 0; col < input_rows; col++)
      {
        tmp +=
          (matrix_data[MATRIX_INDEX(matrix->rows, row, col)] *
          input[MATRIX_INDEX(input_rows, col, i)]) >> 8;
      }
      output[MATRIX_INDEX(output_rows, row, i)] = (tmp+64)>>7;

3) Looking at your get_*() function to compute offsets, I think you're
missing some align() calls in the terms. I think you could end up with
misaligned pointers, which is usually find on x86, but not on other
architectures (e.g. ARM).

Cheers,

	Jean-Marc


On 10/10/17 07:09 PM, Drew Allen wrote:
> 1. Fixed the va_end() thing. Thanks for the clarification.
> 2. I've redone the mapping matrix so it only either handles floats or
> int16 explicitly. please let me know if you'd like another
> functionality, but my thoughts on this were to keep the val16 handled
> entirely inside multistream API.
> 3. endianness fixed. thank you!
> 4. Removed those pointers, accidentally left them in but they were
> functionally useless.
> 
> Attached a new patch with those changes.
> 
> On Tue, Oct 10, 2017 at 12:19 PM Jean-Marc Valin <jmvalin at jmvalin.ca
> <mailto:jmvalin at jmvalin.ca>> wrote:
> 
>     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>
>     > <mailto: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>>
>     >     > <mailto: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> <mailto:opus at xiph.org
>     <mailto:opus at xiph.org>>
>     >     > http://lists.xiph.org/mailman/listinfo/opus
>     >     >
>     >
> 


More information about the opus mailing list