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

Jean-Marc Valin jmvalin at jmvalin.ca
Thu Nov 30 18:38:10 UTC 2017


Hi Drew,

The float code should also be doing float multiplications. Make sure tmp
is an opus_val32 and the multiplication itself casts to float rather
than int32. Otherwise, the float version is likely to overflow.

	Jean-Marc

On 11/30/2017 12:06 PM, Drew Allen wrote:
> My apologies. I forgot to commit the changes before creating that last
> patch. Yes, some in-house control would be good, I agree. I'll ensure my
> future patches are verified internally before submitting anything new.
> 
> Cheers,
> Drew
> 
> On Wed, Nov 29, 2017 at 9:04 AM Jean-Marc Valin <jmvalin at jmvalin.ca
> <mailto:jmvalin at jmvalin.ca>> wrote:
> 
>     On 11/29/2017 03:23 AM, Ulrich Windl wrote:
>     > Following the thread from outside, I think Drew should work on
>     in-house quality assurance ;-)
> 
>     Happens to everyone, myself (definitely) included.
> 
>             Jean-Marc
> 
>     >
>     >
>     >> 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>
>     >>> <mailto: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>>
>     >>>     > <mailto: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>>
>     >>>     >     <mailto: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>>>
>     >>>     >         > <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:
>     >>>     >         >
>     >>>     >         >     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>>>>
>     >>>     >         >     > <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/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>>>>>
>     >>>     >         >     >     > <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:
>     >>>     >         >     >     >
>     >>>     >         >     >     >     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>>>>>>
>     >>>     >         >     >     >     > <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 <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>>>>>>
>     >>>     >         <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 <mailto:opus at xiph.org>>>>>>>
>     >>>     >         >     >     >     >     >
>     >>>     >         http://lists.xiph.org/mailman/listinfo/opus
>     >>>     >         >     >     >     >     >
>     >>>     >         >     >     >     >
>     >>>     >         >     >     >
>     >>>     >         >     >
>     >>>     >         >
>     >>>     >
>     >>>
>     >> _______________________________________________
>     >> opus mailing list
>     >> opus at xiph.org <mailto:opus at xiph.org>
>     >> http://lists.xiph.org/mailman/listinfo/opus
>     > _______________________________________________
>     > opus mailing list
>     > opus at xiph.org <mailto:opus at xiph.org>
>     > http://lists.xiph.org/mailman/listinfo/opus
>     >
> 


More information about the opus mailing list