[FFmpeg-devel] [PATCH] Ignore duplicate ID3 tags if vorbis tags exist

Paul Arzelier paul.arzelier at free.fr
Fri Jan 27 18:32:08 EET 2017


I've added an id3v2_meta AVDictionary for the AVInternalFormat structure 
and edited mp3dec.c as wm4 suggested.

It solves the issue Michael noticed, and passes the fate tests, but 
maybe it still need some work; tell me!


Le 27/01/2017 à 07:02, wm4 a écrit :
> On Fri, 27 Jan 2017 03:03:03 +0100
> Michael Niedermayer <michaelni at gmx.at> wrote:
>
>> On Thu, Jan 26, 2017 at 02:29:21PM +0100, Paul Arzelier wrote:
>>> Alright, attached is the last version (I hope!)
>>>
>>> Paul
>>>
>>>
>>> Le 26/01/2017 à 13:43, wm4 a écrit :
>>>> On Thu, 26 Jan 2017 13:32:08 +0100
>>>> Paul Arzelier <paul.arzelier at free.fr> wrote:
>>>>   
>>>>>  From d84648e1990ad3a12462dfb76990dc7036f5f082 Mon Sep 17 00:00:00 2001
>>>>> From: Polochon-street <polochonstreet at gmx.fr>
>>>>> Date: Thu, 26 Jan 2017 13:25:22 +0100
>>>>> Subject: [PATCH] Ignore ID3 tags if other tags are present (e.g. vorbis
>>>>>   comments)
>>>>> Originally-by: Ben Boeckel <mathstuf at gmail.com>
>>>>> ---
>>>>>   libavformat/utils.c | 17 ++++++++++++++++-
>>>>>   1 file changed, 16 insertions(+), 1 deletion(-)
>>>>>   
>>>> Patch looks generally great to me now.
>>>>   
>>>>> diff --git a/libavformat/utils.c b/libavformat/utils.c
>>>>> index d5dfca7dec..ce24244135 100644
>>>>> --- a/libavformat/utils.c
>>>>> +++ b/libavformat/utils.c
>>>>> @@ -513,6 +513,7 @@ int avformat_open_input(AVFormatContext **ps, const char *filename,
>>>>>       AVFormatContext *s = *ps;
>>>>>       int i, ret = 0;
>>>>>       AVDictionary *tmp = NULL;
>>>>> +    AVDictionary *id3_meta = NULL;
>>>>>       ID3v2ExtraMeta *id3v2_extra_meta = NULL;
>>>>>       if (!s && !(s = avformat_alloc_context()))
>>>>> @@ -588,12 +589,25 @@ int avformat_open_input(AVFormatContext **ps, const char *filename,
>>>>>       /* e.g. AVFMT_NOFILE formats will not have a AVIOContext */
>>>>>       if (s->pb)
>>>>> -        ff_id3v2_read(s, ID3v2_DEFAULT_MAGIC, &id3v2_extra_meta, 0);
>>>>> +        ff_id3v2_read_dict(s->pb, &id3_meta, ID3v2_DEFAULT_MAGIC, &id3v2_extra_meta);
>>>>>       if (!(s->flags&AVFMT_FLAG_PRIV_OPT) && s->iformat->read_header)
>>>>>           if ((ret = s->iformat->read_header(s)) < 0)
>>>>>               goto fail;
>>>>> +    if (!s->metadata) {
>>>>> +        av_dict_copy(&s->metadata, id3_meta, AV_DICT_DONT_OVERWRITE);
>>>>> +        av_dict_free(&id3_meta);
>>>> Strictly speaking, this line is redundant, but it doesn't harm either.
>>>> If you feel like it you could remove it, but it's not necessary.
>>>>   
>>>>> +    }
>>>>> +    else if (id3_meta) {
>>>> If you make another patch, you could merge the } with the next line too
>>>> (I'm not sure, but I think that's preferred coding-style wise).
>>>>   
>>>>> +        int level = AV_LOG_WARNING;
>>>>> +        if (s->error_recognition & AV_EF_COMPLIANT)
>>>>> +            level = AV_LOG_ERROR;
>>>>> +        av_log(s, level, "Discarding ID3 tag because more suitable tags were found.\n");
>>>>> +        if (s->error_recognition & AV_EF_EXPLODE)
>>>>> +            return AVERROR_INVALIDDATA;
>>>>> +    }
>>>>> +
>>>>>       if (id3v2_extra_meta) {
>>>>>           if (!strcmp(s->iformat->name, "mp3") || !strcmp(s->iformat->name, "aac") ||
>>>>>               !strcmp(s->iformat->name, "tta")) {
>>>>> @@ -627,6 +641,7 @@ int avformat_open_input(AVFormatContext **ps, const char *filename,
>>>>>   fail:
>>>>>       ff_id3v2_free_extra_meta(&id3v2_extra_meta);
>>>>>       av_dict_free(&tmp);
>>>>> +   av_dict_free(&id3_meta);
>>>>>       if (s->pb && !(s->flags & AVFMT_FLAG_CUSTOM_IO))
>>>>>           avio_closep(&s->pb);
>>>>>       avformat_free_context(s);
>>>> _______________________________________________
>>>> ffmpeg-devel mailing list
>>>> ffmpeg-devel at ffmpeg.org
>>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>> -- 
>>> Paul ARZELIER
>>> Élève ingénieur à l'École Centrale de Lille
>>> 2014-2017
>>>    
>>>   utils.c |   17 ++++++++++++++++-
>>>   1 file changed, 16 insertions(+), 1 deletion(-)
>>> 477d4f87058a098464e2c1dbfe7e7ff806d2c85f  0001-Ignore-ID3-tags-if-other-tags-are-present-e.g.-vorbi.patch
>>>  From d84648e1990ad3a12462dfb76990dc7036f5f082 Mon Sep 17 00:00:00 2001
>>> From: Polochon-street <polochonstreet at gmx.fr>
>>> Date: Thu, 26 Jan 2017 13:25:22 +0100
>>> Subject: [PATCH] Ignore ID3 tags if other tags are present (e.g. vorbis
>>>   comments)
>>> Originally-by: Ben Boeckel <mathstuf at gmail.com>
>>> ---
>>>   libavformat/utils.c | 17 ++++++++++++++++-
>>>   1 file changed, 16 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/libavformat/utils.c b/libavformat/utils.c
>>> index d5dfca7dec..ce24244135 100644
>>> --- a/libavformat/utils.c
>>> +++ b/libavformat/utils.c
>>> @@ -513,6 +513,7 @@ int avformat_open_input(AVFormatContext **ps, const char *filename,
>>>       AVFormatContext *s = *ps;
>>>       int i, ret = 0;
>>>       AVDictionary *tmp = NULL;
>>> +    AVDictionary *id3_meta = NULL;
>>>       ID3v2ExtraMeta *id3v2_extra_meta = NULL;
>>>   
>>>       if (!s && !(s = avformat_alloc_context()))
>>> @@ -588,12 +589,25 @@ int avformat_open_input(AVFormatContext **ps, const char *filename,
>>>   
>>>       /* e.g. AVFMT_NOFILE formats will not have a AVIOContext */
>>>       if (s->pb)
>>> -        ff_id3v2_read(s, ID3v2_DEFAULT_MAGIC, &id3v2_extra_meta, 0);
>>> +        ff_id3v2_read_dict(s->pb, &id3_meta, ID3v2_DEFAULT_MAGIC, &id3v2_extra_meta);
>>>   
>>>       if (!(s->flags&AVFMT_FLAG_PRIV_OPT) && s->iformat->read_header)
>>>           if ((ret = s->iformat->read_header(s)) < 0)
>>>               goto fail;
>>>   
>>> +    if (!s->metadata) {
>>> +        s->metadata = id3_meta;
>>> +        id3_meta = NULL;
>>> +    } else if (id3_meta) {
>>> +        int level = AV_LOG_WARNING;
>>> +        if (s->error_recognition & AV_EF_COMPLIANT)
>>> +            level = AV_LOG_ERROR;
>>> +        av_log(s, level, "Discarding ID3 tag because more suitable tags were found.\n");
>>> +        av_dict_free(&id3_meta);
>>> +        if (s->error_recognition & AV_EF_EXPLODE)
>>> +            return AVERROR_INVALIDDATA;
>>> +    }
>>> +
>>>       if (id3v2_extra_meta) {
>>>           if (!strcmp(s->iformat->name, "mp3") || !strcmp(s->iformat->name, "aac") ||
>>>               !strcmp(s->iformat->name, "tta")) {
>>> @@ -627,6 +641,7 @@ int avformat_open_input(AVFormatContext **ps, const char *filename,
>>>   fail:
>>>       ff_id3v2_free_extra_meta(&id3v2_extra_meta);
>>>       av_dict_free(&tmp);
>>> +    av_dict_free(&id3_meta);
>>>       if (s->pb && !(s->flags & AVFMT_FLAG_CUSTOM_IO))
>>>           avio_closep(&s->pb);
>>>       avformat_free_context(s);
>> This causes some metadata to be lost
>> for example
>> ~/tickets/4003/mp3_demuxer_EOI.mp3
>> genre           : Ethno / Thrash Métal
>> changes to
>> genre           : Other
>>
>> It also seems to do something to the character encoding of metadata
>> in some files
>>
>> and it results in multiple additional seeks during probe/analysis
>> for example:
>>
>> ./ffplay-new -v 99 /home/michael/tickets//3327/sample.mp3
>> [mp3 @ 0x7effbc000920] After avformat_find_stream_info() pos: 2526661 bytes read:2556032 seeks:2 frames:50
>> vs. before:
>> [mp3 @ 0x7fa1c4000920] After avformat_find_stream_info() pos: 2526661 bytes read:2555904 seeks:0 frames:50
>>
>> [...]
>>
> The mp3 demuxer accesses the netadata field - both with read and write
> accesses. That's a bit nasty, and requires some hacking.
>
> I guess I'm fine with the flac-only patch if the patch author doesn't
> want to bother - I can't really demand from him to clean up bad
> libvformat design decisions.
>
> But if he wants to, I'd suggest either an internal demuxer flag, that
> specifies whether id3v2 should be set to the metadata field immediately
> or not, or adding a dedicated internal AVSteam field for the id3v2
> tags. Both would require changing mp3dec.c - with some luck, it's
> the only demuxer that does this.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

-- 
Paul ARZELIER
Élève ingénieur à l'École Centrale de Lille
2014-2017

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Ignore-ID3-tags-if-other-tags-are-present-e.g.-vorbi.patch
Type: text/x-patch
Size: 3483 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170127/4c7316db/attachment.bin>


More information about the ffmpeg-devel mailing list