similar to: [PATCH] Fix memory issue in Projection API

Hi Jean-Marc, Here's my responses: 1) I didn't change how multistream_decode_float works in the argument list... I noticed it changes it's arguments depending on whether FIXED_POINT is used. I copied this style for the projection API as well. If this isn't desired, we should make those changes separately. 2) See above. 3) I only zero out initially. For the matrix_multiply_out

Hey Jean-Marc, Thanks again! To your first point, I was only trying to copy how _multistream_'s c files function in this way, possibly that's worth refactoring as well (as a separate patch). I made the other changes you suggested. Cheers, Drew On Thu, Nov 23, 2017 at 9:02 AM Jean-Marc Valin <jmvalin at jmvalin.ca> wrote: > On 11/22/2017 06:23 PM, Drew Allen wrote: >

got it. actually that patch i sent you has something wrong with the mapping_matrix_multiply_short_out... let me fix that and will send you another patch soon. Cheers, Drew On Thu, Nov 23, 2017 at 10:34 AM Jean-Marc Valin <jmvalin at jmvalin.ca> wrote: > On 11/23/2017 01:28 PM, Drew Allen wrote: > > To your first point, I was only trying to copy how _multistream_'s c > >

Hi Jean-Marc, Attached is an updated patch. I had to include some of Mark's suggestions in order to get the tests to work correctly. I will still submit a separate patch for him for a few other concerns he had after this one clears. Cheers, Drew On Thu, Nov 23, 2017 at 10:42 AM Jean-Marc Valin <jmvalin at jmvalin.ca> wrote: > Actually, there's also something wrong with the

I've solely addressed this concern here. Cheers, Drew On Fri, Nov 24, 2017 at 9:15 AM Jean-Marc Valin <jmvalin at jmvalin.ca> wrote: > Hi Drew, > > I noticed you reverted the > output[output_rows * i] = (tmp + 16384) >> 15; > from the previous patch. That's still good. What should have been > changed is the float version: > output[output_rows * i] =

Aha good point! Im travelling this weekend but will submit another patch Monday morning. Cheers, Drew On Fri, Nov 24, 2017 at 9:15 AM Jean-Marc Valin <jmvalin at jmvalin.ca> wrote: > Hi Drew, > > I noticed you reverted the > output[output_rows * i] = (tmp + 16384) >> 15; > from the previous patch. That's still good. What should have been > changed is the float

I only had a quick look, but your patch looks good except for the: output[output_rows * i] = (1/(32768.f*128.f))*tmp; For floating point, you shouldn't do the >>7 either. Just remove the >>8 from the floating-point calculation of tmp so that all the scaling is done in float. Cheers, Jean-Marc On 11/27/2017 04:01 PM, Drew Allen wrote: > Hi Jean-Marc, > > Attached is

I think you just attached the wrong (previous) version of the patch. Jean-Marc On 11/28/2017 12:24 PM, Drew Allen wrote: > Done! > > On Tue, Nov 28, 2017 at 12:12 AM Jean-Marc Valin <jmvalin at jmvalin.ca > <mailto:jmvalin at jmvalin.ca>> wrote: > > I only had a quick look, but your patch looks good except for the: >   output[output_rows * i] =

Hi Drew, Had a look at your patch. I think it's on the right track -- there's no fundamental issue with it as far as I can tell. There's a few implementation issues though. 1) In opus_multi_stream_decode_float(), you changed the pcm argument from float* to opus_val16*. That's a mistake and will fail for fixed-point. This should have shown up as an error on a fixed-point build.

On 11/22/2017 06:23 PM, Drew Allen wrote: > 1) I didn't change how multistream_decode_float works in the argument > list... I noticed it changes it's arguments depending on whether > FIXED_POINT is used. I copied this style for the projection API as well. > If this isn't desired, we should make those changes separately. > > 2) See above. Just noticed the code will

On 11/23/2017 01:28 PM, Drew Allen wrote: > To your first point, I was only trying to copy how _multistream_'s c > files function in this way, possibly that's worth refactoring as well > (as a separate patch). Well, the opus_multistream_decode_* calls were correct before your patch. It's only the encode ones that have the issues (should be addressed separately). Jean-Marc

Actually, there's also something wrong with the in_short() function. For floating point (#else case), you shouldn't need shifting since you're already doing the scaling through a float multiply. Jean-Marc On 11/23/2017 01:39 PM, Drew Allen wrote: > got it. actually that patch i sent you has something wrong with the > mapping_matrix_multiply_short_out... let me fix that and

Hi Drew, I noticed you reverted the output[output_rows * i] = (tmp + 16384) >> 15; from the previous patch. That's still good. What should have been changed is the float version: output[output_rows * i] = (1/32768.f) * ((tmp + 16384) >> 15); which should just be: output[output_rows * i] = (1/(32768.f*32768.f)) * tmp; since there's no point in doing integer rounding when you

Hi Jean-Marc, Attached is an updated patch with your suggestions + additional corrections I caught over the weekend. Cheers, Drew On Fri, Nov 24, 2017 at 10:08 AM Drew Allen <bitllama at google.com> wrote: > Aha good point! Im travelling this weekend but will submit another patch > Monday morning. > > Cheers, > Drew > On Fri, Nov 24, 2017 at 9:15 AM Jean-Marc Valin

Made a few minor tweaks to your patch (attached). Can you confirm you're OK with those and I haven't missed anything? Cheers, Jean-Marc On 12/04/2017 06:34 PM, Drew Allen wrote: > I've solely addressed this concern here. > > Cheers, > Drew > > On Fri, Nov 24, 2017 at 9:15 AM Jean-Marc Valin <jmvalin at jmvalin.ca > <mailto:jmvalin at jmvalin.ca>>

Done! On Tue, Nov 28, 2017 at 12:12 AM Jean-Marc Valin <jmvalin at jmvalin.ca> wrote: > I only had a quick look, but your patch looks good except for the: > output[output_rows * i] = (1/(32768.f*128.f))*tmp; > > For floating point, you shouldn't do the >>7 either. Just remove the >>8 > from the floating-point calculation of tmp so that all the scaling is

Hello, for your consideration. The following patch moves the channel_pos() function from within the #if !defined(DISABLE_FLOAT_API). This change is required when compiling with FIXED_POINT and DISABLE_FLOAT_API defined. #### ### diff --git a/src/opus_multistream_encoder.c b/src/opus_multistream_encoder.c index 3efab53..6f3eb53 100644 --- a/src/opus_multistream_encoder.c +++

Following the thread from outside, I think Drew should work on in-house quality assurance ;-) > I think you just attached the wrong (previous) version of the patch. > > Jean-Marc > > On 11/28/2017 12:24 PM, Drew Allen wrote: >> Done! >> >> On Tue, Nov 28, 2017 at 12:12 AM Jean-Marc Valin <jmvalin at jmvalin.ca >> <mailto:jmvalin at

Hi Drew, The float code should also be doing float multiplications. Make sure tmp is an opus_val32 and the multiplication itself casts to float rather than int32. Otherwise, the float version is likely to overflow. Jean-Marc On 11/30/2017 12:06 PM, Drew Allen wrote: > My apologies. I forgot to commit the changes before creating that last > patch. Yes, some in-house control would be good,

Should call celt_inner_prod(). --- celt/bands.c | 7 ++++--- celt/bands.h | 2 +- celt/celt_encoder.c | 6 +++--- celt/pitch.c | 2 +- src/opus_multistream_encoder.c | 2 +- 5 files changed, 10 insertions(+), 9 deletions(-) diff --git a/celt/bands.c b/celt/bands.c index bbe8a4c..1ab24aa 100644 --- a/celt/bands.c +++ b/celt/bands.c