[FFmpeg-devel] [PATCH] atrac1 decoder and aea demuxer

[Diego Biurrun]

On Tue, Sep 01, 2009 at 10:08:00PM +0200, Benjamin Larsson wrote:
> 
> --- libavcodec/atrac1.c	(revision 0)
> +++ libavcodec/atrac1.c	(revision 0)
> @@ -0,0 +1,447 @@
> +
> +/* Many thanks to Tim Craig who's help was vital during the development of this decoder */

whose

This line is unnecessarily long.

> +    DECLARE_ALIGNED_16(float,prev_spec[AT1_SU_SAMPLES]);    ///< previous frame mdct spectrum for each band(used for TDAC overlap)

MDCT spectrum of previous frame for each band (used for TDAC overlap)

It still sounds suboptimal..

> +    DECLARE_ALIGNED_16(float,fst_qmf_delay[46]);            ///< delay line for the 1st stacked qmf filter
> +    DECLARE_ALIGNED_16(float,snd_qmf_delay[46]);            ///< delay line for the 2nd stacked qmf filter
> +    DECLARE_ALIGNED_16(float,last_qmf_delay[256+23]);       ///< delay line for the last stacked qmf filter

QMF

> +    DECLARE_ALIGNED_16(float,spec[AT1_SU_SAMPLES]);         ///< the mdct spectrum buffer

MDCT

> +static float            sf_tab[64];

weird spacing

> +static void at1_imdct_transform(ATRAC1Context *q, float *spec, float *out, int nbits, int reverse_spectrum)

long line

This applies to most function declarations.

> +    switch(nbits) {

switch (

> +        for (i=0; i<transf_size/2; i++)

Please add spaces around operators.

> +    int     band_num, band_samples, bsm, nbits, num_blocks, block_size;
> +    unsigned int     start_pos, ref_pos=0, pos = 0;

weird spacing

> +static int at1_parse_block_size_mode(GetBitContext* gb, int bsm[AT1_QMF_BANDS])

Are all the at1_ prefixes for static functions necessary?

> +    if (bsm_tmp&1)

Spaces around & would aid readability here.

> +    if (bsm_tmp&1)

ditto

> +    /* get word length index (idwl) for each BFU */
> +    for (i=0 ; i<su->num_bfus ; i++)
> +
> +    /* get scalefactor index (idsf) for each BFU */
> +    for (i=0 ; i<su->num_bfus ; i++)
> +
> +    /* zero idwl/idsf for empty BFUs */
> +    for (i = su->num_bfus; i < AT1_MAX_BFU; i++)
> +
> +    /* read in the spectral data and reconstruct MDCT spectrum of this channel */
> +    for (band_num=0 ; band_num<AT1_QMF_BANDS ; band_num++) {
> +        for (bfu_num=bfu_bands_t[band_num] ; bfu_num<bfu_bands_t[band_num+1] ; bfu_num++) {

Sometimes there are spaces around operators, sometimes not; sometimes
there are spaces before semicolons, sometimes not.  Please use spaces
around operators and drop the spaces before semicolons.

This applies to all other if/for constructs below..

> +            int         pos;

weird spaces

> +            if (word_len) {
> +                float       max_quant = 1.0/(float)((1 << (word_len - 1)) - 1);

ditto

> +                    spec[pos + i] = get_sbits(gb, word_len) *
> +                                            sf_tab[su->idsfs[bfu_num]] * max_quant;

indentation

> +//Same as atrac3 will be moved to a common file
> +void at1_iqmf(float *inlo, float *inhi, int32_t nIn, float *pOut, float *delayBuf, float *temp)

ff_ prefix?

> +    /* delay the signal of the high band by 23 samples */
> +    memcpy( su->last_qmf_delay,     &su->last_qmf_delay[256], sizeof(float)*23);
> +    memcpy(&su->last_qmf_delay[23], q->bands[2],       sizeof(float)*256);

That's only half-aligned :)

> +static int atrac1_decode_frame(AVCodecContext *avctx,
> +            void *data, int *data_size,
> +            AVPacket *avpkt)

indentation

> +        /* parse block_size_mode, 1st byte */
> +        if(ret)
> +        if(ret)
> +        if(ret)

if (ret)

> +static av_cold void init_mdct_windows()

(void)

> +AVCodec atrac1_decoder =
> +{

AVCodec atrac1_decoder = {

Diego