[opus] Antw: Re: [PATCH] Fix memory issue in Projection API
Ulrich Windl
Ulrich.Windl at rz.uni-regensburg.de
Wed Nov 29 08:23:04 UTC 2017
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 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
More information about the opus
mailing list