diff mbox series

[FFmpeg-devel] avformat: add apic to AVStream

Message ID 20210328155831.54434-1-jamrial@gmail.com
State New
Headers show
Series [FFmpeg-devel] avformat: add apic to AVStream
Related show

Checks

Context Check Description
andriy/x86_make_warn warning New warnings during build
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

James Almer March 28, 2021, 3:58 p.m. UTC
As a replacement for attached_pic, which is in turn deprecated and scheduled
for removal.

Signed-off-by: James Almer <jamrial@gmail.com>
---
TODO: APIChanges entry and version bump.

Decided to use the name apic for the field, since it's how id3v2 and other
formats call it.

Also, in a fortunate coincidence, the first "non public" field in AVStream is a
unused void pointer that had to be left in place when the field was moved to
AVStreamInternal to keep every offset intact for the sake of ffmpeg accessing
fields after it, so I'm going to take advantage of it so this doesn't have to
wait until the major bump.

 libavformat/apetag.c       | 15 ++++++++++++---
 libavformat/asfdec_f.c     | 21 +++++++++++++++------
 libavformat/asfdec_o.c     | 21 +++++++++++++++------
 libavformat/avformat.h     | 21 ++++++++++++++++-----
 libavformat/flac_picture.c | 21 +++++++++++++++------
 libavformat/hls.c          |  6 +++---
 libavformat/id3v2.c        | 23 +++++++++++++++++------
 libavformat/matroskadec.c  |  9 ++++++++-
 libavformat/mov.c          | 31 +++++++++++++++++++++++--------
 libavformat/utils.c        | 14 ++++++++++++--
 libavformat/version.h      |  3 +++
 libavformat/wtvdec.c       | 13 ++++++++++---
 12 files changed, 149 insertions(+), 49 deletions(-)

Comments

Marton Balint March 28, 2021, 8:32 p.m. UTC | #1
On Sun, 28 Mar 2021, James Almer wrote:

> As a replacement for attached_pic, which is in turn deprecated and scheduled
> for removal.
>
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> TODO: APIChanges entry and version bump.
>
> Decided to use the name apic for the field, since it's how id3v2 and other
> formats call it.

Fine by me.

> diff --git a/libavformat/flac_picture.c b/libavformat/flac_picture.c
> index f15cfa877a..f771df3fc2 100644
> --- a/libavformat/flac_picture.c
> +++ b/libavformat/flac_picture.c
> @@ -165,12 +165,20 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size, int tr
>         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;
> +    av_packet_unref(st->apic);
> +    st->apic->buf          = data;
> +    st->apic->data         = data->data;
> +    st->apic->size         = len;
> +    st->apic->stream_index = st->index;
> +    st->apic->flags       |= AV_PKT_FLAG_KEY;
> +    data = NULL;
> +#if FF_API_ATTACHED_PIC
> +FF_DISABLE_DEPRECATION_WARNINGS

If st->apic is unreffed above, shouldn't you also unref st->attached_pic 
here?

Thanks,
Marton

> +    ret = av_packet_ref(&st->attached_pic, st->apic);
> +    if (ret < 0)
> +        goto fail;
> +FF_ENABLE_DEPRECATION_WARNINGS
> +#endif
>
James Almer March 28, 2021, 8:42 p.m. UTC | #2
On 3/28/2021 5:32 PM, Marton Balint wrote:
> 
> 
> On Sun, 28 Mar 2021, James Almer wrote:
> 
>> As a replacement for attached_pic, which is in turn deprecated and 
>> scheduled
>> for removal.
>>
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>> TODO: APIChanges entry and version bump.
>>
>> Decided to use the name apic for the field, since it's how id3v2 and 
>> other
>> formats call it.
> 
> Fine by me.
> 
>> diff --git a/libavformat/flac_picture.c b/libavformat/flac_picture.c
>> index f15cfa877a..f771df3fc2 100644
>> --- a/libavformat/flac_picture.c
>> +++ b/libavformat/flac_picture.c
>> @@ -165,12 +165,20 @@ int ff_flac_parse_picture(AVFormatContext *s, 
>> uint8_t *buf, int buf_size, int tr
>>         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;
>> +    av_packet_unref(st->apic);
>> +    st->apic->buf          = data;
>> +    st->apic->data         = data->data;
>> +    st->apic->size         = len;
>> +    st->apic->stream_index = st->index;
>> +    st->apic->flags       |= AV_PKT_FLAG_KEY;
>> +    data = NULL;
>> +#if FF_API_ATTACHED_PIC
>> +FF_DISABLE_DEPRECATION_WARNINGS
> 
> If st->apic is unreffed above, shouldn't you also unref st->attached_pic 
> here?

No, i can in fact just remove it, since the stream was allocated right 
before it.

I think this unref is there because it was previously an 
av_init_packet(), which i replaced as part of my deprecation work to 
achieve the same effect of initializing st->attached_pic (a zeroed 
packet), for the sake of setting fields like pts to no_pts (This is not 
needed for st->apic, which is properly initialized when it's allocated).
The av_packet_ref() below will set all st->attached_pic fields.

> 
> Thanks,
> Marton
> 
>> +    ret = av_packet_ref(&st->attached_pic, st->apic);
>> +    if (ret < 0)
>> +        goto fail;
>> +FF_ENABLE_DEPRECATION_WARNINGS
>> +#endif
>>
> _______________________________________________
> 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".
Lynne March 29, 2021, 1:11 a.m. UTC | #3
Mar 28, 2021, 17:58 by jamrial@gmail.com:

> As a replacement for attached_pic, which is in turn deprecated and scheduled
> for removal.
>
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> TODO: APIChanges entry and version bump.
>
> Decided to use the name apic for the field, since it's how id3v2 and other
> formats call it.
>

Personally I don't really like 'apic'. Maybe 'pic_attachment' or 'attachment_pic'?
Nicolas George March 29, 2021, 7:38 a.m. UTC | #4
Lynne (12021-03-29):
> Personally I don't really like 'apic'. Maybe 'pic_attachment' or 'attachment_pic'?

By the way, do we want it to be a field?

Attached pictures can be quite big. Do we want them to stay in AVStream
forever, even when the application has already finished using them, or
does not use them at all?

Regards,
Andreas Rheinhardt March 29, 2021, 9:46 a.m. UTC | #5
James Almer:
> As a replacement for attached_pic, which is in turn deprecated and scheduled
> for removal.
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> TODO: APIChanges entry and version bump.
> 
> Decided to use the name apic for the field, since it's how id3v2 and other
> formats call it.
> 
> Also, in a fortunate coincidence, the first "non public" field in AVStream is a
> unused void pointer that had to be left in place when the field was moved to
> AVStreamInternal to keep every offset intact for the sake of ffmpeg accessing
> fields after it, so I'm going to take advantage of it so this doesn't have to
> wait until the major bump.
> 
>  libavformat/apetag.c       | 15 ++++++++++++---
>  libavformat/asfdec_f.c     | 21 +++++++++++++++------
>  libavformat/asfdec_o.c     | 21 +++++++++++++++------
>  libavformat/avformat.h     | 21 ++++++++++++++++-----
>  libavformat/flac_picture.c | 21 +++++++++++++++------
>  libavformat/hls.c          |  6 +++---
>  libavformat/id3v2.c        | 23 +++++++++++++++++------
>  libavformat/matroskadec.c  |  9 ++++++++-
>  libavformat/mov.c          | 31 +++++++++++++++++++++++--------
>  libavformat/utils.c        | 14 ++++++++++++--
>  libavformat/version.h      |  3 +++
>  libavformat/wtvdec.c       | 13 ++++++++++---
>  12 files changed, 149 insertions(+), 49 deletions(-)
> 
> diff --git a/libavformat/apetag.c b/libavformat/apetag.c
> index 23ee6b516d..e0e96c3f4c 100644
> --- a/libavformat/apetag.c
> +++ b/libavformat/apetag.c
> @@ -81,7 +81,7 @@ static int ape_tag_read_field(AVFormatContext *s)
>          if ((id = ff_guess_image2_codec(filename)) != AV_CODEC_ID_NONE) {
>              int ret;
>  
> -            ret = av_get_packet(s->pb, &st->attached_pic, size);
> +            ret = av_get_packet(s->pb, st->apic, size);
>              if (ret < 0) {
>                  av_log(s, AV_LOG_ERROR, "Error reading cover art.\n");
>                  return ret;
> @@ -91,8 +91,17 @@ static int ape_tag_read_field(AVFormatContext *s)
>              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;
> +            st->apic->stream_index   = st->index;
> +            st->apic->flags         |= AV_PKT_FLAG_KEY;
> +#if FF_API_ATTACHED_PIC
> +FF_DISABLE_DEPRECATION_WARNINGS
> +            ret = av_packet_ref(&st->attached_pic, st->apic);
> +            if (ret < 0) {
> +                av_packet_unref(st->apic);
> +                return ret;
> +            }
> +FF_ENABLE_DEPRECATION_WARNINGS
> +#endif
>          } else {
>              if ((ret = ff_get_extradata(s, st->codecpar, s->pb, size)) < 0)
>                  return ret;
> diff --git a/libavformat/asfdec_f.c b/libavformat/asfdec_f.c
> index 2fae528f4d..838fc924d5 100644
> --- a/libavformat/asfdec_f.c
> +++ b/libavformat/asfdec_f.c
> @@ -223,7 +223,7 @@ static int get_value(AVIOContext *pb, int type, int type2_size)
>   * but in reality this is only loosely similar */
>  static int asf_read_picture(AVFormatContext *s, int len)
>  {
> -    AVPacket pkt          = { 0 };
> +    AVPacket *pkt = s->internal->parse_pkt;
>      const CodecMime *mime = ff_id3v2_mime_tags;
>      enum  AVCodecID id    = AV_CODEC_ID_NONE;
>      char mimetype[64];
> @@ -277,7 +277,7 @@ static int asf_read_picture(AVFormatContext *s, int len)
>          return AVERROR(ENOMEM);
>      len -= avio_get_str16le(s->pb, len - picsize, desc, desc_len);
>  
> -    ret = av_get_packet(s->pb, &pkt, picsize);
> +    ret = av_get_packet(s->pb, pkt, picsize);
>      if (ret < 0)
>          goto fail;
>  
> @@ -286,12 +286,21 @@ static int asf_read_picture(AVFormatContext *s, int len)
>          ret = AVERROR(ENOMEM);
>          goto fail;
>      }
> +    av_packet_move_ref(st->apic, pkt);
>      st->disposition              |= AV_DISPOSITION_ATTACHED_PIC;
>      st->codecpar->codec_type      = AVMEDIA_TYPE_VIDEO;
>      st->codecpar->codec_id        = id;
> -    st->attached_pic              = pkt;
> -    st->attached_pic.stream_index = st->index;
> -    st->attached_pic.flags       |= AV_PKT_FLAG_KEY;
> +    st->apic->stream_index        = st->index;
> +    st->apic->flags              |= AV_PKT_FLAG_KEY;
> +#if FF_API_ATTACHED_PIC
> +FF_DISABLE_DEPRECATION_WARNINGS
> +    ret = av_packet_ref(&st->attached_pic, st->apic);
> +    if (ret < 0) {
> +        av_packet_unref(st->apic);
> +        goto fail;
> +    }
> +FF_ENABLE_DEPRECATION_WARNINGS
> +#endif
>  
>      if (*desc)
>          av_dict_set(&st->metadata, "title", desc, AV_DICT_DONT_STRDUP_VAL);
> @@ -304,7 +313,7 @@ static int asf_read_picture(AVFormatContext *s, int len)
>  
>  fail:
>      av_freep(&desc);
> -    av_packet_unref(&pkt);
> +    av_packet_unref(pkt);
>      return ret;
>  }
>  
> diff --git a/libavformat/asfdec_o.c b/libavformat/asfdec_o.c
> index 34ae541934..51577064b8 100644
> --- a/libavformat/asfdec_o.c
> +++ b/libavformat/asfdec_o.c
> @@ -360,7 +360,7 @@ static int asf_set_metadata(AVFormatContext *s, const uint8_t *name,
>   * but in reality this is only loosely similar */
>  static int asf_read_picture(AVFormatContext *s, int len)
>  {
> -    AVPacket pkt          = { 0 };
> +    AVPacket *pkt = s->internal->parse_pkt;
>      const CodecMime *mime = ff_id3v2_mime_tags;
>      enum  AVCodecID id    = AV_CODEC_ID_NONE;
>      char mimetype[64];
> @@ -414,7 +414,7 @@ static int asf_read_picture(AVFormatContext *s, int len)
>          return AVERROR(ENOMEM);
>      len -= avio_get_str16le(s->pb, len - picsize, desc, desc_len);
>  
> -    ret = av_get_packet(s->pb, &pkt, picsize);
> +    ret = av_get_packet(s->pb, pkt, picsize);
>      if (ret < 0)
>          goto fail;
>  
> @@ -424,12 +424,21 @@ static int asf_read_picture(AVFormatContext *s, int len)
>          goto fail;
>      }
>  
> +    av_packet_move_ref(st->apic, pkt);
>      st->disposition              |= AV_DISPOSITION_ATTACHED_PIC;
>      st->codecpar->codec_type      = AVMEDIA_TYPE_VIDEO;
>      st->codecpar->codec_id        = id;
> -    st->attached_pic              = pkt;
> -    st->attached_pic.stream_index = st->index;
> -    st->attached_pic.flags       |= AV_PKT_FLAG_KEY;
> +    st->apic->stream_index        = st->index;
> +    st->apic->flags              |= AV_PKT_FLAG_KEY;
> +#if FF_API_ATTACHED_PIC
> +FF_DISABLE_DEPRECATION_WARNINGS
> +    ret = av_packet_ref(&st->attached_pic, st->apic);
> +    if (ret < 0) {
> +        av_packet_unref(st->apic);
> +        goto fail;
> +    }
> +FF_ENABLE_DEPRECATION_WARNINGS
> +#endif
>  
>      if (*desc) {
>          if (av_dict_set(&st->metadata, "title", desc, AV_DICT_DONT_STRDUP_VAL) < 0)
> @@ -444,7 +453,7 @@ static int asf_read_picture(AVFormatContext *s, int len)
>  
>  fail:
>      av_freep(&desc);
> -    av_packet_unref(&pkt);
> +    av_packet_unref(pkt);
>      return ret;
>  }
>  
> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
> index 765bc3b6f5..769d27d84a 100644
> --- a/libavformat/avformat.h
> +++ b/libavformat/avformat.h
> @@ -947,14 +947,19 @@ typedef struct AVStream {
>       */
>      AVRational avg_frame_rate;
>  
> +#if FF_API_ATTACHED_PIC
>      /**
>       * For streams with AV_DISPOSITION_ATTACHED_PIC disposition, this packet
>       * will contain the attached picture.
>       *
>       * decoding: set by libavformat, must not be modified by the caller.
>       * encoding: unused
> +     *
> +     * @deprecated Use apic instead.
>       */
> +    attribute_deprecated
>      AVPacket attached_pic;
> +#endif
>  
>      /**
>       * An array of side data that applies to the whole stream (i.e. the
> @@ -1039,6 +1044,17 @@ typedef struct AVStream {
>       */
>      AVCodecParameters *codecpar;
>  
> +    /**
> +     * For streams with AV_DISPOSITION_ATTACHED_PIC disposition, this packet
> +     * will contain the attached picture. It is allocated and freed by
> +     * libavformat in avformat_new_stream() and avformat_free_context()
> +     * respectively.
> +     *
> +     * - demuxing: set by libavformat, must not be modified by the caller.
> +     * - muxing: unused

Does this actually allow using apic internally? (I intend to use this
for muxers to store their attached pics; and eventually users should
also be allowed to set this before avformat_init_output() (this can
reduce delay and might be easier for users that already have the
attached pics). Here:
https://github.com/mkver/FFmpeg/commits/matroska_muxer is a branch (not
based upon master; probably doesn't apply to master at all) that
contains this.)

> +     */
> +    AVPacket *apic;
> +
>      /*****************************************************************
>       * All fields below this line are not part of the public API. They
>       * may not be used outside of libavformat and can be changed and
> @@ -1049,11 +1065,6 @@ typedef struct AVStream {
>       *****************************************************************
>       */
>  
> -#if LIBAVFORMAT_VERSION_MAJOR < 59
> -    // kept for ABI compatibility only, do not access in any way
> -    void *unused;
> -#endif
> -
>      int pts_wrap_bits; /**< number of bits in pts (used for wrapping control) */
>  
>      // Timestamp generation support:
> diff --git a/libavformat/flac_picture.c b/libavformat/flac_picture.c
> index f15cfa877a..f771df3fc2 100644
> --- a/libavformat/flac_picture.c
> +++ b/libavformat/flac_picture.c
> @@ -165,12 +165,20 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size, int tr
>          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;
> +    av_packet_unref(st->apic);
> +    st->apic->buf          = data;
> +    st->apic->data         = data->data;
> +    st->apic->size         = len;
> +    st->apic->stream_index = st->index;
> +    st->apic->flags       |= AV_PKT_FLAG_KEY;
> +    data = NULL;
> +#if FF_API_ATTACHED_PIC
> +FF_DISABLE_DEPRECATION_WARNINGS
> +    ret = av_packet_ref(&st->attached_pic, st->apic);
> +    if (ret < 0)
> +        goto fail;
> +FF_ENABLE_DEPRECATION_WARNINGS
> +#endif
>  
>      st->disposition      |= AV_DISPOSITION_ATTACHED_PIC;
>      st->codecpar->codec_type = AVMEDIA_TYPE_VIDEO;
> @@ -185,6 +193,7 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size, int tr
>  
>  fail:
>      av_buffer_unref(&data);
> +    av_packet_unref(st->apic);
>      av_freep(&desc);
>  
>      return ret;
> diff --git a/libavformat/hls.c b/libavformat/hls.c
> index 597bea7f25..2b18385496 100644
> --- a/libavformat/hls.c
> +++ b/libavformat/hls.c
> @@ -1068,15 +1068,15 @@ static int id3_has_changed_values(struct playlist *pls, AVDictionary *metadata,
>      }
>  
>      /* check if apic appeared */
> -    if (apic && (pls->ctx->nb_streams != 2 || !pls->ctx->streams[1]->attached_pic.data))
> +    if (apic && (pls->ctx->nb_streams != 2 || !pls->ctx->streams[1]->apic->data))
>          return 1;
>  
>      if (apic) {
> -        int size = pls->ctx->streams[1]->attached_pic.size;
> +        int size = pls->ctx->streams[1]->apic->size;
>          if (size != apic->buf->size - AV_INPUT_BUFFER_PADDING_SIZE)
>              return 1;
>  
> -        if (memcmp(apic->buf->data, pls->ctx->streams[1]->attached_pic.data, size) != 0)
> +        if (memcmp(apic->buf->data, pls->ctx->streams[1]->apic->data, size) != 0)
>              return 1;
>      }
>  
> diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c
> index f33b7ba93a..f8c10ab90c 100644
> --- a/libavformat/id3v2.c
> +++ b/libavformat/id3v2.c
> @@ -1142,6 +1142,7 @@ 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;
> @@ -1162,14 +1163,24 @@ int ff_id3v2_parse_apic(AVFormatContext *s, ID3v2ExtraMeta *extra_meta)
>  
>          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;
> +        av_packet_unref(st->apic);
> +        st->apic->buf          = apic->buf;
> +        st->apic->data         = apic->buf->data;
> +        st->apic->size         = apic->buf->size - AV_INPUT_BUFFER_PADDING_SIZE;
> +        st->apic->stream_index = st->index;
> +        st->apic->flags       |= AV_PKT_FLAG_KEY;
>  
>          apic->buf = NULL;
> +
> +#if FF_API_ATTACHED_PIC
> +FF_DISABLE_DEPRECATION_WARNINGS
> +        ret = av_packet_ref(&st->attached_pic, st->apic);
> +        if (ret < 0) {
> +            av_packet_unref(st->apic);
> +            return ret;
> +        }
> +FF_ENABLE_DEPRECATION_WARNINGS
> +#endif
>      }
>  
>      return 0;
> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> index 1dc188c946..92e3b8f183 100644
> --- a/libavformat/matroskadec.c
> +++ b/libavformat/matroskadec.c
> @@ -3007,7 +3007,7 @@ 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;
> +                AVPacket *pkt = st->apic;
>  
>                  st->disposition         |= AV_DISPOSITION_ATTACHED_PIC;
>                  st->codecpar->codec_type = AVMEDIA_TYPE_VIDEO;
> @@ -3019,6 +3019,13 @@ static int matroska_read_header(AVFormatContext *s)
>                  pkt->size         = attachments[j].bin.size;
>                  pkt->stream_index = st->index;
>                  pkt->flags       |= AV_PKT_FLAG_KEY;
> +#if FF_API_ATTACHED_PIC
> +FF_DISABLE_DEPRECATION_WARNINGS
> +                res = av_packet_ref(&st->attached_pic, pkt);
> +                if (res < 0)
> +                    goto fail;
> +FF_ENABLE_DEPRECATION_WARNINGS
> +#endif
>              } 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..d99f922709 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -204,12 +204,21 @@ static int mov_read_covr(MOVContext *c, AVIOContext *pb, int type, int len)
>          return AVERROR(ENOMEM);
>      st->priv_data = sc;
>  
> -    ret = av_get_packet(pb, &st->attached_pic, len);
> +    ret = av_get_packet(pb, st->apic, len);
>      if (ret < 0)
>          return ret;
> +#if FF_API_ATTACHED_PIC
> +FF_DISABLE_DEPRECATION_WARNINGS
> +    ret = av_packet_ref(&st->attached_pic, st->apic);
> +    if (ret < 0) {
> +        av_packet_unref(st->apic);
> +        return ret;
> +    }
> +FF_ENABLE_DEPRECATION_WARNINGS
> +#endif
>  
> -    if (st->attached_pic.size >= 8 && id != AV_CODEC_ID_BMP) {
> -        if (AV_RB64(st->attached_pic.data) == 0x89504e470d0a1a0a) {
> +    if (st->apic->size >= 8 && id != AV_CODEC_ID_BMP) {
> +        if (AV_RB64(st->apic->data) == 0x89504e470d0a1a0a) {
>              id = AV_CODEC_ID_PNG;
>          } else {
>              id = AV_CODEC_ID_MJPEG;
> @@ -218,8 +227,8 @@ static int mov_read_covr(MOVContext *c, AVIOContext *pb, int type, int len)
>  
>      st->disposition              |= AV_DISPOSITION_ATTACHED_PIC;
>  
> -    st->attached_pic.stream_index = st->index;
> -    st->attached_pic.flags       |= AV_PKT_FLAG_KEY;
> +    st->apic->stream_index        = st->index;
> +    st->apic->flags              |= AV_PKT_FLAG_KEY;
>  
>      st->codecpar->codec_type = AVMEDIA_TYPE_VIDEO;
>      st->codecpar->codec_id   = id;
> @@ -7229,11 +7238,17 @@ static void mov_read_chapters(AVFormatContext *s)
>                      goto finish;
>                  }
>  
> -                if (av_get_packet(sc->pb, &st->attached_pic, sample->size) < 0)
> +                if (av_get_packet(sc->pb, st->apic, sample->size) < 0)
>                      goto finish;
>  
> -                st->attached_pic.stream_index = st->index;
> -                st->attached_pic.flags       |= AV_PKT_FLAG_KEY;
> +                st->apic->stream_index = st->index;
> +                st->apic->flags       |= AV_PKT_FLAG_KEY;
> +#if FF_API_ATTACHED_PIC
> +FF_DISABLE_DEPRECATION_WARNINGS
> +                if (av_packet_ref(&st->attached_pic, st->apic) < 0)
> +                    goto finish;
> +FF_ENABLE_DEPRECATION_WARNINGS
> +#endif
>              }
>          } else {
>              st->codecpar->codec_type = AVMEDIA_TYPE_DATA;
> diff --git a/libavformat/utils.c b/libavformat/utils.c
> index 524765aeb4..9308b53106 100644
> --- a/libavformat/utils.c
> +++ b/libavformat/utils.c
> @@ -457,7 +457,7 @@ int avformat_queue_attached_pictures(AVFormatContext *s)
>      for (i = 0; i < s->nb_streams; i++)
>          if (s->streams[i]->disposition & AV_DISPOSITION_ATTACHED_PIC &&
>              s->streams[i]->discard < AVDISCARD_ALL) {
> -            if (s->streams[i]->attached_pic.size <= 0) {
> +            if (s->streams[i]->apic->size <= 0) {
>                  av_log(s, AV_LOG_WARNING,
>                      "Attached picture on stream %d has invalid size, "
>                      "ignoring\n", i);
> @@ -466,7 +466,7 @@ int avformat_queue_attached_pictures(AVFormatContext *s)
>  
>              ret = avpriv_packet_list_put(&s->internal->raw_packet_buffer,
>                                       &s->internal->raw_packet_buffer_end,
> -                                     &s->streams[i]->attached_pic,
> +                                     s->streams[i]->apic,
>                                       av_packet_ref, 0);
>              if (ret < 0)
>                  return ret;
> @@ -4371,8 +4371,14 @@ static void free_stream(AVStream **pst)
>      if (st->parser)
>          av_parser_close(st->parser);
>  
> +#if FF_API_ATTACHED_PIC
> +FF_DISABLE_DEPRECATION_WARNINGS
>      if (st->attached_pic.data)
>          av_packet_unref(&st->attached_pic);
> +FF_ENABLE_DEPRECATION_WARNINGS
> +#endif
> +
> +    av_packet_free(&st->apic);
>  
>      if (st->internal) {
>          avcodec_free_context(&st->internal->avctx);
> @@ -4530,6 +4536,10 @@ FF_ENABLE_DEPRECATION_WARNINGS
>      if (!st->codecpar)
>          goto fail;
>  
> +    st->apic = av_packet_alloc();
> +    if (!st->apic)
> +        goto fail;
> +

Does this have to be always allocated? It feels wrong to have this set
for streams that don't have the ATTACHED_PIC disposition.

>      st->internal->avctx = avcodec_alloc_context3(NULL);
>      if (!st->internal->avctx)
>          goto fail;
> diff --git a/libavformat/version.h b/libavformat/version.h
> index ced5600034..dce936794a 100644
> --- a/libavformat/version.h
> +++ b/libavformat/version.h
> @@ -115,6 +115,9 @@
>  #ifndef FF_API_LAVF_PRIV_OPT
>  #define FF_API_LAVF_PRIV_OPT            (LIBAVFORMAT_VERSION_MAJOR < 60)
>  #endif
> +#ifndef FF_API_ATTACHED_PIC
> +#define FF_API_ATTACHED_PIC             (LIBAVFORMAT_VERSION_MAJOR < 60)
> +#endif
>  
>  
>  #ifndef FF_API_R_FRAME_RATE
> diff --git a/libavformat/wtvdec.c b/libavformat/wtvdec.c
> index 7def9d2348..05386add76 100644
> --- a/libavformat/wtvdec.c
> +++ b/libavformat/wtvdec.c
> @@ -454,11 +454,18 @@ static void get_attachment(AVFormatContext *s, AVIOContext *pb, int length)
>      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);
> +    ret = av_get_packet(pb, st->apic, filesize);
>      if (ret < 0)
>          goto done;
> -    st->attached_pic.stream_index = st->index;
> -    st->attached_pic.flags       |= AV_PKT_FLAG_KEY;
> +    st->apic->stream_index = st->index;
> +    st->apic->flags  |= AV_PKT_FLAG_KEY;
> +#if FF_API_ATTACHED_PIC
> +FF_DISABLE_DEPRECATION_WARNINGS
> +    ret = av_packet_ref(&st->attached_pic, st->apic);
> +    if (ret < 0)
> +        goto done;
> +FF_ENABLE_DEPRECATION_WARNINGS
> +#endif
>      st->disposition              |= AV_DISPOSITION_ATTACHED_PIC;
>  done:
>      avio_seek(pb, pos + length, SEEK_SET);
>
James Almer March 29, 2021, 12:42 p.m. UTC | #6
On 3/29/2021 4:38 AM, Nicolas George wrote:
> Lynne (12021-03-29):
>> Personally I don't really like 'apic'. Maybe 'pic_attachment' or 'attachment_pic'?
> 
> By the way, do we want it to be a field?
> 
> Attached pictures can be quite big. Do we want them to stay in AVStream
> forever, even when the application has already finished using them, or
> does not use them at all?
> 
> Regards,

The idea for attached_pic afaik was that it's always available and 
easily accessible to the user from the start, instead of just the one 
time the demuxing process would return it in an av_read_frame() call.
The doxy for AV_DISPOSITION_ATTACHED_PIC even mentions that if you seek 
the relevant packet may not be returned at all.
James Almer March 29, 2021, 12:50 p.m. UTC | #7
On 3/29/2021 6:46 AM, Andreas Rheinhardt wrote:
> James Almer:
>> As a replacement for attached_pic, which is in turn deprecated and scheduled
>> for removal.
>>
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>> TODO: APIChanges entry and version bump.
>>
>> Decided to use the name apic for the field, since it's how id3v2 and other
>> formats call it.
>>
>> Also, in a fortunate coincidence, the first "non public" field in AVStream is a
>> unused void pointer that had to be left in place when the field was moved to
>> AVStreamInternal to keep every offset intact for the sake of ffmpeg accessing
>> fields after it, so I'm going to take advantage of it so this doesn't have to
>> wait until the major bump.
>>
>>   libavformat/apetag.c       | 15 ++++++++++++---
>>   libavformat/asfdec_f.c     | 21 +++++++++++++++------
>>   libavformat/asfdec_o.c     | 21 +++++++++++++++------
>>   libavformat/avformat.h     | 21 ++++++++++++++++-----
>>   libavformat/flac_picture.c | 21 +++++++++++++++------
>>   libavformat/hls.c          |  6 +++---
>>   libavformat/id3v2.c        | 23 +++++++++++++++++------
>>   libavformat/matroskadec.c  |  9 ++++++++-
>>   libavformat/mov.c          | 31 +++++++++++++++++++++++--------
>>   libavformat/utils.c        | 14 ++++++++++++--
>>   libavformat/version.h      |  3 +++
>>   libavformat/wtvdec.c       | 13 ++++++++++---
>>   12 files changed, 149 insertions(+), 49 deletions(-)
>>
>> diff --git a/libavformat/apetag.c b/libavformat/apetag.c
>> index 23ee6b516d..e0e96c3f4c 100644
>> --- a/libavformat/apetag.c
>> +++ b/libavformat/apetag.c
>> @@ -81,7 +81,7 @@ static int ape_tag_read_field(AVFormatContext *s)
>>           if ((id = ff_guess_image2_codec(filename)) != AV_CODEC_ID_NONE) {
>>               int ret;
>>   
>> -            ret = av_get_packet(s->pb, &st->attached_pic, size);
>> +            ret = av_get_packet(s->pb, st->apic, size);
>>               if (ret < 0) {
>>                   av_log(s, AV_LOG_ERROR, "Error reading cover art.\n");
>>                   return ret;
>> @@ -91,8 +91,17 @@ static int ape_tag_read_field(AVFormatContext *s)
>>               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;
>> +            st->apic->stream_index   = st->index;
>> +            st->apic->flags         |= AV_PKT_FLAG_KEY;
>> +#if FF_API_ATTACHED_PIC
>> +FF_DISABLE_DEPRECATION_WARNINGS
>> +            ret = av_packet_ref(&st->attached_pic, st->apic);
>> +            if (ret < 0) {
>> +                av_packet_unref(st->apic);
>> +                return ret;
>> +            }
>> +FF_ENABLE_DEPRECATION_WARNINGS
>> +#endif
>>           } else {
>>               if ((ret = ff_get_extradata(s, st->codecpar, s->pb, size)) < 0)
>>                   return ret;
>> diff --git a/libavformat/asfdec_f.c b/libavformat/asfdec_f.c
>> index 2fae528f4d..838fc924d5 100644
>> --- a/libavformat/asfdec_f.c
>> +++ b/libavformat/asfdec_f.c
>> @@ -223,7 +223,7 @@ static int get_value(AVIOContext *pb, int type, int type2_size)
>>    * but in reality this is only loosely similar */
>>   static int asf_read_picture(AVFormatContext *s, int len)
>>   {
>> -    AVPacket pkt          = { 0 };
>> +    AVPacket *pkt = s->internal->parse_pkt;
>>       const CodecMime *mime = ff_id3v2_mime_tags;
>>       enum  AVCodecID id    = AV_CODEC_ID_NONE;
>>       char mimetype[64];
>> @@ -277,7 +277,7 @@ static int asf_read_picture(AVFormatContext *s, int len)
>>           return AVERROR(ENOMEM);
>>       len -= avio_get_str16le(s->pb, len - picsize, desc, desc_len);
>>   
>> -    ret = av_get_packet(s->pb, &pkt, picsize);
>> +    ret = av_get_packet(s->pb, pkt, picsize);
>>       if (ret < 0)
>>           goto fail;
>>   
>> @@ -286,12 +286,21 @@ static int asf_read_picture(AVFormatContext *s, int len)
>>           ret = AVERROR(ENOMEM);
>>           goto fail;
>>       }
>> +    av_packet_move_ref(st->apic, pkt);
>>       st->disposition              |= AV_DISPOSITION_ATTACHED_PIC;
>>       st->codecpar->codec_type      = AVMEDIA_TYPE_VIDEO;
>>       st->codecpar->codec_id        = id;
>> -    st->attached_pic              = pkt;
>> -    st->attached_pic.stream_index = st->index;
>> -    st->attached_pic.flags       |= AV_PKT_FLAG_KEY;
>> +    st->apic->stream_index        = st->index;
>> +    st->apic->flags              |= AV_PKT_FLAG_KEY;
>> +#if FF_API_ATTACHED_PIC
>> +FF_DISABLE_DEPRECATION_WARNINGS
>> +    ret = av_packet_ref(&st->attached_pic, st->apic);
>> +    if (ret < 0) {
>> +        av_packet_unref(st->apic);
>> +        goto fail;
>> +    }
>> +FF_ENABLE_DEPRECATION_WARNINGS
>> +#endif
>>   
>>       if (*desc)
>>           av_dict_set(&st->metadata, "title", desc, AV_DICT_DONT_STRDUP_VAL);
>> @@ -304,7 +313,7 @@ static int asf_read_picture(AVFormatContext *s, int len)
>>   
>>   fail:
>>       av_freep(&desc);
>> -    av_packet_unref(&pkt);
>> +    av_packet_unref(pkt);
>>       return ret;
>>   }
>>   
>> diff --git a/libavformat/asfdec_o.c b/libavformat/asfdec_o.c
>> index 34ae541934..51577064b8 100644
>> --- a/libavformat/asfdec_o.c
>> +++ b/libavformat/asfdec_o.c
>> @@ -360,7 +360,7 @@ static int asf_set_metadata(AVFormatContext *s, const uint8_t *name,
>>    * but in reality this is only loosely similar */
>>   static int asf_read_picture(AVFormatContext *s, int len)
>>   {
>> -    AVPacket pkt          = { 0 };
>> +    AVPacket *pkt = s->internal->parse_pkt;
>>       const CodecMime *mime = ff_id3v2_mime_tags;
>>       enum  AVCodecID id    = AV_CODEC_ID_NONE;
>>       char mimetype[64];
>> @@ -414,7 +414,7 @@ static int asf_read_picture(AVFormatContext *s, int len)
>>           return AVERROR(ENOMEM);
>>       len -= avio_get_str16le(s->pb, len - picsize, desc, desc_len);
>>   
>> -    ret = av_get_packet(s->pb, &pkt, picsize);
>> +    ret = av_get_packet(s->pb, pkt, picsize);
>>       if (ret < 0)
>>           goto fail;
>>   
>> @@ -424,12 +424,21 @@ static int asf_read_picture(AVFormatContext *s, int len)
>>           goto fail;
>>       }
>>   
>> +    av_packet_move_ref(st->apic, pkt);
>>       st->disposition              |= AV_DISPOSITION_ATTACHED_PIC;
>>       st->codecpar->codec_type      = AVMEDIA_TYPE_VIDEO;
>>       st->codecpar->codec_id        = id;
>> -    st->attached_pic              = pkt;
>> -    st->attached_pic.stream_index = st->index;
>> -    st->attached_pic.flags       |= AV_PKT_FLAG_KEY;
>> +    st->apic->stream_index        = st->index;
>> +    st->apic->flags              |= AV_PKT_FLAG_KEY;
>> +#if FF_API_ATTACHED_PIC
>> +FF_DISABLE_DEPRECATION_WARNINGS
>> +    ret = av_packet_ref(&st->attached_pic, st->apic);
>> +    if (ret < 0) {
>> +        av_packet_unref(st->apic);
>> +        goto fail;
>> +    }
>> +FF_ENABLE_DEPRECATION_WARNINGS
>> +#endif
>>   
>>       if (*desc) {
>>           if (av_dict_set(&st->metadata, "title", desc, AV_DICT_DONT_STRDUP_VAL) < 0)
>> @@ -444,7 +453,7 @@ static int asf_read_picture(AVFormatContext *s, int len)
>>   
>>   fail:
>>       av_freep(&desc);
>> -    av_packet_unref(&pkt);
>> +    av_packet_unref(pkt);
>>       return ret;
>>   }
>>   
>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
>> index 765bc3b6f5..769d27d84a 100644
>> --- a/libavformat/avformat.h
>> +++ b/libavformat/avformat.h
>> @@ -947,14 +947,19 @@ typedef struct AVStream {
>>        */
>>       AVRational avg_frame_rate;
>>   
>> +#if FF_API_ATTACHED_PIC
>>       /**
>>        * For streams with AV_DISPOSITION_ATTACHED_PIC disposition, this packet
>>        * will contain the attached picture.
>>        *
>>        * decoding: set by libavformat, must not be modified by the caller.
>>        * encoding: unused
>> +     *
>> +     * @deprecated Use apic instead.
>>        */
>> +    attribute_deprecated
>>       AVPacket attached_pic;
>> +#endif
>>   
>>       /**
>>        * An array of side data that applies to the whole stream (i.e. the
>> @@ -1039,6 +1044,17 @@ typedef struct AVStream {
>>        */
>>       AVCodecParameters *codecpar;
>>   
>> +    /**
>> +     * For streams with AV_DISPOSITION_ATTACHED_PIC disposition, this packet
>> +     * will contain the attached picture. It is allocated and freed by
>> +     * libavformat in avformat_new_stream() and avformat_free_context()
>> +     * respectively.
>> +     *
>> +     * - demuxing: set by libavformat, must not be modified by the caller.
>> +     * - muxing: unused
> 
> Does this actually allow using apic internally? (I intend to use this
> for muxers to store their attached pics; and eventually users should
> also be allowed to set this before avformat_init_output() (this can
> reduce delay and might be easier for users that already have the
> attached pics). Here:
> https://github.com/mkver/FFmpeg/commits/matroska_muxer is a branch (not
> based upon master; probably doesn't apply to master at all) that
> contains this.)

By saying "unused" in the context of muxing the user shouldn't really 
care or look what's on it and assume it's effectively empty, but 
ideally, ffmpeg would not use it internally either in that case when it 
can use an st->internal AVPacket that's not exposed instead.

That being said, your suggestion to let the user set them on muxers 
sounds good.

> 
>> +     */
>> +    AVPacket *apic;
>> +
>>       /*****************************************************************
>>        * All fields below this line are not part of the public API. They
>>        * may not be used outside of libavformat and can be changed and
>> @@ -1049,11 +1065,6 @@ typedef struct AVStream {
>>        *****************************************************************
>>        */
>>   
>> -#if LIBAVFORMAT_VERSION_MAJOR < 59
>> -    // kept for ABI compatibility only, do not access in any way
>> -    void *unused;
>> -#endif
>> -
>>       int pts_wrap_bits; /**< number of bits in pts (used for wrapping control) */
>>   
>>       // Timestamp generation support:
>> diff --git a/libavformat/flac_picture.c b/libavformat/flac_picture.c
>> index f15cfa877a..f771df3fc2 100644
>> --- a/libavformat/flac_picture.c
>> +++ b/libavformat/flac_picture.c
>> @@ -165,12 +165,20 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size, int tr
>>           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;
>> +    av_packet_unref(st->apic);
>> +    st->apic->buf          = data;
>> +    st->apic->data         = data->data;
>> +    st->apic->size         = len;
>> +    st->apic->stream_index = st->index;
>> +    st->apic->flags       |= AV_PKT_FLAG_KEY;
>> +    data = NULL;
>> +#if FF_API_ATTACHED_PIC
>> +FF_DISABLE_DEPRECATION_WARNINGS
>> +    ret = av_packet_ref(&st->attached_pic, st->apic);
>> +    if (ret < 0)
>> +        goto fail;
>> +FF_ENABLE_DEPRECATION_WARNINGS
>> +#endif
>>   
>>       st->disposition      |= AV_DISPOSITION_ATTACHED_PIC;
>>       st->codecpar->codec_type = AVMEDIA_TYPE_VIDEO;
>> @@ -185,6 +193,7 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size, int tr
>>   
>>   fail:
>>       av_buffer_unref(&data);
>> +    av_packet_unref(st->apic);
>>       av_freep(&desc);
>>   
>>       return ret;
>> diff --git a/libavformat/hls.c b/libavformat/hls.c
>> index 597bea7f25..2b18385496 100644
>> --- a/libavformat/hls.c
>> +++ b/libavformat/hls.c
>> @@ -1068,15 +1068,15 @@ static int id3_has_changed_values(struct playlist *pls, AVDictionary *metadata,
>>       }
>>   
>>       /* check if apic appeared */
>> -    if (apic && (pls->ctx->nb_streams != 2 || !pls->ctx->streams[1]->attached_pic.data))
>> +    if (apic && (pls->ctx->nb_streams != 2 || !pls->ctx->streams[1]->apic->data))
>>           return 1;
>>   
>>       if (apic) {
>> -        int size = pls->ctx->streams[1]->attached_pic.size;
>> +        int size = pls->ctx->streams[1]->apic->size;
>>           if (size != apic->buf->size - AV_INPUT_BUFFER_PADDING_SIZE)
>>               return 1;
>>   
>> -        if (memcmp(apic->buf->data, pls->ctx->streams[1]->attached_pic.data, size) != 0)
>> +        if (memcmp(apic->buf->data, pls->ctx->streams[1]->apic->data, size) != 0)
>>               return 1;
>>       }
>>   
>> diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c
>> index f33b7ba93a..f8c10ab90c 100644
>> --- a/libavformat/id3v2.c
>> +++ b/libavformat/id3v2.c
>> @@ -1142,6 +1142,7 @@ 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;
>> @@ -1162,14 +1163,24 @@ int ff_id3v2_parse_apic(AVFormatContext *s, ID3v2ExtraMeta *extra_meta)
>>   
>>           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;
>> +        av_packet_unref(st->apic);
>> +        st->apic->buf          = apic->buf;
>> +        st->apic->data         = apic->buf->data;
>> +        st->apic->size         = apic->buf->size - AV_INPUT_BUFFER_PADDING_SIZE;
>> +        st->apic->stream_index = st->index;
>> +        st->apic->flags       |= AV_PKT_FLAG_KEY;
>>   
>>           apic->buf = NULL;
>> +
>> +#if FF_API_ATTACHED_PIC
>> +FF_DISABLE_DEPRECATION_WARNINGS
>> +        ret = av_packet_ref(&st->attached_pic, st->apic);
>> +        if (ret < 0) {
>> +            av_packet_unref(st->apic);
>> +            return ret;
>> +        }
>> +FF_ENABLE_DEPRECATION_WARNINGS
>> +#endif
>>       }
>>   
>>       return 0;
>> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
>> index 1dc188c946..92e3b8f183 100644
>> --- a/libavformat/matroskadec.c
>> +++ b/libavformat/matroskadec.c
>> @@ -3007,7 +3007,7 @@ 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;
>> +                AVPacket *pkt = st->apic;
>>   
>>                   st->disposition         |= AV_DISPOSITION_ATTACHED_PIC;
>>                   st->codecpar->codec_type = AVMEDIA_TYPE_VIDEO;
>> @@ -3019,6 +3019,13 @@ static int matroska_read_header(AVFormatContext *s)
>>                   pkt->size         = attachments[j].bin.size;
>>                   pkt->stream_index = st->index;
>>                   pkt->flags       |= AV_PKT_FLAG_KEY;
>> +#if FF_API_ATTACHED_PIC
>> +FF_DISABLE_DEPRECATION_WARNINGS
>> +                res = av_packet_ref(&st->attached_pic, pkt);
>> +                if (res < 0)
>> +                    goto fail;
>> +FF_ENABLE_DEPRECATION_WARNINGS
>> +#endif
>>               } 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..d99f922709 100644
>> --- a/libavformat/mov.c
>> +++ b/libavformat/mov.c
>> @@ -204,12 +204,21 @@ static int mov_read_covr(MOVContext *c, AVIOContext *pb, int type, int len)
>>           return AVERROR(ENOMEM);
>>       st->priv_data = sc;
>>   
>> -    ret = av_get_packet(pb, &st->attached_pic, len);
>> +    ret = av_get_packet(pb, st->apic, len);
>>       if (ret < 0)
>>           return ret;
>> +#if FF_API_ATTACHED_PIC
>> +FF_DISABLE_DEPRECATION_WARNINGS
>> +    ret = av_packet_ref(&st->attached_pic, st->apic);
>> +    if (ret < 0) {
>> +        av_packet_unref(st->apic);
>> +        return ret;
>> +    }
>> +FF_ENABLE_DEPRECATION_WARNINGS
>> +#endif
>>   
>> -    if (st->attached_pic.size >= 8 && id != AV_CODEC_ID_BMP) {
>> -        if (AV_RB64(st->attached_pic.data) == 0x89504e470d0a1a0a) {
>> +    if (st->apic->size >= 8 && id != AV_CODEC_ID_BMP) {
>> +        if (AV_RB64(st->apic->data) == 0x89504e470d0a1a0a) {
>>               id = AV_CODEC_ID_PNG;
>>           } else {
>>               id = AV_CODEC_ID_MJPEG;
>> @@ -218,8 +227,8 @@ static int mov_read_covr(MOVContext *c, AVIOContext *pb, int type, int len)
>>   
>>       st->disposition              |= AV_DISPOSITION_ATTACHED_PIC;
>>   
>> -    st->attached_pic.stream_index = st->index;
>> -    st->attached_pic.flags       |= AV_PKT_FLAG_KEY;
>> +    st->apic->stream_index        = st->index;
>> +    st->apic->flags              |= AV_PKT_FLAG_KEY;
>>   
>>       st->codecpar->codec_type = AVMEDIA_TYPE_VIDEO;
>>       st->codecpar->codec_id   = id;
>> @@ -7229,11 +7238,17 @@ static void mov_read_chapters(AVFormatContext *s)
>>                       goto finish;
>>                   }
>>   
>> -                if (av_get_packet(sc->pb, &st->attached_pic, sample->size) < 0)
>> +                if (av_get_packet(sc->pb, st->apic, sample->size) < 0)
>>                       goto finish;
>>   
>> -                st->attached_pic.stream_index = st->index;
>> -                st->attached_pic.flags       |= AV_PKT_FLAG_KEY;
>> +                st->apic->stream_index = st->index;
>> +                st->apic->flags       |= AV_PKT_FLAG_KEY;
>> +#if FF_API_ATTACHED_PIC
>> +FF_DISABLE_DEPRECATION_WARNINGS
>> +                if (av_packet_ref(&st->attached_pic, st->apic) < 0)
>> +                    goto finish;
>> +FF_ENABLE_DEPRECATION_WARNINGS
>> +#endif
>>               }
>>           } else {
>>               st->codecpar->codec_type = AVMEDIA_TYPE_DATA;
>> diff --git a/libavformat/utils.c b/libavformat/utils.c
>> index 524765aeb4..9308b53106 100644
>> --- a/libavformat/utils.c
>> +++ b/libavformat/utils.c
>> @@ -457,7 +457,7 @@ int avformat_queue_attached_pictures(AVFormatContext *s)
>>       for (i = 0; i < s->nb_streams; i++)
>>           if (s->streams[i]->disposition & AV_DISPOSITION_ATTACHED_PIC &&
>>               s->streams[i]->discard < AVDISCARD_ALL) {
>> -            if (s->streams[i]->attached_pic.size <= 0) {
>> +            if (s->streams[i]->apic->size <= 0) {
>>                   av_log(s, AV_LOG_WARNING,
>>                       "Attached picture on stream %d has invalid size, "
>>                       "ignoring\n", i);
>> @@ -466,7 +466,7 @@ int avformat_queue_attached_pictures(AVFormatContext *s)
>>   
>>               ret = avpriv_packet_list_put(&s->internal->raw_packet_buffer,
>>                                        &s->internal->raw_packet_buffer_end,
>> -                                     &s->streams[i]->attached_pic,
>> +                                     s->streams[i]->apic,
>>                                        av_packet_ref, 0);
>>               if (ret < 0)
>>                   return ret;
>> @@ -4371,8 +4371,14 @@ static void free_stream(AVStream **pst)
>>       if (st->parser)
>>           av_parser_close(st->parser);
>>   
>> +#if FF_API_ATTACHED_PIC
>> +FF_DISABLE_DEPRECATION_WARNINGS
>>       if (st->attached_pic.data)
>>           av_packet_unref(&st->attached_pic);
>> +FF_ENABLE_DEPRECATION_WARNINGS
>> +#endif
>> +
>> +    av_packet_free(&st->apic);
>>   
>>       if (st->internal) {
>>           avcodec_free_context(&st->internal->avctx);
>> @@ -4530,6 +4536,10 @@ FF_ENABLE_DEPRECATION_WARNINGS
>>       if (!st->codecpar)
>>           goto fail;
>>   
>> +    st->apic = av_packet_alloc();
>> +    if (!st->apic)
>> +        goto fail;
>> +
> 
> Does this have to be always allocated? It feels wrong to have this set
> for streams that don't have the ATTACHED_PIC disposition.

The demuxer sets the disposition after having allocated the stream, so 
that means they will also have to allocate this packet manually.
I guess that with your ff_add_attached_pic() refactor it will make no 
difference, so i can wait for you to push that set and rebase this patch 
on top of it with this change included.

> 
>>       st->internal->avctx = avcodec_alloc_context3(NULL);
>>       if (!st->internal->avctx)
>>           goto fail;
>> diff --git a/libavformat/version.h b/libavformat/version.h
>> index ced5600034..dce936794a 100644
>> --- a/libavformat/version.h
>> +++ b/libavformat/version.h
>> @@ -115,6 +115,9 @@
>>   #ifndef FF_API_LAVF_PRIV_OPT
>>   #define FF_API_LAVF_PRIV_OPT            (LIBAVFORMAT_VERSION_MAJOR < 60)
>>   #endif
>> +#ifndef FF_API_ATTACHED_PIC
>> +#define FF_API_ATTACHED_PIC             (LIBAVFORMAT_VERSION_MAJOR < 60)
>> +#endif
>>   
>>   
>>   #ifndef FF_API_R_FRAME_RATE
>> diff --git a/libavformat/wtvdec.c b/libavformat/wtvdec.c
>> index 7def9d2348..05386add76 100644
>> --- a/libavformat/wtvdec.c
>> +++ b/libavformat/wtvdec.c
>> @@ -454,11 +454,18 @@ static void get_attachment(AVFormatContext *s, AVIOContext *pb, int length)
>>       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);
>> +    ret = av_get_packet(pb, st->apic, filesize);
>>       if (ret < 0)
>>           goto done;
>> -    st->attached_pic.stream_index = st->index;
>> -    st->attached_pic.flags       |= AV_PKT_FLAG_KEY;
>> +    st->apic->stream_index = st->index;
>> +    st->apic->flags  |= AV_PKT_FLAG_KEY;
>> +#if FF_API_ATTACHED_PIC
>> +FF_DISABLE_DEPRECATION_WARNINGS
>> +    ret = av_packet_ref(&st->attached_pic, st->apic);
>> +    if (ret < 0)
>> +        goto done;
>> +FF_ENABLE_DEPRECATION_WARNINGS
>> +#endif
>>       st->disposition              |= AV_DISPOSITION_ATTACHED_PIC;
>>   done:
>>       avio_seek(pb, pos + length, SEEK_SET);
>>
> 
> _______________________________________________
> 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".
>
Nicolas George March 29, 2021, 12:57 p.m. UTC | #8
James Almer (12021-03-29):
> The idea for attached_pic afaik was that it's always available and easily
> accessible to the user from the start, instead of just the one time the
> demuxing process would return it in an av_read_frame() call.
> The doxy for AV_DISPOSITION_ATTACHED_PIC even mentions that if you seek the
> relevant packet may not be returned at all.

Yes, but was it a GOOD idea? For the convenience of the few applications
that actually use the attached pictures, libavformat will use memory in
all applications. It does not seem like a good compromise to me.

I think a better design would be some kind of:

int avformat_get_attached_pic(AVStream *st, AVPacket **packet, unsigned flags);

In the short run, it could just access the field, but in the long run it
could seek back to load the packet only when required.

Since applications need to update their API anyway, it is a good time to
decide if we want to change something.

Regards,
James Almer March 29, 2021, 1:17 p.m. UTC | #9
On 3/29/2021 9:57 AM, Nicolas George wrote:
> James Almer (12021-03-29):
>> The idea for attached_pic afaik was that it's always available and easily
>> accessible to the user from the start, instead of just the one time the
>> demuxing process would return it in an av_read_frame() call.
>> The doxy for AV_DISPOSITION_ATTACHED_PIC even mentions that if you seek the
>> relevant packet may not be returned at all.
> 
> Yes, but was it a GOOD idea? For the convenience of the few applications
> that actually use the attached pictures, libavformat will use memory in
> all applications. It does not seem like a good compromise to me.
> 
> I think a better design would be some kind of:
> 
> int avformat_get_attached_pic(AVStream *st, AVPacket **packet, unsigned flags);
> 
> In the short run, it could just access the field, but in the long run it
> could seek back to load the packet only when required.

Can this be done? id3v2 attached pics for many formats are handled by 
the generic demux code in avformat_open_input() and not by the actual 
demuxer. And in demuxers like asf, it seems to be done in read_header().
Would seeking be able to fetch these pictures again? The comment in the 
AV_DISPOSITION_ATTACHED_PIC doxy makes me think it's not possible.

Personally, even if possible i think triggering a seek just to fetch an 
attachment is inefficient and disruptive to the user's demuxing/decoding 
process, and a step backwards considering it used to always be 
available, for both seekable and non-seekable input, the latter which 
will no longer be able to access the picture past the original packet.

> 
> Since applications need to update their API anyway, it is a good time to
> decide if we want to change something.

Patches welcome.

> 
> Regards,
> 
> 
> _______________________________________________
> 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".
>
Nicolas George March 29, 2021, 1:20 p.m. UTC | #10
James Almer (12021-03-29):
> Can this be done? id3v2 attached pics for many formats are handled by the
> generic demux code in avformat_open_input() and not by the actual demuxer.
> And in demuxers like asf, it seems to be done in read_header().
> Would seeking be able to fetch these pictures again? The comment in the
> AV_DISPOSITION_ATTACHED_PIC doxy makes me think it's not possible.
> 
> Personally, even if possible i think triggering a seek just to fetch an
> attachment is inefficient and disruptive to the user's demuxing/decoding
> process, and a step backwards considering it used to always be available,
> for both seekable and non-seekable input, the latter which will no longer be
> able to access the picture past the original packet.

As I said, at worse it accesses a field just like now. So of course it
can be done.

The point is that having a function to get the packet rather than just a
pointer gives us more freedom to extend the API later.

Regards,
James Almer March 29, 2021, 1:46 p.m. UTC | #11
On 3/29/2021 10:20 AM, Nicolas George wrote:
> James Almer (12021-03-29):
>> Can this be done? id3v2 attached pics for many formats are handled by the
>> generic demux code in avformat_open_input() and not by the actual demuxer.
>> And in demuxers like asf, it seems to be done in read_header().
>> Would seeking be able to fetch these pictures again? The comment in the
>> AV_DISPOSITION_ATTACHED_PIC doxy makes me think it's not possible.
>>
>> Personally, even if possible i think triggering a seek just to fetch an
>> attachment is inefficient and disruptive to the user's demuxing/decoding
>> process, and a step backwards considering it used to always be available,
>> for both seekable and non-seekable input, the latter which will no longer be
>> able to access the picture past the original packet.
> 
> As I said, at worse it accesses a field just like now. So of course it
> can be done.
> 
> The point is that having a function to get the packet rather than just a
> pointer gives us more freedom to extend the API later.

Not against it in that case, even if it will probably never stop just 
being an abstraction layer to fetch an always available packet, but know 
that this will kill Andreas' idea of defining how the (until now) public 
packet can be used to simplify attached pics in muxing scenarios, or 
make it considerably more complex to the point it's no longer worth it.

> 
> Regards,
> 
> 
> _______________________________________________
> 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:54 p.m. UTC | #12
On 3/29/2021 10:46 AM, James Almer wrote:
> On 3/29/2021 10:20 AM, Nicolas George wrote:
>> James Almer (12021-03-29):
>>> Can this be done? id3v2 attached pics for many formats are handled by 
>>> the
>>> generic demux code in avformat_open_input() and not by the actual 
>>> demuxer.
>>> And in demuxers like asf, it seems to be done in read_header().
>>> Would seeking be able to fetch these pictures again? The comment in the
>>> AV_DISPOSITION_ATTACHED_PIC doxy makes me think it's not possible.
>>>
>>> Personally, even if possible i think triggering a seek just to fetch an
>>> attachment is inefficient and disruptive to the user's demuxing/decoding
>>> process, and a step backwards considering it used to always be 
>>> available,
>>> for both seekable and non-seekable input, the latter which will no 
>>> longer be
>>> able to access the picture past the original packet.
>>
>> As I said, at worse it accesses a field just like now. So of course it
>> can be done.
>>
>> The point is that having a function to get the packet rather than just a
>> pointer gives us more freedom to extend the API later.
> 
> Not against it in that case, even if it will probably never stop just 
> being an abstraction layer to fetch an always available packet, but know 
> that this will kill Andreas' idea of defining how the (until now) public 
> packet can be used to simplify attached pics in muxing scenarios, or 
> make it considerably more complex to the point it's no longer worth it.

Some people on IRC mentioned the possibility of adding a demuxer option 
to disable fetching/storing attached pictures. The streams would not be 
allocated at all in that case. Would that be ok?
diff mbox series

Patch

diff --git a/libavformat/apetag.c b/libavformat/apetag.c
index 23ee6b516d..e0e96c3f4c 100644
--- a/libavformat/apetag.c
+++ b/libavformat/apetag.c
@@ -81,7 +81,7 @@  static int ape_tag_read_field(AVFormatContext *s)
         if ((id = ff_guess_image2_codec(filename)) != AV_CODEC_ID_NONE) {
             int ret;
 
-            ret = av_get_packet(s->pb, &st->attached_pic, size);
+            ret = av_get_packet(s->pb, st->apic, size);
             if (ret < 0) {
                 av_log(s, AV_LOG_ERROR, "Error reading cover art.\n");
                 return ret;
@@ -91,8 +91,17 @@  static int ape_tag_read_field(AVFormatContext *s)
             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;
+            st->apic->stream_index   = st->index;
+            st->apic->flags         |= AV_PKT_FLAG_KEY;
+#if FF_API_ATTACHED_PIC
+FF_DISABLE_DEPRECATION_WARNINGS
+            ret = av_packet_ref(&st->attached_pic, st->apic);
+            if (ret < 0) {
+                av_packet_unref(st->apic);
+                return ret;
+            }
+FF_ENABLE_DEPRECATION_WARNINGS
+#endif
         } else {
             if ((ret = ff_get_extradata(s, st->codecpar, s->pb, size)) < 0)
                 return ret;
diff --git a/libavformat/asfdec_f.c b/libavformat/asfdec_f.c
index 2fae528f4d..838fc924d5 100644
--- a/libavformat/asfdec_f.c
+++ b/libavformat/asfdec_f.c
@@ -223,7 +223,7 @@  static int get_value(AVIOContext *pb, int type, int type2_size)
  * but in reality this is only loosely similar */
 static int asf_read_picture(AVFormatContext *s, int len)
 {
-    AVPacket pkt          = { 0 };
+    AVPacket *pkt = s->internal->parse_pkt;
     const CodecMime *mime = ff_id3v2_mime_tags;
     enum  AVCodecID id    = AV_CODEC_ID_NONE;
     char mimetype[64];
@@ -277,7 +277,7 @@  static int asf_read_picture(AVFormatContext *s, int len)
         return AVERROR(ENOMEM);
     len -= avio_get_str16le(s->pb, len - picsize, desc, desc_len);
 
-    ret = av_get_packet(s->pb, &pkt, picsize);
+    ret = av_get_packet(s->pb, pkt, picsize);
     if (ret < 0)
         goto fail;
 
@@ -286,12 +286,21 @@  static int asf_read_picture(AVFormatContext *s, int len)
         ret = AVERROR(ENOMEM);
         goto fail;
     }
+    av_packet_move_ref(st->apic, pkt);
     st->disposition              |= AV_DISPOSITION_ATTACHED_PIC;
     st->codecpar->codec_type      = AVMEDIA_TYPE_VIDEO;
     st->codecpar->codec_id        = id;
-    st->attached_pic              = pkt;
-    st->attached_pic.stream_index = st->index;
-    st->attached_pic.flags       |= AV_PKT_FLAG_KEY;
+    st->apic->stream_index        = st->index;
+    st->apic->flags              |= AV_PKT_FLAG_KEY;
+#if FF_API_ATTACHED_PIC
+FF_DISABLE_DEPRECATION_WARNINGS
+    ret = av_packet_ref(&st->attached_pic, st->apic);
+    if (ret < 0) {
+        av_packet_unref(st->apic);
+        goto fail;
+    }
+FF_ENABLE_DEPRECATION_WARNINGS
+#endif
 
     if (*desc)
         av_dict_set(&st->metadata, "title", desc, AV_DICT_DONT_STRDUP_VAL);
@@ -304,7 +313,7 @@  static int asf_read_picture(AVFormatContext *s, int len)
 
 fail:
     av_freep(&desc);
-    av_packet_unref(&pkt);
+    av_packet_unref(pkt);
     return ret;
 }
 
diff --git a/libavformat/asfdec_o.c b/libavformat/asfdec_o.c
index 34ae541934..51577064b8 100644
--- a/libavformat/asfdec_o.c
+++ b/libavformat/asfdec_o.c
@@ -360,7 +360,7 @@  static int asf_set_metadata(AVFormatContext *s, const uint8_t *name,
  * but in reality this is only loosely similar */
 static int asf_read_picture(AVFormatContext *s, int len)
 {
-    AVPacket pkt          = { 0 };
+    AVPacket *pkt = s->internal->parse_pkt;
     const CodecMime *mime = ff_id3v2_mime_tags;
     enum  AVCodecID id    = AV_CODEC_ID_NONE;
     char mimetype[64];
@@ -414,7 +414,7 @@  static int asf_read_picture(AVFormatContext *s, int len)
         return AVERROR(ENOMEM);
     len -= avio_get_str16le(s->pb, len - picsize, desc, desc_len);
 
-    ret = av_get_packet(s->pb, &pkt, picsize);
+    ret = av_get_packet(s->pb, pkt, picsize);
     if (ret < 0)
         goto fail;
 
@@ -424,12 +424,21 @@  static int asf_read_picture(AVFormatContext *s, int len)
         goto fail;
     }
 
+    av_packet_move_ref(st->apic, pkt);
     st->disposition              |= AV_DISPOSITION_ATTACHED_PIC;
     st->codecpar->codec_type      = AVMEDIA_TYPE_VIDEO;
     st->codecpar->codec_id        = id;
-    st->attached_pic              = pkt;
-    st->attached_pic.stream_index = st->index;
-    st->attached_pic.flags       |= AV_PKT_FLAG_KEY;
+    st->apic->stream_index        = st->index;
+    st->apic->flags              |= AV_PKT_FLAG_KEY;
+#if FF_API_ATTACHED_PIC
+FF_DISABLE_DEPRECATION_WARNINGS
+    ret = av_packet_ref(&st->attached_pic, st->apic);
+    if (ret < 0) {
+        av_packet_unref(st->apic);
+        goto fail;
+    }
+FF_ENABLE_DEPRECATION_WARNINGS
+#endif
 
     if (*desc) {
         if (av_dict_set(&st->metadata, "title", desc, AV_DICT_DONT_STRDUP_VAL) < 0)
@@ -444,7 +453,7 @@  static int asf_read_picture(AVFormatContext *s, int len)
 
 fail:
     av_freep(&desc);
-    av_packet_unref(&pkt);
+    av_packet_unref(pkt);
     return ret;
 }
 
diff --git a/libavformat/avformat.h b/libavformat/avformat.h
index 765bc3b6f5..769d27d84a 100644
--- a/libavformat/avformat.h
+++ b/libavformat/avformat.h
@@ -947,14 +947,19 @@  typedef struct AVStream {
      */
     AVRational avg_frame_rate;
 
+#if FF_API_ATTACHED_PIC
     /**
      * For streams with AV_DISPOSITION_ATTACHED_PIC disposition, this packet
      * will contain the attached picture.
      *
      * decoding: set by libavformat, must not be modified by the caller.
      * encoding: unused
+     *
+     * @deprecated Use apic instead.
      */
+    attribute_deprecated
     AVPacket attached_pic;
+#endif
 
     /**
      * An array of side data that applies to the whole stream (i.e. the
@@ -1039,6 +1044,17 @@  typedef struct AVStream {
      */
     AVCodecParameters *codecpar;
 
+    /**
+     * For streams with AV_DISPOSITION_ATTACHED_PIC disposition, this packet
+     * will contain the attached picture. It is allocated and freed by
+     * libavformat in avformat_new_stream() and avformat_free_context()
+     * respectively.
+     *
+     * - demuxing: set by libavformat, must not be modified by the caller.
+     * - muxing: unused
+     */
+    AVPacket *apic;
+
     /*****************************************************************
      * All fields below this line are not part of the public API. They
      * may not be used outside of libavformat and can be changed and
@@ -1049,11 +1065,6 @@  typedef struct AVStream {
      *****************************************************************
      */
 
-#if LIBAVFORMAT_VERSION_MAJOR < 59
-    // kept for ABI compatibility only, do not access in any way
-    void *unused;
-#endif
-
     int pts_wrap_bits; /**< number of bits in pts (used for wrapping control) */
 
     // Timestamp generation support:
diff --git a/libavformat/flac_picture.c b/libavformat/flac_picture.c
index f15cfa877a..f771df3fc2 100644
--- a/libavformat/flac_picture.c
+++ b/libavformat/flac_picture.c
@@ -165,12 +165,20 @@  int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size, int tr
         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;
+    av_packet_unref(st->apic);
+    st->apic->buf          = data;
+    st->apic->data         = data->data;
+    st->apic->size         = len;
+    st->apic->stream_index = st->index;
+    st->apic->flags       |= AV_PKT_FLAG_KEY;
+    data = NULL;
+#if FF_API_ATTACHED_PIC
+FF_DISABLE_DEPRECATION_WARNINGS
+    ret = av_packet_ref(&st->attached_pic, st->apic);
+    if (ret < 0)
+        goto fail;
+FF_ENABLE_DEPRECATION_WARNINGS
+#endif
 
     st->disposition      |= AV_DISPOSITION_ATTACHED_PIC;
     st->codecpar->codec_type = AVMEDIA_TYPE_VIDEO;
@@ -185,6 +193,7 @@  int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size, int tr
 
 fail:
     av_buffer_unref(&data);
+    av_packet_unref(st->apic);
     av_freep(&desc);
 
     return ret;
diff --git a/libavformat/hls.c b/libavformat/hls.c
index 597bea7f25..2b18385496 100644
--- a/libavformat/hls.c
+++ b/libavformat/hls.c
@@ -1068,15 +1068,15 @@  static int id3_has_changed_values(struct playlist *pls, AVDictionary *metadata,
     }
 
     /* check if apic appeared */
-    if (apic && (pls->ctx->nb_streams != 2 || !pls->ctx->streams[1]->attached_pic.data))
+    if (apic && (pls->ctx->nb_streams != 2 || !pls->ctx->streams[1]->apic->data))
         return 1;
 
     if (apic) {
-        int size = pls->ctx->streams[1]->attached_pic.size;
+        int size = pls->ctx->streams[1]->apic->size;
         if (size != apic->buf->size - AV_INPUT_BUFFER_PADDING_SIZE)
             return 1;
 
-        if (memcmp(apic->buf->data, pls->ctx->streams[1]->attached_pic.data, size) != 0)
+        if (memcmp(apic->buf->data, pls->ctx->streams[1]->apic->data, size) != 0)
             return 1;
     }
 
diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c
index f33b7ba93a..f8c10ab90c 100644
--- a/libavformat/id3v2.c
+++ b/libavformat/id3v2.c
@@ -1142,6 +1142,7 @@  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;
@@ -1162,14 +1163,24 @@  int ff_id3v2_parse_apic(AVFormatContext *s, ID3v2ExtraMeta *extra_meta)
 
         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;
+        av_packet_unref(st->apic);
+        st->apic->buf          = apic->buf;
+        st->apic->data         = apic->buf->data;
+        st->apic->size         = apic->buf->size - AV_INPUT_BUFFER_PADDING_SIZE;
+        st->apic->stream_index = st->index;
+        st->apic->flags       |= AV_PKT_FLAG_KEY;
 
         apic->buf = NULL;
+
+#if FF_API_ATTACHED_PIC
+FF_DISABLE_DEPRECATION_WARNINGS
+        ret = av_packet_ref(&st->attached_pic, st->apic);
+        if (ret < 0) {
+            av_packet_unref(st->apic);
+            return ret;
+        }
+FF_ENABLE_DEPRECATION_WARNINGS
+#endif
     }
 
     return 0;
diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index 1dc188c946..92e3b8f183 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -3007,7 +3007,7 @@  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;
+                AVPacket *pkt = st->apic;
 
                 st->disposition         |= AV_DISPOSITION_ATTACHED_PIC;
                 st->codecpar->codec_type = AVMEDIA_TYPE_VIDEO;
@@ -3019,6 +3019,13 @@  static int matroska_read_header(AVFormatContext *s)
                 pkt->size         = attachments[j].bin.size;
                 pkt->stream_index = st->index;
                 pkt->flags       |= AV_PKT_FLAG_KEY;
+#if FF_API_ATTACHED_PIC
+FF_DISABLE_DEPRECATION_WARNINGS
+                res = av_packet_ref(&st->attached_pic, pkt);
+                if (res < 0)
+                    goto fail;
+FF_ENABLE_DEPRECATION_WARNINGS
+#endif
             } 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..d99f922709 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -204,12 +204,21 @@  static int mov_read_covr(MOVContext *c, AVIOContext *pb, int type, int len)
         return AVERROR(ENOMEM);
     st->priv_data = sc;
 
-    ret = av_get_packet(pb, &st->attached_pic, len);
+    ret = av_get_packet(pb, st->apic, len);
     if (ret < 0)
         return ret;
+#if FF_API_ATTACHED_PIC
+FF_DISABLE_DEPRECATION_WARNINGS
+    ret = av_packet_ref(&st->attached_pic, st->apic);
+    if (ret < 0) {
+        av_packet_unref(st->apic);
+        return ret;
+    }
+FF_ENABLE_DEPRECATION_WARNINGS
+#endif
 
-    if (st->attached_pic.size >= 8 && id != AV_CODEC_ID_BMP) {
-        if (AV_RB64(st->attached_pic.data) == 0x89504e470d0a1a0a) {
+    if (st->apic->size >= 8 && id != AV_CODEC_ID_BMP) {
+        if (AV_RB64(st->apic->data) == 0x89504e470d0a1a0a) {
             id = AV_CODEC_ID_PNG;
         } else {
             id = AV_CODEC_ID_MJPEG;
@@ -218,8 +227,8 @@  static int mov_read_covr(MOVContext *c, AVIOContext *pb, int type, int len)
 
     st->disposition              |= AV_DISPOSITION_ATTACHED_PIC;
 
-    st->attached_pic.stream_index = st->index;
-    st->attached_pic.flags       |= AV_PKT_FLAG_KEY;
+    st->apic->stream_index        = st->index;
+    st->apic->flags              |= AV_PKT_FLAG_KEY;
 
     st->codecpar->codec_type = AVMEDIA_TYPE_VIDEO;
     st->codecpar->codec_id   = id;
@@ -7229,11 +7238,17 @@  static void mov_read_chapters(AVFormatContext *s)
                     goto finish;
                 }
 
-                if (av_get_packet(sc->pb, &st->attached_pic, sample->size) < 0)
+                if (av_get_packet(sc->pb, st->apic, sample->size) < 0)
                     goto finish;
 
-                st->attached_pic.stream_index = st->index;
-                st->attached_pic.flags       |= AV_PKT_FLAG_KEY;
+                st->apic->stream_index = st->index;
+                st->apic->flags       |= AV_PKT_FLAG_KEY;
+#if FF_API_ATTACHED_PIC
+FF_DISABLE_DEPRECATION_WARNINGS
+                if (av_packet_ref(&st->attached_pic, st->apic) < 0)
+                    goto finish;
+FF_ENABLE_DEPRECATION_WARNINGS
+#endif
             }
         } else {
             st->codecpar->codec_type = AVMEDIA_TYPE_DATA;
diff --git a/libavformat/utils.c b/libavformat/utils.c
index 524765aeb4..9308b53106 100644
--- a/libavformat/utils.c
+++ b/libavformat/utils.c
@@ -457,7 +457,7 @@  int avformat_queue_attached_pictures(AVFormatContext *s)
     for (i = 0; i < s->nb_streams; i++)
         if (s->streams[i]->disposition & AV_DISPOSITION_ATTACHED_PIC &&
             s->streams[i]->discard < AVDISCARD_ALL) {
-            if (s->streams[i]->attached_pic.size <= 0) {
+            if (s->streams[i]->apic->size <= 0) {
                 av_log(s, AV_LOG_WARNING,
                     "Attached picture on stream %d has invalid size, "
                     "ignoring\n", i);
@@ -466,7 +466,7 @@  int avformat_queue_attached_pictures(AVFormatContext *s)
 
             ret = avpriv_packet_list_put(&s->internal->raw_packet_buffer,
                                      &s->internal->raw_packet_buffer_end,
-                                     &s->streams[i]->attached_pic,
+                                     s->streams[i]->apic,
                                      av_packet_ref, 0);
             if (ret < 0)
                 return ret;
@@ -4371,8 +4371,14 @@  static void free_stream(AVStream **pst)
     if (st->parser)
         av_parser_close(st->parser);
 
+#if FF_API_ATTACHED_PIC
+FF_DISABLE_DEPRECATION_WARNINGS
     if (st->attached_pic.data)
         av_packet_unref(&st->attached_pic);
+FF_ENABLE_DEPRECATION_WARNINGS
+#endif
+
+    av_packet_free(&st->apic);
 
     if (st->internal) {
         avcodec_free_context(&st->internal->avctx);
@@ -4530,6 +4536,10 @@  FF_ENABLE_DEPRECATION_WARNINGS
     if (!st->codecpar)
         goto fail;
 
+    st->apic = av_packet_alloc();
+    if (!st->apic)
+        goto fail;
+
     st->internal->avctx = avcodec_alloc_context3(NULL);
     if (!st->internal->avctx)
         goto fail;
diff --git a/libavformat/version.h b/libavformat/version.h
index ced5600034..dce936794a 100644
--- a/libavformat/version.h
+++ b/libavformat/version.h
@@ -115,6 +115,9 @@ 
 #ifndef FF_API_LAVF_PRIV_OPT
 #define FF_API_LAVF_PRIV_OPT            (LIBAVFORMAT_VERSION_MAJOR < 60)
 #endif
+#ifndef FF_API_ATTACHED_PIC
+#define FF_API_ATTACHED_PIC             (LIBAVFORMAT_VERSION_MAJOR < 60)
+#endif
 
 
 #ifndef FF_API_R_FRAME_RATE
diff --git a/libavformat/wtvdec.c b/libavformat/wtvdec.c
index 7def9d2348..05386add76 100644
--- a/libavformat/wtvdec.c
+++ b/libavformat/wtvdec.c
@@ -454,11 +454,18 @@  static void get_attachment(AVFormatContext *s, AVIOContext *pb, int length)
     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);
+    ret = av_get_packet(pb, st->apic, filesize);
     if (ret < 0)
         goto done;
-    st->attached_pic.stream_index = st->index;
-    st->attached_pic.flags       |= AV_PKT_FLAG_KEY;
+    st->apic->stream_index = st->index;
+    st->apic->flags  |= AV_PKT_FLAG_KEY;
+#if FF_API_ATTACHED_PIC
+FF_DISABLE_DEPRECATION_WARNINGS
+    ret = av_packet_ref(&st->attached_pic, st->apic);
+    if (ret < 0)
+        goto done;
+FF_ENABLE_DEPRECATION_WARNINGS
+#endif
     st->disposition              |= AV_DISPOSITION_ATTACHED_PIC;
 done:
     avio_seek(pb, pos + length, SEEK_SET);