[FFmpeg-devel] [PATCH] Pictor/PC Paint PIC decoder

[Reimar Döffinger]

On Tue, Jun 01, 2010 at 10:40:56PM +1000, Peter Ross wrote:
> +static void picmemset_8bpp(PicContext *s, int value, int run, int *x, int *y, int *plane)

The idea was to get rid of the plane argument since it is not used..

> +static void picmemset(PicContext *s, int value, int run, int *x, int *y, int *plane, int bits_per_plane)
> +{
> +    uint8_t *d;
> +    int mask = (1 << bits_per_plane) - 1;
> +
> +    while (run > 0) {
> +        int j;
> +        for (j = 8-bits_per_plane; j >= 0; j -= bits_per_plane) {
> +            d = s->frame.data[0] + *y * s->frame.linesize[0];
> +            d[*x] |= ((value >> j) & mask) << (*plane * bits_per_plane);

My brain hurts.
I think this would be simpler (well, and also faster) if you just shifted
value and mask outside the loop (and adjust them of course when you increment
*plane).

> +    s->nb_planes      = ((buf[10] >> 4) & 0xF) + 1;

The & 0x0f seems pointless.

> +    bpp               = s->nb_planes ? bits_per_plane*s->nb_planes : bits_per_plane;

The shift quoted above will give undefined behaviour if bits_per_plane*(s->nb_planes - 1) > 31.
And I think the code will already just not work if bpp > 8.

> +    nb_blocks = AV_RL16(buf);
> +    buf += 2;

Consider using bytestream_get_le16 etc., here and in a few other places.

> +        for (i = 0; i < nb_blocks && buf_end - buf >= 6; i++) {
> +            const uint8_t *buf_pend = FFMIN(buf + AV_RL16(buf), buf_end);

Calculating buf + AV_RL16(buf) can invoke undefined behaviour.
I also fail to see the point of the nb_blocks variable and caring about it...
In particular since you might end up leaving part of the frame uninitialized
due to it.

> +            while (plane < s->nb_planes && buf_pend - buf >= 1) {
> +                int run;
> +                if (*buf == marker) {
> +                    buf++;
> +                    if (buf_pend - buf < 2)
> +                        break;
> +                    run = *buf++;
> +                    if (run == 0) {
> +                        if (buf_pend - buf < 3)
> +                            break;
> +                        run = AV_RL16(buf);
> +                        buf += 2;
> +                    }
> +                } else {
> +                    run = 1;
> +                }
> +
> +                if (bits_per_plane == 8) {
> +                    picmemset_8bpp(s, *buf, run, &x, &y, &plane);
> +                    if (y < 0)
> +                        break;
> +                }else{
> +                    picmemset(s, *buf, run, &x, &y, &plane, bits_per_plane);
> +                }
> +                buf++;

This seems a bit round-about to me.
while (plane < s->nb_planes) {
    int run = 1;
    int val = *buf++;
    if (val == marker) {
        run = *buf++;
        if (run == 0)
            run = bytestream_get_le32(&buf);
        // TODO: what about run still == 0??
    }
    if (buf > buf_end)
        break;
    [the two picmemset cases]
}