[FFmpeg-devel] [PATCH] Animated GIF Support

[Clément Bœsch]

On Thu, Oct 18, 2012 at 09:19:44AM +0400, Vitaliy Sugrobov wrote:
[...]
> > Also, a bit unrelated, but I think the gif decoder should be moved to the
> > bytestream2 API...
> 
> Now I think so too. However, I want to deal with current patch before
> moving further.
> 

Of course, that wasn't mean to be blocking at all.

[...]
> From ea42466afe6814faeb69f512eed393d7e1a18224 Mon Sep 17 00:00:00 2001
> From: Vitaliy E Sugrobov <vsugrob at hotmail.com>
> Date: Thu, 18 Oct 2012 06:46:03 +0400
> Subject: [PATCH 1/2] GIF: Animated GIF Support
> 
> Added gif demuxer. Changed gif decoder: now it supports all disposal methods.
> 
> Signed-off-by: Vitaliy E Sugrobov <vsugrob at hotmail.com>
> ---
>  libavcodec/gifdec.c      |  315 +++++++++++++++++++++++++++++++++++++---------
>  libavcodec/version.h     |    2 +-
>  libavformat/Makefile     |    1 +
>  libavformat/allformats.c |    2 +-
>  libavformat/gifdec.c     |  309 +++++++++++++++++++++++++++++++++++++++++++++
>  libavformat/version.h    |    2 +-
>  6 files changed, 568 insertions(+), 63 deletions(-)
>  mode change 100644 => 100755 libavcodec/gifdec.c
>  create mode 100755 libavformat/gifdec.c
> 
> diff --git a/libavcodec/gifdec.c b/libavcodec/gifdec.c
> old mode 100644
> new mode 100755
> index 3e7799f..8ba61ae
> --- a/libavcodec/gifdec.c
> +++ b/libavcodec/gifdec.c
[...]
>  static int gif_read_image(GifState *s)
>  {
> -    int left, top, width, height, bits_per_pixel, code_size, flags;
> -    int is_interleaved, has_local_palette, y, pass, y1, linesize, n, i;
> -    uint8_t *ptr, *spal, *palette, *ptr1;
> +    int left, top, width, height,
> +        bits_per_pixel, code_size, flags,
> +        is_interleaved, has_local_palette,
> +        y, pass, y1, linesize, pal_size, ret;
> +    uint8_t *idx;
> +    uint32_t *ptr, *ptr1, *px, *pr, *pal;
> +
> +    /* At least 9 bytes of Image Descriptor. */
> +    if (s->bytestream_end < s->bytestream + 9)
> +        return AVERROR_INVALIDDATA;
>  
> -    left = bytestream_get_le16(&s->bytestream);
> -    top = bytestream_get_le16(&s->bytestream);
> -    width = bytestream_get_le16(&s->bytestream);
> +    left   = bytestream_get_le16(&s->bytestream);
> +    top    = bytestream_get_le16(&s->bytestream);
> +    width  = bytestream_get_le16(&s->bytestream);
>      height = bytestream_get_le16(&s->bytestream);
> -    flags = bytestream_get_byte(&s->bytestream);
> -    is_interleaved = flags & 0x40;
> +    flags  = bytestream_get_byte(&s->bytestream);
> +    is_interleaved    = flags & 0x40;
>      has_local_palette = flags & 0x80;
> -    bits_per_pixel = (flags & 0x07) + 1;
> +    bits_per_pixel    = (flags & 0x07) + 1;
>  

Please avoid all these reformatting in the same file: it took me a while
to see that nothing functional really has changed here, it should be split
in another patch.

[...]
>  static int gif_read_header1(GifState *s)
>  {
>      uint8_t sig[6];
> -    int v, n;
> -    int has_global_palette;
> +    int v, n, background_color_index;
>  
>      if (s->bytestream_end < s->bytestream + 13)
>          return AVERROR_INVALIDDATA;
> @@ -224,23 +381,32 @@ static int gif_read_header1(GifState *s)
>          return AVERROR_INVALIDDATA;
>      }
>  
> +    s->idx_line = av_malloc((size_t)s->screen_width);

cast needed?

[...]
> From d47edbaa0b32e84bf5bcf2900ecaf34f4e2a9ad1 Mon Sep 17 00:00:00 2001
> From: Vitaliy E Sugrobov <vsugrob at hotmail.com>
> Date: Thu, 18 Oct 2012 06:54:52 +0400
> Subject: [PATCH 2/2] GIF: gif decoder: fixed return codes
> 
> 1. different return codes for "end of image" and "erroneous block label"
> 2. return real error code from gif_read_extension() instead of -1.
> 
> Signed-off-by: Vitaliy E Sugrobov <vsugrob at hotmail.com>
> ---
>  libavcodec/gifdec.c |   11 +++++++----
>  1 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/libavcodec/gifdec.c b/libavcodec/gifdec.c
> index 8ba61ae..554b906 100755
> --- a/libavcodec/gifdec.c
> +++ b/libavcodec/gifdec.c
> @@ -412,6 +412,8 @@ static int gif_read_header1(GifState *s)
>  
>  static int gif_parse_next_image(GifState *s)
>  {
> +    int ret;
> +
>      while (s->bytestream < s->bytestream_end) {
>          int code = bytestream_get_byte(&s->bytestream);
>  
> @@ -421,14 +423,15 @@ static int gif_parse_next_image(GifState *s)
>          case ',':
>              return gif_read_image(s);
>          case '!':
> -            if (gif_read_extension(s) < 0)
> -                return -1;
> +            if ((ret = gif_read_extension(s)) < 0)
> +                return ret;
>              break;
>          case ';':
>              /* end of image */
> -        default:
> -            /* error or erroneous EOF */
>              return -1;
> +        default:
> +            /* erroneous block label */
> +            return AVERROR_INVALIDDATA;
>          }
>      }
>      return -1;

You still return -1 in case of end of image, and the unexpected end,
couldn't you replace them as well while you are at it?

[...]

Anyway, no more comment from me if it works as expected and doesn't break
FATE.

Thanks!

-- 
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20121018/0d25e69e/attachment.asc>