[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