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

[Reimar Döffinger]

On Sun, May 30, 2010 at 01:21:21PM +1000, Peter Ross wrote:
> +static void picmemset_8bpp(PicContext *s, int value, int run, int *x, int *y, int *plane)
> +{
> +    while (run > 0) {
> +        uint8_t *d = s->frame.data[0] + *y * s->frame.linesize[0];
> +        if (*x + run >= s->width) {
> +            int n = s->width - *x;
> +            memset(d + *x, value, n);
> +            run -= n;
> +            *x = 0;
> +            *y -= 1;
> +            if (*y < 0) {
> +                *y = s->height - 1;
> +                *plane += 1;
> +                if (*plane >= s->nb_planes) {
> +                    break;
> +                }
> +            }
> +        } else {
> +            memset(d + *x, value, run);
> +            *x += run;
> +            break;
> +        }
> +    }
> +}

Hm, plane is never used inside that function, so there should be no need to handle incrementing
it inside it - then you also don't need to dupliacate that code between the two memset functions.
Should also avoid the compiler genrating code that checks the plane < s->nb_planes condition on
every innner-loop iteration even though we know it will not have changed (I doubt the compiler
will do that kind of optimization, if it even can).
Also calculating n outside the if and using run >= n as condition might avoid the compiler
doing stupid things.

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

I fail how that FFMAX could ever trigger...

> +        if (buf + esize >= buf_end)
> +            return AVERROR_INVALIDDATA;

You can't do checks this way, buf + esize could overflow (unlikely here, but
still not impossible - even if it did not overflow it still would be invalid C).
Simple rule: the value to be validated should always stand alone on one
side of the comparison. If not, think twice whether your check catches all cases.
Then think twice about it again.

> +    palette = (uint32_t*)s->frame.data[1];
> +    if (etype == 1 && esize > 1 && buf[0] < 6) {
> +        int idx = buf[0];
> +        for (i = 0; i < 4; i++)
> +            palette[i] = ff_cga_palette[ cga_mode4_index[idx][i] ];
> +    } else if (etype == 2) {
> +        for (i = 0; i < FFMIN(esize, 16); i++)
> +            palette[i] = ff_cga_palette[ FFMIN(buf[i], 16)];
> +    } else if (etype == 3) {
> +        for (i = 0; i < FFMIN(esize, 16); i++)
> +            palette[i] = ff_ega_palette[ FFMIN(buf[i], 63)];
> +    } else if (etype == 4 || etype == 5) {
> +        for (i = 0; i < FFMIN(esize / 3, 256); i++)
> +            palette[i] = AV_RB24(buf + i*3) << 2;
> +    } else {  // default
> +        if (bpp == 1) {
> +            palette[0] = 0x000000;
> +            palette[1] = 0xFFFFFF;
> +        } else if (bpp == 2) {
> +            for (i = 0; i < 4; i++)
> +                palette[i] = ff_cga_palette[ cga_mode4_index[0][i] ];
> +        } else {
> +            memcpy(palette, ff_cga_palette, 16 * 4);
> +        }
> +    }

I think you're supposed to always initialize the whole palette, e.g. to 0.

> +        for (i = 0; i < nb_blocks && buf + 5 < buf_end; i++) {

Same overflow issue.

> +            psize -= 5;
> +
> +            for (j = 0; j < psize && plane < s->nb_planes; j++) {
> +               int run;
> +               if (buf[j] == marker) {
> +                   run = buf[j + 1];
> +                   j += 2;
> +                   if (run == 0) {
> +                       run = AV_RL16(buf + j);
> +                       j += 2;
> +                   }
> +               } else {
> +                   run = 1;
> +               }
> +
> +               if (bits_per_plane == 8) {
> +                   picmemset_8bpp(s, buf[j], run, &x, &y, &plane);
> +               }else{
> +                   picmemset(s, buf[j], run, &x, &y, &plane, bits_per_plane);
> +               }
> +            }
> +
> +            buf += psize;

Missing a check that psize fits into a buffer?
Also I suspect it would be simpler and faster to just calculate buf+psize and
do comparisons against that instead of having a separate j variable.