[FFmpeg-devel] Bink audio decoder

[Reimar Döffinger]

Hello,

Just a very quick look and only at the decoder...

On Wed, Apr 09, 2008 at 11:47:32PM +1000, pross at xvid.org wrote:
> +        if (s->use_mdct) {
> +        s->channels = avctx->channels;
> +    }else{

Weird indentation.

> +    /* populate bands data */
> +    for (i=0; i<s->num_bands; i++) {
> +        if (i==0)
> +            s->bands[i] = 1;
> +        else
> +            s->bands[i] = wma_critical_freqs[i-1] * (s->frame_len/2) / sample_rate_half;
> +    }

I think this would look much better if you pull the i==0 case out of the
loop.

> +    s->bands[i] = s->frame_len/2;

IMHO it is not good style to use a loop variable outside the loop
without a good reason.

> +static float get_float(GetBitContext *gb)
> +{
> +    int power = get_bits(gb, 5);
> +    float f = get_bits(gb, 23) / (float)(1 << (23-power));
> +    if(get_bits1(gb))
> +        f = -f;
> +    return f;

Hm, maybe check if you can use code more similar to
libavutil/infloat_readwrite.c

> +static const int rle_length_tab[16] = {
> +    2, 3, 4, 5, 6, 8, 9, 10, 11, 12, 13, 14, 15, 16, 32, 64
> +};

Should probably be uint8_t

> +static int decode_block(BinkAudioContext *s, short *out)
> +{
> +    // coeffs is a double because rdft() and ddct() take doubles
> +    DECLARE_ALIGNED_16(double, coeffs[BLOCK_MAX_SIZE]);
> +
> +    // temporary workspace for ddct() and rdft()
> +    DECLARE_ALIGNED_16(double, ddct_w[BLOCK_MAX_SIZE*5/4]);    ///< max size = block_size*5/4
> +    DECLARE_ALIGNED_16(double, rdft_w[BLOCK_MAX_SIZE/2]); ///< max size = block_size*5/4
> +    DECLARE_ALIGNED_16(int,    fft4g_ip[48]);    ///< max size = 2+sqrt(block_max_size/2)

Those could be quite big I think, wouldn't it be better to have them
e.g. in the context?

> +static void get_bits_align32(GetBitContext *s)
> +{
> +    int n= (-get_bits_count(s)) & 31;
> +    if(n) skip_bits(s, n);
> +}

Might be better to add this to the bitstream stuff?

> +static int binkaudio_decode_frame(AVCodecContext *avctx, void *data, int *data_size, const uint8_t *buf, int buf_size)
> +{
> +    BinkAudioContext * s = avctx->priv_data;
> +    short *samples = (short*)data;

Useless cast.