<div dir="ltr">1. Fixed the va_end() thing. Thanks for the clarification.<div>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.</div><div>3. endianness fixed. thank you!</div><div>4. Removed those pointers, accidentally left them in but they were functionally useless.</div><div><br></div><div>Attached a new patch with those changes.</div><br><div class="gmail_quote"><div dir="ltr">On Tue, Oct 10, 2017 at 12:19 PM Jean-Marc Valin <<a href="mailto:jmvalin@jmvalin.ca" target="_blank">jmvalin@jmvalin.ca</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Drew,<br>
<br>
On 10/10/17 02:29 PM, Drew Allen wrote:<br>
> 2) Someone on this list might know better than I do on that one, but for<br>
> the new _ctl_va_list() calls, I believe the convention is for va_start()<br>
> and va_end() to appear in the caller rather than in in the va_list()<br>
> function itself.<br>
><br>
> *My understanding is that it's impossible to pass ellipsis to another<br>
> function.*<br>
<br>
Basically, what I mean is that I *think* you should remove the calls to<br>
va_end() from _ctl_va_list() calls and instead have the regular _ctl()<br>
functions call va_end(). That appears to be how functions like vprintf()<br>
are called.<br>
<br>
> 5) Do you need to expose the "generic" opus_projection_encode() and<br>
> opus_projection_encode_float() in the API?<br>
><br>
> *I'm confused... How else would people be able to encode a packet of<br>
> audio using the projection encoder? *<br>
<br>
Sorry, I got confused when reviewing. Forget about that point!<br>
<br>
> 6) In mapping_matrix_multiply_float(), for the !FIXED_POINT case you're<br>
> multiplying by (1.f / 32768.f), but I think in the FIXED_POINT case, you<br>
> should instead >>15<br>
><br>
> *My mistake. Is there a good resource I can read about fixed point float<br>
> operations?*<br>
<br>
I'm not aware of any unfortunately.<br>
<br>
> 8) For both matrix multiply functions, I'm a bit concerned with the<br>
> fixed-point accuracy. Maybe there's a way to keep the intermediate<br>
> results in 32 bits and rounding at the end of the sum. For example,<br>
> shifting only by 8 in the loop and doing a final shift by 7 outside the<br>
> loop.<br>
><br>
> *Done. I've shifted >>4 to matrix_data and >>4 to input inside the loop<br>
> and >>= 7 to the output outside the loop. Let me know your thoughts.*<br>
<br>
I'm not sure I understand what you mean here (and I can't see where that<br>
change is in the code).<br>
<br>
> 9) For endianness conversion, you shouldn't have to even detect it. I<br>
> think something like this (untested) should work for both little-endian<br>
> and big endian:<br>
> for (i=0;i<...;i++)<br>
> short_ptr[i] = char_ptr[2*i] + (char_ptr[2*i] << 8);<br>
><br>
> *Can you show how to go from short -> char as well? I'm alittle confused<br>
> on how this can be done without knowing the endianness first.*<br>
<br>
Well, the endianness of a machine is only visible when you attempt to<br>
cast multi-byte values to char pointers (input/output them). Unless you<br>
do that, you have no way to know how your "int" value is represented. In<br>
this case, we've decided that the byte ordering<br>
"on the wire" is little endian, so all you need is write out the least<br>
significant byte,then the most significant byte. If you cast to char,<br>
then you need the endianness, but if you do (my_short>>8), you always<br>
get the most significant byte. Similarly, (my_short&0xff) is always the<br>
least significant byte. No need for an endianness check.<br>
<br>
> 10) Please don't store pointers (MappingMatrix, OpusMSEncoder, ...) in<br>
> the OpusPEncoder/OpusPDecoder structs. All the data should be in a<br>
> single struct with no pointers (except to constant data) so that they<br>
> can be shallow-copied.<br>
><br>
> *Done.*<br>
<br>
Well, I still see pointers in your OpusProjectionDecoder.<br>
<br>
Cheers,<br>
<br>
Jean-Marc<br>
<br>
> On Wed, Oct 4, 2017 at 10:30 AM Jean-Marc Valin <<a href="mailto:jmvalin@jmvalin.ca" target="_blank">jmvalin@jmvalin.ca</a><br>
> <mailto:<a href="mailto:jmvalin@jmvalin.ca" target="_blank">jmvalin@jmvalin.ca</a>>> wrote:<br>
><br>
> Hi Drew,<br>
><br>
> Here's some comments on your patch (in no particular order):<br>
><br>
> 1) I see that it's adding an #include of stdarg.h to opus_multistream.h<br>
> Is that left over from the previous version?<br>
><br>
> 2) Someone on this list might know better than I do on that one, but for<br>
> the new _ctl_va_list() calls, I believe the convention is for va_start()<br>
> and va_end() to appear in the caller rather than in in the va_list()<br>
> function itself.<br>
><br>
> 3) I'm not sure I understand why the opus_copy_channel_out*() functions<br>
> had to be moved.<br>
><br>
> 4) OpusPEncoder: I'd prefer a more explicit name: OpusProjectionEncoder<br>
><br>
> 5) Do you need to expose the "generic" opus_projection_encode() and<br>
> opus_projection_encode_float() in the API?<br>
><br>
> 6) In mapping_matrix_multiply_float(), for the !FIXED_POINT case you're<br>
> multiplying by (1.f / 32768.f), but I think in the FIXED_POINT case, you<br>
> should instead >>15<br>
><br>
> 7) In mapping_matrix_multiply_short(), I would prefer >>15 instead of<br>
> /32768 even though most compilers are smart enough by now.<br>
><br>
> 8) For both matrix multiply functions, I'm a bit concerned with the<br>
> fixed-point accuracy. Maybe there's a way to keep the intermediate<br>
> results in 32 bits and rounding at the end of the sum. For example,<br>
> shifting only by 8 in the loop and doing a final shift by 7 outside the<br>
> loop.<br>
><br>
> 9) For endianness conversion, you shouldn't have to even detect it. I<br>
> think something like this (untested) should work for both little-endian<br>
> and big endian:<br>
> for (i=0;i<...;i++)<br>
> short_ptr[i] = char_ptr[2*i] + (char_ptr[2*i] << 8);<br>
><br>
> 10) Please don't store pointers (MappingMatrix, OpusMSEncoder, ...) in<br>
> the OpusPEncoder/OpusPDecoder structs. All the data should be in a<br>
> single struct with no pointers (except to constant data) so that they<br>
> can be shallow-copied.<br>
><br>
> Cheers,<br>
><br>
> Jean-Marc<br>
><br>
> On 27/09/17 03:19 PM, Drew Allen wrote:<br>
> > Hello all,<br>
> ><br>
> > Here is an updated patch for channel mapping 253 support using the<br>
> > proposed "Projection" API. Matrix operations support and tests are<br>
> > included. Looking forward to your feedback.<br>
> ><br>
> > Cheers,<br>
> > Drew<br>
> ><br>
> > On Tue, Sep 19, 2017 at 9:04 AM Drew Allen <<a href="mailto:bitllama@google.com" target="_blank">bitllama@google.com</a><br>
> <mailto:<a href="mailto:bitllama@google.com" target="_blank">bitllama@google.com</a>><br>
> > <mailto:<a href="mailto:bitllama@google.com" target="_blank">bitllama@google.com</a> <mailto:<a href="mailto:bitllama@google.com" target="_blank">bitllama@google.com</a>>>> wrote:<br>
> ><br>
> > Hello all,<br>
> ><br>
> > Attached is an up-to-date patch for supporting channel mapping<br>
> 253.<br>
> > Please advise and Thank you for your time.<br>
> ><br>
> > Cheers,<br>
> > Drew<br>
> ><br>
> ><br>
> ><br>
> ><br>
> > _______________________________________________<br>
> > opus mailing list<br>
> > <a href="mailto:opus@xiph.org" target="_blank">opus@xiph.org</a> <mailto:<a href="mailto:opus@xiph.org" target="_blank">opus@xiph.org</a>><br>
> > <a href="http://lists.xiph.org/mailman/listinfo/opus" rel="noreferrer" target="_blank">http://lists.xiph.org/mailman/listinfo/opus</a><br>
> ><br>
><br>
</blockquote></div></div>