[FFmpeg-devel] AMR-NB decoder

[Michael Niedermayer]

On Thu, Aug 13, 2009 at 09:33:49AM +0100, Colin McQuillan wrote:
> 2009/8/13 Michael Niedermayer <michaelni at gmx.at>:
> > On Wed, Aug 12, 2009 at 11:40:59PM +0100, Robert Swain wrote:
> >> Hello,
> >>
> >> 2009/8/10 Colin McQuillan <m.niloc at googlemail.com>:
> >> > Reynaldo, Robert, could you review your parts of this patch please? I
> >> > checked the QCELP ref decoder before writing the comment.
> >> >
> >> > 2009/8/10 Michael Niedermayer <michaelni at gmx.at>:
> >> >> On Mon, Aug 10, 2009 at 08:42:53AM +0100, Colin McQuillan wrote:
> >> >>> 2009/8/9 M?ns Rullg?rd <mans at mansr.com>:
> >> >>> > Colin McQuillan <m.niloc at googlemail.com> writes:
> >> >>> >
> >> >>> >> 2009/8/8 Michael Niedermayer <michaelni at gmx.at>:
> >> >>> >>> On Sat, Aug 08, 2009 at 04:09:39PM +0100, Colin McQuillan wrote:
> >> >>> >>>> 2009/8/8 Michael Niedermayer <michaelni at gmx.at>:
> >> >>> >>>> > On Fri, Aug 07, 2009 at 08:23:53PM +0100, Colin McQuillan wrote:
> >> >>> >>>> >> 2009/8/6 Michael Niedermayer <michaelni at gmx.at>:
> >> >>> >>>> >> > On Wed, Aug 05, 2009 at 05:51:36PM +0100, Colin McQuillan wrote:
> >> >>> >>>> >> >> Attached is a patch for an AMR-NB decoder.
> >> >>> >>>>
> >> >>> >>>> [...]
> >> >>> >>>>
> >> >>> >>>> >> > that should e a seperate patch
> >> >>> >>>> >>
> >> >>> >>>> >> I'll leave this one until I investigate a version for sparse vectors. Attached:
> >> >>> >>>> >>
> >> >>> >>>> >> 1. Helper functions for gain control in floating-point codecs
> >> >>> >>>> >> I couldn't find a similar fixed point function to copy the function name.
> >> >>> >>>> >>
> >> >>> >>>> >> 2. Floating-point version of ff_acelp_high_pass_filter
> >> >>> >>>> >
> >> >>> >>>> >> ?acelp_vectors.c | ? 22 ++++++++++++++++++++++
> >> >>> >>>> >> ?acelp_vectors.h | ? 27 +++++++++++++++++++++++++++
> >> >>> >>>> >> ?2 files changed, 49 insertions(+)
> >> >>> >>>> >> f1abbee9b62c1779fd5fb1c634d4ab4294d8611d ?get-set-energyf.patch
> >> >>> >>>> >> Index: libavcodec/acelp_vectors.c
> >> >>> >>>> >> ===================================================================
> >> >>> >>>> >> --- libavcodec/acelp_vectors.c ? ? ? ?(revision 19606)
> >> >>> >>>> >> +++ libavcodec/acelp_vectors.c ? ? ? ?(working copy)
> >> >>> >>>> >> @@ -155,3 +155,25 @@
> >> >>> >>>> >> ? ? ? ? ?out[i] = weight_coeff_a * in_a[i]
> >> >>> >>>> >> ? ? ? ? ? ? ? ? + weight_coeff_b * in_b[i];
> >> >>> >>>> >> ?}
> >> >>> >>>> >> +
> >> >>> >>>> >> +float ff_energyf(const float *v, int length)
> >> >>> >>>> >> +{
> >> >>> >>>> >> + ? ?float sum = 0;
> >> >>> >>>> >> + ? ?int i;
> >> >>> >>>> >> +
> >> >>> >>>> >> + ? ?for (i = 0; i < length; i++)
> >> >>> >>>> >> + ? ? ? ?sum += v[i] * v[i];
> >> >>> >>>> >> +
> >> >>> >>>> >> + ? ?return sum;
> >> >>> >>>> >> +}
> >> >>> >>>> >
> >> >>> >>>> > ff_dot_productf)(
> >> >>> >>>>
> >> >>> >>>> Do you mean that ff_energyf is redundant? I've taken it out.
> >> >>> >>>
> >> >>> >>> hmm well, as you say it that way, ff_energyf() could be faster due to
> >> >>> >>> fewer mem reads, if that is te case in practice it could be kept
> >> >>> >>
> >> >>> >> ff_energyf is reliably 4% faster in my test, so I'll add it back in.
> >> >>> >
> >> >>> > That function has high simdicity so it should be added to dsputil and
> >> >>> > simdified.
> >> >>>
> >> >>> I'll try, but I didn't mean to imply that energy calculations are
> >> >>> critical to performance. The slow parts of the AMR decoder are the IIR
> >> >>> and FIR filters, which are already in celp_filters.c.
> >> >>
> >> >> if the SIMD energy is not reaching an overall (whole codec) 0.1% speedup over
> >> >> using a more generic SIMD dot product then its probably not worth it and
> >> >> could be ommited
> >> >
> >> > Great! Then I'll leave out ff_energyf, and avoid using "length" in the comments.
> >> >
> >> > [...]
> >> >
> >> >> alignment requirements are supposed to be written liks:
> >> >> void ff_vp3_idct_put_c(uint8_t *dest/*align 8*/, int line_size, DCTELEM *block/*align 16*/);
> >> >> also "/*" is not doxygen compatible and what the function does should be
> >> >> more verbosely described, energy isnt a mathematically clear term, dot
> >> >> product and sum or squares are.
> >> >>
> >> >
> >> > Changed.
> >>
> >> The only thing I don't like is the ff_scale_to() function name. I
> >> wouldn't know what it did without reading the description. I know a
> >> distaste for the use of 'energy' for a generic function has been
> >> expressed but I'd understand something like ff_acelp_scale_energy().
> >> Does anyone have any better suggestions? Reynaldo? Michael? Colin?
> >
> > if i remember what the function did ...
> >
> > scale_vector_to_given_sum_of_squares()
> >
> > its a little long though ...
> 
> Changed

iam not maintainer of qcelp but i think its ok given it was tested and works

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

When you are offended at any man's fault, turn to yourself and study your
own failings. Then you will forget your anger. -- Epictetus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090813/b782905c/attachment.pgp>