[FFmpeg-devel] Fwd: [PATCH] Psygnosis YOP demuxer

[Reimar Döffinger]

On Sat, Mar 27, 2010 at 07:39:14AM +0530, Mohamed Naufal wrote:
> +static const uint8_t paint_lut[15][5] = { {0, 1, 2, 3, 4},
> +                                          {0, 1, 2, 0, 3},
> +                                          {0, 1, 2, 1, 3},
> +                                          {0, 1, 2, 2, 3},
> +                                          {0, 1, 0, 2, 3},
> +                                          {0, 1, 0, 0, 2},
> +                                          {0, 1, 0, 1, 2},
> +                                          {0, 1, 1, 2, 3},
> +                                          {0, 0, 1, 2, 3},
> +                                          {0, 0, 1, 0, 2},
> +                                          {0, 1, 1, 0, 2},
> +                                          {0, 0, 1, 1, 2},
> +                                          {0, 0, 0, 1, 2},
> +                                          {0, 0, 0, 0, 1},
> +                                          {0, 1, 1, 1, 2},
> +                                        };

First byte is always 0, removing it will simplify address calculations.
It _might_ also be faster to e.g. compress all 4 values into e.g.
a single uint16_t value, but probably not worth the effort.
I do think it would look nicer if you started the data on a new line
with 4 spaces indentation and e.g. 2 {} blocks per line instead of having
them all the way on the right side.

> +        avcodec_check_dimensions(avctx, avctx->width, avctx->height)) {

Only return values < 0 indicate an error, > 0 in FFmpeg does not usually
indicate an error.

> +    s->num_pal_colors      = AV_RL8(avctx->extradata);
> +    s->first_color[0]      = AV_RL8(avctx->extradata + 1);
> +    s->first_color[1]      = AV_RL8(avctx->extradata + 2);

Obfuscation.
avctx->extradata[0]
avctx->extradata[1]
avctx->extradata[2]

> +/**
> + * Copy a previously painted macroblock to the current_block.
> + * @param copy_tag The tag that was in the nibble.
> + */
> +static void yop_copy_previous_block(YopDecContext *s, int copy_tag)
> +{
> +    uint8_t *bufptr;
> +
> +    // Calculate position for the copy source
> +    bufptr = s->dstptr + motion_vector[copy_tag][0] +
> +             s->frame.linesize[0] * motion_vector[copy_tag][1];

How do you know bufptr is a valid pointer?

> +    if (avctx->get_buffer(avctx, &s->frame) < 0) {
> +        av_log(avctx, AV_LOG_ERROR, "get_buffer() failed\n");
> +        return -1;
> +    }
> +
> +    s->frame.linesize[0] = avctx->width;

I strongly object to this, this is outside of any
documented API.
The CODEC_CAP_DR1 documentation indicates that you must set it if
you use get_buffer, but changing linesize is incompatible with DR.
You either need to respect the linesize as it is or do the buffer
management without get_buffer.
Or, if we really want this to be a valid way of doing it, improve
that CODEC_CAP_DR1 documentation and add a comment that this codec
intentionally does not set CODEC_CAP_DR1 because it can not work
with it.

> +    palette      = (uint32_t *)s->frame.data[1];
> +
> +    for (i = 0; i < s->num_pal_colors; i++, s->srcptr += 3)
> +        palette[i + firstcolor] = (s->srcptr[0] << 18) |
> +                                  (s->srcptr[1] << 10) |
> +                                  (s->srcptr[2] << 2);

I think you should initialize the whole palette area to some value,
even the unused parts.

> +    // Reset this so the next frame doesn't use a stale tag.
> +    s->low_nibble = NULL;

Wouldn't it make more sense to initialize this together with
srcptr instead?