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

wm4 nfxjfg at googlemail.com
Fri Jan 27 08:02:22 EET 2017


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.


More information about the ffmpeg-devel mailing list