<div><div dir="auto">Awesome!!!! Ill think about a strategy for that soon. Glad it could finally make it in.</div></div><div><br><div class="gmail_quote"><div>On Tue, Nov 7, 2017 at 3:01 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>
Thanks for the update. Your patch is now in master. Now, it would be<br>
good if you could think of a way to reduce the stack usage as we discussed.<br>
<br>
Cheers,<br>
<br>
Jean-Marc<br>
<br>
On 11/07/2017 04:28 PM, Drew Allen wrote:<br>
> Here's another patch. Cheers!<br>
><br>
> On Mon, Nov 6, 2017 at 10:08 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>
> Ok great. I'll make those changes you suggested and get back to you<br>
> this morning.<br>
><br>
> Cheers,<br>
> Drew<br>
><br>
><br>
><br>
> On 11/03/2017 02:51 PM, Drew Allen wrote:<br>
> > Here's another one.<br>
> ><br>
> > On Thu, Nov 2, 2017 at 9:54 AM Jean-Marc Valin <<a href="mailto:jmvalin@jmvalin.ca" target="_blank">jmvalin@jmvalin.ca</a> <mailto:<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> <mailto:<a href="mailto:jmvalin@jmvalin.ca" target="_blank">jmvalin@jmvalin.ca</a>>>> wrote:<br>
> ><br>
> > 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>
> > Done <br>
> ><br>
> > 2) Why do you have #define MAPPING_MATRIX_C ?<br>
> ><br>
> > No idea. Fixed.<br>
> > <br>
> ><br>
> > 3) Looks like MAPPING_MATRIX_MAX_SIZE is not longer useful, right?<br>
> ><br>
> > Yup. Removed.<br>
> > <br>
> ><br>
> > 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>
> > Done.<br>
> > <br>
> ><br>
> > 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<br>
> > updated<br>
> > > patch.<br>
> > ><br>
> > > On Mon, Oct 30, 2017 at 5:48 PM Jean-Marc Valin<br>
> > <<a href="mailto:jmvalin@jmvalin.ca" target="_blank">jmvalin@jmvalin.ca</a> <mailto:<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> <mailto:<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> <mailto:<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> <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<br>
> patch. Here's<br>
> > some more<br>
> > > in-depth comments:<br>
> > ><br>
> > > 1) I note that your OpusMSEncoder struct in<br>
> private.h adds a<br>
> > > subframe_mem[] that's not in<br>
> opus_multistream_encoder.c. I<br>
> > assume it's<br>
> > > due to a merge problem (that field was removed some<br>
> time ago),<br>
> > 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<br>
> declarations.<br>
> > OPUS_EXPORT<br>
> > > is only meant for functions exposed to the API (and<br>
> in the<br>
> > include/<br>
> > > directory), but I see several of these in<br>
> mapping_matrix.h,<br>
> > which I<br>
> > > think is incorrect.<br>
> > ><br>
> > > Done.<br>
> > > <br>
> > ><br>
> > > 3) opus_projection_ambisonics_encoder_init() (which<br>
> 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<br>
> returning<br>
> > > align(sizeof(MappingMatrix)) + rows * cols *<br>
> sizeof(opus_int16)<br>
> > > rather than:<br>
> > > align(sizeof(MappingMatrix) + rows * cols *<br>
> 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<br>
> > mapping_matrix_validate().<br>
> > > Can the user really cause the matrix not to be<br>
> valid? If yes,<br>
> > then can't<br>
> > > this cause problems even earlier in the code? If<br>
> not, then<br>
> > maybe an<br>
> > > assertion would be better. Also, is the size of the<br>
> matrix the<br>
> > only<br>
> > > thing you can validate?<br>
> > ><br>
> > > Removed it. You're right, it's functionally impossible<br>
> 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<br>
> the code<br>
> > that's<br>
> > > calling it never checks it. Considering that it's an<br>
> internal<br>
> > call, I<br>
> > > would say it's probably better not to return<br>
> anything, but<br>
> > 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<br>
> > checked. So it<br>
> > > should either be replaced by an asssert or (if the<br>
> user can<br>
> > cause a<br>
> > > failure) actually checked.<br>
> > ><br>
> > > Done. <br>
> > ><br>
> > > 8) I see there's a "gain" field in the matrix, but I<br>
> can't see<br>
> > 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<br>
> it to the<br>
> > > overall output gain. We assume the mixing matrix gain is<br>
> always<br>
> > zero and<br>
> > > that this only matters for the output gain from the<br>
> demixing matrix.<br>
> > ><br>
> > ><br>
> > > 9) In get_streams_from_channels(), I think it'd be<br>
> 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(),<br>
> you to see<br>
> > if streams<br>
> > > and coupled_streams are NULL, but unless I missed<br>
> something I<br>
> > don't<br>
> > > think there's any valid case where you wouldn't want<br>
> to get those<br>
> > > values. If you don't have them, then you have no way of<br>
> > knowing what<br>
> > > mapping the encoder used, so no way of decoding.<br>
> Instead, I<br>
> > 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<br>
> > opus_projection_encode() and<br>
> > > opus_projection_encode_float() (same for the<br>
> decoder) use<br>
> > arbitrarily<br>
> > > large amounts of stack memory for "buf". In<br>
> > opus_multistream_encode()<br>
> > > that's avoided by converting just two channels at a<br>
> time, but<br>
> > here it's<br>
> > > not quite clear how to do that without duplicating a<br>
> lot of the<br>
> > > multistream code. If we can't address the issue with<br>
> this<br>
> > patch, the<br>
> > > least would be to abort when trying to use these<br>
> calls with<br>
> > > NONTHREADSAFE_PSEUDOSTACK<br>
> > ><br>
> > > Done. Let me know if we should try designing something<br>
> around this. <br>
> > ><br>
> > ><br>
> > > 12) I think opus_projection_decoder_ctl() is missing a<br>
> > 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<br>
> 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>>><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>><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>>>><br>
> > > > <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><br>
> <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>><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>
> > > > ><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<br>
> opus_compare (and also<br>
> > > opus_demo).<br>
> > > > If you ultimately cast s to (opus_int16), then I'm<br>
> > pretty sure<br>
> > > most<br>
> > > > compilers will turn the second line into a<br>
> no-op and<br>
> > 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>><br>
> <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> <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> <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> <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> <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> <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> <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>
> <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> <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> <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>
><br>
</blockquote></div></div>