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

[Reimar Döffinger]

On Tue, Sep 01, 2009 at 10:08:00PM +0200, Benjamin Larsson wrote:
> +/* idwl to wordlen translation table */
> +static const uint8_t idwl2wordlen[16]= {0, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16};

x + !!x
or
if (x) x++;
a table seems a bit extreme for such a simple thing.

> +/* 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,             // midle band
> +    12, 12, 12, 12, 12, 12, 12, 12, 20, 20, 20, 20, 20, 20, 20, 20              // high band
> +};

Unless there is a real speed difference this should just be reduced by 4
and >> 2 added to the input.

> +    for (i=0 ; i<su->num_bfus ; i++)
> +    for (i=0 ; i<su->num_bfus ; i++)
> +    for (i = su->num_bfus; i < AT1_MAX_BFU; i++)
> +            int         pos;
> +                float       max_quant = 1.0/(float)((1 << (word_len - 1)) - 1);

Those are some seriously strangely placed spaces...

> +                for (i = 0; i < num_specs; i++)
> +                    spec[pos + i] = 0;

memset?

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

Some documentation, e.g. that nIn must be a multiple of 2 would be good,
too.
Also maybe "float delayBuf[46]" would be better for documentation
purposes.

> +    memcpy(temp, delayBuf, 46*sizeof(float));
> +    memcpy(delayBuf, temp + nIn*2, 46*sizeof(float));

sizeof(*delayBuf) seems more appropriate that sizeof(float).
Too bad that eve with above suggested change just sizeof(delayBuf) can't
be used.

> +    if ((buf_size<212 && q->channels==1) || (buf_size<424 && q->channels==2))

buf_size < 212 * q->channels
?

> +    for (ch=0 ; ch<avctx->channels ; ch++) {

Though it would be good to decide whether to use q->channels or
avctx->channels and stick with it.

> +        if(ret)
> +            return ret;

if (ret < 0) seems better/clearer to me.

> +    if (q->channels == 1) {
> +        /* mono */
> +        for (i = 0; i<AT1_SU_SAMPLES; i++)
> +            samples[i]     = av_clipf(q->out_samples[0][i], -32700./(1<<15), 32700./(1<<15));
> +        *data_size = AT1_SU_SAMPLES * sizeof(float);
> +    } else {
> +        /* stereo */
> +        for (i = 0; i < AT1_SU_SAMPLES; i++) {
> +            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));
> +        }
> +        *data_size = 2 * AT1_SU_SAMPLES * sizeof(float);
> +    }

*data_size = ...->channels * AT1_SU_SAMPLES * sizeof(*samples);

> +        qmf_window[i]      = qmf_48tap_half[i] * 2.0;
> +        qmf_window[47 - i] = qmf_48tap_half[i] * 2.0;

> +        qmf_window[i]      =
> +        qmf_window[47 - i] = qmf_48tap_half[i] * 2.0;




> +    if (p->buf_size <= 2048+212)
> +        return 0;

Why not <?


> +        /* Check so that values are != 0 */
> +        checksum = bsm_s + bsm_e + inb_s + inb_e;
> +        if (checksum)
> +            if ((bsm_s == bsm_e) && (inb_s == inb_e) && (bsm_s != inb_s))

Huh, what?
Is that meant to be
if (bsm_s && bsm_e && bsm_s == bsm_e && inb_s == inb_e && bsm_s != inb_s)

written by someone who likes obfuscation too much?
If not it seriously needs some more comments/explanations.

> +    /* First packet starts at 0x800 */
> +    url_fskip(s->pb, 264);

You know, in my opinion comments should ease understanding, not confuse
like this one does.

> +    if (!((st->codec->channels == 1) || (st->codec->channels == 2)))
> +        return -1;

if (st->codec->channels != 1 && st->codec->channels != 2)

> +static int aea_read_packet(AVFormatContext *s, AVPacket *pkt)
> +{
> +    int ret = av_get_packet(s->pb, pkt, s->streams[0]->codec->block_align);
> +
> +    pkt->stream_index = 0;
> +    if (ret <= 0)
> +        return AVERROR(EIO);

if (ret < 0)
    return ret;

with anything else you have to handle more special cases than I think
you want to.
Which of course I think means you can just do
pkt->stream_index = 0;
return av_get_packet(s->pb, pkt, s->streams[0]->codec->block_align);

> +    pcm_read_seek,
> +    .flags= AVFMT_GENERIC_INDEX,

does seeking actually work right? I would have thought you'd have to set
data_offset right for that.