[FFmpeg-devel] [PATCH] LucasArts-SMUSH playback

[Reimar Döffinger]

On Thu, May 03, 2012 at 12:06:01PM +0000, Paul B Mahol wrote:
> +static const uint8_t size_table[] =
> +{
> +    4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4,
> +    4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 5,
> +    5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6,
> +    6, 6, 6, 6, 6, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7

I generally find it much nice to have some nice number per line,
and not like here 23 for example.
> +static const const int8_t* step_index_tables[] =

Huh? There's no point in two const in a row.
And there's a const missing before the variable name.

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

What's the point of malloc'ing it instead of just having that fixed-size
array directly in the struct?

> +            for (count = 32, table_value = step_table[table_pos]; count != 0;
> +                 count >>= 1, table_value >>= 1) {
> +                if (start_pos & count)
> +                    put += table_value;
> +            }

I believe that fully unrolling this would result in much more readable
code.

> +    bytestream2_init(&gb, pkt->data, pkt->size);
> +
> +    if (bytestream2_get_bytes_left(&gb) < 13)

Feels a bit like overkill when you could just check pkt->size...

> +    samples = bytestream2_get_be32u(&gb);
> +    if (samples > INT_MAX) {
> +        bytestream2_skip(&gb, 4);
> +        samples = bytestream2_get_be32u(&gb);

INT_MAX is a system-dependent value (even if it isn't really in
reality).
It makes sense to use for overflow checks, but not for something
that is part of non-error bitstream parsing.

> +        for (sample = 0; sample < samples; sample++) {
> +            int lookup_size, highBit, lowBits, val;
> +
> +            step_index  = av_clip(step_index, 0, 88);
> +            lookup_size = size_table[step_index];
> +            highBit     = 1 << (lookup_size - 1);
> +            lowBits     = highBit - 1;
> +            bit        += lookup_size;
> +            val         = (bits >> (16 - bit)) & (highBit | lowBits);
> +            if (bit > 7) {
> +                if (bytestream2_get_bytes_left(&gb) < 1)
> +                    return -1;
> +
> +                bits = ((bits & 0xff) << 8) | bytestream2_get_byteu(&gb);
> +                bit -= 8;
> +            }
> +
> +            if (val & highBit)
> +                val ^= highBit;
> +            else
> +                highBit = 0;

I believe this code would be a good bit simpler if using the bitstream
reader instead of reimplementing it.

> +#define NGLYPHS 256
> +static int8_t p4x4glyphs[NGLYPHS][16];
> +static int8_t p8x8glyphs[NGLYPHS][64];

Those are huge.
Would be nice to have them available as runtime-generated tables as an
alternative...

> +static int which_edge(int x, int y, int edge_size)
> +{
> +    const int edge_max = edge_size - 1;
> +
> +    if (!y) {
> +        return BOTTOM_EDGE;
> +    } else if (y == edge_max) {
> +        return TOP_EDGE;
> +    } else if (!x) {
> +        return LEFT_EDGE;
> +    } else if (x == edge_max) {
> +        return RIGHT_EDGE;
> +    } else {
> +        return NO_EDGE;
> +    }
> +}
> +
> +static int which_direction(int edge0, int edge1)
> +{
> +    if ((edge0 == LEFT_EDGE && edge1 == RIGHT_EDGE) ||
> +        (edge1 == LEFT_EDGE && edge0 == RIGHT_EDGE) ||
> +        (edge0 == BOTTOM_EDGE && edge1 != TOP_EDGE) ||
> +        (edge1 == BOTTOM_EDGE && edge0 != TOP_EDGE)) {
> +        return DIR_UP;
> +    } else if ((edge0 == TOP_EDGE && edge1 != BOTTOM_EDGE) ||
> +               (edge1 == TOP_EDGE && edge0 != BOTTOM_EDGE)) {
> +        return DIR_DOWN;
> +    } else if ((edge0 == LEFT_EDGE && edge1 != RIGHT_EDGE) ||
> +               (edge1 == LEFT_EDGE && edge0 != RIGHT_EDGE)) {
> +        return DIR_LEFT;
> +    } else if ((edge0 == TOP_EDGE && edge1 == BOTTOM_EDGE) ||
> +               (edge1 == TOP_EDGE && edge0 == BOTTOM_EDGE) ||
> +               (edge0 == RIGHT_EDGE && edge1 != LEFT_EDGE) ||
> +               (edge1 == RIGHT_EDGE && edge0 != LEFT_EDGE)) {
> +        return DIR_RIGHT;
> +    }
> +
> +    return -1;
> +}

I am pretty sure I already reviewed this once and asked for
a few code comments to explain what all that stuff actually means and
does.
I really don't feel like drudging again through this code with
non-obvious, non-standard terms and all without even a single word of
comments.
I at least assume that someone has a bit of an idea what the meaning of
all that stuff is, even if it is mostly guesses.