<div dir="ltr"><div><span style="color:rgb(33,33,33)">Hi Jean-Marc,</span></div><div><span style="color:rgb(33,33,33)"><br></span></div><div><span style="color:rgb(33,33,33)">Thanks for the feedback. Attached are my comments and an updated patch.</span></div><div><span style="color:rgb(33,33,33)"><br></span></div><span style="color:rgb(33,33,33)">1) I see that it's adding an #include of stdarg.h to opus_multistream.h</span><br style="color:rgb(33,33,33)"><span style="color:rgb(33,33,33)">Is that left over from the previous version?</span><br style="color:rgb(33,33,33)"><br><div dir="ltr"><div dir="ltr"><div dir="ltr"><b>That was a typo. Fixed.<br></b></div></div></div><div dir="ltr"><div dir="ltr"><div dir="ltr"><br style="color:rgb(33,33,33)"><span style="color:rgb(33,33,33)">2) Someone on this list might know better than I do on that one, but for</span><br style="color:rgb(33,33,33)"><span style="color:rgb(33,33,33)">the new _ctl_va_list() calls, I believe the convention is for va_start()</span><br style="color:rgb(33,33,33)"><span style="color:rgb(33,33,33)">and va_end() to appear in the caller rather than in in the va_list()</span><br style="color:rgb(33,33,33)"><span style="color:rgb(33,33,33)">function itself.</span><br style="color:rgb(33,33,33)"><br></div></div></div><div dir="ltr"><div dir="ltr"><div dir="ltr"><b>My understanding is that it's impossible to pass ellipsis to another function.</b></div></div></div><div dir="ltr"><div dir="ltr"><div dir="ltr"><br><br style="color:rgb(33,33,33)"><span style="color:rgb(33,33,33)">3) I'm not sure I understand why the opus_copy_channel_out*() functions</span><br style="color:rgb(33,33,33)"><span style="color:rgb(33,33,33)">had to be moved.</span><br style="color:rgb(33,33,33)"><br></div></div></div><div dir="ltr"><div dir="ltr"><div dir="ltr"><b>Not sure how that happened. Reverted.<br></b></div></div></div><div dir="ltr"><div dir="ltr"><div dir="ltr"><br style="color:rgb(33,33,33)"><span style="color:rgb(33,33,33)">4) OpusPEncoder: I'd prefer a more explicit name: OpusProjectionEncoder</span><br style="color:rgb(33,33,33)"><br></div></div></div><div dir="ltr"><div dir="ltr"><div dir="ltr"><b>Done.<br></b></div></div></div><div dir="ltr"><div dir="ltr"><div dir="ltr"><br style="color:rgb(33,33,33)"><span style="color:rgb(33,33,33)">5) Do you need to expose the "generic" opus_projection_encode() and</span><br style="color:rgb(33,33,33)"><span style="color:rgb(33,33,33)">opus_projection_encode_float() in the API?</span><br style="color:rgb(33,33,33)"><br></div></div></div><div dir="ltr"><div dir="ltr"><div dir="ltr"><b>I'm confused... How else would people be able to encode a packet of audio using the projection encoder? </b><div></div></div></div></div><div dir="ltr"><div dir="ltr"><div dir="ltr"><div><br style="color:rgb(33,33,33)"><span style="color:rgb(33,33,33)">6) In mapping_matrix_multiply_float(</span><span style="color:rgb(33,33,33)">), for the !FIXED_POINT case you're</span><br style="color:rgb(33,33,33)"><span style="color:rgb(33,33,33)">multiplying by (1.f / 32768.f), but I think in the FIXED_POINT case, you</span><br style="color:rgb(33,33,33)"><span style="color:rgb(33,33,33)">should instead >>15</span><br style="color:rgb(33,33,33)"><br></div></div></div></div><div dir="ltr"><div dir="ltr"><div dir="ltr"><div><b>My mistake. Is there a good resource I can read about fixed point float operations?</b></div></div></div></div><div dir="ltr"><div dir="ltr"><div dir="ltr"><div><br style="color:rgb(33,33,33)"><span style="color:rgb(33,33,33)">7) In mapping_matrix_multiply_short(</span><span style="color:rgb(33,33,33)">), I would prefer >>15 instead of</span><br style="color:rgb(33,33,33)"><span style="color:rgb(33,33,33)">/32768 even though most compilers are smart enough by now.</span><br style="color:rgb(33,33,33)"><br></div></div></div></div><div dir="ltr"><div dir="ltr"><div dir="ltr"><div><b>Done.</b></div></div></div></div><div dir="ltr"><div dir="ltr"><div dir="ltr"><div><br style="color:rgb(33,33,33)"><span style="color:rgb(33,33,33)">8) For both matrix multiply functions, I'm a bit concerned with the</span><br style="color:rgb(33,33,33)"><span style="color:rgb(33,33,33)">fixed-point accuracy. Maybe there's a way to keep the intermediate</span><br style="color:rgb(33,33,33)"><span style="color:rgb(33,33,33)">results in 32 bits and rounding at the end of the sum. For example,</span><br style="color:rgb(33,33,33)"><span style="color:rgb(33,33,33)">shifting only by 8 in the loop and doing a final shift by 7 outside the</span><br style="color:rgb(33,33,33)"><span style="color:rgb(33,33,33)">loop.</span><br style="color:rgb(33,33,33)"><br></div></div></div></div><div dir="ltr"><div dir="ltr"><div dir="ltr"><div><b>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.</b></div></div><div dir="ltr"><div></div></div></div></div><div dir="ltr"><div dir="ltr"><div dir="ltr"><div><br style="color:rgb(33,33,33)"><span style="color:rgb(33,33,33)">9) For endianness conversion, you shouldn't have to even detect it. I</span><br style="color:rgb(33,33,33)"><span style="color:rgb(33,33,33)">think something like this (untested) should work for both little-endian</span><br style="color:rgb(33,33,33)"><span style="color:rgb(33,33,33)">and big endian:</span><br style="color:rgb(33,33,33)"><span style="color:rgb(33,33,33)">for (i=0;i<...;i++)</span><br style="color:rgb(33,33,33)"><span style="color:rgb(33,33,33)"> short_ptr[i] = char_ptr[2*i] + (char_ptr[2*i] << 8);</span><br style="color:rgb(33,33,33)"><br></div></div></div></div><div dir="ltr"><div dir="ltr"><div dir="ltr"><div><b>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.</b></div></div></div></div><div dir="ltr"><div dir="ltr"><div dir="ltr"><div><br style="color:rgb(33,33,33)"><span style="color:rgb(33,33,33)">10) Please don't store pointers (MappingMatrix, OpusMSEncoder, ...) in</span><br style="color:rgb(33,33,33)"><span style="color:rgb(33,33,33)">the OpusPEncoder/OpusPDecoder structs. All the data should be in a</span><br style="color:rgb(33,33,33)"><span style="color:rgb(33,33,33)">single struct with no pointers (except to constant data) so that they</span><br style="color:rgb(33,33,33)"><span style="color:rgb(33,33,33)">can be shallow-copied.</span><br></div></div></div></div><div dir="ltr"><div dir="ltr"><div dir="ltr"><div></div></div><div><br></div><div><b>Done.</b></div></div></div><div dir="ltr"><div dir="ltr"><div><br></div><div class="gmail_quote"><div dir="ltr">On Wed, Oct 4, 2017 at 10:30 AM 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>
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>>> wrote:<br>
><br>
> Hello all,<br>
><br>
> Attached is an up-to-date patch for supporting channel mapping 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><br>
> <a href="http://lists.xiph.org/mailman/listinfo/opus" rel="noreferrer" target="_blank">http://lists.xiph.org/mailman/listinfo/opus</a><br>
><br>
</blockquote></div></div></div></div>