<div dir="ltr">Hi Jean-Marc,<div><br></div><div>Thanks so much for your review. Attached are my comments and an updated patch.<br><br><div class="gmail_quote"><div dir="ltr">On Mon, Oct 30, 2017 at 5:48 PM Jean-Marc Valin <<a href="mailto:jmvalin@jmvalin.ca">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>
I've had some time to dig more deeply into your patch. Here's some more<br>
in-depth comments:<br>
<br>
1) I note that your OpusMSEncoder struct in private.h adds a<br>
subframe_mem[] that's not in opus_multistream_encoder.c. I assume it's<br>
due to a merge problem (that field was removed some time ago), but can<br>
you confirm/fix the issue?<br></blockquote><div>Done. Yes, this is a merge conflict. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
2) I noticed your patch adds many OPUS_EXPORT declarations. OPUS_EXPORT<br>
is only meant for functions exposed to the API (and in the include/<br>
directory), but I see several of these in mapping_matrix.h, which I<br>
think is incorrect.<br>
<br></blockquote><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
3) opus_projection_ambisonics_encoder_init() (which should be<br>
OPUS_EXPORT) is missing from opus_projection.h<br>
<br></blockquote><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
4) I believe mapping_matrix_get_size() should be returning<br>
align(sizeof(MappingMatrix)) + rows * cols * sizeof(opus_int16)<br>
rather than:<br>
align(sizeof(MappingMatrix) + rows * cols * sizeof(opus_int16))<br>
to match what mapping_matrix_get_data() does<br>
<br></blockquote><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
5) I'm not sure I understand the purpose of mapping_matrix_validate().<br>
Can the user really cause the matrix not to be valid? If yes, then can't<br>
this cause problems even earlier in the code? If not, then maybe an<br>
assertion would be better. Also, is the size of the matrix the only<br>
thing you can validate?<br>
<br></blockquote><div>Removed it. You're right, it's functionally impossible for the user to create an invalid matrix with the current API.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
6) mapping_matrix_init() returns an error code, but the code that's<br>
calling it never checks it. Considering that it's an internal call, I<br>
would say it's probably better not to return anything, but instead to<br>
assert in the function.<br>
<br></blockquote><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
7) Same for mapping_matrix_multiply_short() and<br>
mapping_matrix_multiply_float(), the return value isn't checked. So it<br>
should either be replaced by an asssert or (if the user can cause a<br>
failure) actually checked.<br>
<br></blockquote><div>Done. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
8) I see there's a "gain" field in the matrix, but I can't see it used<br>
anywhere. Did I miss something?<br></blockquote><div>Gain should be pulled out by the user via a  OPUS_PROJECTION_GET_DEMIXING_MATRIX_GAIN call and add it to the overall output gain. We assume the mixing matrix gain is always zero and that this only matters for the output gain from the demixing matrix.</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
9) In get_streams_from_channels(), I think it'd be simpler to just replace:<br>
*streams = channels / 2 + (channels % 2 == 1);<br>
with:<br>
*streams = (channels+1) / 2;<br></blockquote><div>Done. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
10) In opus_projection_ambisonics_encoder_init(), you to see if streams<br>
and coupled_streams are NULL, but unless I missed something I don't<br>
think there's any valid case where you wouldn't want to get those<br>
values. If you don't have them, then you have no way of knowing what<br>
mapping the encoder used, so no way of decoding. Instead, I would just<br>
return OPUS_BAD_ARG if they're NULL.<br></blockquote><div>Done. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
11) So one issue I just noticed is that opus_projection_encode() and<br>
opus_projection_encode_float() (same for the decoder) use arbitrarily<br>
large amounts of stack memory for "buf". In opus_multistream_encode()<br>
that's avoided by converting just two channels at a time, but here it's<br>
not quite clear how to do that without duplicating a lot of the<br>
multistream code. If we can't address the issue with this patch, the<br>
least would be to abort when trying to use these calls with<br>
NONTHREADSAFE_PSEUDOSTACK<br></blockquote><div>Done. Let me know if we should try designing something around this. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
12) I think opus_projection_decoder_ctl() is missing a va_end() call.<br></blockquote><div><br></div><div>Done. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
Cheers,<br>
<br>
        Jean-Marc<br>
<br>
<br>
<br>
On 10/12/2017 05:44 PM, Drew Allen wrote:<br>
> thanks for all your feedback. here's the revised patch:<br>
><br>
> On Wed, Oct 11, 2017 at 2:20 PM Timothy B. Terriberry <<a href="mailto:tterribe@xiph.org" target="_blank">tterribe@xiph.org</a><br>
> <mailto:<a href="mailto:tterribe@xiph.org" target="_blank">tterribe@xiph.org</a>>> wrote:<br>
><br>
>     Jean-Marc Valin wrote:<br>
>     > I think you'll want something like:<br>
>     > (opus_int16)((unsigned)demixing_matrix[2*i+1] << 8)<br>
>     > (though you might want to check it too)<br>
><br>
>     FWIW, we use the construct<br>
>        int s = buf[2*i + 1] << 8 | buf[2*i];<br>
>        s = ((s & 0xFFFF) ^ 0x8000) - 0x8000;<br>
><br>
>     to manually sign-extend the result in opus_compare (and also opus_demo).<br>
>     If you ultimately cast s to (opus_int16), then I'm pretty sure most<br>
>     compilers will turn the second line into a no-op and optimize it away.<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>
><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>