[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