[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