diff mbox

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

Message ID 9f5d1b0d-0336-4a56-e7d6-baea3688ff04@free.fr
State Superseded
Headers show

Commit Message

Paul Arzelier Jan. 25, 2017, 7:33 p.m. UTC
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

Comments

Michael Niedermayer Jan. 26, 2017, 1:12 a.m. UTC | #1
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".


[...]
diff mbox

Patch

From b10a32fecf05b17f6e824b662b7db07bff195d2d Mon Sep 17 00:00:00 2001
From: Paul Arzelier <paul.arzelier@free.fr>
Date: Wed, 25 Jan 2017 18:55:52 +0100
Subject: [PATCH] Fixed behavior when id3 tags were found on FLAC files
Originally-by: Ben Boeckel <mathstuf@gmail.com>

---
 libavformat/flacdec.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/libavformat/flacdec.c b/libavformat/flacdec.c
index 3060dc45fd..f8d4611398 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) {
         avio_read(s->pb, header, 4);
@@ -172,8 +189,17 @@  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");
@@ -181,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