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

[Jean-Marc Valin]

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>> 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>>> 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>>
>     >     > http://lists.xiph.org/mailman/listinfo/opus
>     >     >
>     >
>