diff mbox

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

Message ID a3355bb3-6a9f-26dd-ba41-a292b6ed9d15@free.fr
State Superseded
Headers show

Commit Message

Paul Arzelier Jan. 27, 2017, 4:32 p.m. UTC 
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@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@free.fr> wrote:
>>>>   
>>>>>  From d84648e1990ad3a12462dfb76990dc7036f5f082 Mon Sep 17 00:00:00 2001
>>>>> From: Polochon-street <polochonstreet@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@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@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@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@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@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Comments

wm4 Jan. 27, 2017, 4:42 p.m. UTC | #1 
On Fri, 27 Jan 2017 17:32:08 +0100
Paul Arzelier <paul.arzelier@free.fr> wrote:

> From 227d7d1f4b5665f824900cf2b14863621180dd1c Mon Sep 17 00:00:00 2001
> From: Polochon-street <polochonstreet@gmx.fr>
> Date: Fri, 27 Jan 2017 16:49:41 +0100
> Subject: [PATCH] Ignore ID3 tags if other tags are present (e.g. vorbis)
> Originally-by: Ben Boeckel <mathstuf@gmail.com>
> ---
>  libavformat/internal.h |  5 +++++
>  libavformat/mp3dec.c   |  5 +++++
>  libavformat/options.c  |  1 +
>  libavformat/utils.c    | 16 +++++++++++++++-
>  4 files changed, 26 insertions(+), 1 deletion(-)
> 

Looks largely ok to me, just a few minor nits.

> diff --git a/libavformat/internal.h b/libavformat/internal.h
> index 9d614fb12c..63a1724cfa 100644
> --- a/libavformat/internal.h
> +++ b/libavformat/internal.h
> @@ -140,6 +140,11 @@ struct AVFormatInternal {
>       * Whether or not avformat_init_output fully initialized streams
>       */
>      int streams_initialized;
> +
> +    /**
> +     * ID3v2 tag useful for MP3 demuxing

The tag is used for many file formats (unfortunately), not just mp3. So
I'm not sure if the comment is correct.

> +     */
> +    AVDictionary *id3v2_meta;
>  };
>  
>  struct AVStreamInternal {
> diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
> index 099ca57d24..8540e45e4f 100644
> --- a/libavformat/mp3dec.c
> +++ b/libavformat/mp3dec.c
> @@ -349,6 +349,11 @@ static int mp3_read_header(AVFormatContext *s)
>      int ret;
>      int i;
>  
> +    if (s->internal->id3v2_meta)
> +        s->metadata = s->internal->id3v2_meta;

Slightly odd that it checks for id3v2_meta being NULL. Seems redundant?

> +
> +    s->internal->id3v2_meta = NULL;
> +
>      st = avformat_new_stream(s, NULL);
>      if (!st)
>          return AVERROR(ENOMEM);
> diff --git a/libavformat/options.c b/libavformat/options.c
> index 25a506eef8..d8aca472c5 100644
> --- a/libavformat/options.c
> +++ b/libavformat/options.c
> @@ -144,6 +144,7 @@ AVFormatContext *avformat_alloc_context(void)
>      ic->internal->offset = AV_NOPTS_VALUE;
>      ic->internal->raw_packet_buffer_remaining_size = RAW_PACKET_BUFFER_SIZE;
>      ic->internal->shortest_end = AV_NOPTS_VALUE;
> +    ic->internal->id3v2_meta = NULL;

Not necessary. All fields are initialized to 0 by default, because the
struct is allocated by av_mallocz().

>  
>      return ic;
>  }
> diff --git a/libavformat/utils.c b/libavformat/utils.c
> index d5dfca7dec..b44d688213 100644
> --- a/libavformat/utils.c
> +++ b/libavformat/utils.c
> @@ -588,12 +588,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, &s->internal->id3v2_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 = s->internal->id3v2_meta;
> +        s->internal->id3v2_meta = NULL;
> +    } else if (s->internal->id3v2_meta) {
> +        int level = AV_LOG_WARNING;
> +        if (s->error_recognition & AV_EF_COMPLIANT)
> +            level = AV_LOG_ERROR;
> +        av_log(s, level, "Discarding ID3 tags because more suitable tags were found.\n");
> +        av_dict_free(&s->internal->id3v2_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 +640,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(&s->internal->id3v2_meta);
>      if (s->pb && !(s->flags & AVFMT_FLAG_CUSTOM_IO))
>          avio_closep(&s->pb);
>      avformat_free_context(s);

You could free the tag in avformat_free_context(), which might be
slightly simpler (don't need to care about success vs. error path), but
it doesn't actually matter.
diff mbox

Patch

From 227d7d1f4b5665f824900cf2b14863621180dd1c Mon Sep 17 00:00:00 2001
From: Polochon-street <polochonstreet@gmx.fr>
Date: Fri, 27 Jan 2017 16:49:41 +0100
Subject: [PATCH] Ignore ID3 tags if other tags are present (e.g. vorbis)
Originally-by: Ben Boeckel <mathstuf@gmail.com>
---
 libavformat/internal.h |  5 +++++
 libavformat/mp3dec.c   |  5 +++++
 libavformat/options.c  |  1 +
 libavformat/utils.c    | 16 +++++++++++++++-
 4 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/libavformat/internal.h b/libavformat/internal.h
index 9d614fb12c..63a1724cfa 100644
--- a/libavformat/internal.h
+++ b/libavformat/internal.h
@@ -140,6 +140,11 @@  struct AVFormatInternal {
      * Whether or not avformat_init_output fully initialized streams
      */
     int streams_initialized;
+
+    /**
+     * ID3v2 tag useful for MP3 demuxing
+     */
+    AVDictionary *id3v2_meta;
 };
 
 struct AVStreamInternal {
diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
index 099ca57d24..8540e45e4f 100644
--- a/libavformat/mp3dec.c
+++ b/libavformat/mp3dec.c
@@ -349,6 +349,11 @@  static int mp3_read_header(AVFormatContext *s)
     int ret;
     int i;
 
+    if (s->internal->id3v2_meta)
+        s->metadata = s->internal->id3v2_meta;
+
+    s->internal->id3v2_meta = NULL;
+
     st = avformat_new_stream(s, NULL);
     if (!st)
         return AVERROR(ENOMEM);
diff --git a/libavformat/options.c b/libavformat/options.c
index 25a506eef8..d8aca472c5 100644
--- a/libavformat/options.c
+++ b/libavformat/options.c
@@ -144,6 +144,7 @@  AVFormatContext *avformat_alloc_context(void)
     ic->internal->offset = AV_NOPTS_VALUE;
     ic->internal->raw_packet_buffer_remaining_size = RAW_PACKET_BUFFER_SIZE;
     ic->internal->shortest_end = AV_NOPTS_VALUE;
+    ic->internal->id3v2_meta = NULL;
 
     return ic;
 }
diff --git a/libavformat/utils.c b/libavformat/utils.c
index d5dfca7dec..b44d688213 100644
--- a/libavformat/utils.c
+++ b/libavformat/utils.c
@@ -588,12 +588,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, &s->internal->id3v2_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 = s->internal->id3v2_meta;
+        s->internal->id3v2_meta = NULL;
+    } else if (s->internal->id3v2_meta) {
+        int level = AV_LOG_WARNING;
+        if (s->error_recognition & AV_EF_COMPLIANT)
+            level = AV_LOG_ERROR;
+        av_log(s, level, "Discarding ID3 tags because more suitable tags were found.\n");
+        av_dict_free(&s->internal->id3v2_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 +640,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(&s->internal->id3v2_meta);
     if (s->pb && !(s->flags & AVFMT_FLAG_CUSTOM_IO))
         avio_closep(&s->pb);
     avformat_free_context(s);
-- 
2.11.0