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> 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 tocreate 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>> 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> > > http://lists.xiph.org/mailman/listinfo/opus > > > > > > > > _______________________________________________ > > opus mailing list > > 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/20171031/857c4d93/attachment-0001.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: 0001-Support-for-Channel-Mapping-253.patch Type: text/x-patch Size: 90378 bytes Desc: not available URL: <http://lists.xiph.org/pipermail/opus/attachments/20171031/857c4d93/attachment-0001.bin>
> 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.Yeah OK, I forgot about the feature. Just to make sure I remember correctly... The (user) encoder code calls OPUS_PROJECTION_GET_DEMIXING_MATRIX_GAIN() and uses the result to fill the "gain" field of the header, right?> 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.Yes, I think that needs to be addressed. Since the feature isn't enabled by default and this patch is already complicated enough, I think it should be addressed in a separate patch. I'm not yet sure what the best approach is. That'll require some thought. I'm having another look at your revised patches and I'll get back to you. Cheers, Jean-Marc
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. 2) Why do you have #define MAPPING_MATRIX_C ? 3) Looks like MAPPING_MATRIX_MAX_SIZE is not longer useful, right? 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). 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>> 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>>> 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>> > > http://lists.xiph.org/mailman/listinfo/opus > > > > > > > > _______________________________________________ > > opus mailing list > > opus at xiph.org <mailto:opus at xiph.org> > > http://lists.xiph.org/mailman/listinfo/opus > > >
Here's another one. On Thu, Nov 2, 2017 at 9:54 AM Jean-Marc Valin <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>> 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>>> 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>> > > > http://lists.xiph.org/mailman/listinfo/opus > > > > > > > > > > > > _______________________________________________ > > > opus mailing list > > > 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/20171103/0c20aa71/attachment-0001.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: 0001-Support-for-Channel-Mapping-253.patch Type: text/x-patch Size: 90040 bytes Desc: not available URL: <http://lists.xiph.org/pipermail/opus/attachments/20171103/0c20aa71/attachment-0001.bin>
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>> 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>>> 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>>>> 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>>>>> 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>>>> > > > > 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>>> > > > > http://lists.xiph.org/mailman/listinfo/opus > > > > > > > > > > >
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> 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>> 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>>> > 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>>>> 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>>>>> 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>>>> > > > > > 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>>> > > > > > http://lists.xiph.org/mailman/listinfo/opus > > > > > > > > > > > > > > > > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.xiph.org/pipermail/opus/attachments/20171107/09fc69c3/attachment-0001.html>
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 > > > > > > > > > > > > > > > > >
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>