<div dir="ltr">Here's another one.<br><br><div class="gmail_quote"><div dir="ltr">On Thu, Nov 2, 2017 at 9:54 AM 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>
We're getting there... Some minor comments:<br>
<br>
1) The public header file should not have an<br>
#ifdef ENABLE_EXPERIMENTAL_AMBISONICS<br>
since that would require the user code to define it.<br>
<br></blockquote><div>Done </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
2) Why do you have #define MAPPING_MATRIX_C ?<br>
<br></blockquote><div>No idea. Fixed.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
3) Looks like MAPPING_MATRIX_MAX_SIZE is not longer useful, right?<br>
<br></blockquote><div>Yup. Removed.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
4) Even though it's not strictly necessary here, please add parentheses<br>
to the definition of MATRIX_INDEX() to avoid nasty surprises in the<br>
future (e.g. MATRIX_INDEX(...)*sizeof(foo) would be really bad).<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">
Cheers,<br>
<br>
Jean-Marc<br>
<br>
<br>
On 10/31/2017 04:10 PM, Drew Allen wrote:<br>
> Hi Jean-Marc,<br>
><br>
> Thanks so much for your review. Attached are my comments and an updated<br>
> patch.<br>
><br>
> On Mon, Oct 30, 2017 at 5:48 PM 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>
> 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>
><br>
> Done. Yes, this is a merge conflict. <br>
><br>
><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>
> Done.<br>
> <br>
><br>
> 3) opus_projection_ambisonics_encoder_init() (which should be<br>
> OPUS_EXPORT) is missing from opus_projection.h<br>
><br>
> Done.<br>
> <br>
><br>
> 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>
> Done.<br>
> <br>
><br>
> 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>
> Removed it. You're right, it's functionally impossible for the user to<br>
> create an invalid matrix with the current API.<br>
> <br>
><br>
> 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>
> Done.<br>
> <br>
><br>
> 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>
> Done. <br>
><br>
> 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>
><br>
> Gain should be pulled out by the user via<br>
> a OPUS_PROJECTION_GET_DEMIXING_MATRIX_GAIN call and add it to the<br>
> overall output gain. We assume the mixing matrix gain is always zero and<br>
> that this only matters for the output gain from the demixing matrix.<br>
><br>
><br>
> 9) In get_streams_from_channels(), I think it'd be simpler to just<br>
> replace:<br>
> *streams = channels / 2 + (channels % 2 == 1);<br>
> with:<br>
> *streams = (channels+1) / 2;<br>
><br>
> Done. <br>
><br>
><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>
><br>
> Done. <br>
><br>
><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>
><br>
> Done. Let me know if we should try designing something around this. <br>
><br>
><br>
> 12) I think opus_projection_decoder_ctl() is missing a va_end() call.<br>
><br>
><br>
> Done. <br>
><br>
><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<br>
> <<a href="mailto:tterribe@xiph.org" target="_blank">tterribe@xiph.org</a> <mailto:<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> <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<br>
> opus_demo).<br>
> > If you ultimately cast s to (opus_int16), then I'm pretty sure<br>
> most<br>
> > compilers will turn the second line into a no-op and optimize<br>
> 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>> <mailto:<a href="mailto:opus@xiph.org" target="_blank">opus@xiph.org</a><br>
> <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> <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>
</blockquote></div></div>