[FFmpeg-devel] [PATCH 1/4] avformat: Add and use helper function to add attachment streams
James Almer
jamrial at gmail.com
Tue Mar 30 16:14:12 EEST 2021
On 3/29/2021 5:42 AM, Andreas Rheinhardt wrote:
> All instances of adding attached pictures to a stream or adding
> a stream and an attached packet to said stream have several things
> in common like setting the index and flags of the packet, setting
> the stream disposition etc. This commit therefore factors this out.
>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at outlook.com>
> ---
> I always pondered factoring this out; James' proposal made me do it.
>
> libavformat/apetag.c | 10 +---------
> libavformat/flac_picture.c | 17 ++++-------------
> libavformat/id3v2.c | 21 ++++++---------------
> libavformat/internal.h | 13 +++++++++++++
> libavformat/matroskadec.c | 15 +++------------
> libavformat/mov.c | 25 +++++++------------------
> libavformat/utils.c | 30 ++++++++++++++++++++++++++++++
> libavformat/wtvdec.c | 12 ++----------
> 8 files changed, 66 insertions(+), 77 deletions(-)
>
> diff --git a/libavformat/apetag.c b/libavformat/apetag.c
> index 23ee6b516d..6f82fbe202 100644
> --- a/libavformat/apetag.c
> +++ b/libavformat/apetag.c
> @@ -79,20 +79,12 @@ static int ape_tag_read_field(AVFormatContext *s)
> av_dict_set(&st->metadata, key, filename, 0);
>
> if ((id = ff_guess_image2_codec(filename)) != AV_CODEC_ID_NONE) {
> - int ret;
> -
> - ret = av_get_packet(s->pb, &st->attached_pic, size);
> + int ret = ff_add_attached_pic(s, st, s->pb, NULL, size);
> if (ret < 0) {
> av_log(s, AV_LOG_ERROR, "Error reading cover art.\n");
> return ret;
> }
> -
> - st->disposition |= AV_DISPOSITION_ATTACHED_PIC;
> - st->codecpar->codec_type = AVMEDIA_TYPE_VIDEO;
> st->codecpar->codec_id = id;
> -
> - st->attached_pic.stream_index = st->index;
> - st->attached_pic.flags |= AV_PKT_FLAG_KEY;
> } else {
> if ((ret = ff_get_extradata(s, st->codecpar, s->pb, size)) < 0)
> return ret;
> diff --git a/libavformat/flac_picture.c b/libavformat/flac_picture.c
> index f15cfa877a..96e14f76c9 100644
> --- a/libavformat/flac_picture.c
> +++ b/libavformat/flac_picture.c
> @@ -160,20 +160,11 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size, int tr
> if (AV_RB64(data->data) == PNGSIG)
> id = AV_CODEC_ID_PNG;
>
> - st = avformat_new_stream(s, NULL);
> - if (!st) {
> - RETURN_ERROR(AVERROR(ENOMEM));
> - }
> -
> - av_packet_unref(&st->attached_pic);
> - st->attached_pic.buf = data;
> - st->attached_pic.data = data->data;
> - st->attached_pic.size = len;
> - st->attached_pic.stream_index = st->index;
> - st->attached_pic.flags |= AV_PKT_FLAG_KEY;
> + ret = ff_add_attached_pic(s, NULL, NULL, &data, 0);
> + if (ret < 0)
> + RETURN_ERROR(ret);
>
> - st->disposition |= AV_DISPOSITION_ATTACHED_PIC;
> - st->codecpar->codec_type = AVMEDIA_TYPE_VIDEO;
> + st = s->streams[s->nb_streams - 1];
> st->codecpar->codec_id = id;
> st->codecpar->width = width;
> st->codecpar->height = height;
> diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c
> index f33b7ba93a..863709abbf 100644
> --- a/libavformat/id3v2.c
> +++ b/libavformat/id3v2.c
> @@ -1142,34 +1142,25 @@ int ff_id3v2_parse_apic(AVFormatContext *s, ID3v2ExtraMeta *extra_meta)
> for (cur = extra_meta; cur; cur = cur->next) {
> ID3v2ExtraMetaAPIC *apic;
> AVStream *st;
> + int ret;
>
> if (strcmp(cur->tag, "APIC"))
> continue;
> apic = &cur->data.apic;
>
> - if (!(st = avformat_new_stream(s, NULL)))
> - return AVERROR(ENOMEM);
> -
> - st->disposition |= AV_DISPOSITION_ATTACHED_PIC;
> - st->codecpar->codec_type = AVMEDIA_TYPE_VIDEO;
> + ret = ff_add_attached_pic(s, NULL, NULL, &apic->buf, 0);
> + if (ret < 0)
> + return ret;
> + st = s->streams[s->nb_streams - 1];
> st->codecpar->codec_id = apic->id;
>
> - if (AV_RB64(apic->buf->data) == PNGSIG)
> + if (AV_RB64(st->attached_pic.data) == PNGSIG)
> st->codecpar->codec_id = AV_CODEC_ID_PNG;
>
> if (apic->description[0])
> av_dict_set(&st->metadata, "title", apic->description, 0);
>
> av_dict_set(&st->metadata, "comment", apic->type, 0);
> -
> - av_packet_unref(&st->attached_pic);
> - st->attached_pic.buf = apic->buf;
> - st->attached_pic.data = apic->buf->data;
> - st->attached_pic.size = apic->buf->size - AV_INPUT_BUFFER_PADDING_SIZE;
> - st->attached_pic.stream_index = st->index;
> - st->attached_pic.flags |= AV_PKT_FLAG_KEY;
> -
> - apic->buf = NULL;
> }
>
> return 0;
> diff --git a/libavformat/internal.h b/libavformat/internal.h
> index 8631694d00..b3c5d8a1d5 100644
> --- a/libavformat/internal.h
> +++ b/libavformat/internal.h
> @@ -669,6 +669,19 @@ int ff_framehash_write_header(AVFormatContext *s);
> */
> int ff_read_packet(AVFormatContext *s, AVPacket *pkt);
>
> +/**
> + * Add an attached pic to an AVStream.
> + *
> + * @param st if set, the stream to add the attached pic to;
> + * if unset, a new stream will be added to s.
> + * @param pb AVIOContext to read data from if buf is unset.
> + * @param buf if set, it contains the data and size information to be used
> + * for the attached pic; if unset, data is read from pb.
> + * @param size the size of the data to read if buf is unset.
> + */
> +int ff_add_attached_pic(AVFormatContext *s, AVStream *st, AVIOContext *pb,
> + AVBufferRef **buf, int size);
> +
> /**
> * Interleave an AVPacket per dts so it can be muxed.
> *
> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> index 1dc188c946..e8c76f9cfb 100644
> --- a/libavformat/matroskadec.c
> +++ b/libavformat/matroskadec.c
> @@ -3007,18 +3007,9 @@ static int matroska_read_header(AVFormatContext *s)
> attachments[j].stream = st;
>
> if (st->codecpar->codec_id != AV_CODEC_ID_NONE) {
> - AVPacket *pkt = &st->attached_pic;
> -
> - st->disposition |= AV_DISPOSITION_ATTACHED_PIC;
> - st->codecpar->codec_type = AVMEDIA_TYPE_VIDEO;
> -
> - av_packet_unref(pkt);
> - pkt->buf = attachments[j].bin.buf;
> - attachments[j].bin.buf = NULL;
> - pkt->data = attachments[j].bin.data;
> - pkt->size = attachments[j].bin.size;
> - pkt->stream_index = st->index;
> - pkt->flags |= AV_PKT_FLAG_KEY;
> + res = ff_add_attached_pic(s, st, NULL, &attachments[j].bin.buf, 0);
> + if (res < 0)
> + goto fail;
> } else {
> st->codecpar->codec_type = AVMEDIA_TYPE_ATTACHMENT;
> if (ff_alloc_extradata(st->codecpar, attachments[j].bin.size))
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index cb818ebe0e..097aa2bfb2 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -196,17 +196,16 @@ static int mov_read_covr(MOVContext *c, AVIOContext *pb, int type, int len)
> return 0;
> }
>
> - st = avformat_new_stream(c->fc, NULL);
> - if (!st)
> - return AVERROR(ENOMEM);
> sc = av_mallocz(sizeof(*sc));
> if (!sc)
> return AVERROR(ENOMEM);
> - st->priv_data = sc;
> -
> - ret = av_get_packet(pb, &st->attached_pic, len);
> - if (ret < 0)
> + ret = ff_add_attached_pic(c->fc, NULL, pb, NULL, len);
> + if (ret < 0) {
> + av_free(sc);
> return ret;
> + }
> + st = c->fc->streams[c->fc->nb_streams - 1];
> + st->priv_data = sc;
>
> if (st->attached_pic.size >= 8 && id != AV_CODEC_ID_BMP) {
> if (AV_RB64(st->attached_pic.data) == 0x89504e470d0a1a0a) {
> @@ -215,13 +214,6 @@ static int mov_read_covr(MOVContext *c, AVIOContext *pb, int type, int len)
> id = AV_CODEC_ID_MJPEG;
> }
> }
> -
> - st->disposition |= AV_DISPOSITION_ATTACHED_PIC;
> -
> - st->attached_pic.stream_index = st->index;
> - st->attached_pic.flags |= AV_PKT_FLAG_KEY;
> -
> - st->codecpar->codec_type = AVMEDIA_TYPE_VIDEO;
> st->codecpar->codec_id = id;
>
> return 0;
> @@ -7229,11 +7221,8 @@ static void mov_read_chapters(AVFormatContext *s)
> goto finish;
> }
>
> - if (av_get_packet(sc->pb, &st->attached_pic, sample->size) < 0)
> + if (ff_add_attached_pic(s, st, sc->pb, NULL, sample->size) < 0)
> goto finish;
> -
> - st->attached_pic.stream_index = st->index;
> - st->attached_pic.flags |= AV_PKT_FLAG_KEY;
> }
> } else {
> st->codecpar->codec_type = AVMEDIA_TYPE_DATA;
> diff --git a/libavformat/utils.c b/libavformat/utils.c
> index 88f6f18f1f..2bd7dd8ec7 100644
> --- a/libavformat/utils.c
> +++ b/libavformat/utils.c
> @@ -474,6 +474,36 @@ int avformat_queue_attached_pictures(AVFormatContext *s)
> return 0;
> }
>
> +int ff_add_attached_pic(AVFormatContext *s, AVStream *st, AVIOContext *pb,
> + AVBufferRef **buf, int size)
> +{
> + AVPacket *pkt;
> + int ret;
> +
> + if (!st && !(st = avformat_new_stream(s, NULL)))
> + return AVERROR(ENOMEM);
> + pkt = &st->attached_pic;
> + if (buf) {
> + av_assert1(*buf);
The code below is going to crash as soon as *buf is dereferenced if it's
NULL, so unless this is changed to av_assert0(), adding this seems
pointless.
> + av_packet_unref(pkt);
> + pkt->buf = *buf;
> + pkt->data = (*buf)->data;
> + pkt->size = (*buf)->size - AV_INPUT_BUFFER_PADDING_SIZE;
I suppose all buffers were allocated with padding, right? (I see
matroskadec did, but didn't take it into account when setting pkt->size,
which this patch here fixes).
> + *buf = NULL;
> + } else {
> + ret = av_get_packet(pb, pkt, size);
> + if (ret < 0)
> + return ret;
> + }
> + st->disposition |= AV_DISPOSITION_ATTACHED_PIC;
> + st->codecpar->codec_type = AVMEDIA_TYPE_VIDEO;
> +
> + pkt->stream_index = st->index;
> + pkt->flags |= AV_PKT_FLAG_KEY;
> +
> + return 0;
> +}
> +
> static int update_stream_avctx(AVFormatContext *s)
> {
> int i, ret;
> diff --git a/libavformat/wtvdec.c b/libavformat/wtvdec.c
> index 7def9d2348..e67e03a033 100644
> --- a/libavformat/wtvdec.c
> +++ b/libavformat/wtvdec.c
> @@ -434,7 +434,6 @@ static void get_attachment(AVFormatContext *s, AVIOContext *pb, int length)
> char description[1024];
> unsigned int filesize;
> AVStream *st;
> - int ret;
> int64_t pos = avio_tell(pb);
>
> avio_get_str16le(pb, INT_MAX, mime, sizeof(mime));
> @@ -447,19 +446,12 @@ static void get_attachment(AVFormatContext *s, AVIOContext *pb, int length)
> if (!filesize)
> goto done;
>
> - st = avformat_new_stream(s, NULL);
> - if (!st)
> + if (ff_add_attached_pic(s, NULL, pb, NULL, filesize) < 0)
> goto done;
> + st = s->streams[s->nb_streams - 1];
> av_dict_set(&st->metadata, "title", description, 0);
> - st->codecpar->codec_type = AVMEDIA_TYPE_VIDEO;
> st->codecpar->codec_id = AV_CODEC_ID_MJPEG;
> st->id = -1;
> - ret = av_get_packet(pb, &st->attached_pic, filesize);
> - if (ret < 0)
> - goto done;
> - st->attached_pic.stream_index = st->index;
> - st->attached_pic.flags |= AV_PKT_FLAG_KEY;
> - st->disposition |= AV_DISPOSITION_ATTACHED_PIC;
> done:
> avio_seek(pb, pos + length, SEEK_SET);
> }
Should be ok.
More information about the ffmpeg-devel
mailing list