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