[FFmpeg-devel] [PATCH] AAC decoder round 8

[Michael Niedermayer]

On Mon, Aug 18, 2008 at 06:42:32PM +0100, Robert Swain wrote:
> 2008/8/18 Robert Swain <robert.swain at gmail.com>:
> > 2008/8/18 Michael Niedermayer <michaelni at gmx.at>:
> >> On Mon, Aug 18, 2008 at 02:03:53PM +0100, Robert Swain wrote:
> >>> 2008/8/18 Michael Niedermayer <michaelni at gmx.at>:
> >>> > On Mon, Aug 18, 2008 at 01:47:42PM +0100, Robert Swain wrote:
> >>> >> 2008/8/18 Robert Swain <robert.swain at gmail.com>:
> >>> >> > 2008/8/18 Robert Swain <robert.swain at gmail.com>:
> >>> >> >> 2008/8/18 Robert Swain <robert.swain at gmail.com>:
> >> [...]
> >>> > [...]
> >>> >> +            for (m = 0; m < size; m++, start += inc)
> >>> >> +                for (i = 1; i <= FFMIN(m, order); i++)
> >>> >> +                    coef[start] -= coef[start - i] * lpc[i];
> >>> >>          }
> >>> >
> >>> > are you sure this is correct for both values of inc ?
> >>> > i mean one will have coef[start - i] unfiltered and one filtered as far as
> >>> > i can see ...
> >>>
> >>> You're right and I thought this was strange, but it's the same
> >>> behaviour as before from what I see and so in that sense it is as
> >>> correct as it was before. The spec says:
> >>>
> >>> tns_ar_filter( spectrum[], size, inc, lpc[], order )
> >>> {
> >>>  - Simple all-pole filter of order "order" defined by
> >>>    y(n) =  x(n) - lpc[1]*y(n-1) - ... - lpc[order]*y(n-order)
> >>>
> >>>  - The state variables of the filter are initialized to zero every time
> >>>
> >>>  - The output data is written over the input data ("in-place operation")
> >>>
> >>>  - An input vector of "size" samples is processed and the index increment
> >>>    to the next data sample is given by "inc"
> >>> }
> >>>
> >>> I think the third point suggests that it's the intended approach.
> >>
> >> but the first suggests it is not
> >> isnt there some reference implementation or something around?
> >
> > 3GPP reference code, tns.c (with cruft removed and indented sanely):
> >
> >
> > for (i=0; i<order; i++)
> >    state[i] = 0.0F;
> >
> > if (inc == -1)
> >    spec += size - 1;
> >
> > for (i=0; i<size; i++) {
> >    accu = *spec * lpc[0];
> >
> >    for (j=0; j<order; j++)
> >        accu -= lpc[j+1] * state[j];
> >
> >    for (j=order-1; j>0; j--)
> >        state[j] = state[j-1];
> >
> >    state[0] = accu;
> >    *spec = accu;
> >
> >    spec += inc;
> > }
> >
> >
> > 'spec' is basically equivalent to 'coef'. The ref code stores the
> > states from the beginning of the state array and shifts all the states
> > along to add a new one to the beginning. The SoC code stores them
> > starting from the end of the array and works backwards, wrapping back
> > to the end when the index goes past the beginning of the array. That
> > is, the ith state[0] == b[(order-1-i)%order].
> >
> > Maybe with some index trickery the above can be simplified to
> > something similar to what I was suggesting for the incorrect code.
> 
> I don't think much index trickery is needed actually, unless you can
> come up with something better than:
> 
> for (m = 0; m < size; m++, start += inc)
>     for (i = 1; i <= FFMIN(m, order); i++)
>         coef[start] -= coef[start - i*inc] * lpc[i];
> 
> The only difference is the *inc in the coef[] index. The reference
> code considers the y[n-i] relative to the filter direction so if inc
> is -1 the equation should be coef[start + i]. New patch attached.
> Still the same PSNR with the zodiac trailer sample. I guess it doesn't
> use any reverse direction TNS.

patch ok

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

I hate to see young programmers poisoned by the kind of thinking
Ulrich Drepper puts forward since it is simply too narrow -- Roman Shaposhnik
-------------- 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/20080818/49a5c143/attachment.pgp>