[FFmpeg-devel] AAC decoder round 9

Robert Swain robert.swain
Thu Aug 21 09:28:15 CEST 2008


2008/8/21 Michael Niedermayer <michaelni at gmx.at>:
> 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 ...

They were previously OKed. I have committed everything I want to
commit to trunk, from SoC now and enabled the AAC decoder. I will post
a news article on ffmpeg.org shortly with the additions since the last
one.

I'd like to extend my sincere gratitude to Michael for all his
reviewing efforts and his perfectionism, to Kostya for all the
assistance and suggestions he's given me and to anyone else who...
helped made this possible. Hehehe. :) Thank you very much. If we were
in closer proximity I would suggest we have a partay. ;)

Of course, it doesn't end here. I will add myself as the maintainer of
the AAC decoder files. I intend to fix bugs, continue clean up of bits
that remain to be tended to that were not of immediate priority and to
add SBR and PS support.

Best regards,
Rob




More information about the ffmpeg-devel mailing list