[FFmpeg-devel] AAC decoder round 9

[Robert Swain]

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

> of course iam still rather interrested which variable exactly is responsible
> for the compression loss ...

Aren't we all? :) And then Loren could have fun writing some new SIMD
for floats in yasm. :D

Rob