diff mbox series

[FFmpeg-devel,1/4] avformat: Add and use helper function to add attachment streams

Message ID HE1PR0301MB21546A10BC5A180D6A1E752B8F7E9@HE1PR0301MB2154.eurprd03.prod.outlook.com
State Accepted
Headers show
Series [FFmpeg-devel,1/4] avformat: Add and use helper function to add attachment streams
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Andreas Rheinhardt March 29, 2021, 8:42 a.m. UTC
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@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(-)

Comments

James Almer March 30, 2021, 1:14 p.m. UTC | #1
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@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.
Andreas Rheinhardt March 30, 2021, 1:45 p.m. UTC | #2
James Almer:
> 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@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.

I disagree: It more directly conveys to every reader that the case of
(buf && !*buf) is illegal.

> 
>> +        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).
> 

Not true: ebml_read_binary() sets EbmlBin.size to the size without
padding, whereas EbmlBin.buf->size gets set to the size + padding. The
current code in matroskadec.c uses EbmlBin.size, the code above uses the
size inferred from the AVBufferRef (as the id3v2 code already does).
flac_picture is the third user providing an AVBufferRef; it also adds
padding.

>> +        *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.
> _______________________________________________
> 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".
James Almer March 30, 2021, 1:53 p.m. UTC | #3
On 3/30/2021 10:45 AM, Andreas Rheinhardt wrote:
> James Almer:
>> 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@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.
> 
> I disagree: It more directly conveys to every reader that the case of
> (buf && !*buf) is illegal.

I personally consider asserts exist mainly to detect and prevent 
internal bugs from causing mayhem rather than to hint code readers of 
something that's pretty apparent like in this case with the dereferences 
three lines below, but i'm not going to oposse you adding this.

> 
>>
>>> +        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).
>>
> 
> Not true: ebml_read_binary() sets EbmlBin.size to the size without
> padding, whereas EbmlBin.buf->size gets set to the size + padding. The
> current code in matroskadec.c uses EbmlBin.size, the code above uses the
> size inferred from the AVBufferRef (as the id3v2 code already does).
> flac_picture is the third user providing an AVBufferRef; it also adds
> padding.

Ah right, it was setting the value from EbmlBin and not AVBufferRef. 
Nevemrind then.

> 
>>> +        *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.
>> _______________________________________________
>> 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".
> 
> _______________________________________________
> 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".
>
diff mbox series

Patch

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);
+        av_packet_unref(pkt);
+        pkt->buf  = *buf;
+        pkt->data = (*buf)->data;
+        pkt->size = (*buf)->size - AV_INPUT_BUFFER_PADDING_SIZE;
+        *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);
 }