[FFmpeg-devel] [PATCH] atrac decoder

[Diego Biurrun]

On Mon, Aug 10, 2009 at 11:10:40PM +0200, Benjamin Larsson wrote:
> 
> I hope this is in a state so I can start merging the code. How should I
> split out the code shared by atrac3 ? svn copy or just move the needed
> code, it is just one function (iqmf)) and a table.

In the past, Michael insisted on 'svn copy'.

> --- libavcodec/atrac1data.h	(revision 0)
> +++ libavcodec/atrac1data.h	(revision 0)
> @@ -0,0 +1,71 @@
> +
> +/* number of spectral lines in each BFU */
> +static const uint8_t specs_per_bfu[52] = {
> + 8,  8,  8,  8,  4,  4,  4,  4,  8,  8,  8,  8,  6,  6,  6,  6, 6, 6, 6, 6, // low band
> + 6,  6,  6,  6,  7,  7,  7,  7,  9,  9,  9,  9, 10, 10, 10, 10,  // middle band
> +12, 12, 12, 12, 12, 12, 12, 12, 20, 20, 20, 20, 20, 20, 20, 20 // high band
> +};
> +
> +/* start position of each BFU in the MDCT spectrum for the long mode */
> +static const uint16_t bfu_start_long[52] = {
> +      0,   8,  16,  24,  32,  36,  40,  44,  48,  56,  64,  72,  80,  86,  92,  98, 104, 110, 116, 122,
> +    128, 134, 140, 146, 152, 159, 166, 173, 180, 189, 198, 207, 216, 226, 236, 246,
> +    256, 268, 280, 292, 304, 316, 328, 340, 352, 372, 392, 412, 432, 452, 472, 492,
> +};

nit: sometimes you indent, sometimes you don't

> +#endif /* AVCODEC_ATRAC1DATA_H */
> \ No newline at end of file

Add the newline, the repository will reject it otherwise.

> --- libavcodec/atrac1.c	(revision 0)
> +++ libavcodec/atrac1.c	(revision 0)
> @@ -0,0 +1,428 @@
> +
> +/* Many thanks to Tim Craig who help alot during the development of this decoder */

"helped a lot" and that's  a long line ;)

> +/** size of the transform in samples in the long mode for each QMF band */
> +static const uint16_t samples_per_band[3] = {128, 128, 256};
> +static const uint8_t  mdct_long_nbits[3] = {7, 7, 8};

align

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

long line, more below

> +    switch(nbits) {
> +        case 5:

K&R would be:

    switch (nbits) {
    case 5:

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

Sometimes you put spaces around operators, sometimes you don't.  Extra
good karma for adding them always, more opportunities below.

> +            s1 += p1[i] * qmf_window[i];
> +            s2 += p1[i+1] * qmf_window[i+1];

align

> +        p1 += 2;
> +        pOut += 2;

ditto

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

indentation

> +    const uint8_t *buf = avpkt->data;
> +    int buf_size = avpkt->size;

align

> +        if(ret)
> +            return ret;
> +
> +        ret = at1_unpack_dequant(&q->gb, su, q->spec);
> +        if(ret)
> +            return ret;
> +
> +        ret = at1_imdct(su, q);
> +        if(ret)
> +            return ret;

if (

> +            samples[i]     = av_clipf(q->out_samples[0][i], -32700./(1<<15), 32700./(1<<15));
> +            samples[i*2]   = av_clipf(q->out_samples[0][i], -32700./(1<<15), 32700./(1<<15));
> +            samples[i*2+1] = av_clipf(q->out_samples[1][i], -32700./(1<<15), 32700./(1<<15));

Some spaces could aid readability here..

> +    /* Init the mdct transforms */
> +    ff_mdct_init(&q->mdct_ctx[0], 6, 1, -1.0/ (1<<15));
> +    ff_mdct_init(&q->mdct_ctx[1], 8, 1, -1.0/ (1<<15));
> +    ff_mdct_init(&q->mdct_ctx[2], 9, 1, -1.0/ (1<<15));
> +    ff_sine_window_init(mdct_window,32);

ditto

> +AVCodec atrac1_decoder =
> +{

AVCodec atrac1_decoder = {

> +    .name = "atrac1",
> +    .type = CODEC_TYPE_AUDIO,
> +    .id = CODEC_ID_ATRAC1,
> +    .priv_data_size = sizeof(ATRAC1Context),
> +    .init = atrac1_decode_init,
> +    .close = NULL,
> +    .decode = atrac1_decode_frame,
> +    .long_name = NULL_IF_CONFIG_SMALL("Atrac 1 (Adaptive TRansform Acoustic Coding)"),

This would look nicer aligned.

You are missing documentation and changelog updates.

Diego