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

[Peter Ross]

On Sun, May 30, 2010 at 08:12:57AM +0200, Reimar D?ffinger wrote:
> On Sun, May 30, 2010 at 01:21:21PM +1000, Peter Ross wrote:

Thanks for the review Reimar.

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

Fixed.

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

Yeah, i have a habit of doing & repeating this.

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

Fixed.

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

Ok fixed.

-- Peter
(A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pic-r2.diff
Type: text/x-diff
Size: 11832 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100601/c1267381/attachment.diff>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100601/c1267381/attachment.pgp>