Sure, ill send that asap On Wed, Nov 8, 2017 at 4:44 PM Jean-Marc Valin <jmvalin at jmvalin.ca> wrote:> Hi Drew, > > Your ambisonics patch is already merged. Can you send a patch that > applies to master? > > Jean-Marc > > On 11/08/2017 07:05 PM, Drew Allen wrote: > > Hey Jean-Marc, > > > > I found a bug regarding exporting the matrix that wasn't always grabbing > > the correct values, causing incorrect mixing behavior. This patch > > resolves that issue. > > > > Cheers, > > Drew > > > > On Tue, Nov 7, 2017 at 3:04 PM Drew Allen <bitllama at google.com > > <mailto:bitllama at google.com>> wrote: > > > > Awesome!!!! Ill think about a strategy for that soon. Glad it could > > finally make it in. > > > > On Tue, Nov 7, 2017 at 3:01 PM Jean-Marc Valin <jmvalin at jmvalin.ca > > <mailto:jmvalin at jmvalin.ca>> wrote: > > > > Hi Drew, > > > > Thanks for the update. Your patch is now in master. Now, it > would be > > good if you could think of a way to reduce the stack usage as we > > discussed. > > > > Cheers, > > > > Jean-Marc > > > > On 11/07/2017 04:28 PM, Drew Allen wrote: > > > Here's another patch. Cheers! > > > > > > On Mon, Nov 6, 2017 at 10:08 AM Drew Allen > > <bitllama at google.com <mailto:bitllama at google.com> > > > <mailto:bitllama at google.com <mailto:bitllama at google.com>>> > wrote: > > > > > > Ok great. I'll make those changes you suggested and get > > back to you > > > this morning. > > > > > > Cheers, > > > Drew > > > > > > > > > > > > On 11/03/2017 02:51 PM, Drew Allen wrote: > > > > Here's another one. > > > > > > > > On Thu, Nov 2, 2017 at 9:54 AM Jean-Marc Valin > > <jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca> > > <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca>> > > > > <mailto:jmvalin at jmvalin.ca > > <mailto:jmvalin at jmvalin.ca> <mailto:jmvalin at jmvalin.ca > > <mailto:jmvalin at jmvalin.ca>>>> wrote: > > > > > > > > Hi Drew, > > > > > > > > We're getting there... Some minor comments: > > > > > > > > 1) The public header file should not have an > > > > #ifdef ENABLE_EXPERIMENTAL_AMBISONICS > > > > since that would require the user code to define > it. > > > > > > > > Done > > > > > > > > 2) Why do you have #define MAPPING_MATRIX_C ? > > > > > > > > No idea. Fixed. > > > > > > > > > > > > 3) Looks like MAPPING_MATRIX_MAX_SIZE is not > > longer useful, right? > > > > > > > > Yup. Removed. > > > > > > > > > > > > 4) Even though it's not strictly necessary here, > > please add parentheses > > > > to the definition of MATRIX_INDEX() to avoid > > nasty surprises in the > > > > future (e.g. MATRIX_INDEX(...)*sizeof(foo) would > > be really bad). > > > > > > > > Done. > > > > > > > > > > > > Cheers, > > > > > > > > Jean-Marc > > > > > > > > > > > > On 10/31/2017 04:10 PM, Drew Allen wrote: > > > > > Hi Jean-Marc, > > > > > > > > > > Thanks so much for your review. Attached are > > my comments and an > > > > updated > > > > > patch. > > > > > > > > > > On Mon, Oct 30, 2017 at 5:48 PM Jean-Marc Valin > > > > <jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca> > > <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca>> > > > <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca> > > <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca>>> > > > > > <mailto:jmvalin at jmvalin.ca > > <mailto:jmvalin at jmvalin.ca> <mailto:jmvalin at jmvalin.ca > > <mailto:jmvalin at jmvalin.ca>> > > > <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca> > > <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca>>>>> > wrote: > > > > > > > > > > Hi Drew, > > > > > > > > > > I've had some time to dig more deeply into > > your > > > patch. Here's > > > > some more > > > > > in-depth comments: > > > > > > > > > > 1) I note that your OpusMSEncoder struct in > > > private.h adds a > > > > > subframe_mem[] that's not in > > > opus_multistream_encoder.c. I > > > > assume it's > > > > > due to a merge problem (that field was > > removed some > > > time ago), > > > > but can > > > > > you confirm/fix the issue? > > > > > > > > > > Done. Yes, this is a merge conflict. > > > > > > > > > > > > > > > 2) I noticed your patch adds many > OPUS_EXPORT > > > declarations. > > > > OPUS_EXPORT > > > > > is only meant for functions exposed to the > > API (and > > > in the > > > > include/ > > > > > directory), but I see several of these in > > > mapping_matrix.h, > > > > which I > > > > > think is incorrect. > > > > > > > > > > Done. > > > > > > > > > > > > > > > 3) > > opus_projection_ambisonics_encoder_init() (which > > > should be > > > > > OPUS_EXPORT) is missing from > opus_projection.h > > > > > > > > > > Done. > > > > > > > > > > > > > > > 4) I believe mapping_matrix_get_size() > > should be > > > returning > > > > > align(sizeof(MappingMatrix)) + rows * cols > * > > > sizeof(opus_int16) > > > > > rather than: > > > > > align(sizeof(MappingMatrix) + rows * cols * > > > sizeof(opus_int16)) > > > > > to match what mapping_matrix_get_data() > does > > > > > > > > > > Done. > > > > > > > > > > > > > > > 5) I'm not sure I understand the purpose of > > > > mapping_matrix_validate(). > > > > > Can the user really cause the matrix not > to be > > > valid? If yes, > > > > then can't > > > > > this cause problems even earlier in the > > code? If > > > not, then > > > > maybe an > > > > > assertion would be better. Also, is the > > size of the > > > matrix the > > > > only > > > > > thing you can validate? > > > > > > > > > > Removed it. You're right, it's functionally > > impossible > > > for the user to > > > > > create an invalid matrix with the current API. > > > > > > > > > > > > > > > 6) mapping_matrix_init() returns an error > > code, but > > > the code > > > > that's > > > > > calling it never checks it. Considering > > that it's an > > > internal > > > > call, I > > > > > would say it's probably better not to > return > > > anything, but > > > > instead to > > > > > assert in the function. > > > > > > > > > > Done. > > > > > > > > > > > > > > > 7) Same for > > mapping_matrix_multiply_short() and > > > > > mapping_matrix_multiply_float(), the > > return value isn't > > > > checked. So it > > > > > should either be replaced by an asssert or > > (if the > > > user can > > > > cause a > > > > > failure) actually checked. > > > > > > > > > > Done. > > > > > > > > > > 8) I see there's a "gain" field in the > > matrix, but I > > > can't see > > > > it used > > > > > anywhere. Did I miss something? > > > > > > > > > > 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. > > > > > > > > > > > > > > > 9) In get_streams_from_channels(), I think > > it'd be > > > simpler to just > > > > > replace: > > > > > *streams = channels / 2 + (channels % 2 => 1); > > > > > with: > > > > > *streams = (channels+1) / 2; > > > > > > > > > > Done. > > > > > > > > > > > > > > > 10) In > > opus_projection_ambisonics_encoder_init(), > > > you to see > > > > if streams > > > > > and coupled_streams are NULL, but unless I > > missed > > > something I > > > > don't > > > > > think there's any valid case where you > > wouldn't want > > > to get those > > > > > values. If you don't have them, then you > > have no way of > > > > knowing what > > > > > mapping the encoder used, so no way of > > decoding. > > > Instead, I > > > > would just > > > > > return OPUS_BAD_ARG if they're NULL. > > > > > > > > > > Done. > > > > > > > > > > > > > > > 11) So one issue I just noticed is that > > > > opus_projection_encode() and > > > > > opus_projection_encode_float() (same for > the > > > decoder) use > > > > arbitrarily > > > > > large amounts of stack memory for "buf". In > > > > opus_multistream_encode() > > > > > that's avoided by converting just two > > channels at a > > > time, but > > > > here it's > > > > > not quite clear how to do that without > > duplicating a > > > lot of the > > > > > multistream code. If we can't address the > > issue with > > > this > > > > patch, the > > > > > least would be to abort when trying to use > > these > > > calls with > > > > > NONTHREADSAFE_PSEUDOSTACK > > > > > > > > > > Done. Let me know if we should try designing > > something > > > around this. > > > > > > > > > > > > > > > 12) I think opus_projection_decoder_ctl() > > is missing a > > > > va_end() call. > > > > > > > > > > > > > > > Done. > > > > > > > > > > > > > > > > > > > > Cheers, > > > > > > > > > > Jean-Marc > > > > > > > > > > > > > > > > > > > > On 10/12/2017 05:44 PM, Drew Allen wrote: > > > > > > thanks for all your feedback. here's the > > revised > > > patch: > > > > > > > > > > > > On Wed, Oct 11, 2017 at 2:20 PM Timothy > > B. Terriberry > > > > > <tterribe at xiph.org > > <mailto:tterribe at xiph.org> <mailto:tterribe at xiph.org > > <mailto:tterribe at xiph.org>> > > > <mailto:tterribe at xiph.org <mailto:tterribe at xiph.org> > > <mailto:tterribe at xiph.org <mailto:tterribe at xiph.org>>> > > > > <mailto:tterribe at xiph.org > > <mailto:tterribe at xiph.org> <mailto:tterribe at xiph.org > > <mailto:tterribe at xiph.org>> > > > <mailto:tterribe at xiph.org <mailto:tterribe at xiph.org> > > <mailto:tterribe at xiph.org <mailto:tterribe at xiph.org>>>> > > > > > > <mailto:tterribe at xiph.org > > <mailto:tterribe at xiph.org> > > > <mailto:tterribe at xiph.org <mailto:tterribe at xiph.org>> > > <mailto:tterribe at xiph.org <mailto:tterribe at xiph.org> > > > <mailto:tterribe at xiph.org <mailto:tterribe at xiph.org>>> > > > > <mailto:tterribe at xiph.org > > <mailto:tterribe at xiph.org> <mailto:tterribe at xiph.org > > <mailto:tterribe at xiph.org>> > > > <mailto:tterribe at xiph.org <mailto:tterribe at xiph.org> > > <mailto:tterribe at xiph.org <mailto:tterribe at xiph.org>>>>>> wrote: > > > > > > > > > > > > Jean-Marc Valin wrote: > > > > > > > I think you'll want something like: > > > > > > > > > > (opus_int16)((unsigned)demixing_matrix[2*i+1] << 8) > > > > > > > (though you might want to check it > > too) > > > > > > > > > > > > FWIW, we use the construct > > > > > > int s = buf[2*i + 1] << 8 | > buf[2*i]; > > > > > > s = ((s & 0xFFFF) ^ 0x8000) - > 0x8000; > > > > > > > > > > > > to manually sign-extend the result in > > > opus_compare (and also > > > > > opus_demo). > > > > > > If you ultimately cast s to > > (opus_int16), then I'm > > > > pretty sure > > > > > most > > > > > > compilers will turn the second line > > into a > > > no-op and > > > > optimize > > > > > it away. > > > > > > > > _______________________________________________ > > > > > > opus mailing list > > > > > > opus at xiph.org <mailto:opus at xiph.org> > > <mailto:opus at xiph.org <mailto:opus at xiph.org>> > > > <mailto:opus at xiph.org <mailto:opus at xiph.org> > > <mailto:opus at xiph.org <mailto:opus at xiph.org>>> > > > > <mailto:opus at xiph.org <mailto:opus at xiph.org> > > <mailto:opus at xiph.org <mailto:opus at xiph.org>> > > > <mailto:opus at xiph.org <mailto:opus at xiph.org> > > <mailto:opus at xiph.org <mailto:opus at xiph.org>>>> > > > <mailto:opus at xiph.org <mailto:opus at xiph.org> > > <mailto:opus at xiph.org <mailto:opus at xiph.org>> > > > > <mailto:opus at xiph.org <mailto:opus at xiph.org> > > <mailto:opus at xiph.org <mailto:opus at xiph.org>>> > > > > > <mailto:opus at xiph.org > > <mailto:opus at xiph.org> <mailto:opus at xiph.org <mailto: > opus at xiph.org>> > > > <mailto:opus at xiph.org <mailto:opus at xiph.org> > > <mailto:opus at xiph.org <mailto:opus at xiph.org>>>>> > > > > > > > > http://lists.xiph.org/mailman/listinfo/opus > > > > > > > > > > > > > > > > > > > > > > > > > > _______________________________________________ > > > > > > opus mailing list > > > > > > opus at xiph.org <mailto:opus at xiph.org> > > <mailto:opus at xiph.org <mailto:opus at xiph.org>> > > > <mailto:opus at xiph.org <mailto:opus at xiph.org> > > <mailto:opus at xiph.org <mailto:opus at xiph.org>>> > > > <mailto:opus at xiph.org <mailto:opus at xiph.org> > > <mailto:opus at xiph.org <mailto:opus at xiph.org>> > > > > <mailto:opus at xiph.org <mailto:opus at xiph.org> > > <mailto:opus at xiph.org <mailto:opus at xiph.org>>>> > > > > > > > http://lists.xiph.org/mailman/listinfo/opus > > > > > > > > > > > > > > > > > > > > > > > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.xiph.org/pipermail/opus/attachments/20171109/d09265bd/attachment-0001.html>
Hi all, Attached is a quick patch that addresses a bug when exporting the matrix from the encoder. Cheers, Drew On Wed, Nov 8, 2017 at 4:44 PM Drew Allen <bitllama at google.com> wrote:> Sure, ill send that asap > On Wed, Nov 8, 2017 at 4:44 PM Jean-Marc Valin <jmvalin at jmvalin.ca> wrote: > >> Hi Drew, >> >> Your ambisonics patch is already merged. Can you send a patch that >> applies to master? >> >> Jean-Marc >> >> On 11/08/2017 07:05 PM, Drew Allen wrote: >> > Hey Jean-Marc, >> > >> > I found a bug regarding exporting the matrix that wasn't always grabbing >> > the correct values, causing incorrect mixing behavior. This patch >> > resolves that issue. >> > >> > Cheers, >> > Drew >> > >> > On Tue, Nov 7, 2017 at 3:04 PM Drew Allen <bitllama at google.com >> > <mailto:bitllama at google.com>> wrote: >> > >> > Awesome!!!! Ill think about a strategy for that soon. Glad it could >> > finally make it in. >> > >> > On Tue, Nov 7, 2017 at 3:01 PM Jean-Marc Valin <jmvalin at jmvalin.ca >> > <mailto:jmvalin at jmvalin.ca>> wrote: >> > >> > Hi Drew, >> > >> > Thanks for the update. Your patch is now in master. Now, it >> would be >> > good if you could think of a way to reduce the stack usage as we >> > discussed. >> > >> > Cheers, >> > >> > Jean-Marc >> > >> > On 11/07/2017 04:28 PM, Drew Allen wrote: >> > > Here's another patch. Cheers! >> > > >> > > On Mon, Nov 6, 2017 at 10:08 AM Drew Allen >> > <bitllama at google.com <mailto:bitllama at google.com> >> > > <mailto:bitllama at google.com <mailto:bitllama at google.com>>> >> wrote: >> > > >> > > Ok great. I'll make those changes you suggested and get >> > back to you >> > > this morning. >> > > >> > > Cheers, >> > > Drew >> > > >> > > >> > > >> > > On 11/03/2017 02:51 PM, Drew Allen wrote: >> > > > Here's another one. >> > > > >> > > > On Thu, Nov 2, 2017 at 9:54 AM Jean-Marc Valin >> > <jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca> >> > <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca>> >> > > > <mailto:jmvalin at jmvalin.ca >> > <mailto:jmvalin at jmvalin.ca> <mailto:jmvalin at jmvalin.ca >> > <mailto:jmvalin at jmvalin.ca>>>> wrote: >> > > > >> > > > Hi Drew, >> > > > >> > > > We're getting there... Some minor comments: >> > > > >> > > > 1) The public header file should not have an >> > > > #ifdef ENABLE_EXPERIMENTAL_AMBISONICS >> > > > since that would require the user code to >> define it. >> > > > >> > > > Done >> > > > >> > > > 2) Why do you have #define MAPPING_MATRIX_C ? >> > > > >> > > > No idea. Fixed. >> > > > >> > > > >> > > > 3) Looks like MAPPING_MATRIX_MAX_SIZE is not >> > longer useful, right? >> > > > >> > > > Yup. Removed. >> > > > >> > > > >> > > > 4) Even though it's not strictly necessary here, >> > please add parentheses >> > > > to the definition of MATRIX_INDEX() to avoid >> > nasty surprises in the >> > > > future (e.g. MATRIX_INDEX(...)*sizeof(foo) would >> > be really bad). >> > > > >> > > > Done. >> > > > >> > > > >> > > > Cheers, >> > > > >> > > > Jean-Marc >> > > > >> > > > >> > > > On 10/31/2017 04:10 PM, Drew Allen wrote: >> > > > > Hi Jean-Marc, >> > > > > >> > > > > Thanks so much for your review. Attached are >> > my comments and an >> > > > updated >> > > > > patch. >> > > > > >> > > > > On Mon, Oct 30, 2017 at 5:48 PM Jean-Marc >> Valin >> > > > <jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca> >> > <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca>> >> > > <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca >> > >> > <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca>>> >> > > > > <mailto:jmvalin at jmvalin.ca >> > <mailto:jmvalin at jmvalin.ca> <mailto:jmvalin at jmvalin.ca >> > <mailto:jmvalin at jmvalin.ca>> >> > > <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca >> > >> > <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca>>>>> >> wrote: >> > > > > >> > > > > Hi Drew, >> > > > > >> > > > > I've had some time to dig more deeply into >> > your >> > > patch. Here's >> > > > some more >> > > > > in-depth comments: >> > > > > >> > > > > 1) I note that your OpusMSEncoder struct >> in >> > > private.h adds a >> > > > > subframe_mem[] that's not in >> > > opus_multistream_encoder.c. I >> > > > assume it's >> > > > > due to a merge problem (that field was >> > removed some >> > > time ago), >> > > > but can >> > > > > you confirm/fix the issue? >> > > > > >> > > > > Done. Yes, this is a merge conflict. >> > > > > >> > > > > >> > > > > 2) I noticed your patch adds many >> OPUS_EXPORT >> > > declarations. >> > > > OPUS_EXPORT >> > > > > is only meant for functions exposed to the >> > API (and >> > > in the >> > > > include/ >> > > > > directory), but I see several of these in >> > > mapping_matrix.h, >> > > > which I >> > > > > think is incorrect. >> > > > > >> > > > > Done. >> > > > > >> > > > > >> > > > > 3) >> > opus_projection_ambisonics_encoder_init() (which >> > > should be >> > > > > OPUS_EXPORT) is missing from >> opus_projection.h >> > > > > >> > > > > Done. >> > > > > >> > > > > >> > > > > 4) I believe mapping_matrix_get_size() >> > should be >> > > returning >> > > > > align(sizeof(MappingMatrix)) + rows * >> cols * >> > > sizeof(opus_int16) >> > > > > rather than: >> > > > > align(sizeof(MappingMatrix) + rows * cols >> * >> > > sizeof(opus_int16)) >> > > > > to match what mapping_matrix_get_data() >> does >> > > > > >> > > > > Done. >> > > > > >> > > > > >> > > > > 5) I'm not sure I understand the purpose >> of >> > > > mapping_matrix_validate(). >> > > > > Can the user really cause the matrix not >> to be >> > > valid? If yes, >> > > > then can't >> > > > > this cause problems even earlier in the >> > code? If >> > > not, then >> > > > maybe an >> > > > > assertion would be better. Also, is the >> > size of the >> > > matrix the >> > > > only >> > > > > thing you can validate? >> > > > > >> > > > > Removed it. You're right, it's functionally >> > impossible >> > > for the user to >> > > > > create an invalid matrix with the current API. >> > > > > >> > > > > >> > > > > 6) mapping_matrix_init() returns an error >> > code, but >> > > the code >> > > > that's >> > > > > calling it never checks it. Considering >> > that it's an >> > > internal >> > > > call, I >> > > > > would say it's probably better not to >> return >> > > anything, but >> > > > instead to >> > > > > assert in the function. >> > > > > >> > > > > Done. >> > > > > >> > > > > >> > > > > 7) Same for >> > mapping_matrix_multiply_short() and >> > > > > mapping_matrix_multiply_float(), the >> > return value isn't >> > > > checked. So it >> > > > > should either be replaced by an asssert or >> > (if the >> > > user can >> > > > cause a >> > > > > failure) actually checked. >> > > > > >> > > > > Done. >> > > > > >> > > > > 8) I see there's a "gain" field in the >> > matrix, but I >> > > can't see >> > > > it used >> > > > > anywhere. Did I miss something? >> > > > > >> > > > > 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. >> > > > > >> > > > > >> > > > > 9) In get_streams_from_channels(), I think >> > it'd be >> > > simpler to just >> > > > > replace: >> > > > > *streams = channels / 2 + (channels % 2 >> == 1); >> > > > > with: >> > > > > *streams = (channels+1) / 2; >> > > > > >> > > > > Done. >> > > > > >> > > > > >> > > > > 10) In >> > opus_projection_ambisonics_encoder_init(), >> > > you to see >> > > > if streams >> > > > > and coupled_streams are NULL, but unless I >> > missed >> > > something I >> > > > don't >> > > > > think there's any valid case where you >> > wouldn't want >> > > to get those >> > > > > values. If you don't have them, then you >> > have no way of >> > > > knowing what >> > > > > mapping the encoder used, so no way of >> > decoding. >> > > Instead, I >> > > > would just >> > > > > return OPUS_BAD_ARG if they're NULL. >> > > > > >> > > > > Done. >> > > > > >> > > > > >> > > > > 11) So one issue I just noticed is that >> > > > opus_projection_encode() and >> > > > > opus_projection_encode_float() (same for >> the >> > > decoder) use >> > > > arbitrarily >> > > > > large amounts of stack memory for "buf". >> In >> > > > opus_multistream_encode() >> > > > > that's avoided by converting just two >> > channels at a >> > > time, but >> > > > here it's >> > > > > not quite clear how to do that without >> > duplicating a >> > > lot of the >> > > > > multistream code. If we can't address the >> > issue with >> > > this >> > > > patch, the >> > > > > least would be to abort when trying to use >> > these >> > > calls with >> > > > > NONTHREADSAFE_PSEUDOSTACK >> > > > > >> > > > > Done. Let me know if we should try designing >> > something >> > > around this. >> > > > > >> > > > > >> > > > > 12) I think opus_projection_decoder_ctl() >> > is missing a >> > > > va_end() call. >> > > > > >> > > > > >> > > > > Done. >> > > > > >> > > > > >> > > > > >> > > > > Cheers, >> > > > > >> > > > > Jean-Marc >> > > > > >> > > > > >> > > > > >> > > > > On 10/12/2017 05:44 PM, Drew Allen wrote: >> > > > > > thanks for all your feedback. here's the >> > revised >> > > patch: >> > > > > > >> > > > > > On Wed, Oct 11, 2017 at 2:20 PM Timothy >> > B. Terriberry >> > > > > <tterribe at xiph.org >> > <mailto:tterribe at xiph.org> <mailto:tterribe at xiph.org >> > <mailto:tterribe at xiph.org>> >> > > <mailto:tterribe at xiph.org <mailto:tterribe at xiph.org> >> > <mailto:tterribe at xiph.org <mailto:tterribe at xiph.org>>> >> > > > <mailto:tterribe at xiph.org >> > <mailto:tterribe at xiph.org> <mailto:tterribe at xiph.org >> > <mailto:tterribe at xiph.org>> >> > > <mailto:tterribe at xiph.org <mailto:tterribe at xiph.org> >> > <mailto:tterribe at xiph.org <mailto:tterribe at xiph.org>>>> >> > > > > > <mailto:tterribe at xiph.org >> > <mailto:tterribe at xiph.org> >> > > <mailto:tterribe at xiph.org <mailto:tterribe at xiph.org>> >> > <mailto:tterribe at xiph.org <mailto:tterribe at xiph.org> >> > > <mailto:tterribe at xiph.org <mailto:tterribe at xiph.org >> >>> >> > > > <mailto:tterribe at xiph.org >> > <mailto:tterribe at xiph.org> <mailto:tterribe at xiph.org >> > <mailto:tterribe at xiph.org>> >> > > <mailto:tterribe at xiph.org <mailto:tterribe at xiph.org> >> > <mailto:tterribe at xiph.org <mailto:tterribe at xiph.org>>>>>> >> wrote: >> > > > > > >> > > > > > Jean-Marc Valin wrote: >> > > > > > > I think you'll want something >> like: >> > > > > > > >> > > (opus_int16)((unsigned)demixing_matrix[2*i+1] << 8) >> > > > > > > (though you might want to check it >> > too) >> > > > > > >> > > > > > FWIW, we use the construct >> > > > > > int s = buf[2*i + 1] << 8 | >> buf[2*i]; >> > > > > > s = ((s & 0xFFFF) ^ 0x8000) - >> 0x8000; >> > > > > > >> > > > > > to manually sign-extend the result >> in >> > > opus_compare (and also >> > > > > opus_demo). >> > > > > > If you ultimately cast s to >> > (opus_int16), then I'm >> > > > pretty sure >> > > > > most >> > > > > > compilers will turn the second line >> > into a >> > > no-op and >> > > > optimize >> > > > > it away. >> > > > > > >> > _______________________________________________ >> > > > > > opus mailing list >> > > > > > opus at xiph.org <mailto:opus at xiph.org >> > >> > <mailto:opus at xiph.org <mailto:opus at xiph.org>> >> > > <mailto:opus at xiph.org <mailto:opus at xiph.org> >> > <mailto:opus at xiph.org <mailto:opus at xiph.org>>> >> > > > <mailto:opus at xiph.org <mailto:opus at xiph.org> >> > <mailto:opus at xiph.org <mailto:opus at xiph.org>> >> > > <mailto:opus at xiph.org <mailto:opus at xiph.org> >> > <mailto:opus at xiph.org <mailto:opus at xiph.org>>>> >> > > <mailto:opus at xiph.org <mailto:opus at xiph.org> >> > <mailto:opus at xiph.org <mailto:opus at xiph.org>> >> > > > <mailto:opus at xiph.org <mailto:opus at xiph.org> >> > <mailto:opus at xiph.org <mailto:opus at xiph.org>>> >> > > > > <mailto:opus at xiph.org >> > <mailto:opus at xiph.org> <mailto:opus at xiph.org <mailto: >> opus at xiph.org>> >> > > <mailto:opus at xiph.org <mailto:opus at xiph.org> >> > <mailto:opus at xiph.org <mailto:opus at xiph.org>>>>> >> > > > > > >> > http://lists.xiph.org/mailman/listinfo/opus >> > > > > > >> > > > > > >> > > > > > >> > > > > > >> > _______________________________________________ >> > > > > > opus mailing list >> > > > > > opus at xiph.org <mailto:opus at xiph.org> >> > <mailto:opus at xiph.org <mailto:opus at xiph.org>> >> > > <mailto:opus at xiph.org <mailto:opus at xiph.org> >> > <mailto:opus at xiph.org <mailto:opus at xiph.org>>> >> > > <mailto:opus at xiph.org <mailto:opus at xiph.org> >> > <mailto:opus at xiph.org <mailto:opus at xiph.org>> >> > > > <mailto:opus at xiph.org <mailto:opus at xiph.org> >> > <mailto:opus at xiph.org <mailto:opus at xiph.org>>>> >> > > > > > >> http://lists.xiph.org/mailman/listinfo/opus >> > > > > > >> > > > > >> > > > >> > > >> > > >> > >> >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.xiph.org/pipermail/opus/attachments/20171109/7bb62065/attachment-0001.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: 0001-Fix-matrix-export-via-CTL-func.patch Type: text/x-patch Size: 2308 bytes Desc: not available URL: <http://lists.xiph.org/pipermail/opus/attachments/20171109/7bb62065/attachment-0001.bin>
On 11/09/2017 01:58 PM, Drew Allen wrote:> Attached is a quick patch that addresses a bug when exporting the matrix > from the encoder.Actually, I don't see what your encoder change is supposed to do. Are there cases where demixing_matrix->rows != nb_output_streams ? Cheers, Jean-Marc> Cheers, > Drew > > On Wed, Nov 8, 2017 at 4:44 PM Drew Allen <bitllama at google.com > <mailto:bitllama at google.com>> wrote: > > Sure, ill send that asap > On Wed, Nov 8, 2017 at 4:44 PM Jean-Marc Valin <jmvalin at jmvalin.ca > <mailto:jmvalin at jmvalin.ca>> wrote: > > Hi Drew, > > Your ambisonics patch is already merged. Can you send a patch that > applies to master? > > Jean-Marc > > On 11/08/2017 07:05 PM, Drew Allen wrote: > > Hey Jean-Marc, > > > > I found a bug regarding exporting the matrix that wasn't > always grabbing > > the correct values, causing incorrect mixing behavior. This patch > > resolves that issue. > > > > Cheers, > > Drew > > > > On Tue, Nov 7, 2017 at 3:04 PM Drew Allen <bitllama at google.com > <mailto:bitllama at google.com> > > <mailto:bitllama at google.com <mailto:bitllama at google.com>>> wrote: > > > > Awesome!!!! Ill think about a strategy for that soon. Glad > it could > > finally make it in. > > > > On Tue, Nov 7, 2017 at 3:01 PM Jean-Marc Valin > <jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca> > > <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca>>> > wrote: > > > > Hi Drew, > > > > Thanks for the update. Your patch is now in master. > Now, it would be > > good if you could think of a way to reduce the stack > usage as we > > discussed. > > > > Cheers, > > > > Jean-Marc > > > > On 11/07/2017 04:28 PM, Drew Allen wrote: > > > Here's another patch. Cheers! > > > > > > On Mon, Nov 6, 2017 at 10:08 AM Drew Allen > > <bitllama at google.com <mailto:bitllama at google.com> > <mailto:bitllama at google.com <mailto:bitllama at google.com>> > > > <mailto:bitllama at google.com > <mailto:bitllama at google.com> <mailto:bitllama at google.com > <mailto:bitllama at google.com>>>> wrote: > > > > > > Ok great. I'll make those changes you suggested > and get > > back to you > > > this morning. > > > > > > Cheers, > > > Drew > > > > > > > > > > > > On 11/03/2017 02:51 PM, Drew Allen wrote: > > > > Here's another one. > > > > > > > > On Thu, Nov 2, 2017 at 9:54 AM Jean-Marc Valin > > <jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca> > <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca>> > > <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca> > <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca>>> > > > > <mailto:jmvalin at jmvalin.ca > <mailto:jmvalin at jmvalin.ca> > > <mailto:jmvalin at jmvalin.ca > <mailto:jmvalin at jmvalin.ca>> <mailto:jmvalin at jmvalin.ca > <mailto:jmvalin at jmvalin.ca> > > <mailto:jmvalin at jmvalin.ca > <mailto:jmvalin at jmvalin.ca>>>>> wrote: > > > > > > > > Hi Drew, > > > > > > > > We're getting there... Some minor > comments: > > > > > > > > 1) The public header file should not > have an > > > > #ifdef ENABLE_EXPERIMENTAL_AMBISONICS > > > > since that would require the user code > to define it. > > > > > > > > Done > > > > > > > > 2) Why do you have #define > MAPPING_MATRIX_C ? > > > > > > > > No idea. Fixed. > > > > > > > > > > > > 3) Looks like MAPPING_MATRIX_MAX_SIZE > is not > > longer useful, right? > > > > > > > > Yup. Removed. > > > > > > > > > > > > 4) Even though it's not strictly > necessary here, > > please add parentheses > > > > to the definition of MATRIX_INDEX() to > avoid > > nasty surprises in the > > > > future (e.g. > MATRIX_INDEX(...)*sizeof(foo) would > > be really bad). > > > > > > > > Done. > > > > > > > > > > > > Cheers, > > > > > > > > Jean-Marc > > > > > > > > > > > > On 10/31/2017 04:10 PM, Drew Allen wrote: > > > > > Hi Jean-Marc, > > > > > > > > > > Thanks so much for your review. > Attached are > > my comments and an > > > > updated > > > > > patch. > > > > > > > > > > On Mon, Oct 30, 2017 at 5:48 PM > Jean-Marc Valin > > > > <jmvalin at jmvalin.ca > <mailto:jmvalin at jmvalin.ca> <mailto:jmvalin at jmvalin.ca > <mailto:jmvalin at jmvalin.ca>> > > <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca> > <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca>>> > > > <mailto:jmvalin at jmvalin.ca > <mailto:jmvalin at jmvalin.ca> <mailto:jmvalin at jmvalin.ca > <mailto:jmvalin at jmvalin.ca>> > > <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca> > <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca>>>> > > > > > <mailto:jmvalin at jmvalin.ca > <mailto:jmvalin at jmvalin.ca> > > <mailto:jmvalin at jmvalin.ca > <mailto:jmvalin at jmvalin.ca>> <mailto:jmvalin at jmvalin.ca > <mailto:jmvalin at jmvalin.ca> > > <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca>>> > > > <mailto:jmvalin at jmvalin.ca > <mailto:jmvalin at jmvalin.ca> <mailto:jmvalin at jmvalin.ca > <mailto:jmvalin at jmvalin.ca>> > > <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca> > <mailto:jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca>>>>>> wrote: > > > > > > > > > > Hi Drew, > > > > > > > > > > I've had some time to dig more > deeply into > > your > > > patch. Here's > > > > some more > > > > > in-depth comments: > > > > > > > > > > 1) I note that your > OpusMSEncoder struct in > > > private.h adds a > > > > > subframe_mem[] that's not in > > > opus_multistream_encoder.c. I > > > > assume it's > > > > > due to a merge problem (that > field was > > removed some > > > time ago), > > > > but can > > > > > you confirm/fix the issue? > > > > > > > > > > Done. Yes, this is a merge conflict. > > > > > > > > > > > > > > > 2) I noticed your patch adds > many OPUS_EXPORT > > > declarations. > > > > OPUS_EXPORT > > > > > is only meant for functions > exposed to the > > API (and > > > in the > > > > include/ > > > > > directory), but I see several of > these in > > > mapping_matrix.h, > > > > which I > > > > > think is incorrect. > > > > > > > > > > Done. > > > > > > > > > > > > > > > 3) > > opus_projection_ambisonics_encoder_init() (which > > > should be > > > > > OPUS_EXPORT) is missing from > opus_projection.h > > > > > > > > > > Done. > > > > > > > > > > > > > > > 4) I believe > mapping_matrix_get_size() > > should be > > > returning > > > > > align(sizeof(MappingMatrix)) + > rows * cols * > > > sizeof(opus_int16) > > > > > rather than: > > > > > align(sizeof(MappingMatrix) + > rows * cols * > > > sizeof(opus_int16)) > > > > > to match what > mapping_matrix_get_data() does > > > > > > > > > > Done. > > > > > > > > > > > > > > > 5) I'm not sure I understand the > purpose of > > > > mapping_matrix_validate(). > > > > > Can the user really cause the > matrix not to be > > > valid? If yes, > > > > then can't > > > > > this cause problems even earlier > in the > > code? If > > > not, then > > > > maybe an > > > > > assertion would be better. Also, > is the > > size of the > > > matrix the > > > > only > > > > > thing you can validate? > > > > > > > > > > Removed it. You're right, it's > functionally > > impossible > > > for the user to > > > > > create an invalid matrix with the > current API. > > > > > > > > > > > > > > > 6) mapping_matrix_init() returns > an error > > code, but > > > the code > > > > that's > > > > > calling it never checks it. > Considering > > that it's an > > > internal > > > > call, I > > > > > would say it's probably better > not to return > > > anything, but > > > > instead to > > > > > assert in the function. > > > > > > > > > > Done. > > > > > > > > > > > > > > > 7) Same for > > mapping_matrix_multiply_short() and > > > > > mapping_matrix_multiply_float(), the > > return value isn't > > > > checked. So it > > > > > should either be replaced by an > asssert or > > (if the > > > user can > > > > cause a > > > > > failure) actually checked. > > > > > > > > > > Done. > > > > > > > > > > 8) I see there's a "gain" field > in the > > matrix, but I > > > can't see > > > > it used > > > > > anywhere. Did I miss something? > > > > > > > > > > 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. > > > > > > > > > > > > > > > 9) In > get_streams_from_channels(), I think > > it'd be > > > simpler to just > > > > > replace: > > > > > *streams = channels / 2 + > (channels % 2 == 1); > > > > > with: > > > > > *streams = (channels+1) / 2; > > > > > > > > > > Done. > > > > > > > > > > > > > > > 10) In > > opus_projection_ambisonics_encoder_init(), > > > you to see > > > > if streams > > > > > and coupled_streams are NULL, > but unless I > > missed > > > something I > > > > don't > > > > > think there's any valid case > where you > > wouldn't want > > > to get those > > > > > values. If you don't have them, > then you > > have no way of > > > > knowing what > > > > > mapping the encoder used, so no > way of > > decoding. > > > Instead, I > > > > would just > > > > > return OPUS_BAD_ARG if they're NULL. > > > > > > > > > > Done. > > > > > > > > > > > > > > > 11) So one issue I just noticed > is that > > > > opus_projection_encode() and > > > > > opus_projection_encode_float() > (same for the > > > decoder) use > > > > arbitrarily > > > > > large amounts of stack memory > for "buf". In > > > > opus_multistream_encode() > > > > > that's avoided by converting > just two > > channels at a > > > time, but > > > > here it's > > > > > not quite clear how to do that > without > > duplicating a > > > lot of the > > > > > multistream code. If we can't > address the > > issue with > > > this > > > > patch, the > > > > > least would be to abort when > trying to use > > these > > > calls with > > > > > NONTHREADSAFE_PSEUDOSTACK > > > > > > > > > > Done. Let me know if we should try > designing > > something > > > around this. > > > > > > > > > > > > > > > 12) I think > opus_projection_decoder_ctl() > > is missing a > > > > va_end() call. > > > > > > > > > > > > > > > Done. > > > > > > > > > > > > > > > > > > > > Cheers, > > > > > > > > > > Jean-Marc > > > > > > > > > > > > > > > > > > > > On 10/12/2017 05:44 PM, Drew > Allen wrote: > > > > > > thanks for all your feedback. > here's the > > revised > > > patch: > > > > > > > > > > > > On Wed, Oct 11, 2017 at 2:20 > PM Timothy > > B. Terriberry > > > > > <tterribe at xiph.org > <mailto:tterribe at xiph.org> > > <mailto:tterribe at xiph.org <mailto:tterribe at xiph.org>> > <mailto:tterribe at xiph.org <mailto:tterribe at xiph.org> > > <mailto:tterribe at xiph.org <mailto:tterribe at xiph.org>>> > > > <mailto:tterribe at xiph.org > <mailto:tterribe at xiph.org> <mailto:tterribe at xiph.org > <mailto:tterribe at xiph.org>> > > <mailto:tterribe at xiph.org <mailto:tterribe at xiph.org> > <mailto:tterribe at xiph.org <mailto:tterribe at xiph.org>>>> > > > > <mailto:tterribe at xiph.org > <mailto:tterribe at xiph.org> > > <mailto:tterribe at xiph.org <mailto:tterribe at xiph.org>> > <mailto:tterribe at xiph.org <mailto:tterribe at xiph.org> > > <mailto:tterribe at xiph.org <mailto:tterribe at xiph.org>>> > > > <mailto:tterribe at xiph.org > <mailto:tterribe at xiph.org> <mailto:tterribe at xiph.org > <mailto:tterribe at xiph.org>> > > <mailto:tterribe at xiph.org <mailto:tterribe at xiph.org> > <mailto:tterribe at xiph.org <mailto:tterribe at xiph.org>>>>> > > > > > > <mailto:tterribe at xiph.org > <mailto:tterribe at xiph.org> > > <mailto:tterribe at xiph.org <mailto:tterribe at xiph.org>> > > > <mailto:tterribe at xiph.org > <mailto:tterribe at xiph.org> <mailto:tterribe at xiph.org > <mailto:tterribe at xiph.org>>> > > <mailto:tterribe at xiph.org <mailto:tterribe at xiph.org> > <mailto:tterribe at xiph.org <mailto:tterribe at xiph.org>> > > > <mailto:tterribe at xiph.org > <mailto:tterribe at xiph.org> <mailto:tterribe at xiph.org > <mailto:tterribe at xiph.org>>>> > > > > <mailto:tterribe at xiph.org > <mailto:tterribe at xiph.org> > > <mailto:tterribe at xiph.org <mailto:tterribe at xiph.org>> > <mailto:tterribe at xiph.org <mailto:tterribe at xiph.org> > > <mailto:tterribe at xiph.org <mailto:tterribe at xiph.org>>> > > > <mailto:tterribe at xiph.org > <mailto:tterribe at xiph.org> <mailto:tterribe at xiph.org > <mailto:tterribe at xiph.org>> > > <mailto:tterribe at xiph.org <mailto:tterribe at xiph.org> > <mailto:tterribe at xiph.org <mailto:tterribe at xiph.org>>>>>>> wrote: > > > > > > > > > > > > Jean-Marc Valin wrote: > > > > > > > I think you'll want > something like: > > > > > > > > > > > (opus_int16)((unsigned)demixing_matrix[2*i+1] << 8) > > > > > > > (though you might want > to check it > > too) > > > > > > > > > > > > FWIW, we use the construct > > > > > > int s = buf[2*i + 1] << > 8 | buf[2*i]; > > > > > > s = ((s & 0xFFFF) ^ > 0x8000) - 0x8000; > > > > > > > > > > > > to manually sign-extend > the result in > > > opus_compare (and also > > > > > opus_demo). > > > > > > If you ultimately cast s to > > (opus_int16), then I'm > > > > pretty sure > > > > > most > > > > > > compilers will turn the > second line > > into a > > > no-op and > > > > optimize > > > > > it away. > > > > > > > > _______________________________________________ > > > > > > opus mailing list > > > > > > opus at xiph.org > <mailto:opus at xiph.org> <mailto:opus at xiph.org <mailto:opus at xiph.org>> > > <mailto:opus at xiph.org <mailto:opus at xiph.org> > <mailto:opus at xiph.org <mailto:opus at xiph.org>>> > > > <mailto:opus at xiph.org <mailto:opus at xiph.org> > <mailto:opus at xiph.org <mailto:opus at xiph.org>> > > <mailto:opus at xiph.org <mailto:opus at xiph.org> > <mailto:opus at xiph.org <mailto:opus at xiph.org>>>> > > > > <mailto:opus at xiph.org > <mailto:opus at xiph.org> <mailto:opus at xiph.org <mailto:opus at xiph.org>> > > <mailto:opus at xiph.org <mailto:opus at xiph.org> > <mailto:opus at xiph.org <mailto:opus at xiph.org>>> > > > <mailto:opus at xiph.org <mailto:opus at xiph.org> > <mailto:opus at xiph.org <mailto:opus at xiph.org>> > > <mailto:opus at xiph.org <mailto:opus at xiph.org> > <mailto:opus at xiph.org <mailto:opus at xiph.org>>>>> > > > <mailto:opus at xiph.org <mailto:opus at xiph.org> > <mailto:opus at xiph.org <mailto:opus at xiph.org>> > > <mailto:opus at xiph.org <mailto:opus at xiph.org> > <mailto:opus at xiph.org <mailto:opus at xiph.org>>> > > > > <mailto:opus at xiph.org > <mailto:opus at xiph.org> <mailto:opus at xiph.org <mailto:opus at xiph.org>> > > <mailto:opus at xiph.org <mailto:opus at xiph.org> > <mailto:opus at xiph.org <mailto:opus at xiph.org>>>> > > > > > <mailto:opus at xiph.org > <mailto:opus at xiph.org> > > <mailto:opus at xiph.org <mailto:opus at xiph.org>> > <mailto:opus at xiph.org <mailto:opus at xiph.org> > <mailto:opus at xiph.org <mailto:opus at xiph.org>>> > > > <mailto:opus at xiph.org <mailto:opus at xiph.org> > <mailto:opus at xiph.org <mailto:opus at xiph.org>> > > <mailto:opus at xiph.org <mailto:opus at xiph.org> > <mailto:opus at xiph.org <mailto:opus at xiph.org>>>>>> > > > > > > > > http://lists.xiph.org/mailman/listinfo/opus > > > > > > > > > > > > > > > > > > > > > > > > > > _______________________________________________ > > > > > > opus mailing list > > > > > > opus at xiph.org > <mailto:opus at xiph.org> <mailto:opus at xiph.org <mailto:opus at xiph.org>> > > <mailto:opus at xiph.org <mailto:opus at xiph.org> > <mailto:opus at xiph.org <mailto:opus at xiph.org>>> > > > <mailto:opus at xiph.org <mailto:opus at xiph.org> > <mailto:opus at xiph.org <mailto:opus at xiph.org>> > > <mailto:opus at xiph.org <mailto:opus at xiph.org> > <mailto:opus at xiph.org <mailto:opus at xiph.org>>>> > > > <mailto:opus at xiph.org <mailto:opus at xiph.org> > <mailto:opus at xiph.org <mailto:opus at xiph.org>> > > <mailto:opus at xiph.org <mailto:opus at xiph.org> > <mailto:opus at xiph.org <mailto:opus at xiph.org>>> > > > > <mailto:opus at xiph.org > <mailto:opus at xiph.org> <mailto:opus at xiph.org <mailto:opus at xiph.org>> > > <mailto:opus at xiph.org <mailto:opus at xiph.org> > <mailto:opus at xiph.org <mailto:opus at xiph.org>>>>> > > > > > > > http://lists.xiph.org/mailman/listinfo/opus > > > > > > > > > > > > > > > > > > > > > > > >