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 >
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>
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 > > >
Ulrich Windl
2017-Oct-16 10:54 UTC
[opus] Antw: Re: [PATCH] Support for Channel Mapping 253.
>>> Drew Allen <bitllama at google.com> schrieb am 10.10.2017 um 20:29 in Nachricht<CABQ9DctQ0+gBgUif7BBJpjjKR7_V_H5OC1JM47w50oaaLXL4Tg at mail.gmail.com>:> 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.*>From "man stdarg.h": " The object ap may be passed as an argument to another function; (...)". See also "va_copy()".[...] Regards, Ulrich