[FFmpeg-devel] [PATCH 1/3] LucasArts Smush VIMA audio decoder

[Reimar Döffinger]

On Tue, Mar 27, 2012 at 05:28:43PM +0000, Paul B Mahol wrote:
> +static const int8_t* step_index_tables[] =

one const more to make this table itself const, too.

> +    vima->predict_table = av_mallocz(5786 * 2);

* sizeof(*vima->predict_table)
is better IMO, then there's no risk in changing the type of the
variable.

> +    for (start_pos = 0, i = 0; start_pos < 64; start_pos++, i++) {

I can't see why you need both start_pos and i here.

> +    int            bits, bitPtr = 0, ret, chan, samples, channels = 1;

The camel-case of bitPtr doesn't quite fit with the rest of the names.

> +    samples = bytestream2_get_be32u(&gb);
> +    if (samples < 0) {

IMO you should avoid assigning an unsigned value to a signed variable.
Even more so since int is not really guaranteed to be 32 bit, so that
code does not necessarily work as expected.

> +    if ((ret = avctx->get_buffer(avctx, &vima->frame)) < 0) {

I guess you know my opinion on assignment inside if by heart now ;-)

> +    for (chan = 0; chan < channels; chan++) {
> +        uint16_t *dest = (uint16_t*)vima->frame.data[0] + chan;
> +        int step_index = channel_hint[chan];
> +        int outputWord = pcm_data[chan];

outputWord doesn't fit in naming style again.

> +            val         = (bits >> (16 - bitPtr)) & (highBit | lowBits);
> +            if (bitPtr > 7) {
> +                if (bytestream2_get_bytes_left(&gb) < 1)
> +                    return -1;

Wouldn't it be possible/make more sense to use get_bits for that?

> +            if (val & highBit)
> +                val ^= highBit;
> +            else
> +                highBit = 0;

With get_bits it might make more sense to read the sign bit first,
independently with get_bits1.

> +                outputWord  = av_clip(outputWord, -0x8000, 0x7fff);

av_clip_int16 should be faster.