[FFmpeg-devel] AAC decoder round 9

[Michael Niedermayer]

On Thu, Aug 21, 2008 at 01:00:07AM +0100, Robert Swain wrote:
> 2008/8/21 Michael Niedermayer <michaelni at gmx.at>:
> > On Wed, Aug 20, 2008 at 11:00:11PM +0100, Robert Swain wrote:
> >> 2008/8/20 Robert Swain <robert.swain at gmail.com>:
> >> > 2008/8/20 Michael Niedermayer <michaelni at gmx.at>:
> >> >> On Wed, Aug 20, 2008 at 07:44:24PM +0100, Robert Swain wrote:
> >> >>> 2008/8/20 Robert Swain <robert.swain at gmail.com>:
> >> >>> > 2008/8/20 Vitor Sessak <vitor1001 at gmail.com>:
> >> >>> >> Robert Swain wrote:
> >> >> [...]
> >> >>> >>> It seems the various conditions on in[] and f0 cause it to exit with
> >> >>> >>> an error with the coef[] vectors I've tried and as they're potentially
> >> >>> >>> valid, that's disconcerting for using this code.
> >> >>> >>>
> >> >>> >>> compute_lpc_coefs() accepts a two dimensional lpc array as an
> >> >>> >>> argument. I'm guessing this so that the coefficients are available for
> >> >>> >>> various orders ready for testing to choose which is best or something
> >> >>> >>> like that.
> >> >>> >>>
> >> >>> >>> Do you have any advice for how to proceed? I'm going to keep prodding
> >> >>> >>> eval_lpc_coeffs() in the mean time.
> >> >>> >>
> >> >>> >> Can the following patch be modified to work with AAC too?
> >> >>> >
> >> >>> > I don't like the patch as it is, but you've helped me quite a bit to
> >> >>> > figure this out. :)
> >> >>> >
> >> >>> > lpc[0] = 1 isn't used so I'd like to remove it and start the useful
> >> >>> > coefficients from index 0. With this, the above-quoted loop can be
> >> >>> > rewritten to use part of compute_lpc_coefs():
> >> >>> >
> >> >>> > // tns_decode_coef
> >> >>> > for (m = 0; m < order; m++) {
> >> >>> >    float tmp;
> >> >>> >    lpc[m] = tns->coef[w][filt][m];
> >> >>> >    for (i = 0; i < m/2; i++) {
> >> >>> >        tmp = lpc[i];
> >> >>> >        lpc[i]     += lpc[m] * lpc[m-1-i];
> >> >>> >        lpc[m-1-i] += lpc[m] * tmp;
> >> >>> >    }
> >> >>> >    if(m & 1)
> >> >>> >        lpc[i]     += lpc[m] * lpc[i];
> >> >>> > }
> >> >>> >
> >> >>> > Doing it this way avoids the need for the b[] temporary array and
> >> >>> > produces identical output I think. You should be able to see clearly
> >> >>> > that this is the same as the main loops within compute_lpc_coefs().
> >> >>> > So, now the question is how to refactor this code to satisfy everyone.
> >> >>> >
> >> >>> > Michael - would you like a single generic macro like the one Vitor
> >> >>> > proposes? Or maybe two macros, one for this inner set of loops and one
> >> >>> > for the extra autocorrelation function return value normalisation to
> >> >>> > convert to autocorrelation coefficients and checks that are needed for
> >> >>> > the other implementations? Or something else?
> >> >>>
> >> >>> Actually, I think an inline function will be better. I'll have a look
> >> >>> a go at implementing this after I've eaten.
> >> >>
> >> >> thats exactly what i wanted to suggest, all these macros are ugly ...
> >> >> (the reason behind vitors macros was IIRC a compression loss with float
> >> >>  vs.double in relation to flac but i didnt had the time to investigate
> >> >>  what excatly was causing it or if there is a better solution ...)
> >> >
> >> > Hmm. Will this cause a problem if I use a double temporary variable? I
> >> > think I've figured everything else out in my head. Though maybe
> >> > there's something I've missed, I'll find out shortly. :)
> >>
> >> We could:
> >>
> >> - have two shared versions of this function - one float and one double
> >> - share a float version between aac and ra288
> >> - have one function which uses void pointers and uses nasty pointer arithmetic
> >> - use a macro like Vitor tried
> >> - try to find the issue in flac causing the decrease in compression
> >> efficiency when using float rather than double as it is for this
> >> function at the moment but this could take some time
> >>
> >> Unless we can choose a clear course of action, I'm inclined to push
> >> that this last hunk be committed so I can work on more worthwhile
> >> things such as imdct_half() porting and SBR support. 8 lines of code
> >> in aac.c that could use something shared from elsewhere doesn't seem
> >> all that important in the grand scheme of things... but that's my
> >> opinion. :)
> >
> > ok, commit it, its not reasonable to hold this up due to a non trivial
> > simplification of 8 lines of code ...
> 
> So that's the final OK? Commit all that remains and the build system
> and documentation and so on? I'll also make a commit to ffmpeg.org
> news I think. :)

documentation and build system changes are not my area ....
the tns addition is fine ...

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

Awnsering whenever a program halts or runs forever is
On a turing machine, in general impossible (turings halting problem).
On any real computer, always possible as a real computer has a finite number
of states N, and will either halt in less than N cycles or never halt.
-------------- 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/20080821/0f16b610/attachment.pgp>