[opus] [PATCH] Fix memory issue in Projection API
Drew Allen
bitllama at google.com
Tue Nov 28 17:24:31 UTC 2017
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
> 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>> 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>> 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>>>
> 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>>>> 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>>>>>
> 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>>>>>>
> 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>>>>>
> > > > > > >
> > http://lists.xiph.org/mailman/listinfo/opus
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.xiph.org/pipermail/opus/attachments/20171128/801e9b50/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Fix-memory-issues-in-Projection-API.patch
Type: text/x-patch
Size: 41342 bytes
Desc: not available
URL: <http://lists.xiph.org/pipermail/opus/attachments/20171128/801e9b50/attachment-0001.bin>
More information about the opus
mailing list