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

Ah, missed the leaked, sorry.

Regarding the explode message, I kept it because of this message 
http://ffmpeg.org/pipermail/ffmpeg-devel/2015-February/168250.html . 
Maybe it is not that relevant anymore though, because we're talking 
about all existing codecs there, and not only FLAC (which seems to be a 
bit more strict when it comes down to tags).


Le 26/01/2017 à 13:10, wm4 a écrit :
> On Thu, 26 Jan 2017 12:55:15 +0100
> Paul Arzelier <paul.arzelier@free.fr> wrote:
>
>>  From a3dc6068fb06722aacea56365f948afdb8df841f Mon Sep 17 00:00:00 2001
>> From: Paul Arzelier <paul.arzelier@free.fr>
>> Date: Thu, 26 Jan 2017 12:51:33 +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 | 16 +++++++++++++++-
>>   1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavformat/utils.c b/libavformat/utils.c
>> index d5dfca7dec..bbe5c1ff1c 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;
> Looks like this would leak on "goto fail;".
>
>>       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);
>> +    }
>> +    else if (id3_meta) {
>> +        int level = AV_LOG_WARNING;
>> +        if (s->error_recognition & AV_EF_COMPLIANT)
>> +            level = AV_LOG_ERROR;
>> +        av_log(s, level, "Spec-compliant flac files do not support ID3 tags.\n");
> Seems ok, but this message will be printed for non-flac formats too,
> which is weird at best. Maybe something along the id3 tag being
> discarded or so?
>
>> +        if (s->error_recognition & AV_EF_EXPLODE)
>> +            return AVERROR_INVALIDDATA;
> Not sure about this one, but I'd be fine with it personally.
>
>> +    }
>> +
>>       if (id3v2_extra_meta) {
>>           if (!strcmp(s->iformat->name, "mp3") || !strcmp(s->iformat->name, "aac") ||
>>               !strcmp(s->iformat->name, "tta")) {
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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);