[FFmpeg-devel] [PATCH] Animated GIF Support

Clément Bœsch ubitux at gmail.com
Sun Oct 14 00:02:22 CEST 2012


On Fri, Oct 12, 2012 at 02:19:37AM +0400, Vitaliy Sugrobov wrote:
[...]
> From 8b529f4fc90b3cdc811bd0539894024873243595 Mon Sep 17 00:00:00 2001
> From: Vitaliy E Sugrobov <vsugrob at hotmail.com>
> Date: Fri, 12 Oct 2012 02:02:14 +0400
> Subject: [PATCH] Animated GIF Support
> 
> Added gif demuxer. Changed gif decoder: now it supports all disposal methods.
> 

And this is awesome, I was wondering the other day why we weren't able to
ffplay generated animated gif...

BTW, are you going to work on the encode? There is room for improvement
there; I think it is intra-only at the moment (because the output gif are
pretty huge). Also, you need some tricks to get a nice gif output (like
doing things such as -vf ...,format=rgb8,format=rgb24 to get better
colors).

Anyway, following is a small review of your code, with a lot of style &
nit stuff, sorry :)

> Signed-off-by: Vitaliy E Sugrobov <vsugrob at hotmail.com>
> ---
>  libavcodec/gifdec.c      |  301 +++++++++++++++++++++++++++++++++++--------
>  libavformat/Makefile     |    1 +
>  libavformat/allformats.c |    2 +-
>  libavformat/gifdec.c     |  318 ++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 566 insertions(+), 56 deletions(-)
>  mode change 100644 => 100755 libavcodec/gifdec.c
>  create mode 100755 libavformat/gifdec.c
> 

Don't forget the version.h minor bump in lavf and lavc

> diff --git a/libavcodec/gifdec.c b/libavcodec/gifdec.c
> old mode 100644
> new mode 100755
> index 3e7799f..a33dbf6
> --- a/libavcodec/gifdec.c
> +++ b/libavcodec/gifdec.c
> @@ -2,6 +2,7 @@
>   * GIF decoder
>   * Copyright (c) 2003 Fabrice Bellard
>   * Copyright (c) 2006 Baptiste Coudurier
> + * Copyright (c) 2012 Vitaliy E Sugrobov
>   *
>   * This file is part of FFmpeg.
>   *
> @@ -32,19 +33,35 @@
>  #define GCE_DISPOSAL_BACKGROUND 2
>  #define GCE_DISPOSAL_RESTORE    3
>  
> +/* This value is intentionally set to "transparent white" color.
> + * It is much better to have white background instead of black
> + * when gif image converted to format not supporting transparency.

The last part of the sentence is weirdly worded, maybe missing a verb or
something.

> + */
> +#define GIF_TRANSPARENT_COLOR    0x00ffffff
> +
>  typedef struct GifState {
>      AVFrame picture;
>      int screen_width;
>      int screen_height;
> +    int has_global_palette;
>      int bits_per_pixel;
> +    uint32_t bg_color;
>      int background_color_index;
>      int transparent_color_index;
>      int color_resolution;
> -    uint32_t *image_palette;
> +    uint8_t *idx_line;
>  
> -    /* after the frame is displayed, the disposal method is used */
> +    /**
> +     * After the frame is displayed, the disposal method is used.
> +     */

looks unrelated

> +    int gce_prev_disposal;
>      int gce_disposal;
> -    /* delay during which the frame is shown */
> +    int gce_l, gce_t, gce_w, gce_h;

what is this gce thing? could you add a comment?

> +    uint32_t * stored_img;
> +    int stored_bg_color;
> +    /**
> +     * Delay during which the frame is shown.
> +     */

ditto unrelated, please keep the comment identical

>      int gce_delay;
>  
>      /* LZW compatible decoder */
> @@ -53,20 +70,77 @@ typedef struct GifState {
>      LZWState *lzw;
>  
>      /* aux buffers */
> -    uint8_t global_palette[256 * 3];
> -    uint8_t local_palette[256 * 3];
> +    uint32_t global_palette[256];
> +    uint32_t local_palette[256];
>  
>      AVCodecContext *avctx;
> +    int first_frame;
>  } GifState;
>  
>  static const uint8_t gif87a_sig[6] = "GIF87a";
>  static const uint8_t gif89a_sig[6] = "GIF89a";
>  
> +static void gif_read_palette(const uint8_t **buf, uint32_t *pal, int nb) {

style: we usually put the '{' to the next line for functions, it's by the
way the case in this file, please stick with it

> +    const uint8_t *pal_end = *buf + nb * 3;
> +
> +    for (; *buf < pal_end; *buf += 3, pal++)
> +        *pal = (0xffu << 24) | AV_RB24(*buf);
> +}
> +
> +static void gif_fill(AVFrame *picture, uint32_t color) {

ditto '{'

> +    uint32_t *p = (uint32_t *)picture->data[0];
> +    uint32_t *p_end = p + (picture->linesize[0] / sizeof(uint32_t)) * picture->height;
> +
> +    for (; p < p_end; p++)
> +        *p = color;
> +}
> +
> +static void gif_fill_rect(AVFrame *picture, uint32_t color, int l, int t, int w, int h) {
> +    int linesize = picture->linesize[0] / sizeof(uint32_t);

nit: const int linesize = ...

> +    uint32_t *px, *pr,
> +             *py = (uint32_t *)picture->data[0] + t * linesize;
> +    uint32_t *pb = py + (t + h) * linesize;
> +

looks like pr and pb can be const.

> +    for (; py < pb; py += linesize) {
> +        px = py + l;
> +        pr = px + w;
> +
> +        for (; px < pr; px++)
> +            *px = color;
> +    }
> +}
> +
> +static void gif_copy_img_rect(uint32_t *src, uint32_t *dst,
> +    int linesize, int l, int t, int w, int h)
> +{

style: vertical align the parameters:

static void gif_copy_img_rect(uint32_t *src, uint32_t *dst,
                             int linesize, int l, int t, int w, int h)

Also, src could be const

> +    int y_start = t * linesize;
> +    uint32_t *src_px, *dst_px, *src_pr,
> +             *src_py = src + y_start,
> +             *dst_py = dst + y_start;
> +    uint32_t *src_pb = src_py + (t + h) * linesize;
> +

And maybe some of them as well.

> +    for (; src_py < src_pb; src_py += linesize, dst_py += linesize) {
> +        src_px = src_py + l;
> +        dst_px = dst_py + l;
> +        src_pr = src_px + w;
> +
> +        for (; src_px < src_pr; src_px++, dst_px++)
> +            *dst_px = *src_px;
> +    }
> +}
> +
>  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 is_interleaved, has_local_palette, y, pass, y1, linesize, pal_size;
> +    int ret;
> +    uint8_t *idx;
> +    uint32_t *ptr, *ptr1, *px, *pr;
> +    uint32_t *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);
> @@ -80,11 +154,31 @@ static int gif_read_image(GifState *s)
>      av_dlog(s->avctx, "image x=%d y=%d w=%d h=%d\n", left, top, width, height);
>  
>      if (has_local_palette) {
> -        bytestream_get_buffer(&s->bytestream, s->local_palette, 3 * (1 << bits_per_pixel));
> -        palette = s->local_palette;
> +        pal_size = 1 << bits_per_pixel;
> +
> +        if (s->bytestream_end < s->bytestream + pal_size * 3)
> +            return AVERROR_INVALIDDATA;
> +
> +        gif_read_palette(&s->bytestream, s->local_palette, pal_size);
> +        pal = s->local_palette;
>      } else {
> -        palette = s->global_palette;
> -        bits_per_pixel = s->bits_per_pixel;
> +        if (!s->has_global_palette) {
> +            av_log(s->avctx, AV_LOG_FATAL, "picture doesn't have either global or local palette.\n");
> +            return AVERROR_INVALIDDATA;
> +        }
> +
> +        pal = s->global_palette;
> +    }
> +
> +    if (s->first_frame) {
> +        if (s->transparent_color_index == -1 && s->has_global_palette) {
> +            /* transparency wasn't set before the first frame, fill with background color */
> +            gif_fill(&s->picture, s->bg_color);
> +        } else {
> +            /* otherwise fill with transparent color.
> +             * this is necessary since by default picture filled with 0x80808080. */
> +            gif_fill(&s->picture, GIF_TRANSPARENT_COLOR);
> +        }
>      }
>  
>      /* verify that all the image is inside the screen dimensions */
> @@ -92,32 +186,66 @@ static int gif_read_image(GifState *s)
>          top + height > s->screen_height)
>          return AVERROR(EINVAL);
>  
> -    /* build the palette */
> -    n = (1 << bits_per_pixel);
> -    spal = palette;
> -    for(i = 0; i < n; i++) {
> -        s->image_palette[i] = (0xffu << 24) | AV_RB24(spal);
> -        spal += 3;
> +    /* process disposal method */
> +    if (s->gce_prev_disposal == GCE_DISPOSAL_BACKGROUND) {
> +        gif_fill_rect(&s->picture, s->stored_bg_color, s->gce_l, s->gce_t, s->gce_w, s->gce_h);
> +    } else if (s->gce_prev_disposal == GCE_DISPOSAL_RESTORE) {
> +        gif_copy_img_rect(s->stored_img, (uint32_t *)s->picture.data[0],
> +            s->picture.linesize[0] / sizeof(uint32_t), s->gce_l, s->gce_t, s->gce_w, s->gce_h);

nit: here and a few other times: vertical align

> +    }
> +
> +    s->gce_prev_disposal = s->gce_disposal;
> +
> +    if (s->gce_disposal != GCE_DISPOSAL_NONE) {
> +        s->gce_l = left;  s->gce_t = top;
> +        s->gce_w = width; s->gce_h = height;
> +
> +        if (s->gce_disposal == GCE_DISPOSAL_BACKGROUND) {
> +            if (s->background_color_index == s->transparent_color_index)
> +                s->stored_bg_color = GIF_TRANSPARENT_COLOR;
> +            else
> +                s->stored_bg_color = s->bg_color;
> +        } else if (s->gce_disposal == GCE_DISPOSAL_RESTORE) {
> +            if (!s->stored_img) {
> +                s->stored_img = av_malloc((size_t)(s->picture.linesize[0] * s->picture.height));

I don't think that cast is needed

[...]
>          av_dlog(s->avctx, "gce_flags=%x delay=%d tcolor=%d disposal=%d\n",
>                 gce_flags, s->gce_delay,
>                 s->transparent_color_index, s->gce_disposal);
> @@ -190,20 +342,24 @@ static int gif_read_extension(GifState *s)
>      /* NOTE: many extension blocks can come after */
>   discard_ext:
>      while (ext_len != 0) {
> +        /* There must be at least ext_len bytes and 1 for next block size byte. */
> +        if (s->bytestream_end < s->bytestream + ext_len + 1)
> +            return AVERROR_INVALIDDATA;
> +
>          for (i = 0; i < ext_len; i++)
>              bytestream_get_byte(&s->bytestream);
> -        ext_len = bytestream_get_byte(&s->bytestream);
>  
> +        ext_len = bytestream_get_byte(&s->bytestream);

unrelated change

>          av_dlog(s->avctx, "ext_len1=%d\n", ext_len);
>      }
> +
>      return 0;
>  }
>  
>  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,28 +380,40 @@ static int gif_read_header1(GifState *s)
>          return AVERROR_INVALIDDATA;
>      }
>  
> +    s->idx_line = av_malloc((size_t)s->screen_width);
> +    if (!s->idx_line)
> +        return AVERROR(ENOMEM);
> +
>      v = bytestream_get_byte(&s->bytestream);
>      s->color_resolution = ((v & 0x70) >> 4) + 1;
> -    has_global_palette = (v & 0x80);
> +    s->has_global_palette = (v & 0x80);
>      s->bits_per_pixel = (v & 0x07) + 1;
> -    s->background_color_index = bytestream_get_byte(&s->bytestream);
> +    background_color_index = bytestream_get_byte(&s->bytestream);
>      bytestream_get_byte(&s->bytestream);                /* ignored */
>  
>      av_dlog(s->avctx, "screen_w=%d screen_h=%d bpp=%d global_palette=%d\n",
>             s->screen_width, s->screen_height, s->bits_per_pixel,
> -           has_global_palette);
> +           s->has_global_palette);
>  
> -    if (has_global_palette) {
> +    if (s->has_global_palette) {
> +        s->background_color_index = background_color_index;
>          n = 1 << s->bits_per_pixel;
> +
>          if (s->bytestream_end < s->bytestream + n * 3)
>              return AVERROR_INVALIDDATA;
> -        bytestream_get_buffer(&s->bytestream, s->global_palette, n * 3);
> -    }
> +
> +        gif_read_palette(&s->bytestream, s->global_palette, n);
> +        s->bg_color = s->global_palette[s->background_color_index];
> +    } else
> +        s->background_color_index = -1;
> +
>      return 0;
>  }
>  
>  static int gif_parse_next_image(GifState *s)
>  {
> +    int ret;
> +
>      while (s->bytestream < s->bytestream_end) {
>          int code = bytestream_get_byte(&s->bytestream);
>  
> @@ -255,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;

These changes likely belong to another patch. Please split your work so
it's easier to review

>          }
>      }
>      return -1;
> @@ -289,39 +458,61 @@ static int gif_decode_frame(AVCodecContext *avctx, void *data, int *data_size, A
>      AVFrame *picture = data;
>      int ret;
>  
> +    s->picture.pts = avpkt->pts;
> +    s->picture.pkt_pts = avpkt->pts;
> +    s->picture.pkt_dts = avpkt->dts;
> +    s->picture.pkt_duration = avpkt->duration;
> +
>      s->bytestream = buf;
>      s->bytestream_end = buf + buf_size;
> -    if ((ret = gif_read_header1(s)) < 0)
> -        return ret;
>  
> -    avctx->pix_fmt = AV_PIX_FMT_PAL8;
> -    if (av_image_check_size(s->screen_width, s->screen_height, 0, avctx))
> -        return -1;
> -    avcodec_set_dimensions(avctx, s->screen_width, s->screen_height);
> +    if (buf_size >= 6) {
> +        s->first_frame = memcmp(s->bytestream, gif87a_sig, 6) == 0 ||
> +                         memcmp(s->bytestream, gif89a_sig, 6) == 0;
> +    } else
> +        s->first_frame = 0;
>  
> -    if (s->picture.data[0])
> -        avctx->release_buffer(avctx, &s->picture);
> -    if ((ret = avctx->get_buffer(avctx, &s->picture)) < 0) {
> -        av_log(avctx, AV_LOG_ERROR, "get_buffer() failed\n");
> -        return ret;

> +    if (s->first_frame) {
> +        if ((ret = gif_read_header1(s)) < 0)
> +            return ret;
> +
> +        avctx->pix_fmt = PIX_FMT_RGB32;
> +        if ((ret = av_image_check_size(s->screen_width, s->screen_height, 0, avctx)) < 0)
> +            return ret;
> +        avcodec_set_dimensions(avctx, s->screen_width, s->screen_height);
> +
> +        if (s->picture.data[0])
> +            avctx->release_buffer(avctx, &s->picture);
> +
> +        if ((ret = avctx->get_buffer(avctx, &s->picture)) < 0) {
> +            av_log(avctx, AV_LOG_ERROR, "get_buffer() failed\n");
> +            return ret;
> +        }

Please don't re-indent in the patch when you only change a few things,
it's hard to tell what changed at first glance. It belongs to a later
commit.

>      }
> -    s->image_palette = (uint32_t *)s->picture.data[1];
> +
>      ret = gif_parse_next_image(s);
>      if (ret < 0)
>          return ret;
>  
>      *picture = s->picture;
> -    *data_size = sizeof(AVPicture);
> +    *data_size = sizeof(AVFrame);
> +

Not saying that's wrong but... why?

>      return s->bytestream - buf;
>  }
>  
>  static av_cold int gif_decode_close(AVCodecContext *avctx)
>  {
>      GifState *s = avctx->priv_data;
> -

unrelated

>      ff_lzw_decode_close(&s->lzw);
>      if(s->picture.data[0])
>          avctx->release_buffer(avctx, &s->picture);
> +
> +    if(s->idx_line)
> +        av_free(s->idx_line);
> +
> +    if(s->stored_img)
> +        av_free(s->stored_img);
> +

those if are uneeded

[...]
> +/**
> + * GIF format allows variable delay between frames without having
> + * any notion of "frames per second". But since frames decoded with ffmpeg demuxer,
> + * we have to adhere requirement of some fixed fps.
> + */
> +#define GIF_DEFAULT_RATE    25
> +/**
> + * Major web browsers display gifs at ~10-15fps when rate
> + * is not explicitly set or have too low values. We assume default rate to be 10.
> + * Default delay = 100hundredths of second / 10fps = 10hos per frame.
> + */
> +#define GIF_DEFAULT_DELAY   10

It really plays it faster in Firefox and Chromium for me: I'm trying with
the following generated gif, and ffplay has a way slower playback:

./ffmpeg -i ~/samples/big_buck_bunny_1080p_h264.mov -ss 45 -vf \
        'scale=320:160,format=rgb8,format=rgb24' -r 20  -frames:v 50 -y bbb.gif

> +/**
> + * By default delay values less than this threshold considered to be invalid.
> + */
> +#define GIF_MIN_DELAY       3
> +/**
> + * Converts gif time to pts where
> + * t is integer time (in hundredths of second) and r is frame rate.
> + * Note that result is rounded to the nearest integer.
> + */
> +#define GIF_TIME_TO_PTS(t,r) ((int)(((t) * (r) / 100.0f) + 0.5f))
> +

Can't you just set the timebase correctly and let the FFmpeg internals
handle the rescale?

> +static int gif_probe(AVProbeData *p) {

style: again the '{' nit

> +    if(p->buf_size < 6)
> +        return 0;
> +

style nit again: here and below: if<space>(

> +    /* check magick */
> +    if(memcmp(p->buf, "GIF87a", 6) && memcmp(p->buf, "GIF89a", 6))
> +        return 0;
> +
> +    /* header available for probing? */
> +    if(p->buf_size < 10)
> +        return AVPROBE_SCORE_MAX / 2;
> +
> +    /* width or height contains zero? */
> +    if(!AV_RL16(&p->buf[6]) || !AV_RL16(&p->buf[8]))
> +        return 0;
> +
> +    return AVPROBE_SCORE_MAX;
> +}
> +
[...]
> +
> +static const AVOption options[] = {
> +    { "gif_rate",
> +      "implied gif frame rate",
> +      offsetof(GIFDemuxContext, rate),
> +      AV_OPT_TYPE_INT,
> +      {.i64 = GIF_DEFAULT_RATE}, 1, 100,
> +      AV_OPT_FLAG_DECODING_PARAM },
> +    { "gif_min_delay",
> +      "minimum allowed delay between frames (in hundredths of second)",
> +      offsetof(GIFDemuxContext, min_delay),
> +      AV_OPT_TYPE_INT,
> +      {.i64 = GIF_MIN_DELAY}, 0, 100 * 60,
> +      AV_OPT_FLAG_DECODING_PARAM },
> +    { "gif_default_delay",
> +      "default delay between frames (in hundredths of second)",
> +      offsetof(GIFDemuxContext, default_delay),
> +      AV_OPT_TYPE_INT,
> +      {.i64 = GIF_DEFAULT_DELAY}, 0, 100 * 60,
> +      AV_OPT_FLAG_DECODING_PARAM },
> +    { NULL },

This is often where we ignore the small column count and prefer a more
obvious vertical align for readability:

{ "gif_rate",          "implied gif frame rate",                      ...
{ "gif_min_delay",     "minimum allowed delay between frames (in hundr...
{ "gif_default_delay", "default delay between frames (in hundredths of...
{ NULL },

> +};
> +
> +static const AVClass demuxer_class = {
> +    .class_name = "GIFDemuxContext",
> +    .item_name  = av_default_item_name,
> +    .option     = options,
> +    .version    = LIBAVUTIL_VERSION_INT,
> +    .category   = AV_CLASS_CATEGORY_DEMUXER,
> +};
> +
> +AVInputFormat ff_gif_demuxer = {
> +    .name           = "gif",
> +    .long_name      = NULL_IF_CONFIG_SMALL("CompuServe Graphics Interchange Format (GIF)"),
> +    .priv_data_size = sizeof(GIFDemuxContext),
> +    .read_probe     = gif_probe,
> +    .read_header    = gif_read_header,
> +    .read_packet    = gif_read_packet,
> +    .priv_class     = &demuxer_class,
> +};

Overall comment: can't you avoid the image2 demuxer to be selected by
default? IIRC a mjpeg stream is similar to this gif thing, and is playable
directly;

try ./ffmpeg -f lavfi -i testsrc=d=5 out.mjpeg && ./ffplay out.mjpeg for
instance.

Also, a bit unrelated, but I think the gif decoder should be moved to the
bytestream2 API...

Anyway, good work overall. I'm not a GIF maintainer at all, so please wait
a little for someone else for a LGTM.

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/20121014/909e9271/attachment.asc>


More information about the ffmpeg-devel mailing list