[opus] Antw: Re: [PATCH] Fix memory issue in Projection API
Jean-Marc Valin
jmvalin at jmvalin.ca
Wed Nov 29 17:04:08 UTC 2017
On 11/29/2017 03:23 AM, Ulrich Windl wrote:
> Following the thread from outside, I think Drew should work on in-house quality assurance ;-)
Happens to everyone, myself (definitely) included.
Jean-Marc
>
>
>> 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] = (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 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
>>> <mailto:bitllama at google.com>
>>> > <mailto:bitllama at google.com <mailto: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
>>> <jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca>
>>> > <mailto:jmvalin at jmvalin.ca <mailto: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] = (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
>>> have float
>>> > available.
>>> >
>>> > Cheers,
>>> >
>>> > Jean-Marc
>>> >
>>> > On 11/23/2017 10:35 PM, Drew Allen wrote:
>>> > > 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 <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:
>>> > >
>>> > > 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
>>> > will send you
>>> > > > another patch soon.
>>> > > >
>>> > > > Cheers,
>>> > > > Drew
>>> > > >
>>> > > > On Thu, Nov 23, 2017 at 10:34 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:
>>> > > >
>>> > > > 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
>>> > > >
>>> > > > > On Thu, Nov 23, 2017 at 9:02 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>>>>
>>> > > > > <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:
>>> > > > >
>>> > > > > 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 actually work
>>> because
>>> > > opus_val16 is
>>> > > > defined
>>> > > > > to the right thing, but I still think it's a
>>> > bad idea. For
>>> > > > example, if
>>> > > > > you look at the public header,
>>> > opus_projection_decode()
>>> > > has a pcm
>>> > > > > argument of type opus_int16*, so it's a bit
>>> > confusing
>>> > > for the
>>> > > > C file to
>>> > > > > define pcm as opus_val16*, even if the
>>> two map
>>> > to the same.
>>> > > > >
>>> > > > > > 3) I only zero out initially. For the
>>> > matrix_multiply_out
>>> > > > > functions, we
>>> > > > > > need to be able to take a single decoded
>>> > stream and
>>> > > add weighted
>>> > > > > > versions of it to each of the final output
>>> > channels. This
>>> > > > zero-ing out
>>> > > > > > on stream 0 ensures the block is
>>> zero-set so
>>> > we can
>>> > > > incrementally add
>>> > > > > > all the decoded streams to the output
>>> channels.
>>> > > > >
>>> > > > > OK, I see. In that case, you should replace
>>> > the two for()
>>> > > > loops with
>>> > > > > just a single call to OPUS_COPY(). That
>>> should
>>> > be both
>>> > > faster and
>>> > > > > simpler.
>>> > > > >
>>> > > > > > 4) See 3), short and float versions of
>>> > multiply_out should
>>> > > > > function the
>>> > > > > > same.
>>> > > > >
>>> > > > > Again, I now see what that code is doing.
>>> > Unfortunately,
>>> > > it means
>>> > > > > increased noise for fixed-point since we
>>> have
>>> > to round
>>> > > > multiple times. I
>>> > > > > don't have a proposed fix for that, so I
>>> guess
>>> > we'll have to
>>> > > > deal with
>>> > > > > it. However, instead of rounding tmp twice
>>> > (>>8 followed by
>>> > > > +64>>7), you
>>> > > > > should only round once. That means
>>> (tmp+16384)>>15
>>> > > > >
>>> > > > > Cheers,
>>> > > > >
>>> > > > > Jean-Marc
>>> > > > >
>>> > > > > > Let me know your thoughts.
>>> > > > > >
>>> > > > > > Cheers,
>>> > > > > > Drew
>>> > > > > >
>>> > > > > > On Tue, Nov 21, 2017 at 7:39 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>>>>>
>>> > > > > > <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 <mailto:jmvalin at jmvalin.ca>>>>>>> wrote:
>>> > > > > >
>>> > > > > > 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. Make sure you test that.
>>> > > > > >
>>> > > > > > 2) In opus_projection_decode(),
>>> you also
>>> > changed
>>> > > the pcm
>>> > > > > argument to an
>>> > > > > > opus_val16*, which again will cause
>>> > problems for
>>> > > the same
>>> > > > > reasons as 1).
>>> > > > > > Please check you haven't made that
>>> same
>>> > mistake
>>> > > elsewhere.
>>> > > > > >
>>> > > > > > 3) In
>>> > opus_projection_copy_channel_out_float() and
>>> > > > > >
>>> > opus_projection_copy_channel_out_short(), you're
>>> > > zeroing the
>>> > > > > values with
>>> > > > > > this loop:
>>> > > > > > for (i=0;i<frame_size;i++)
>>> > > > > > {
>>> > > > > > for (j=0;j<dst_stride;j++)
>>> > > > > > {
>>> > > > > > float_dst[i*dst_stride+j] = 0;
>>> > > > > > }
>>> > > > > > }
>>> > > > > > That looks wrong, since you'll be
>>> > overwriting all
>>> > > channels
>>> > > > > instead of
>>> > > > > > just the one you're copying. That
>>> should
>>> > have come
>>> > > up in
>>> > > > > testing, no?
>>> > > > > >
>>> > > > > > 4) You might want to have a look at
>>> > > > > >
>>> > mapping_matrix_multiply_channel_out_short(). I
>>> > > can't quite
>>> > > > > follow what
>>> > > > > > it's doing, but it seems wrong
>>> since the
>>> > whole
>>> > > point of
>>> > > > adding
>>> > > > > the "tmp"
>>> > > > > > is to avoid rounding multiple times.
>>> > > > > >
>>> > > > > > I haven't had time to thoroughly
>>> review
>>> > the changes to
>>> > > > > mapping_matrix.c,
>>> > > > > > but I'll do that on your revised
>>> version.
>>> > > > > >
>>> > > > > > Cheers,
>>> > > > > >
>>> > > > > > Jean-Marc
>>> > > > > >
>>> > > > > >
>>> > > > > >
>>> > > > > >
>>> > > > > > On 11/20/2017 05:15 PM, Drew Allen
>>> wrote:
>>> > > > > > > Hello,
>>> > > > > > >
>>> > > > > > > Attached is a patch to resolve a
>>> > memory issue
>>> > > using the
>>> > > > > Projection API
>>> > > > > > > when compiling using a
>>> psuedo-stack /
>>> > limited
>>> > > memory.
>>> > > > > > >
>>> > > > > > > Please let me any ?s you might have.
>>> > > > > > >
>>> > > > > > > Cheers,
>>> > > > > > > Drew
>>> > > > > > >
>>> > > > > > >
>>> > > > > > >
>>> > _______________________________________________
>>> > > > > > > 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
>> http://lists.xiph.org/mailman/listinfo/opus
> _______________________________________________
> opus mailing list
> opus at xiph.org
> http://lists.xiph.org/mailman/listinfo/opus
>
More information about the opus
mailing list