diff mbox

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

Message ID cfe32344-5f0c-df8e-1ebc-6554cd4cd46b@free.fr
State New
Headers show

Commit Message

Paul Arzelier Jan. 28, 2017, 4:28 p.m. UTC 
Hopefuly, everything is okay right now :)


Le 27/01/2017 à 17:42, wm4 a écrit :
> 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.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Comments

wm4 Jan. 28, 2017, 5:55 p.m. UTC | #1 
On Sat, 28 Jan 2017 17:28:29 +0100
Paul Arzelier <paul.arzelier@free.fr> wrote:

> Hopefuly, everything is okay right now :)

Patch is fine with me.

Bonus points for adding a fate tests for conflicting tags (I suppose
that would be a tiny FLAC file), and maybe one for mp3+ID3 tags. Of
course that would be a separate patch.
Michael Niedermayer Jan. 28, 2017, 9:33 p.m. UTC | #2 
On Sat, Jan 28, 2017 at 06:55:15PM +0100, wm4 wrote:
> On Sat, 28 Jan 2017 17:28:29 +0100
> Paul Arzelier <paul.arzelier@free.fr> wrote:
> 
> > Hopefuly, everything is okay right now :)
> 
> Patch is fine with me.

applied

thx

[...]
diff mbox

Patch

From 08de8e059393f4f6b412d482aa3e045f6a6b06c7 Mon Sep 17 00:00:00 2001
From: Paul Arzelier <paul.arzelier@free.fr>
Date: Sat, 28 Jan 2017 17:25:27 +0100
Subject: [PATCH] Ignore ID3v2 tags if other tags are present e.g. vorbis
Originally-by: Ben Boeckel <mathstuf@gmail.com>
---
 libavformat/internal.h |  5 +++++
 libavformat/mp3dec.c   |  3 +++
 libavformat/utils.c    | 17 ++++++++++++++++-
 3 files changed, 24 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..b45a066686 100644
--- a/libavformat/mp3dec.c
+++ b/libavformat/mp3dec.c
@@ -349,6 +349,9 @@  static int mp3_read_header(AVFormatContext *s)
     int ret;
     int i;
 
+    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/utils.c b/libavformat/utils.c
index d5dfca7dec..0711310792 100644
--- a/libavformat/utils.c
+++ b/libavformat/utils.c
@@ -588,12 +588,26 @@  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")) {
@@ -4178,6 +4192,7 @@  void avformat_free_context(AVFormatContext *s)
     }
     av_freep(&s->chapters);
     av_dict_free(&s->metadata);
+    av_dict_free(&s->internal->id3v2_meta);
     av_freep(&s->streams);
     av_freep(&s->internal);
     flush_packet_queue(s);
-- 
2.11.0