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

[Drew Allen]

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:
> > 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>> 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>
> >     > http://lists.xiph.org/mailman/listinfo/opus
> >     >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.xiph.org/pipermail/opus/attachments/20171123/141db788/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Fix-memory-issue-in-Projection-API.patch
Type: application/octet-stream
Size: 36813 bytes
Desc: not available
URL: <http://lists.xiph.org/pipermail/opus/attachments/20171123/141db788/attachment-0001.obj>