From patchwork Thu Jan 26 10:08:45 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Paul Arzelier X-Patchwork-Id: 2324 Delivered-To: ffmpegpatchwork@gmail.com Received: by 10.103.89.21 with SMTP id n21csp104253vsb; Thu, 26 Jan 2017 02:08:38 -0800 (PST) X-Received: by 10.28.97.2 with SMTP id v2mr28170602wmb.3.1485425318083; Thu, 26 Jan 2017 02:08:38 -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 d4si1368784wrc.156.2017.01.26.02.08.37; Thu, 26 Jan 2017 02:08:38 -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 2C5C068A7F3; Thu, 26 Jan 2017 12:08:34 +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 32F2E68A788 for ; Thu, 26 Jan 2017 12:08:28 +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 79E1A5FFB6 for ; Thu, 26 Jan 2017 11:08:27 +0100 (CET) To: FFmpeg development discussions and patches References: <76132ba8-af6a-ce81-87ce-5e4703103425@free.fr> <9f5d1b0d-0336-4a56-e7d6-baea3688ff04@free.fr> <20170126011234.GJ4698@nb4> From: Paul Arzelier Message-ID: Date: Thu, 26 Jan 2017 11:08:45 +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: <20170126011234.GJ4698@nb4> 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" 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 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, &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(&s->metadata, other_meta, AV_DICT_DONT_OVERWRITE); + av_dict_free(&other_meta); + } + /* parse the channels mask if present */ chmask = av_dict_get(s->metadata, "WAVEFORMATEXTENSIBLE_CHANNEL_MASK", NULL, 0); if (chmask) { -- 2.11.0