[FFmpeg-devel] [PATCH] AAC Decoder round 3

[Robert Swain]

2008/7/1 Diego Biurrun <diego at biurrun.de>:
> On Tue, Jul 01, 2008 at 12:37:11PM +0100, Robert Swain wrote:
>>
>> Another submission for full review, this time without both LTP and
>> SSR. The code for LTP and SSR is still in the Summer of Code
>> repository for anyone who is interested in working on either of those
>> features in the future.
>
> Build system part OK, but changelog and documentation updates are
> missing.

OK. I'll have a look to see what needs doing for these.

> What I also miss is a comment at the top of the file that explains which
> parts of AAC are supported and which are not.  I must have asked you
> about a dozen times and I keep forgetting, so it's a good idea to put
> this into writing :)

I don't recall you saying this to me but maybe I'm wrong. I was
actually thinking about looking through the code and figuring this out
so we know what isn't supported and so on. I'll look into this but I'd
appreciate it not being a blocker for getting this into svn. I will
add it but it will take some time to check.

> Some nitpicking below..
>
>> --- libavcodec/aactab.h       (revision 0)
>> +++ libavcodec/aactab.h       (revision 0)
>> @@ -0,0 +1,1016 @@
>> +
>> +static const float tns_tmp2_map_1_3[TNS_MAX_ORDER] = {
>> +    0.00000000,    0.43388373, -0.64278758, -0.34202015,
>> +    0.97492790, 0.78183150, -0.64278758, -0.34202015,
>> +    -0.43388373, -0.78183150, -0.64278758, -0.34202015,
>> +    -0.78183150, -0.43388373, -0.64278758, -0.34202015,
>> +    0.78183150, 0.97492790, -0.64278758, -0.34202015
>> +};
>
> Some of the tables, like this one for example, could be more readable if
> you aligned them.  This is not terribly important of course.

Done.

>> --- libavcodec/aac.c  (revision 0)
>> +++ libavcodec/aac.c  (revision 0)
>> @@ -0,0 +1,1780 @@
>> +
>> +typedef struct {
>> +    int che_type[4][MAX_TAGID];   ///< channel element type with the first index as the first 4 raw_data_block IDs
>> +
>> +    int mono_mixdown;         ///< The SCE tag to use if user requests mono   output, -1 if not available.
>> +    int stereo_mixdown;       ///< The CPE tag to use if user requests stereo output, -1 if not available.
>> +    int mixdown_coeff_index;  ///< 0-3
>> +    int pseudo_surround;      ///< Mix surround channels out of phase.
>> +
>> +} ProgramConfig;
>
> Why the empty lines?  You don't do that in other places.  And the
> comments could be aligned.

Done. This code isn't all mine. They're probably latent new lines from
other changes. :)

>> +typedef struct {
>> +    // CPE specific
>> +    int common_window;     ///< Set if channels share a common 'IndividualChannelStream' in bitstream.
>
> in the bitstream
>
>> +    /**
>> +     * @defgroup temporary aligned temporary buffers (We do not want to have these on the stack.)
>
> unnecessary long line

Added \n for the reeeeeally long lines.

>> +//aux
>> +// TODO: Maybe add to dsputil?!
>> +#if 0
>> +static void vector_fmul_add_add_add(AACContext * ac, float * dst, const float * src0, const float * src1, const float * src2, const float * src3, float src4, int len) {
>> +    int i;
>> +    ac->dsp.vector_fmul_add_add(dst, src0, src1, src2, src4, len, 1);
>> +    for (i = 0; i < len; i++)
>> +        dst[i] += src3[i];
>> +}
>> +#endif
>
> And what is this #if 0 code doing here?  The function is rather trivial,
> IMO you can remove it.

There is code under #if 0 further down that uses it and we may need to
reenable it if we find a sample that needs it. As such I would like to
keep it there for the moment unless Michael says to rip it out.

>> +/** Parse extension data (incomplete).
>> + */
>
> nit: Keep this comment on one line.

I made it consistent with other such comments rather than making this
one an exception.

Rob