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

[Drew Allen]

Hi Jean-Marc,

Here's my responses:

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.

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.

4) See 3), short and float versions of multiply_out should function the
same.

Let me know your thoughts.

Cheers,
Drew

On Tue, Nov 21, 2017 at 7:39 PM Jean-Marc Valin <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
> > http://lists.xiph.org/mailman/listinfo/opus
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.xiph.org/pipermail/opus/attachments/20171122/700f8064/attachment.html>