[FFmpeg-devel] AAC decoder round 9

Vitor Sessak vitor1001
Thu Aug 21 03:32:20 CEST 2008

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

Could you just change it so it is almost identical to the version in 
lpc.c (as you described in your previous post)? It'll make it easier the 
day someone decide to clean it up. Maybe a "/* FIXME: Duplication of 
code in lpc.c */" would be nice too.

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

Cool! Congratulations!


More information about the ffmpeg-devel mailing list