From patchwork Fri Jan 27 16:32:08 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Paul Arzelier X-Patchwork-Id: 2336 Delivered-To: ffmpegpatchwork@gmail.com Received: by 10.103.89.21 with SMTP id n21csp324124vsb; Fri, 27 Jan 2017 08:32:00 -0800 (PST) X-Received: by 10.28.166.215 with SMTP id p206mr4119777wme.87.1485534720119; Fri, 27 Jan 2017 08:32:00 -0800 (PST) Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org. [79.124.17.100]) by mx.google.com with ESMTP id v13si6514658wrv.32.2017.01.27.08.31.59; Fri, 27 Jan 2017 08:32:00 -0800 (PST) Received-SPF: pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) client-ip=79.124.17.100; Authentication-Results: mx.google.com; spf=pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) smtp.mailfrom=ffmpeg-devel-bounces@ffmpeg.org Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 2E99F68A8A8; Fri, 27 Jan 2017 18:31:55 +0200 (EET) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from smtp5-g21.free.fr (smtp5-g21.free.fr [212.27.42.5]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 9B66F68A83B for ; Fri, 27 Jan 2017 18:31:48 +0200 (EET) Received: from [192.168.1.47] (unknown [93.163.38.190]) (Authenticated sender: paul.arzelier) by smtp5-g21.free.fr (Postfix) with ESMTPSA id BC3F660035 for ; Fri, 27 Jan 2017 17:31:47 +0100 (CET) To: ffmpeg-devel@ffmpeg.org References: <76132ba8-af6a-ce81-87ce-5e4703103425@free.fr> <9f5d1b0d-0336-4a56-e7d6-baea3688ff04@free.fr> <20170126011234.GJ4698@nb4> <20170126112457.0ed024e9@debian> <1f383fee-feb5-c43d-ef0d-46bf1cb227fe@free.fr> <20170126131047.5169000e@debian> <20170126134326.2b997bee@debian> <6f6bf9cf-145e-a087-b633-79baa096ed3c@free.fr> <20170127020303.GW4698@nb4> <20170127070222.58a42b32@debian> From: Paul Arzelier Message-ID: Date: Fri, 27 Jan 2017 17:32:08 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.6.0 MIME-Version: 1.0 In-Reply-To: <20170127070222.58a42b32@debian> X-Content-Filtered-By: Mailman/MimeDel 2.1.20 Subject: Re: [FFmpeg-devel] [PATCH] Ignore duplicate ID3 tags if vorbis tags exist X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" 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, &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 >>> 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, &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 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(-) 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