[opus] Antw: Re: [PATCH] Fix memory issue in Projection API

Ulrich Windl Ulrich.Windl at rz.uni-regensburg.de
Wed Nov 29 08:23:04 UTC 2017


Following the thread from outside, I think Drew should work on in-house quality assurance ;-)



> I think you just attached the wrong (previous) version of the patch.
> 
> 	Jean-Marc
> 
> On 11/28/2017 12:24 PM, Drew Allen wrote:
>> Done!
>> 
>> On Tue, Nov 28, 2017 at 12:12 AM Jean-Marc Valin <jmvalin at jmvalin.ca 
>> <mailto:jmvalin at jmvalin.ca>> wrote:
>> 
>>     I only had a quick look, but your patch looks good except for the:
>>       output[output_rows * i] = (1/(32768.f*128.f))*tmp;
>> 
>>     For floating point, you shouldn't do the >>7 either. Just remove the >>8
>>     from the floating-point calculation of tmp so that all the scaling is
>>     done in float.
>> 
>>     Cheers,
>> 
>>             Jean-Marc
>> 
>>     On 11/27/2017 04:01 PM, Drew Allen wrote:
>>     > Hi Jean-Marc,
>>     >
>>     > Attached is an updated patch with your suggestions + additional
>>     > corrections I caught over the weekend.
>>     >
>>     > Cheers,
>>     > Drew
>>     >
>>     > On Fri, Nov 24, 2017 at 10:08 AM Drew Allen <bitllama at google.com 
>>     <mailto:bitllama at google.com>
>>     > <mailto:bitllama at google.com <mailto:bitllama at google.com>>> wrote:
>>     >
>>     >     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 <mailto:jmvalin at jmvalin.ca>
>>     >     <mailto:jmvalin at jmvalin.ca <mailto: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>
>>     <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:
>>     >         >
>>     >         >     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>>
>>     >         <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:
>>     >         >     >
>>     >         >     >     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>>>
>>     >         >     <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 <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:
>>     >         >     >     >
>>     >         >     >     >     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>>>>
>>     >         >     >     <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 <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>
>>     <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 <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>>>>
>>     >         >     >     <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 <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>
>>     <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 <mailto:opus at xiph.org>
>>     <mailto:opus at xiph.org <mailto:opus at xiph.org>>>>>>
>>     >         >     >     >     >     >
>>     >         http://lists.xiph.org/mailman/listinfo/opus 
>>     >         >     >     >     >     >
>>     >         >     >     >     >
>>     >         >     >     >
>>     >         >     >
>>     >         >
>>     >
>> 
> _______________________________________________
> opus mailing list
> opus at xiph.org 
> http://lists.xiph.org/mailman/listinfo/opus


More information about the opus mailing list