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

[Diego Biurrun]

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.

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

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.

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

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

> +//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

ditto

And what is this #if 0 code doing here?  The function is rather trivial,
IMO you can remove it.

> +/**
> + * Decode scale_factor_data; reference: table 4.47.
> + */
> +static int decode_scale_factor_data(AACContext * ac, GetBitContext * gb, float mix_gain, unsigned int global_gain, IndividualChannelStream * ics, const int cb[][64], const int cb_run_end[][64], float sf[][64]) {

looooong line, same in other places

> +/** Parse extension data (incomplete).
> + */

nit: Keep this comment on one line.

Diego