[FFmpeg-devel] AAC decoder round 9

[Robert Swain]

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. :)

Rob