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

[wm4]

On Fri, 27 Jan 2017 17:32:08 +0100
Paul Arzelier <paul.arzelier at free.fr> wrote:

> From 227d7d1f4b5665f824900cf2b14863621180dd1c Mon Sep 17 00:00:00 2001
> From: Polochon-street <polochonstreet at 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 at 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.