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

[Paul B Mahol]

Hi,

On 5/3/12, Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:
> 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.

Done.

>> +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.
>

Fixed.

>> +    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?
>

Fixed.

>> +            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.
>

Done.

>> +    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...
>

Indeed, fixed.

>> +    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.
>

Fixed.

>> +        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.
>

I really can not see point in that.

>> +#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...
>

Fixed.

>> +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.

I'm short on time and also working on another game codec so I can not
promise any improvements in that area, sorry.