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

[Ridley Combs <rcombs@rcombs.me>]

> On Jun 20, 2022, at 17:15, Andreas Rheinhardt <andreas.rheinhardt@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

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 <mailto:ffmpeg-devel@ffmpeg.org>
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel <https://ffmpeg.org/mailman/listinfo/ffmpeg-devel>
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org <mailto: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".