[MPlayer-dev-eng] [PATCH] Various updates to GIF demuxer

[Reimar Döffinger]

Hello,
On Sat, Jan 13, 2007 at 10:01:49AM +0100, Diego Biurrun wrote:
> +struct gif_priv_s {
> +    GifFileType*    gif_hndl;
> +    unsigned char*  palette;
> +    unsigned char*  canvas;
> +    unsigned int    canvas_sz;
> +};
> +#define gif_hndl  (((struct gif_priv_s*)(demuxer->priv))->gif_hndl)
> +#define palette   (((struct gif_priv_s*)(demuxer->priv))->palette)
> +#define canvas    (((struct gif_priv_s*)(demuxer->priv))->canvas)
> +#define canvas_sz (((struct gif_priv_s*)(demuxer->priv))->canvas_sz)

Very ugly IMO.

> +static int current_pts = 0,
> +    ilace_offset[] = { 0, 4, 2, 1 },
> +    ilace_jmp[]    = { 8, 8, 4, 2 };

More evil global variables.

> -    // copy the palette
> +    // Copy the palette.

Cosmetics and in general multiple unrelated issues mixed in one patch.

[...]
> +        // Do a byte by byte blit for all non-transparent pixels.
> +        for (x = 0; x < width; x++) {
> +          if(gbuf[x] != trans_color)
> +            drow[x] = gbuf[x];
> +        }
[...]
> +      // Fill the image area with the background color.
> +      for (y = 0; y < height; y++) {
> +        unsigned char *drow = canvas;
> +        drow += gif->SWidth * (y + gif->Image.Top);
> +        drow += gif->Image.Left;
> +        memset(drow, gif->SBackGroundColor, width);
> +      }

I'm sure the performance of these could be optimized a lot. Don't know
if that is important though.

>  static void demux_close_gif(demuxer_t* demuxer)
>  {
> -  GifFileType *gif = (GifFileType *)demuxer->priv;
> +  GifFileType *gif = (demuxer->priv)?gif_hndl:NULL;
>  
>    if(!gif)
>      return;

I'm not sure that gif_hndl != NULL always is a valid assumption. If it
is not, this code would leave demuxer->priv unfreed.
Since this code needs an overhaul anyway, I'm for applying my patch
first.

Greetings,
Reimar Döffinger