From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTP id 27C3A43662 for ; Mon, 20 Jun 2022 22:33:05 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 03E9C68B5CD; Tue, 21 Jun 2022 01:33:03 +0300 (EEST) Received: from mail-qk1-f180.google.com (mail-qk1-f180.google.com [209.85.222.180]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 9A90A68B590 for ; Tue, 21 Jun 2022 01:32:56 +0300 (EEST) Received: by mail-qk1-f180.google.com with SMTP id o73so8826564qke.7 for ; Mon, 20 Jun 2022 15:32:56 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=rcombs.me; s=google; h=from:mime-version:subject:date:references:to:in-reply-to:message-id; bh=uGCbQ4ZdKd02FwZzf7VEtPDq81r3dz2cwojLWUwK6nU=; b=KLyE6nhg/aiDGT7zIjQ8dPURjs1T1WdtL/MYR4QCov1IbsGGdbP9JX8WK2FgNf8ucs Fcs4F2tFCLN91Obgs5ZJtL8TKDX4DnQCgdO7vntZXY/m3lmuppT0+maSlEIFce3vcNqx 7O7JwLlaSQHmZ3HMMrGxPbnYGHg7AXPJY8r8mnPrGR1CqDQ660Jzr42XSTxU32gesxMu hh4fiHopNnHbrRIoG/qolfPwEzSpFJgZMqqh9itRCUDlH2Pp6RgmDdqV9qjyt1lHvqzv TNIvSV0kYNyrd+bJbkcKzp7hyRI6o8u/vdAhUXd7UKSAcB8a4mow6KUTDVU+q70ZKCM/ vCwg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:mime-version:subject:date:references:to :in-reply-to:message-id; bh=uGCbQ4ZdKd02FwZzf7VEtPDq81r3dz2cwojLWUwK6nU=; b=Hd7sTSayzKLJADLHewuEWgxUdojG5clIOBceaC45J9HpkcxeIciKomRX09DHcmy0TE LcL/lX8lNBxExJHcav8OlqG3UQyz1AKhKUi/7I85wjKvHt7iISfPR6GGoUgg4bWF8rJu 6fNyo5AGQFN5F30ybAtp5VniwGw1+m0p3Vm0X4fPATVTK05OwhJLJmxhvVNRrdZyZz0+ HZoxt+Tr0nTTHRI47+sw279pi/6kJAI8Tk/FmBgcHR6R5h4uRwtoZ/dMTd3IekUw6pm5 Z00WJzra4LemS2lCZQOpX89Wzqsj84X+lPIpoMZdqlUx1bU3mQzUffk9xHE2ykaGrrHV Lfpw== X-Gm-Message-State: AJIora+tWT/Qfye6sOUm3iIxbcWj4L3PFio+GFMngJjoQtJlonjEnnwV 107l4JyjVnS20g5rcSUgOcheIBFLMcPf X-Google-Smtp-Source: AGRyM1tZJgnghUBkGGRjL0hqR3Uq3cdpQ927W5nAhTg5CSBpe9FKZ3jXsqaeIs0L/5lsDXS6RMFdUA== X-Received: by 2002:ae9:ed0a:0:b0:6a6:abb6:d511 with SMTP id c10-20020ae9ed0a000000b006a6abb6d511mr17596588qkg.248.1655764375117; Mon, 20 Jun 2022 15:32:55 -0700 (PDT) Received: from smtpclient.apple ([2601:243:2000:5ac:183b:4c6e:3bf3:753]) by smtp.gmail.com with ESMTPSA id x5-20020a05620a258500b006a75a0ffc97sm13672784qko.3.2022.06.20.15.32.54 for (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Mon, 20 Jun 2022 15:32:54 -0700 (PDT) From: Ridley Combs Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3696.100.31\)) Date: Mon, 20 Jun 2022 17:32:53 -0500 References: <20220620013711.91482-1-rcombs@rcombs.me> <20220620013711.91482-2-rcombs@rcombs.me> To: ffmpeg-devel In-Reply-To: Message-Id: <766EA680-FF62-45A2-853A-E7C7B646A8E6@rcombs.me> X-Mailer: Apple Mail (2.3696.100.31) X-Content-Filtered-By: Mailman/MimeDel 2.1.29 Subject: Re: [FFmpeg-devel] [PATCH 2/2] lavf/id3v2dec: support multiple values and TIPL frames X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 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 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: > On Jun 20, 2022, at 17:15, Andreas Rheinhardt wrote: > > rcombs: >> Fixes https://trac.ffmpeg.org/ticket/6949 >> >> Ordinary text frames in ID3v2 are allowed to have multiple >> (null-separated) values. This technically isn't allowed in TXXX, >> but it's used in practice by Picard, and supporting it is harmless. >> >> TIPL/IPL (Involved People List) and TMCL (Musician Credits List) work >> similarly to TXXX, but alternate key-value-key-value. >> --- >> libavformat/id3v2.c | 49 ++++++++++++++++++++++++++------------------- >> 1 file changed, 28 insertions(+), 21 deletions(-) >> >> diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c >> index 191a305ffb..667105e9f9 100644 >> --- a/libavformat/id3v2.c >> +++ b/libavformat/id3v2.c >> @@ -321,8 +321,12 @@ static void read_ttag(AVFormatContext *s, AVIOContext *pb, int taglen, >> AVDictionary **metadata, const char *key) >> { >> uint8_t *dst; >> - int encoding, dict_flags = AV_DICT_DONT_OVERWRITE | AV_DICT_DONT_STRDUP_VAL; >> + uint8_t *dst_key = NULL; >> + int encoding, dict_flags = AV_DICT_MULTIKEY | AV_DICT_DONT_STRDUP_VAL; >> unsigned genre; >> + int count = 0; >> + int is_tipl = !(strcmp(key, "TIPL") && strcmp(key, "TMCL") && >> + strcmp(key, "IPL")); >> >> if (taglen < 1) >> return; >> @@ -330,30 +334,33 @@ static void read_ttag(AVFormatContext *s, AVIOContext *pb, int taglen, >> encoding = avio_r8(pb); >> taglen--; /* account for encoding type byte */ >> >> - if (decode_str(s, pb, encoding, &dst, &taglen) < 0) { >> - av_log(s, AV_LOG_ERROR, "Error reading frame %s, skipped\n", key); >> - return; >> - } >> - >> - if (!(strcmp(key, "TCON") && strcmp(key, "TCO")) && >> - (sscanf(dst, "(%d)", &genre) == 1 || sscanf(dst, "%d", &genre) == 1) && >> - genre <= ID3v1_GENRE_MAX) { >> - av_freep(&dst); >> - dst = av_strdup(ff_id3v1_genre_str[genre]); >> - } else if (!(strcmp(key, "TXXX") && strcmp(key, "TXX"))) { >> - /* dst now contains the key, need to get value */ >> - key = dst; >> + while (taglen > 1) { >> if (decode_str(s, pb, encoding, &dst, &taglen) < 0) { >> av_log(s, AV_LOG_ERROR, "Error reading frame %s, skipped\n", key); >> - av_freep(&key); >> return; >> } >> - dict_flags |= AV_DICT_DONT_STRDUP_KEY; >> - } else if (!*dst) >> - av_freep(&dst); >> >> - if (dst) >> - av_dict_set(metadata, key, dst, dict_flags); >> + count++; >> + >> + if (!(strcmp(key, "TCON") && strcmp(key, "TCO")) && >> + (sscanf(dst, "(%d)", &genre) == 1 || sscanf(dst, "%d", &genre) == 1) && >> + genre <= ID3v1_GENRE_MAX) { >> + av_freep(&dst); >> + dst = av_strdup(ff_id3v1_genre_str[genre]); >> + } else if (!(strcmp(key, "TXXX") && strcmp(key, "TXX")) || >> + (is_tipl && (count & 1))) { >> + /* dst now contains the key, need to get value */ >> + av_free(dst_key); >> + key = dst_key = dst; >> + continue; >> + } else if (!*dst) >> + av_freep(&dst); >> + >> + if (dst) >> + av_dict_set(metadata, key, dst, dict_flags); >> + } >> + >> + av_free(dst_key); >> } >> >> static void read_uslt(AVFormatContext *s, AVIOContext *pb, int taglen, >> @@ -1039,7 +1046,7 @@ static void id3v2_parse(AVIOContext *pb, AVDictionary **metadata, >> pbx = &pb_local.pub; // read from sync buffer >> } >> #endif >> - if (tag[0] == 'T') >> + if (tag[0] == 'T' || !strcmp(tag, "IPL")) >> /* parse text tag */ >> read_ttag(s, pbx, tlen, metadata, tag); >> else if (!memcmp(tag, "USLT", 4)) > > From avformat.h: > > "Keys are unique; there can never be 2 tags with the same key. This is > also meant semantically, i.e., a demuxer should not knowingly produce > several keys that are literally different but semantically identical. > E.g., key=Author5, key=Author6. In this example, all authors must be > placed in the same tag." > > If this requirement did not exist, I would have fixed #6949 and #9641 > long ago. > > - Andreas Well, we have 3 options, then: - Export these kinds of duplicate tags concatenated in a single dict entry (split by slashes or something?) - Add an AVOption in lavf/options_table.h to opt into multi-key support - Remove the requirement altogether in a major API version bump I don't see any particular technical reason for this requirement (there are a couple implementation details that make it awkward but they're fixable in code); it seems like it's largely just there because historically there was no support for colliding keys. I'm not especially fond of this requirement, since it requires ad-hoc in-band signaling when multiple tags exist, it doesn't round-trip cleanly, and whatever separator we use for concatenation could collide with actual tag contents. Meanwhile, as to actual technical issues: I've been experimenting with this in FATE and run into some interesting snags; the most significant is handling cases of redundant metadata (i.e. id3v2 block and container both have a title tag). Currently, this just means that whichever one gets parsed second loses. In FATE, the two values are always identical, so I'm currently addressing this by just adding an AV_DICT_ flag to skip insertion if an identical tag (both key and value) already exists. > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".