[FFmpeg-devel] [PATCH] avformat: fix id3 chapters
Lukas Stabe
lukas at stabe.de
Thu Oct 5 12:27:11 EEST 2017
> On 5. Oct 2017, at 10:51, wm4 <nfxjfg at googlemail.com> wrote:
>
> On Thu, 5 Oct 2017 03:34:19 +0200
> Lukas Stabe <lukas at stabe.de> wrote:
>
>> These changes store id3 chapter data in ID3v2ExtraMeta and introduce ff_id3v2_parse_chapters to parse them into the format context if needed.
>>
>> Encoders using ff_id3v2_read, which previously parsed chapters into the format context automatically, were adjusted to call ff_id3v2_parse_chapters.
>> ---
>> libavformat/aiffdec.c | 3 +-
>> libavformat/asfdec_f.c | 4 +-
>> libavformat/asfdec_o.c | 4 +-
>> libavformat/dsfdec.c | 4 +-
>> libavformat/id3v2.c | 120 ++++++++++++++++++++++++++++++++++++-------------
>> libavformat/id3v2.h | 15 +++++--
>> libavformat/iff.c | 3 +-
>> libavformat/omadec.c | 5 +++
>> libavformat/utils.c | 2 +
>> 9 files changed, 122 insertions(+), 38 deletions(-)
>>
>> diff --git a/libavformat/aiffdec.c b/libavformat/aiffdec.c
>> index 2ef9386edb..99e05c78ec 100644
>> --- a/libavformat/aiffdec.c
>> +++ b/libavformat/aiffdec.c
>> @@ -258,7 +258,8 @@ static int aiff_read_header(AVFormatContext *s)
>> position = avio_tell(pb);
>> ff_id3v2_read(s, ID3v2_DEFAULT_MAGIC, &id3v2_extra_meta, size);
>> if (id3v2_extra_meta)
>> - if ((ret = ff_id3v2_parse_apic(s, &id3v2_extra_meta)) < 0) {
>> + if ((ret = ff_id3v2_parse_apic(s, &id3v2_extra_meta)) < 0 ||
>> + (ret = ff_id3v2_parse_chapters(s, &id3v2_extra_meta)) < 0) {
>> ff_id3v2_free_extra_meta(&id3v2_extra_meta);
>> return ret;
>> }
>> diff --git a/libavformat/asfdec_f.c b/libavformat/asfdec_f.c
>> index cc648b9a2f..64a0b9d7f2 100644
>> --- a/libavformat/asfdec_f.c
>> +++ b/libavformat/asfdec_f.c
>> @@ -307,8 +307,10 @@ static void get_id3_tag(AVFormatContext *s, int len)
>> ID3v2ExtraMeta *id3v2_extra_meta = NULL;
>>
>> ff_id3v2_read(s, ID3v2_DEFAULT_MAGIC, &id3v2_extra_meta, len);
>> - if (id3v2_extra_meta)
>> + if (id3v2_extra_meta) {
>> ff_id3v2_parse_apic(s, &id3v2_extra_meta);
>> + ff_id3v2_parse_chapters(s, &id3v2_extra_meta);
>> + }
>> ff_id3v2_free_extra_meta(&id3v2_extra_meta);
>> }
>>
>> diff --git a/libavformat/asfdec_o.c b/libavformat/asfdec_o.c
>> index 818d6f3573..5122e33c78 100644
>> --- a/libavformat/asfdec_o.c
>> +++ b/libavformat/asfdec_o.c
>> @@ -460,8 +460,10 @@ static void get_id3_tag(AVFormatContext *s, int len)
>> ID3v2ExtraMeta *id3v2_extra_meta = NULL;
>>
>> ff_id3v2_read(s, ID3v2_DEFAULT_MAGIC, &id3v2_extra_meta, len);
>> - if (id3v2_extra_meta)
>> + if (id3v2_extra_meta) {
>> ff_id3v2_parse_apic(s, &id3v2_extra_meta);
>> + ff_id3v2_parse_chapters(s, &id3v2_extra_meta);
>> + }
>> ff_id3v2_free_extra_meta(&id3v2_extra_meta);
>> }
>>
>> diff --git a/libavformat/dsfdec.c b/libavformat/dsfdec.c
>> index 49ca336427..41538fd83c 100644
>> --- a/libavformat/dsfdec.c
>> +++ b/libavformat/dsfdec.c
>> @@ -53,8 +53,10 @@ static void read_id3(AVFormatContext *s, uint64_t id3pos)
>> return;
>>
>> ff_id3v2_read(s, ID3v2_DEFAULT_MAGIC, &id3v2_extra_meta, 0);
>> - if (id3v2_extra_meta)
>> + if (id3v2_extra_meta) {
>> ff_id3v2_parse_apic(s, &id3v2_extra_meta);
>> + ff_id3v2_parse_chapters(s, &id3v2_extra_meta);
>> + }
>> ff_id3v2_free_extra_meta(&id3v2_extra_meta);
>> }
>>
>> diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c
>> index f15cefee47..6c216ba7a2 100644
>> --- a/libavformat/id3v2.c
>> +++ b/libavformat/id3v2.c
>> @@ -670,59 +670,68 @@ fail:
>> avio_seek(pb, end, SEEK_SET);
>> }
>>
>> +static void free_chapter(void *obj)
>> +{
>> + ID3v2ExtraMetaCHAP *chap = obj;
>> + av_freep(&chap->element_id);
>> + av_dict_free(&chap->meta);
>> + av_freep(&chap);
>> +}
>> +
>> static void read_chapter(AVFormatContext *s, AVIOContext *pb, int len, const char *ttag, ID3v2ExtraMeta **extra_meta, int isv34)
>> {
>> - AVRational time_base = {1, 1000};
>> - uint32_t start, end;
>> - AVChapter *chapter;
>> - uint8_t *dst = NULL;
>> int taglen;
>> char tag[5];
>> + ID3v2ExtraMeta *new_extra = NULL;
>> + ID3v2ExtraMetaCHAP *chap = NULL;
>>
>> - if (!s) {
>> - /* We should probably just put the chapter data to extra_meta here
>> - * and do the AVFormatContext-needing part in a separate
>> - * ff_id3v2_parse_apic()-like function. */
>> - av_log(NULL, AV_LOG_DEBUG, "No AVFormatContext, skipped ID3 chapter data\n");
>> - return;
>> - }
>> + new_extra = av_mallocz(sizeof(*new_extra));
>> + chap = av_mallocz(sizeof(*chap));
>> +
>> + if (!new_extra || !chap)
>> + goto fail;
>> +
>> + if (decode_str(s, pb, 0, &chap->element_id, &len) < 0)
>> + goto fail;
>>
>> - if (decode_str(s, pb, 0, &dst, &len) < 0)
>> - return;
>> if (len < 16)
>> - return;
>> + goto fail;
>>
>> - start = avio_rb32(pb);
>> - end = avio_rb32(pb);
>> + chap->start = avio_rb32(pb);
>> + chap->end = avio_rb32(pb);
>> avio_skip(pb, 8);
>>
>> - chapter = avpriv_new_chapter(s, s->nb_chapters + 1, time_base, start, end, dst);
>> - if (!chapter) {
>> - av_free(dst);
>> - return;
>> - }
>> -
>> len -= 16;
>> while (len > 10) {
>> if (avio_read(pb, tag, 4) < 4)
>> - goto end;
>> + goto fail;
>> tag[4] = 0;
>> taglen = avio_rb32(pb);
>> avio_skip(pb, 2);
>> len -= 10;
>> if (taglen < 0 || taglen > len)
>> - goto end;
>> + goto fail;
>> if (tag[0] == 'T')
>> - read_ttag(s, pb, taglen, &chapter->metadata, tag);
>> + read_ttag(s, pb, taglen, &chap->meta, tag);
>> else
>> avio_skip(pb, taglen);
>> len -= taglen;
>> }
>>
>> - ff_metadata_conv(&chapter->metadata, NULL, ff_id3v2_34_metadata_conv);
>> - ff_metadata_conv(&chapter->metadata, NULL, ff_id3v2_4_metadata_conv);
>> -end:
>> - av_free(dst);
>> + ff_metadata_conv(&chap->meta, NULL, ff_id3v2_34_metadata_conv);
>> + ff_metadata_conv(&chap->meta, NULL, ff_id3v2_4_metadata_conv);
>> +
>> + new_extra->tag = "CHAP";
>> + new_extra->data = chap;
>> + new_extra->next = *extra_meta;
>> + *extra_meta = new_extra;
>> +
>> + return;
>> +
>> +fail:
>> + if (chap)
>> + free_chapter(chap);
>> + av_freep(&new_extra);
>> }
>>
>> static void free_priv(void *obj)
>> @@ -782,7 +791,7 @@ typedef struct ID3v2EMFunc {
>> static const ID3v2EMFunc id3v2_extra_meta_funcs[] = {
>> { "GEO", "GEOB", read_geobtag, free_geobtag },
>> { "PIC", "APIC", read_apic, free_apic },
>> - { "CHAP","CHAP", read_chapter, NULL },
>> + { "CHAP","CHAP", read_chapter, free_chapter },
>> { "PRIV","PRIV", read_priv, free_priv },
>> { NULL }
>> };
>> @@ -1164,3 +1173,54 @@ int ff_id3v2_parse_apic(AVFormatContext *s, ID3v2ExtraMeta **extra_meta)
>>
>> return 0;
>> }
>> +
>> +int ff_id3v2_parse_chapters(AVFormatContext *s, ID3v2ExtraMeta **extra_meta)
>> +{
>> + int ret = 0;
>> + ID3v2ExtraMeta *cur;
>> + AVRational time_base = {1, 1000};
>> + ID3v2ExtraMetaCHAP **chapters = NULL;
>> + int num_chapters = 0;
>> + int i;
>> +
>> + // since extra_meta is a linked list where elements are prepended,
>> + // we need to reverse the order of chapters
>> + for (cur = *extra_meta; cur; cur = cur->next) {
>> + ID3v2ExtraMetaCHAP *chap;
>> +
>> + if (strcmp(cur->tag, "CHAP"))
>> + continue;
>> + chap = cur->data;
>> +
>> + if ((ret = av_dynarray_add_nofree(&chapters, &num_chapters, chap)) < 0)
>> + goto end;
>> + }
>> +
>> + for (i = 0; i < (num_chapters / 2); i++) {
>> + ID3v2ExtraMetaCHAP *right;
>> + int right_index;
>> +
>> + right_index = (num_chapters - 1) - i;
>> + right = chapters[right_index];
>> +
>> + chapters[right_index] = chapters[i];
>> + chapters[i] = right;
>> + }
>> +
>> + for (i = 0; i < num_chapters; i++) {
>> + ID3v2ExtraMetaCHAP *chap;
>> + AVChapter *chapter;
>> +
>> + chap = chapters[i];
>> + chapter = avpriv_new_chapter(s, i, time_base, chap->start, chap->end, chap->element_id);
>> + if (!chapter)
>> + continue;
>> +
>> + if ((ret = av_dict_copy(&chapter->metadata, chap->meta, 0)) < 0)
>> + goto end;
>> + }
>> +
>> +end:
>> + av_freep(&chapters);
>> + return ret;
>> +}
>> diff --git a/libavformat/id3v2.h b/libavformat/id3v2.h
>> index 6e7a8c9abf..5e64ead096 100644
>> --- a/libavformat/id3v2.h
>> +++ b/libavformat/id3v2.h
>> @@ -79,6 +79,12 @@ typedef struct ID3v2ExtraMetaPRIV {
>> uint32_t datasize;
>> } ID3v2ExtraMetaPRIV;
>>
>> +typedef struct ID3v2ExtraMetaCHAP {
>> + uint8_t *element_id;
>> + uint32_t start, end;
>> + AVDictionary *meta;
>> +} ID3v2ExtraMetaCHAP;
>> +
>> /**
>> * Detect ID3v2 Header.
>> * @param buf must be ID3v2_HEADER_SIZE byte long
>> @@ -97,8 +103,6 @@ int ff_id3v2_tag_len(const uint8_t *buf);
>> /**
>> * Read an ID3v2 tag into specified dictionary and retrieve supported extra metadata.
>> *
>> - * Chapters are not currently read by this variant.
>> - *
>> * @param metadata Parsed metadata is stored here
>> * @param extra_meta If not NULL, extra metadata is parsed into a list of
>> * ID3v2ExtraMeta structs and *extra_meta points to the head of the list
>> @@ -106,7 +110,7 @@ int ff_id3v2_tag_len(const uint8_t *buf);
>> void ff_id3v2_read_dict(AVIOContext *pb, AVDictionary **metadata, const char *magic, ID3v2ExtraMeta **extra_meta);
>>
>> /**
>> - * Read an ID3v2 tag, including supported extra metadata and chapters.
>> + * Read an ID3v2 tag, including supported extra metadata.
>> *
>> * Data is read from and stored to AVFormatContext.
>> *
>> @@ -158,6 +162,11 @@ void ff_id3v2_free_extra_meta(ID3v2ExtraMeta **extra_meta);
>> */
>> int ff_id3v2_parse_apic(AVFormatContext *s, ID3v2ExtraMeta **extra_meta);
>>
>> +/**
>> + * Create chapters for all CHAP tags found in the ID3v2 header.
>> + */
>> +int ff_id3v2_parse_chapters(AVFormatContext *s, ID3v2ExtraMeta **extra_meta);
>> +
>> extern const AVMetadataConv ff_id3v2_34_metadata_conv[];
>> extern const AVMetadataConv ff_id3v2_4_metadata_conv[];
>>
>> diff --git a/libavformat/iff.c b/libavformat/iff.c
>> index 6a09eb0e31..4cf17f6e1a 100644
>> --- a/libavformat/iff.c
>> +++ b/libavformat/iff.c
>> @@ -312,7 +312,8 @@ static int parse_dsd_prop(AVFormatContext *s, AVStream *st, uint64_t eof)
>> id3v2_extra_meta = NULL;
>> ff_id3v2_read(s, ID3v2_DEFAULT_MAGIC, &id3v2_extra_meta, size);
>> if (id3v2_extra_meta) {
>> - if ((ret = ff_id3v2_parse_apic(s, &id3v2_extra_meta)) < 0) {
>> + if ((ret = ff_id3v2_parse_apic(s, &id3v2_extra_meta)) < 0 ||
>> + (ret = ff_id3v2_parse_chapters(s, &id3v2_extra_meta)) < 0) {
>> ff_id3v2_free_extra_meta(&id3v2_extra_meta);
>> return ret;
>> }
>> diff --git a/libavformat/omadec.c b/libavformat/omadec.c
>> index fa53636f1a..423d52b3aa 100644
>> --- a/libavformat/omadec.c
>> +++ b/libavformat/omadec.c
>> @@ -397,6 +397,11 @@ static int oma_read_header(AVFormatContext *s)
>> OMAContext *oc = s->priv_data;
>>
>> ff_id3v2_read(s, ID3v2_EA3_MAGIC, &extra_meta, 0);
>> + if ((ret = ff_id3v2_parse_chapters(s, &extra_meta)) < 0) {
>> + ff_id3v2_free_extra_meta(&extra_meta);
>> + return ret;
>> + }
>> +
>> ret = avio_read(s->pb, buf, EA3_HEADER_SIZE);
>> if (ret < EA3_HEADER_SIZE)
>> return -1;
>> diff --git a/libavformat/utils.c b/libavformat/utils.c
>> index 7abca632b5..1a7996c4fd 100644
>> --- a/libavformat/utils.c
>> +++ b/libavformat/utils.c
>> @@ -613,6 +613,8 @@ int avformat_open_input(AVFormatContext **ps, const char *filename,
>> !strcmp(s->iformat->name, "tta")) {
>> if ((ret = ff_id3v2_parse_apic(s, &id3v2_extra_meta)) < 0)
>> goto fail;
>> + if ((ret = ff_id3v2_parse_chapters(s, &id3v2_extra_meta)) < 0)
>> + goto fail;
>> } else
>> av_log(s, AV_LOG_DEBUG, "demuxer does not support additional id3 data, skipping\n");
>> }
>
> I like this much more. Does fate pass? Will probably apply later this
> day.
Yup, fate passes. I haven't yet added a fate test for id3 chapters, but will probably follow up with a patch for one when I get around to throwing together a sample file.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Message signed with OpenPGP
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20171005/2d9a1e6b/attachment.sig>
More information about the ffmpeg-devel
mailing list