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 <michae...@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.arzel...@free.fr> wrote:
From d84648e1990ad3a12462dfb76990dc7036f5f082 Mon Sep 17 00:00:00 2001
From: Polochon-street <polochonstr...@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 <maths...@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@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 <polochonstr...@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 <maths...@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.
_______________________________________________
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 227d7d1f4b5665f824900cf2b14863621180dd1c Mon Sep 17 00:00:00 2001
From: Polochon-street <polochonstr...@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 <maths...@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(-)
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..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;
+
+ 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;
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);
--
2.11.0
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
