[FFmpeg-devel] AAC decoder round 9

[Robert Swain]

2008/8/21 Vitor Sessak <vitor1001 at gmail.com>:
> 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.

Done.

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

Thanks. :)

Rob