[FFmpeg-devel] [PATCH 2/2] lavf/id3v2dec: support multiple values and TIPL frames

[Soft Works]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> Ridley Combs
> Sent: Tuesday, June 21, 2022 12:33 AM
> To: ffmpeg-devel <ffmpeg-devel at ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 2/2] lavf/id3v2dec: support
> multiple values and TIPL frames
> 
> 
> > On Jun 20, 2022, at 17:15, Andreas Rheinhardt
> <andreas.rheinhardt at outlook.com> 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

That's the way I would prefer.

Doing nothing for formal reasons is not a useful way. Most
softwares can read multivalue ID3v2 tags, ffmpeg/probe is more a 
kind of exception.

I think I should also mention that half part of this patch has
been submitted before

https://patchwork.ffmpeg.org/project/ffmpeg/patch/1118083116.39618.1617751350909@office.mailbox.org/

Best regards,
softworkz