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

[Peter Ross]

On Tue, Jun 01, 2010 at 09:08:18PM +0200, Reimar D?ffinger wrote:
> 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..

Roger.

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

Yeah, i dont for miss these arcane formats at all.

> > +    s->nb_planes      = ((buf[10] >> 4) & 0xF) + 1;
> 
> The & 0x0f seems pointless.

Yes.

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

You're right. Can't find any samples of 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.

Okay.

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

True, but what if there is trailing data at the end of the file, not related
to the image? Removing check would cause that data to be processed.

> 
> > +            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]
> }

Done.

Also... any thoughts on CODEC_ID_PIC. It is rather generic, _PICTOR
might be more appropriate.

-- Peter
(A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pic-r3.diff
Type: text/x-diff
Size: 11876 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100602/4a958c89/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/20100602/4a958c89/attachment.pgp>