Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
 help / color / mirror / Atom feed
* [FFmpeg-devel] [PATCH 1/2] lavf/metadata: support duplicate keys in ff_metadata_conv
@ 2022-06-20  1:37 rcombs
  2022-06-20  1:37 ` [FFmpeg-devel] [PATCH 2/2] lavf/id3v2dec: support multiple values and TIPL frames rcombs
  0 siblings, 1 reply; 6+ messages in thread
From: rcombs @ 2022-06-20  1:37 UTC (permalink / raw)
  To: ffmpeg-devel

---
 libavformat/metadata.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavformat/metadata.c b/libavformat/metadata.c
index b9b6de7972..d5c1800079 100644
--- a/libavformat/metadata.c
+++ b/libavformat/metadata.c
@@ -50,7 +50,7 @@ void ff_metadata_conv(AVDictionary **pm, const AVMetadataConv *d_conv,
                     key = dc->native;
                     break;
                 }
-        av_dict_set(&dst, key, mtag->value, 0);
+        av_dict_set(&dst, key, mtag->value, AV_DICT_MULTIKEY);
     }
     av_dict_free(pm);
     *pm = dst;
-- 
2.36.1

_______________________________________________
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".

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [FFmpeg-devel] [PATCH 2/2] lavf/id3v2dec: support multiple values and TIPL frames
  2022-06-20  1:37 [FFmpeg-devel] [PATCH 1/2] lavf/metadata: support duplicate keys in ff_metadata_conv rcombs
@ 2022-06-20  1:37 ` rcombs
  2022-06-20 22:15   ` Andreas Rheinhardt
  2022-06-21  0:39   ` Michael Niedermayer
  0 siblings, 2 replies; 6+ messages in thread
From: rcombs @ 2022-06-20  1:37 UTC (permalink / raw)
  To: ffmpeg-devel

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))
-- 
2.36.1

_______________________________________________
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".

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [FFmpeg-devel] [PATCH 2/2] lavf/id3v2dec: support multiple values and TIPL frames
  2022-06-20  1:37 ` [FFmpeg-devel] [PATCH 2/2] lavf/id3v2dec: support multiple values and TIPL frames rcombs
@ 2022-06-20 22:15   ` Andreas Rheinhardt
  2022-06-20 22:32     ` Ridley Combs
  2022-06-21  0:39   ` Michael Niedermayer
  1 sibling, 1 reply; 6+ messages in thread
From: Andreas Rheinhardt @ 2022-06-20 22:15 UTC (permalink / raw)
  To: ffmpeg-devel

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
_______________________________________________
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".

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [FFmpeg-devel] [PATCH 2/2] lavf/id3v2dec: support multiple values and TIPL frames
  2022-06-20 22:15   ` Andreas Rheinhardt
@ 2022-06-20 22:32     ` Ridley Combs
  2022-06-20 22:49       ` Soft Works
  0 siblings, 1 reply; 6+ messages in thread
From: Ridley Combs @ 2022-06-20 22:32 UTC (permalink / raw)
  To: ffmpeg-devel


> 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".

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [FFmpeg-devel] [PATCH 2/2] lavf/id3v2dec: support multiple values and TIPL frames
  2022-06-20 22:32     ` Ridley Combs
@ 2022-06-20 22:49       ` Soft Works
  0 siblings, 0 replies; 6+ messages in thread
From: Soft Works @ 2022-06-20 22:49 UTC (permalink / raw)
  To: FFmpeg development discussions and patches



> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Ridley Combs
> Sent: Tuesday, June 21, 2022 12:33 AM
> To: ffmpeg-devel <ffmpeg-devel@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@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

_______________________________________________
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".

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [FFmpeg-devel] [PATCH 2/2] lavf/id3v2dec: support multiple values and TIPL frames
  2022-06-20  1:37 ` [FFmpeg-devel] [PATCH 2/2] lavf/id3v2dec: support multiple values and TIPL frames rcombs
  2022-06-20 22:15   ` Andreas Rheinhardt
@ 2022-06-21  0:39   ` Michael Niedermayer
  1 sibling, 0 replies; 6+ messages in thread
From: Michael Niedermayer @ 2022-06-21  0:39 UTC (permalink / raw)
  To: FFmpeg development discussions and patches


[-- Attachment #1.1: Type: text/plain, Size: 854 bytes --]

On Sun, Jun 19, 2022 at 08:37:11PM -0500, rcombs wrote:
> 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(-)

It appears this looses a track # metadata on one file
i will send you the file privatly

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I am the wisest man alive, for I know one thing, and that is that I know
nothing. -- Socrates

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2022-06-21  0:39 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-20  1:37 [FFmpeg-devel] [PATCH 1/2] lavf/metadata: support duplicate keys in ff_metadata_conv rcombs
2022-06-20  1:37 ` [FFmpeg-devel] [PATCH 2/2] lavf/id3v2dec: support multiple values and TIPL frames rcombs
2022-06-20 22:15   ` Andreas Rheinhardt
2022-06-20 22:32     ` Ridley Combs
2022-06-20 22:49       ` Soft Works
2022-06-21  0:39   ` Michael Niedermayer

Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \
		ffmpegdev@gitmailbox.com
	public-inbox-index ffmpegdev

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git