From: James Almer <jamrial@gmail.com>
To: ffmpeg-devel@ffmpeg.org
Subject: Re: [FFmpeg-devel] [PATCH] avformat/gifdec: cleanup
Date: Mon, 22 May 2023 13:04:00 -0300
Message-ID: <4757bce8-1185-714a-88a3-2135bc51390b@gmail.com> (raw)
In-Reply-To: <CAPYw7P5SNno30A6KL8zMdtuyfmaQmhq4-04gn+DGNZwtsmT27w@mail.gmail.com>
On 5/22/2023 12:49 PM, Paul B Mahol wrote:
> On 5/21/23, Paul B Mahol <onemda@gmail.com> wrote:
>> Attached patches.
>>
>> This finally removes giant hacks in gif demuxer and allows using gif
>> files via pipe reliably.
>>
>
> Now with smaller diff.
[...]
> From 208d1e83ae9aef8e9b37007df16569cdd4cf25d2 Mon Sep 17 00:00:00 2001
> From: Paul B Mahol <onemda@gmail.com>
> Date: Sat, 20 May 2023 14:13:27 +0200
> Subject: [PATCH 2/2] avformat/gifdec: switch to using gif parser
>
> Update fate tests, more correct as timebase is no more reduced.
This is not the case anymore.
>
> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> ---
> libavcodec/gifdec.c | 11 +-
> libavformat/gifdec.c | 244 +++++++++++--------------------------------
> tests/ref/lavf/gif | 2 +-
> 3 files changed, 70 insertions(+), 187 deletions(-)
>
> diff --git a/libavcodec/gifdec.c b/libavcodec/gifdec.c
> index 0835c5bdd0..f2ab783ef0 100644
> --- a/libavcodec/gifdec.c
> +++ b/libavcodec/gifdec.c
> @@ -472,10 +472,6 @@ static int gif_decode_frame(AVCodecContext *avctx, AVFrame *rframe,
>
> bytestream2_init(&s->gb, avpkt->data, avpkt->size);
>
> - s->frame->pts = avpkt->pts;
> - s->frame->pkt_dts = avpkt->dts;
> - s->frame->duration = avpkt->duration;
> -
> if (avpkt->size >= 6) {
> s->keyframe = memcmp(avpkt->data, gif87a_sig, 6) == 0 ||
> memcmp(avpkt->data, gif89a_sig, 6) == 0;
> @@ -522,6 +518,13 @@ static int gif_decode_frame(AVCodecContext *avctx, AVFrame *rframe,
>
> if ((ret = av_frame_ref(rframe, s->frame)) < 0)
> return ret;
> + if (s->keyframe) {
> + rframe->pict_type = AV_PICTURE_TYPE_I;
> + rframe->flags |= AV_FRAME_FLAG_KEY;
> + } else {
> + rframe->pict_type = AV_PICTURE_TYPE_P;
> + rframe->flags &= ~AV_FRAME_FLAG_KEY;
> + }
> *got_frame = 1;
>
> return bytestream2_tell(&s->gb);
> diff --git a/libavformat/gifdec.c b/libavformat/gifdec.c
> index 1977f46e3a..11fcde36b7 100644
> --- a/libavformat/gifdec.c
> +++ b/libavformat/gifdec.c
> @@ -28,9 +28,12 @@
> #include "libavutil/bprint.h"
> #include "libavutil/intreadwrite.h"
> #include "libavutil/opt.h"
> +#include "avio_internal.h"
> #include "internal.h"
> #include "libavcodec/gif.h"
>
> +#define GIF_PACKET_SIZE 1024
> +
> typedef struct GIFDemuxContext {
> const AVClass *class;
> /**
> @@ -53,9 +56,6 @@ typedef struct GIFDemuxContext {
> int total_iter;
> int iter_count;
> int ignore_loop;
> -
> - int nb_frames;
> - int last_duration;
> } GIFDemuxContext;
>
> /**
> @@ -84,8 +84,8 @@ static int gif_probe(const AVProbeData *p)
>
> static int resync(AVIOContext *pb)
> {
> - int i;
> - for (i = 0; i < 6; i++) {
> + ffio_ensure_seekback(pb, 13);
> + for (int i = 0; i < 6; i++) {
> int b = avio_r8(pb);
> if (b != gif87a_sig[i] && b != gif89a_sig[i])
> i = -(b != 'G');
> @@ -132,6 +132,9 @@ static int gif_read_header(AVFormatContext *s)
> if (!st)
> return AVERROR(ENOMEM);
>
> + if (!(pb->seekable & AVIO_SEEKABLE_NORMAL))
> + goto skip;
> +
> if (flags & 0x80)
> avio_skip(pb, 3 * (1 << ((flags & 0x07) + 1)));
>
> @@ -158,15 +161,37 @@ static int gif_read_header(AVFormatContext *s)
>
> avio_skip(pb, 1);
> delay = avio_rl16(pb);
> - if (delay < gdc->min_delay)
> - delay = gdc->default_delay;
> - delay = FFMIN(delay, gdc->max_delay);
> + delay = delay ? delay : gdc->default_delay;
> duration += delay;
> avio_skip(pb, 1);
> } else {
> avio_skip(pb, block_size);
> }
> gif_skip_subblocks(pb);
> + } else if (subtype == GIF_APP_EXT_LABEL) {
> + uint8_t data[256];
> + int sb_size;
> +
> + sb_size = avio_r8(pb);
> + ret = avio_read(pb, data, sb_size);
> + if (ret < 0 || !sb_size)
> + break;
> +
> + if (sb_size == strlen(NETSCAPE_EXT_STR)) {
> + sb_size = avio_r8(pb);
> + ret = avio_read(pb, data, sb_size);
> + if (ret < 0 || !sb_size)
> + break;
> +
> + if (sb_size == 3 && data[0] == 1) {
> + gdc->total_iter = AV_RL16(data+1);
> + av_log(s, AV_LOG_DEBUG, "Loop count is %d\n", gdc->total_iter);
> +
> + if (gdc->total_iter == 0)
> + gdc->total_iter = -1;
> + }
> + }
> + gif_skip_subblocks(pb);
> } else {
> gif_skip_subblocks(pb);
> }
> @@ -183,203 +208,57 @@ static int gif_read_header(AVFormatContext *s)
> }
> }
>
> +skip:
> + /* jump to start because gif decoder needs header data too */
> + if (avio_seek(pb, 0, SEEK_SET) != 0)
> + return AVERROR(EIO);
> +
> /* GIF format operates with time in "hundredths of second",
> * therefore timebase is 1/100 */
> avpriv_set_pts_info(st, 64, 1, 100);
> + ffstream(st)->need_parsing = AVSTREAM_PARSE_FULL_RAW;
> st->codecpar->codec_type = AVMEDIA_TYPE_VIDEO;
> st->codecpar->codec_id = AV_CODEC_ID_GIF;
> st->codecpar->width = width;
> st->codecpar->height = height;
> + if (nb_frames > 1) {
> + av_reduce(&st->avg_frame_rate.num, &st->avg_frame_rate.den,
> + 100, duration / nb_frames, INT_MAX);
> + } else if (duration) {
> + st->avg_frame_rate = (AVRational) { 100, duration };
> + }
> st->start_time = 0;
> st->duration = duration;
> st->nb_frames = nb_frames;
> - if (n) {
> - st->codecpar->sample_aspect_ratio.num = n + 15;
> - st->codecpar->sample_aspect_ratio.den = 64;
> - }
> -
> - /* jump to start because gif decoder needs header data too */
> - if (avio_seek(pb, 0, SEEK_SET) != 0)
> - return AVERROR(EIO);
> + if (n)
> + st->codecpar->sample_aspect_ratio = av_make_q(n + 15, 64);
>
> return 0;
> }
>
> -static int gif_read_ext(AVFormatContext *s)
> +static int gif_read_packet(AVFormatContext *s, AVPacket *pkt)
> {
> GIFDemuxContext *gdc = s->priv_data;
> AVIOContext *pb = s->pb;
> - int sb_size, ext_label = avio_r8(pb);
> int ret;
>
> - if (ext_label == GIF_GCE_EXT_LABEL) {
> - if ((sb_size = avio_r8(pb)) < 4) {
> - av_log(s, AV_LOG_FATAL, "Graphic Control Extension block's size less than 4.\n");
> - return AVERROR_INVALIDDATA;
> - }
> -
> - /* skip packed fields */
> - if ((ret = avio_skip(pb, 1)) < 0)
> - return ret;
> -
> - gdc->delay = avio_rl16(pb);
> -
> - if (gdc->delay < gdc->min_delay)
> - gdc->delay = gdc->default_delay;
> - gdc->delay = FFMIN(gdc->delay, gdc->max_delay);
> -
> - /* skip the rest of the Graphic Control Extension block */
> - if ((ret = avio_skip(pb, sb_size - 3)) < 0 )
> - return ret;
> - } else if (ext_label == GIF_APP_EXT_LABEL) {
> - uint8_t data[256];
> -
> - sb_size = avio_r8(pb);
> - ret = avio_read(pb, data, sb_size);
> - if (ret < 0 || !sb_size)
> - return ret;
> -
> - if (sb_size == strlen(NETSCAPE_EXT_STR)) {
> - sb_size = avio_r8(pb);
> - ret = avio_read(pb, data, sb_size);
> - if (ret < 0 || !sb_size)
> - return ret;
> -
> - if (sb_size == 3 && data[0] == 1) {
> - gdc->total_iter = AV_RL16(data+1);
> - av_log(s, AV_LOG_DEBUG, "Loop count is %d\n", gdc->total_iter);
> -
> - if (gdc->total_iter == 0)
> - gdc->total_iter = -1;
> - }
> - }
> + if ((pb->seekable & AVIO_SEEKABLE_NORMAL) &&
> + !gdc->ignore_loop && avio_feof(pb) &&
> + (gdc->total_iter < 0 || (++gdc->iter_count < gdc->total_iter))) {
> + avio_seek(pb, 0, SEEK_SET);
> }
> -
> - if ((ret = gif_skip_subblocks(pb)) < 0)
> + if ((ret = av_new_packet(pkt, GIF_PACKET_SIZE)) < 0)
> return ret;
>
> - return 0;
> -}
> -
> -static int gif_read_packet(AVFormatContext *s, AVPacket *pkt)
> -{
> - GIFDemuxContext *gdc = s->priv_data;
> - AVIOContext *pb = s->pb;
> - int packed_fields, block_label, ct_size,
> - keyframe, frame_parsed = 0, ret;
> - int64_t frame_start = avio_tell(pb), frame_end;
> - unsigned char buf[6];
> -
> - if ((ret = avio_read(pb, buf, 6)) == 6) {
> - keyframe = memcmp(buf, gif87a_sig, 6) == 0 ||
> - memcmp(buf, gif89a_sig, 6) == 0;
> - } else if (ret < 0) {
> + pkt->pos = avio_tell(pb);
> + pkt->stream_index = 0;
> + ret = avio_read_partial(pb, pkt->data, GIF_PACKET_SIZE);
> + if (ret < 0) {
> + av_packet_unref(pkt);
> return ret;
> - } else {
> - keyframe = 0;
> - }
> -
> - if (keyframe) {
> -parse_keyframe:
> - /* skip 2 bytes of width and 2 of height */
> - if ((ret = avio_skip(pb, 4)) < 0)
> - return ret;
> -
> - packed_fields = avio_r8(pb);
> -
> - /* skip 1 byte of Background Color Index and 1 byte of Pixel Aspect Ratio */
> - if ((ret = avio_skip(pb, 2)) < 0)
> - return ret;
> -
> - /* global color table presence */
> - if (packed_fields & 0x80) {
> - ct_size = 3 * (1 << ((packed_fields & 0x07) + 1));
> -
> - if ((ret = avio_skip(pb, ct_size)) < 0)
> - return ret;
> - }
> - } else {
> - avio_seek(pb, -ret, SEEK_CUR);
> - ret = AVERROR_EOF;
> }
> -
> - while (GIF_TRAILER != (block_label = avio_r8(pb)) && !avio_feof(pb)) {
> - if (block_label == GIF_EXTENSION_INTRODUCER) {
> - if ((ret = gif_read_ext (s)) < 0 )
> - goto resync;
> - } else if (block_label == GIF_IMAGE_SEPARATOR) {
> - /* skip to last byte of Image Descriptor header */
> - if ((ret = avio_skip(pb, 8)) < 0)
> - return ret;
> -
> - packed_fields = avio_r8(pb);
> -
> - /* local color table presence */
> - if (packed_fields & 0x80) {
> - ct_size = 3 * (1 << ((packed_fields & 0x07) + 1));
> -
> - if ((ret = avio_skip(pb, ct_size)) < 0)
> - return ret;
> - }
> -
> - /* read LZW Minimum Code Size */
> - if (avio_r8(pb) < 1) {
> - av_log(s, AV_LOG_ERROR, "lzw minimum code size must be >= 1\n");
> - goto resync;
> - }
> -
> - if ((ret = gif_skip_subblocks(pb)) < 0)
> - goto resync;
> -
> - frame_end = avio_tell(pb);
> -
> - if (avio_seek(pb, frame_start, SEEK_SET) != frame_start)
> - return AVERROR(EIO);
> -
> - ret = av_get_packet(pb, pkt, frame_end - frame_start);
> - if (ret < 0)
> - return ret;
> -
> - if (keyframe)
> - pkt->flags |= AV_PKT_FLAG_KEY;
> -
> - pkt->stream_index = 0;
> - pkt->duration = gdc->delay;
> -
> - gdc->nb_frames ++;
> - gdc->last_duration = pkt->duration;
> -
> - /* Graphic Control Extension's scope is single frame.
> - * Remove its influence. */
> - gdc->delay = gdc->default_delay;
> - frame_parsed = 1;
> -
> - break;
> - } else {
> - av_log(s, AV_LOG_ERROR, "invalid block label\n");
> -resync:
> - if (!keyframe)
> - avio_seek(pb, frame_start, SEEK_SET);
> - if ((ret = resync(pb)) < 0)
> - return ret;
> - frame_start = avio_tell(pb) - 6;
> - keyframe = 1;
> - goto parse_keyframe;
> - }
> - }
> -
> - if ((ret >= 0 && !frame_parsed) || ret == AVERROR_EOF) {
> - if (gdc->nb_frames == 1) {
> - s->streams[0]->r_frame_rate = (AVRational) {100, gdc->last_duration};
> - }
> - /* This might happen when there is no image block
> - * between extension blocks and GIF_TRAILER or EOF */
> - if (!gdc->ignore_loop && (block_label == GIF_TRAILER || avio_feof(pb))
> - && (gdc->total_iter < 0 || ++gdc->iter_count < gdc->total_iter))
> - return avio_seek(pb, 0, SEEK_SET);
> - return AVERROR_EOF;
> - } else
> - return ret;
> + av_shrink_packet(pkt, ret);
> + return ret;
> }
>
> static const AVOption options[] = {
> @@ -405,6 +284,7 @@ const AVInputFormat ff_gif_demuxer = {
> .read_probe = gif_probe,
> .read_header = gif_read_header,
> .read_packet = gif_read_packet,
> - .flags = AVFMT_GENERIC_INDEX,
> + .flags = AVFMT_GENERIC_INDEX | AVFMT_NOTIMESTAMPS,
> + .extensions = "gif",
> .priv_class = &demuxer_class,
> };
> diff --git a/tests/ref/lavf/gif b/tests/ref/lavf/gif
> index fc94b9df3d..30eb85325a 100644
> --- a/tests/ref/lavf/gif
> +++ b/tests/ref/lavf/gif
> @@ -1,3 +1,3 @@
> e35f5ea283bbcb249818e0078ec72664 *tests/data/lavf/lavf.gif
> 2011766 tests/data/lavf/lavf.gif
> -tests/data/lavf/lavf.gif CRC=0x2429faff
> +tests/data/lavf/lavf.gif CRC=0x563cec26
> --
> 2.39.1
TEST gif-demux
--- /home/Ethaniel/FFmpeg/tests/ref/fate/gif-demux 2023-05-21
22:13:04.358066200 -0300
+++ tests/data/fate/gif-demux 2023-05-22 13:00:37.259301700 -0300
@@ -38,4 +38,4 @@
0, 74, 74, 2, 4633, 0x8f64fda7, F=0x0
0, 76, 76, 2, 4700, 0x45f40805, F=0x0
0, 78, 78, 2, 5117, 0x4eb4c5fb, F=0x0
-0, 80, 80, 2, 5370, 0xb10c6910, F=0x0
+0, 80, 80, 2, 5371, 0x1a66694b, F=0x0
Test gif-demux failed. Look at tests/data/fate/gif-demux.err for details.
And
TEST lavf-gif
--- /home/Ethaniel/FFmpeg/tests/ref/lavf/gif 2023-05-22
13:00:14.259590500 -0300
+++ tests/data/fate/lavf-gif 2023-05-22 13:01:19.316371100 -0300
@@ -1,3 +1,3 @@
e35f5ea283bbcb249818e0078ec72664 *tests/data/lavf/lavf.gif
2011766 tests/data/lavf/lavf.gif
-tests/data/lavf/lavf.gif CRC=0x563cec26
+tests/data/lavf/lavf.gif CRC=0x2429faff
Test lavf-gif failed. Look at tests/data/fate/lavf-gif.err for details.
This one giving the same result as pre-patch. Are you sure you ran fate
on a clean tree and build folder?
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
next prev parent reply other threads:[~2023-05-22 16:03 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-21 9:38 Paul B Mahol
2023-05-21 10:05 ` Anton Khirnov
2023-05-21 11:24 ` Paul B Mahol
2023-05-21 17:36 ` Michael Niedermayer
2023-05-21 17:59 ` Paul B Mahol
2023-05-21 19:25 ` Paul B Mahol
2023-05-21 19:37 ` James Almer
2023-05-21 20:02 ` Paul B Mahol
2023-05-21 20:06 ` James Almer
2023-05-21 21:42 ` Paul B Mahol
2023-05-22 15:49 ` Paul B Mahol
2023-05-22 16:04 ` James Almer [this message]
2023-05-22 16:07 ` James Almer
2023-05-22 16:10 ` Anton Khirnov
2023-05-22 16:19 ` Paul B Mahol
2023-05-22 20:01 ` James Almer
2023-05-23 21:51 ` Michael Niedermayer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4757bce8-1185-714a-88a3-2135bc51390b@gmail.com \
--to=jamrial@gmail.com \
--cc=ffmpeg-devel@ffmpeg.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
This inbox may be cloned and mirrored by anyone:
git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git
# If you have public-inbox 1.1+ installed, you may
# initialize and index your mirror using the following commands:
public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \
ffmpegdev@gitmailbox.com
public-inbox-index ffmpegdev
Example config snippet for mirrors.
AGPL code for this site: git clone https://public-inbox.org/public-inbox.git