[opus] [PATCH] Fix memory issue in Projection API

Drew Allen bitllama at google.com
Fri Nov 24 18:08:09 UTC 2017


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 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>> 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>>> 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>>>> 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>>>>> 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>>>>
> >     >     >     >     > http://lists.xiph.org/mailman/listinfo/opus
> >     >     >     >     >
> >     >     >     >
> >     >     >
> >     >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.xiph.org/pipermail/opus/attachments/20171124/d5029211/attachment-0001.html>


More information about the opus mailing list