search for: matrix_index

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

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[]

...x[2*i+1] << 8) (though you might want to check it too) 2) In mapping_matrix_multiply_short(), I would recommend something along these lines (untested) to improve accuracy: opus_int32 tmp = 0; for (col = 0; col < input_rows; col++) { tmp += (matrix_data[MATRIX_INDEX(matrix->rows, row, col)] * input[MATRIX_INDEX(input_rows, col, i)]) >> 8; } output[MATRIX_INDEX(output_rows, row, i)] = (tmp+64)>>7; 3) Looking at your get_*() function to compute offsets, I think you're missing some align() calls in the terms. I think you...

...ef 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, Oc...

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

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

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

...2 bytes per cell, that limits us to only order 11 for a full-order square matrix). I can recast align before summing them though. > 3) In several places including the intermediate multiplication in the > computation of "tmp" in mapping_matrix_multiply_short, and even in the > MATRIX_INDEX macro, multiplication may overflow if int is 16 bits. > One of the operands of the multiplication should be cast to opus_int32 > in such cases to ensure a 32-bit multiply and avoid overflow. > > Done. > 4) get_multistream_decoder: align() must be applied separately to > sizeof(...

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

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

Hi Drew, On 10/10/17 02:29 PM, Drew Allen wrote: > 2) Someone on this list might know better than I do on that one, but for > the new _ctl_va_list() calls, I believe the convention is for va_start() > and va_end() to appear in the caller rather than in in the va_list() > function itself. > > *My understanding is that it's impossible to pass ellipsis to another >

...use opus_int32 since int may not be large enough for order 11 to 14. A complication is that align() currently takes and returns "int". 3) In several places including the intermediate multiplication in the computation of "tmp" in mapping_matrix_multiply_short, and even in the MATRIX_INDEX macro, multiplication may overflow if int is 16 bits. One of the operands of the multiplication should be cast to opus_int32 in such cases to ensure a 32-bit multiply and avoid overflow. 4) get_multistream_decoder: align() must be applied separately to sizeof(OpusProjectionDecoder) and st->dem...

...to only order 11 > for a full-order square matrix). I can recast align before summing them > though. >   > >  3) In several places including the intermediate multiplication in the > computation of "tmp" in mapping_matrix_multiply_short, and even in the > MATRIX_INDEX macro, multiplication may overflow if int is 16 bits. > One of the operands of the multiplication should be cast to opus_int32 > in such cases to ensure a 32-bit multiply and avoid overflow. > > Done. >   > >  4) get_multistream_decoder: align() must be applied se...