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.rheinha...@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-requ...@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-requ...@ffmpeg.org with subject "unsubscribe".
