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

2017-01-28 Thread Michael Niedermayer 
On Sat, Jan 28, 2017 at 06:55:15PM +0100, wm4 wrote:
> On Sat, 28 Jan 2017 17:28:29 +0100
> Paul Arzelier  wrote:
> 
> > Hopefuly, everything is okay right now :)
> 
> Patch is fine with me.

applied

thx

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In a rich man's house there is no place to spit but his face.
-- Diogenes of Sinope


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2017-01-28 Thread wm4 
On Sat, 28 Jan 2017 17:28:29 +0100
Paul Arzelier  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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2017-01-28 Thread Paul Arzelier 

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  wrote:


 From 227d7d1f4b5665f824900cf2b14863621180dd1c Mon Sep 17 00:00:00 2001
From: Polochon-street 
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 
---
  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, _extra_meta, 0);
+ff_id3v2_read_dict(s->pb, >internal->id3v2_meta, ID3v2_DEFAULT_MAGIC, 
_extra_meta);
  
  if (!(s->flags_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(>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(_extra_meta);
  av_dict_free();
+av_dict_free(>internal->id3v2_meta);
  if (s->pb && !(s->flags & AVFMT_FLAG_CUSTOM_IO))
  avio_closep(>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
>From 08de8e059393f4f6b412d482aa3e045f6a6b06c7 Mon Sep 17 00:00:00 2001
From: Paul Arzelier 
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 
---
 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

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

2017-01-27 Thread wm4 
On Fri, 27 Jan 2017 17:32:08 +0100
Paul Arzelier  wrote:

> From 227d7d1f4b5665f824900cf2b14863621180dd1c Mon Sep 17 00:00:00 2001
> From: Polochon-street 
> 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 
> ---
>  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, _extra_meta, 0);
> +ff_id3v2_read_dict(s->pb, >internal->id3v2_meta, 
> ID3v2_DEFAULT_MAGIC, _extra_meta);
>  
>  if (!(s->flags_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(>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(_extra_meta);
>  av_dict_free();
> +av_dict_free(>internal->id3v2_meta);
>  if (s->pb && !(s->flags & AVFMT_FLAG_CUSTOM_IO))
>  avio_closep(>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

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

2017-01-27 Thread Paul Arzelier 
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  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  wrote:
  

 From d84648e1990ad3a12462dfb76990dc7036f5f082 Mon Sep 17 00:00:00 2001
From: Polochon-street 
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 
---
  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, _extra_meta, 0);
+ff_id3v2_read_dict(s->pb, _meta, ID3v2_DEFAULT_MAGIC, 
_extra_meta);
  if (!(s->flags_FLAG_PRIV_OPT) && s->iformat->read_header)
  if ((ret = s->iformat->read_header(s)) < 0)
  goto fail;
+if (!s->metadata) {
+av_dict_copy(>metadata, id3_meta, AV_DICT_DONT_OVERWRITE);
+av_dict_free(_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(_extra_meta);
  av_dict_free();
+   av_dict_free(_meta);
  if (s->pb && !(s->flags & AVFMT_FLAG_CUSTOM_IO))
  avio_closep(>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 
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 
---
  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, _extra_meta, 0);
+ff_id3v2_read_dict(s->pb, _meta, ID3v2_DEFAULT_MAGIC, 
_extra_meta);
  
  if (!(s->flags_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) {

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

2017-01-26 Thread wm4 
On Fri, 27 Jan 2017 03:03:03 +0100
Michael Niedermayer  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  wrote:
> > >  
> > >> From d84648e1990ad3a12462dfb76990dc7036f5f082 Mon Sep 17 00:00:00 2001
> > >>From: Polochon-street 
> > >>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 
> > >>---
> > >>  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, _extra_meta, 0);
> > >>+ff_id3v2_read_dict(s->pb, _meta, ID3v2_DEFAULT_MAGIC, 
> > >>_extra_meta);
> > >>  if (!(s->flags_FLAG_PRIV_OPT) && s->iformat->read_header)
> > >>  if ((ret = s->iformat->read_header(s)) < 0)
> > >>  goto fail;
> > >>+if (!s->metadata) {
> > >>+av_dict_copy(>metadata, id3_meta, AV_DICT_DONT_OVERWRITE);
> > >>+av_dict_free(_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(_extra_meta);
> > >>  av_dict_free();
> > >>+   av_dict_free(_meta);
> > >>  if (s->pb && !(s->flags & AVFMT_FLAG_CUSTOM_IO))
> > >>  avio_closep(>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 
> > 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 
> > ---
> >  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, _extra_meta, 0);
> > +

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

2017-01-26 Thread Michael Niedermayer 
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  wrote:
> >
> >> From d84648e1990ad3a12462dfb76990dc7036f5f082 Mon Sep 17 00:00:00 2001
> >>From: Polochon-street 
> >>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 
> >>---
> >>  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, _extra_meta, 0);
> >>+ff_id3v2_read_dict(s->pb, _meta, ID3v2_DEFAULT_MAGIC, 
> >>_extra_meta);
> >>  if (!(s->flags_FLAG_PRIV_OPT) && s->iformat->read_header)
> >>  if ((ret = s->iformat->read_header(s)) < 0)
> >>  goto fail;
> >>+if (!s->metadata) {
> >>+av_dict_copy(>metadata, id3_meta, AV_DICT_DONT_OVERWRITE);
> >>+av_dict_free(_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(_extra_meta);
> >>  av_dict_free();
> >>+   av_dict_free(_meta);
> >>  if (s->pb && !(s->flags & AVFMT_FLAG_CUSTOM_IO))
> >>  avio_closep(>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 
> 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 
> ---
>  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, _extra_meta, 0);
> +ff_id3v2_read_dict(s->pb, _meta, ID3v2_DEFAULT_MAGIC, 
> _extra_meta);
>  
>  if (!(s->flags_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)

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

2017-01-26 Thread wm4 
On Thu, 26 Jan 2017 14:29:21 +0100
Paul Arzelier  wrote:

> From d84648e1990ad3a12462dfb76990dc7036f5f082 Mon Sep 17 00:00:00 2001
> From: Polochon-street 
> 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 
> ---
>  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, _extra_meta, 0);
> +ff_id3v2_read_dict(s->pb, _meta, ID3v2_DEFAULT_MAGIC, 
> _extra_meta);
>  
>  if (!(s->flags_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(_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(_extra_meta);
>  av_dict_free();
> +av_dict_free(_meta);
>  if (s->pb && !(s->flags & AVFMT_FLAG_CUSTOM_IO))
>  avio_closep(>pb);
>  avformat_free_context(s);

LGTM
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2017-01-26 Thread Paul Arzelier 

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  wrote:


 From d84648e1990ad3a12462dfb76990dc7036f5f082 Mon Sep 17 00:00:00 2001
From: Polochon-street 
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 
---
  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, _extra_meta, 0);
+ff_id3v2_read_dict(s->pb, _meta, ID3v2_DEFAULT_MAGIC, 
_extra_meta);
  
  if (!(s->flags_FLAG_PRIV_OPT) && s->iformat->read_header)

  if ((ret = s->iformat->read_header(s)) < 0)
  goto fail;
  
+if (!s->metadata) {

+av_dict_copy(>metadata, id3_meta, AV_DICT_DONT_OVERWRITE);
+av_dict_free(_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(_extra_meta);
  av_dict_free();
+   av_dict_free(_meta);
  if (s->pb && !(s->flags & AVFMT_FLAG_CUSTOM_IO))
  avio_closep(>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

>From d84648e1990ad3a12462dfb76990dc7036f5f082 Mon Sep 17 00:00:00 2001
From: Polochon-street 
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 
---
 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, _extra_meta, 0);
+ff_id3v2_read_dict(s->pb, _meta, ID3v2_DEFAULT_MAGIC, _extra_meta);
 
 if (!(s->flags_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(_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:

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

2017-01-26 Thread wm4 
On Thu, 26 Jan 2017 13:32:08 +0100
Paul Arzelier  wrote:

> From d84648e1990ad3a12462dfb76990dc7036f5f082 Mon Sep 17 00:00:00 2001
> From: Polochon-street 
> 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 
> ---
>  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, _extra_meta, 0);
> +ff_id3v2_read_dict(s->pb, _meta, ID3v2_DEFAULT_MAGIC, 
> _extra_meta);
>  
>  if (!(s->flags_FLAG_PRIV_OPT) && s->iformat->read_header)
>  if ((ret = s->iformat->read_header(s)) < 0)
>  goto fail;
>  
> +if (!s->metadata) {
> +av_dict_copy(>metadata, id3_meta, AV_DICT_DONT_OVERWRITE);

> +av_dict_free(_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(_extra_meta);
>  av_dict_free();
> +   av_dict_free(_meta);
>  if (s->pb && !(s->flags & AVFMT_FLAG_CUSTOM_IO))
>  avio_closep(>pb);
>  avformat_free_context(s);

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2017-01-26 Thread Paul Arzelier 

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  wrote:


 From a3dc6068fb06722aacea56365f948afdb8df841f Mon Sep 17 00:00:00 2001
From: Paul Arzelier 
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 
---
  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, _extra_meta, 0);
+ff_id3v2_read_dict(s->pb, _meta, ID3v2_DEFAULT_MAGIC, 
_extra_meta);
  
  if (!(s->flags_FLAG_PRIV_OPT) && s->iformat->read_header)

  if ((ret = s->iformat->read_header(s)) < 0)
  goto fail;
  
+if (!s->metadata) {

+av_dict_copy(>metadata, id3_meta, AV_DICT_DONT_OVERWRITE);
+av_dict_free(_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


--
Paul ARZELIER
Élève ingénieur à l'École Centrale de Lille
2014-2017

>From d84648e1990ad3a12462dfb76990dc7036f5f082 Mon Sep 17 00:00:00 2001
From: Polochon-street 
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 
---
 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, _extra_meta, 0);
+ff_id3v2_read_dict(s->pb, _meta, ID3v2_DEFAULT_MAGIC, _extra_meta);
 
 if (!(s->flags_FLAG_PRIV_OPT) && s->iformat->read_header)
 if ((ret = s->iformat->read_header(s)) < 0)
 goto fail;
 
+if (!s->metadata) {
+av_dict_copy(>metadata, id3_meta, AV_DICT_DONT_OVERWRITE);
+av_dict_free(_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, "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(_extra_meta);
 av_dict_free();
+

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

2017-01-26 Thread wm4 
On Thu, 26 Jan 2017 12:55:15 +0100
Paul Arzelier  wrote:

> From a3dc6068fb06722aacea56365f948afdb8df841f Mon Sep 17 00:00:00 2001
> From: Paul Arzelier 
> 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 
> ---
>  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, _extra_meta, 0);
> +ff_id3v2_read_dict(s->pb, _meta, ID3v2_DEFAULT_MAGIC, 
> _extra_meta);
>  
>  if (!(s->flags_FLAG_PRIV_OPT) && s->iformat->read_header)
>  if ((ret = s->iformat->read_header(s)) < 0)
>  goto fail;
>  
> +if (!s->metadata) {
> +av_dict_copy(>metadata, id3_meta, AV_DICT_DONT_OVERWRITE);
> +av_dict_free(_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

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

2017-01-26 Thread Paul Arzelier 

You're right, I made a patch for libavformat/utils.c instead.

I modified a bit Ben's version and kept only the "spec-compliant flac 
files do not support ID3 tags" warning, I hope it's okay with you.


Paul

Le 26/01/2017 à 11:24, wm4 a écrit :

On Thu, 26 Jan 2017 11:08:45 +0100
Paul Arzelier  wrote:


 From bb5dc73c42ad2e0bffd7e38fac18c6f01ec05ab3 Mon Sep 17 00:00:00 2001
From: Paul Arzelier 
Date: Thu, 26 Jan 2017 11:04:44 +0100
Subject: [PATCH] Fixed behavior when id3 tags were found on FLAC files
Originally-by: Ben Boeckel 
---
  libavformat/flacdec.c | 30 ++
  1 file changed, 30 insertions(+)

diff --git a/libavformat/flacdec.c b/libavformat/flacdec.c
index 66baba5922..869936621b 100644
--- a/libavformat/flacdec.c
+++ b/libavformat/flacdec.c
@@ -48,6 +48,7 @@ static int flac_read_header(AVFormatContext *s)
  int ret, metadata_last=0, metadata_type, metadata_size, 
found_streaminfo=0;
  uint8_t header[4];
  uint8_t *buffer=NULL;
+int has_id3 = 0;
  FLACDecContext *flac = s->priv_data;
  AVStream *st = avformat_new_stream(s, NULL);
  if (!st)
@@ -63,6 +64,22 @@ static int flac_read_header(AVFormatContext *s)
  return 0;
  }
  
+if (av_dict_count(s->metadata)) {

+ /* XXX: Is there a better way to parse this out? ID3 parsing is done
+ * all the way out in avformat_open_input. */
+ has_id3 = 1;
+}
+
+if (has_id3) {
+ 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");
+ if (s->error_recognition & AV_EF_EXPLODE)
+  return AVERROR_INVALIDDATA;
+}
+
+
  /* process metadata blocks */
  while (!avio_feof(s->pb) && !metadata_last) {
  if (avio_read(s->pb, header, 4) != 4)
@@ -173,8 +190,16 @@ static int flac_read_header(AVFormatContext *s)
  }
  /* process supported blocks other than STREAMINFO */
  if (metadata_type == FLAC_METADATA_TYPE_VORBIS_COMMENT) {
+AVDictionary *other_meta = NULL;
  AVDictionaryEntry *chmask;
  
+if (has_id3) {

+av_log(s, AV_LOG_VERBOSE, "FLAC found with ID3 and vorbis tags; 
ignoring ID3 tags also provided by vorbis.\n");
+
+other_meta = s->metadata;
+s->metadata = NULL;
+}
+
  ret = ff_vorbis_comment(s, >metadata, buffer, 
metadata_size, 1);
  if (ret < 0) {
  av_log(s, AV_LOG_WARNING, "error parsing VorbisComment 
metadata\n");
@@ -182,6 +207,11 @@ static int flac_read_header(AVFormatContext *s)
  s->event_flags |= AVFMT_EVENT_FLAG_METADATA_UPDATED;
  }
  
+if (other_meta) {

+av_dict_copy(>metadata, other_meta, 
AV_DICT_DONT_OVERWRITE);
+av_dict_free(_meta);
+}
+
  /* parse the channels mask if present */
  chmask = av_dict_get(s->metadata, 
"WAVEFORMATEXTENSIBLE_CHANNEL_MASK", NULL, 0);
  if (chmask) {

I feel like there should be a more general solution.

ID3v2 tags are read by common code, and such tags are supported by
basically all file formats in libavformat due to the way it's
implemented.

This fixes it for FLAC only. Are we going to duplicate this for every
format?

I suggest reading id3 tags into a separate dict instead, and then
copying it in libavformat/utils.c after read_header if the demuxer
hasn't set any tags.
___
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

>From a3dc6068fb06722aacea56365f948afdb8df841f Mon Sep 17 00:00:00 2001
From: Paul Arzelier 
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 
---
 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;
 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,

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

2017-01-26 Thread wm4 
On Thu, 26 Jan 2017 11:08:45 +0100
Paul Arzelier  wrote:

> From bb5dc73c42ad2e0bffd7e38fac18c6f01ec05ab3 Mon Sep 17 00:00:00 2001
> From: Paul Arzelier 
> Date: Thu, 26 Jan 2017 11:04:44 +0100
> Subject: [PATCH] Fixed behavior when id3 tags were found on FLAC files
> Originally-by: Ben Boeckel 
> ---
>  libavformat/flacdec.c | 30 ++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/libavformat/flacdec.c b/libavformat/flacdec.c
> index 66baba5922..869936621b 100644
> --- a/libavformat/flacdec.c
> +++ b/libavformat/flacdec.c
> @@ -48,6 +48,7 @@ static int flac_read_header(AVFormatContext *s)
>  int ret, metadata_last=0, metadata_type, metadata_size, 
> found_streaminfo=0;
>  uint8_t header[4];
>  uint8_t *buffer=NULL;
> +int has_id3 = 0;
>  FLACDecContext *flac = s->priv_data;
>  AVStream *st = avformat_new_stream(s, NULL);
>  if (!st)
> @@ -63,6 +64,22 @@ static int flac_read_header(AVFormatContext *s)
>  return 0;
>  }
>  
> +if (av_dict_count(s->metadata)) {
> + /* XXX: Is there a better way to parse this out? ID3 parsing is done
> + * all the way out in avformat_open_input. */
> + has_id3 = 1;
> +}
> +
> +if (has_id3) {
> + 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");
> + if (s->error_recognition & AV_EF_EXPLODE)
> +  return AVERROR_INVALIDDATA;
> +}
> +
> +
>  /* process metadata blocks */
>  while (!avio_feof(s->pb) && !metadata_last) {
>  if (avio_read(s->pb, header, 4) != 4)
> @@ -173,8 +190,16 @@ static int flac_read_header(AVFormatContext *s)
>  }
>  /* process supported blocks other than STREAMINFO */
>  if (metadata_type == FLAC_METADATA_TYPE_VORBIS_COMMENT) {
> +AVDictionary *other_meta = NULL;
>  AVDictionaryEntry *chmask;
>  
> +if (has_id3) {
> +av_log(s, AV_LOG_VERBOSE, "FLAC found with ID3 and 
> vorbis tags; ignoring ID3 tags also provided by vorbis.\n");
> +
> +other_meta = s->metadata;
> +s->metadata = NULL;
> +}
> +
>  ret = ff_vorbis_comment(s, >metadata, buffer, 
> metadata_size, 1);
>  if (ret < 0) {
>  av_log(s, AV_LOG_WARNING, "error parsing VorbisComment 
> metadata\n");
> @@ -182,6 +207,11 @@ static int flac_read_header(AVFormatContext *s)
>  s->event_flags |= AVFMT_EVENT_FLAG_METADATA_UPDATED;
>  }
>  
> +if (other_meta) {
> +av_dict_copy(>metadata, other_meta, 
> AV_DICT_DONT_OVERWRITE);
> +av_dict_free(_meta);
> +}
> +
>  /* parse the channels mask if present */
>  chmask = av_dict_get(s->metadata, 
> "WAVEFORMATEXTENSIBLE_CHANNEL_MASK", NULL, 0);
>  if (chmask) {

I feel like there should be a more general solution.

ID3v2 tags are read by common code, and such tags are supported by
basically all file formats in libavformat due to the way it's
implemented.

This fixes it for FLAC only. Are we going to duplicate this for every
format?

I suggest reading id3 tags into a separate dict instead, and then
copying it in libavformat/utils.c after read_header if the demuxer
hasn't set any tags.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2017-01-26 Thread Paul Arzelier 

Oops, I don't know what went wrong, but now it seems to work.

I've attached the working patched, sorry!


Le 26/01/2017 à 02:12, Michael Niedermayer a écrit :

On Wed, Jan 25, 2017 at 08:33:53PM +0100, Paul Arzelier wrote:

Hi all,

Would it be possible to continue the discussion began there ?
http://ffmpeg.org/pipermail/ffmpeg-devel/2015-February/168248.html

The current behavior of FFmpeg when it comes to manage FLAC tags with
both vorbis tags and ID3 tags is to append both, which leads to, for
example, "Artist: Pink Floyd;Pink Floyd" in FFprobe. This is not a
desired output: as the XIPH faq says about FLAC
(https://xiph.org/flac/faq.html):
"FLAC has it's own native tagging system which is identical to that of
Vorbis. They are called alternately "FLAC tags" and "Vorbis comments".
It is the only tagging system required and guaranteed to be supported by
FLAC implementations.
Out of convenience, the reference decoder knows how to skip ID3 tags so
that they don't interfere with decoding. But you should not expect any
tags beside FLAC tags to be supported in applications; some
implementations may not even be able to decode a FLAC file with ID3 tags."

This issue was also submitted here https://trac.ffmpeg.org/ticket/3799,
but still discuted in the above thread.
Ben Boeckel made the attached patch (which I edited a bit to fit FFmpeg
last version), which fixes the issue.
The problem that remained was that many other files could have
irrelevant id3v2 tags, not only Vorbis/FLAC files, so another more
global patch may be needed to fix that definitely.

So, do you think Ben's patch could be merged "as-is", or does it only
raise more questions?

Regards,
Paul


the attached patch is corrupted:

Applying: Fixed behavior when id3 tags were found on FLAC files
error: corrupt patch at line 58
Patch failed at 0001 Fixed behavior when id3 tags were found on FLAC files
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".


[...]



___
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

>From bb5dc73c42ad2e0bffd7e38fac18c6f01ec05ab3 Mon Sep 17 00:00:00 2001
From: Paul Arzelier 
Date: Thu, 26 Jan 2017 11:04:44 +0100
Subject: [PATCH] Fixed behavior when id3 tags were found on FLAC files
Originally-by: Ben Boeckel 
---
 libavformat/flacdec.c | 30 ++
 1 file changed, 30 insertions(+)

diff --git a/libavformat/flacdec.c b/libavformat/flacdec.c
index 66baba5922..869936621b 100644
--- a/libavformat/flacdec.c
+++ b/libavformat/flacdec.c
@@ -48,6 +48,7 @@ static int flac_read_header(AVFormatContext *s)
 int ret, metadata_last=0, metadata_type, metadata_size, found_streaminfo=0;
 uint8_t header[4];
 uint8_t *buffer=NULL;
+int has_id3 = 0;
 FLACDecContext *flac = s->priv_data;
 AVStream *st = avformat_new_stream(s, NULL);
 if (!st)
@@ -63,6 +64,22 @@ static int flac_read_header(AVFormatContext *s)
 return 0;
 }
 
+if (av_dict_count(s->metadata)) {
+ /* XXX: Is there a better way to parse this out? ID3 parsing is done
+ * all the way out in avformat_open_input. */
+ has_id3 = 1;
+}
+
+if (has_id3) {
+ 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");
+ if (s->error_recognition & AV_EF_EXPLODE)
+  return AVERROR_INVALIDDATA;
+}
+
+
 /* process metadata blocks */
 while (!avio_feof(s->pb) && !metadata_last) {
 if (avio_read(s->pb, header, 4) != 4)
@@ -173,8 +190,16 @@ static int flac_read_header(AVFormatContext *s)
 }
 /* process supported blocks other than STREAMINFO */
 if (metadata_type == FLAC_METADATA_TYPE_VORBIS_COMMENT) {
+AVDictionary *other_meta = NULL;
 AVDictionaryEntry *chmask;
 
+if (has_id3) {
+av_log(s, AV_LOG_VERBOSE, "FLAC found with ID3 and vorbis tags; ignoring ID3 tags also provided by vorbis.\n");
+
+other_meta = s->metadata;
+s->metadata = NULL;
+}
+
 ret = ff_vorbis_comment(s, >metadata, buffer, metadata_size, 1);
 if (ret < 0) {
 av_log(s, AV_LOG_WARNING, "error parsing VorbisComment metadata\n");
@@ -182,6 +207,11 @@ static int flac_read_header(AVFormatContext *s)
 s->event_flags |= AVFMT_EVENT_FLAG_METADATA_UPDATED;
 }
 
+if

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

2017-01-25 Thread Michael Niedermayer 
On Wed, Jan 25, 2017 at 08:33:53PM +0100, Paul Arzelier wrote:
> Hi all,
> 
> Would it be possible to continue the discussion began there ?
> http://ffmpeg.org/pipermail/ffmpeg-devel/2015-February/168248.html
> 
> The current behavior of FFmpeg when it comes to manage FLAC tags with
> both vorbis tags and ID3 tags is to append both, which leads to, for
> example, "Artist: Pink Floyd;Pink Floyd" in FFprobe. This is not a
> desired output: as the XIPH faq says about FLAC
> (https://xiph.org/flac/faq.html):
> "FLAC has it's own native tagging system which is identical to that of
> Vorbis. They are called alternately "FLAC tags" and "Vorbis comments".
> It is the only tagging system required and guaranteed to be supported by
> FLAC implementations.
> Out of convenience, the reference decoder knows how to skip ID3 tags so
> that they don't interfere with decoding. But you should not expect any
> tags beside FLAC tags to be supported in applications; some
> implementations may not even be able to decode a FLAC file with ID3 tags."
> 
> This issue was also submitted here https://trac.ffmpeg.org/ticket/3799,
> but still discuted in the above thread.
> Ben Boeckel made the attached patch (which I edited a bit to fit FFmpeg
> last version), which fixes the issue.
> The problem that remained was that many other files could have
> irrelevant id3v2 tags, not only Vorbis/FLAC files, so another more
> global patch may be needed to fix that definitely.
> 
> So, do you think Ben's patch could be merged "as-is", or does it only
> raise more questions?
> 
> Regards,
> Paul
> 

the attached patch is corrupted:

Applying: Fixed behavior when id3 tags were found on FLAC files
error: corrupt patch at line 58
Patch failed at 0001 Fixed behavior when id3 tags were found on FLAC files
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".


[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Good people do not need laws to tell them to act responsibly, while bad
people will find a way around the laws. -- Plato


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel