search for: opus_projection_encod

...gt; at 0x100001330: test_creation_arguments (test_opus_projection.c:252) > > Additionally, the demixing_matrix appears to be used uninitialized > when order_plus_one is 1 (or greater than 4): > > Conditional jump or move depends on uninitialised value(s) > at 0x10006A6DF: opus_projection_encoder_init_impl > (opus_projection_encoder.c:125) > by 0x10006A9CA: opus_projection_ambisonics_encoder_init > (opus_projection_encoder.c:196) > by 0x10006AC7B: opus_projection_ambisonics_encoder_create > (opus_projection_encoder.c:261) > by 0x1000013D9: test_creation_argum...

Hello all, Attached is the initial proposed implementation for ch.mapping 253/3, based on the IETF proposal: https://tools.ietf.org/html/draft-ietf-codec-ambisonics-03 A brief overview of the patch, as it is slightly lengthy: After discussion with Jean-Marc, we determined that ch.253/3 will need the demixing matrix as part of the encoder/decoder struct stack and thus will require a new

...(test_opus_projection.c:252) >>> >>> Additionally, the demixing_matrix appears to be used uninitialized >>> when order_plus_one is 1 (or greater than 4): >>> >>> Conditional jump or move depends on uninitialised value(s) >>> at 0x10006A6DF: opus_projection_encoder_init_impl >>> (opus_projection_encoder.c:125) >>> by 0x10006A9CA: opus_projection_ambisonics_encoder_init >>> (opus_projection_encoder.c:196) >>> by 0x10006AC7B: opus_projection_ambisonics_encoder_create >>> (opus_projection_encoder.c:261) >...

...s created by a stack allocation at 0x100001330: test_creation_arguments (test_opus_projection.c:252) Additionally, the demixing_matrix appears to be used uninitialized when order_plus_one is 1 (or greater than 4): Conditional jump or move depends on uninitialised value(s) at 0x10006A6DF: opus_projection_encoder_init_impl (opus_projection_encoder.c:125) by 0x10006A9CA: opus_projection_ambisonics_encoder_init (opus_projection_encoder.c:196) by 0x10006AC7B: opus_projection_ambisonics_encoder_create (opus_projection_encoder.c:261) by 0x1000013D9: test_creation_arguments (test_opus_projection.c:2...

...test_creation_arguments (test_opus_projection.c:252) >> >> Additionally, the demixing_matrix appears to be used uninitialized >> when order_plus_one is 1 (or greater than 4): >> >> Conditional jump or move depends on uninitialised value(s) >> at 0x10006A6DF: opus_projection_encoder_init_impl >> (opus_projection_encoder.c:125) >> by 0x10006A9CA: opus_projection_ambisonics_encoder_init >> (opus_projection_encoder.c:196) >> by 0x10006AC7B: opus_projection_ambisonics_encoder_create >> (opus_projection_encoder.c:261) >> by 0x100001...

...rename the function that validates the correct order before this is > called (It fails and hence the init() func fails if order_plus_one is < 2 or >> 4.) Where does it check for order_plus_one < 2 or > 4? The draft allows order 0 and also higher orders. The current check in src/opus_projection_encoder.c line 65 checks for "order_plus_one < 1 || order_plus_one > 15". I don't see a more restrictive check, and if there is one somewhere it doesn't prevent opus_projection_ambisonics_encoder_init() from accessing uninitialized memory. This can be seen by running test_opus_p...

Hi Mark, Attached are corrections based on your comments. I will extend these to the patch I recently submitted to fix memory issues once that is resolved as well. Cheers, Drew On Sat, Nov 18, 2017 at 5:48 PM Mark Harris <mark.hsj at gmail.com> wrote: > Hi Drew, > > Some additional comments on your mapping family 253 changes: > > 1) mapping_matrix_get_data: The computed

...end() to appear in the caller rather than in in the va_list() function itself. 3) I'm not sure I understand why the opus_copy_channel_out*() functions had to be moved. 4) OpusPEncoder: I'd prefer a more explicit name: OpusProjectionEncoder 5) Do you need to expose the "generic" opus_projection_encode() and opus_projection_encode_float() in the API? 6) In mapping_matrix_multiply_float(), for the !FIXED_POINT case you're multiplying by (1.f / 32768.f), but I think in the FIXED_POINT case, you should instead >>15 7) In mapping_matrix_multiply_short(), I would prefer >>15 instead...

...> function.* Basically, what I mean is that I *think* you should remove the calls to va_end() from _ctl_va_list() calls and instead have the regular _ctl() functions call va_end(). That appears to be how functions like vprintf() are called. > 5) Do you need to expose the "generic" opus_projection_encode() and > opus_projection_encode_float() in the API? > > *I'm confused... How else would people be able to encode a packet of > audio using the projection encoder? * Sorry, I got confused when reviewing. Forget about that point! > 6) In mapping_matrix_multiply_float(), for the...

...sis to another function.* 3) I'm not sure I understand why the opus_copy_channel_out*() functions had to be moved. *Not sure how that happened. Reverted.* 4) OpusPEncoder: I'd prefer a more explicit name: OpusProjectionEncoder *Done.* 5) Do you need to expose the "generic" opus_projection_encode() and opus_projection_encode_float() in the API? *I'm confused... How else would people be able to encode a packet of audio using the projection encoder? * 6) In mapping_matrix_multiply_float(), for the !FIXED_POINT case you're multiplying by (1.f / 32768.f), but I think in the FIXED_POI...

...ly, what I mean is that I *think* you should remove the calls to > va_end() from _ctl_va_list() calls and instead have the regular _ctl() > functions call va_end(). That appears to be how functions like vprintf() > are called. > > > 5) Do you need to expose the "generic" opus_projection_encode() and > > opus_projection_encode_float() in the API? > > > > *I'm confused... How else would people be able to encode a packet of > > audio using the projection encoder? * > > Sorry, I got confused when reviewing. Forget about that point! > > > 6) In mapp...

...re 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 dup...

...s that I *think* you should remove the calls to > va_end() from _ctl_va_list() calls and instead have the regular _ctl() > functions call va_end(). That appears to be how functions like vprintf() > are called. > > > 5) Do you need to expose the "generic" opus_projection_encode() and > > opus_projection_encode_float() in the API? > > > > *I'm confused... How else would people be able to encode a packet of > > audio using the projection encoder? * > > Sorry, I got confused when reviewing. Forget about that point! >...

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> 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 =

...'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 > > n...

...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. 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 th...

Hello all, Attached is an up-to-date patch for supporting channel mapping 253. Please advise and Thank you for your time. Cheers, Drew -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.xiph.org/pipermail/opus/attachments/20170919/055d9034/attachment-0001.html> -------------- next part -------------- A non-text attachment was scrubbed... Name:

...et 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...

...>     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() > >     &...

...> > > > > > > > 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&quot...