[opus] [PATCH] Fix memory issue in Projection API
Jean-Marc Valin
jmvalin at jmvalin.ca
Thu Nov 23 18:42:57 UTC 2017
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>> 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>>> 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>>>> 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>>>
> > > > http://lists.xiph.org/mailman/listinfo/opus
> > > >
> > >
> >
>
More information about the opus
mailing list