[FFmpeg-devel] [PATCH 1/2] avformat/id3v2: Print the unknown encoding

[FFmpeg-devel] [PATCH 1/2] avformat/id3v2: Print the unknown encoding

[Michael Niedermayer]

Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/id3v2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c
index 3bdd23a2509..90314583a74 100644
--- a/libavformat/id3v2.c
+++ b/libavformat/id3v2.c
@@ -304,7 +304,7 @@ static int decode_str(AVFormatContext *s, AVIOContext *pb, int encoding,
         }
         break;
     default:
-        av_log(s, AV_LOG_WARNING, "Unknown encoding\n");
+        av_log(s, AV_LOG_WARNING, "Unknown encoding %d\n", encoding);
     }
 
     if (ch)
-- 
2.49.0

_______________________________________________
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] [PATCH 2/2] avformat/id3v2: Check that decode_str() did advance

[Michael Niedermayer]

Fixes infinite loop with unknown encodings

We could alternatively error out from decode_str() or consume all of taglen
this would affect other callers though.

Fixes: 409819224/clusterfuzz-testcase-minimized-ffmpeg_dem_H261_fuzzer-6003527535362048
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/id3v2.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c
index 90314583a74..e3f7f9e2a90 100644
--- a/libavformat/id3v2.c
+++ b/libavformat/id3v2.c
@@ -341,10 +341,13 @@ static void read_ttag(AVFormatContext *s, AVIOContext *pb, int taglen,
     taglen--; /* account for encoding type byte */
 
     while (taglen > 1) {
+        int current_taglen = taglen;
         if (decode_str(s, pb, encoding, &dst, &taglen) < 0) {
             av_log(s, AV_LOG_ERROR, "Error reading frame %s, skipped\n", key);
             return;
         }
+        if (current_taglen == taglen)
+            return;
 
         count++;
 
-- 
2.49.0

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

Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that decode_str() did advance

[softworkz .]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Michael Niedermayer
> Sent: Samstag, 12. April 2025 00:27
> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> Subject: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
> decode_str() did advance
> 
> Fixes infinite loop with unknown encodings
> 
> We could alternatively error out from decode_str() or consume all of
> taglen
> this would affect other callers though.
> 
> Fixes: 409819224/clusterfuzz-testcase-minimized-ffmpeg_dem_H261_fuzzer-
> 6003527535362048
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavformat/id3v2.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c
> index 90314583a74..e3f7f9e2a90 100644
> --- a/libavformat/id3v2.c
> +++ b/libavformat/id3v2.c
> @@ -341,10 +341,13 @@ static void read_ttag(AVFormatContext *s,
> AVIOContext *pb, int taglen,
>      taglen--; /* account for encoding type byte */
> 
>      while (taglen > 1) {
> +        int current_taglen = taglen;
>          if (decode_str(s, pb, encoding, &dst, &taglen) < 0) {
>              av_log(s, AV_LOG_ERROR, "Error reading frame %s,
> skipped\n", key);
>              return;
>          }
> +        if (current_taglen == taglen)
> +            return;
> 
>          count++;
> 
> --
> 2.49.0
> 
> _______________________________________________

Hi Michael,

this kind of conflicts with this patch that I had submitted recently:

https://patchwork.ffmpeg.org/project/ffmpeg/patch/pull.54.ffstaging.FFmpeg.1740873449247.ffmpegagent@gmail.com/


I wonder whether my patch would still be prone to the issue your patch is addressing - do you have a test file perhaps?

Thanks
sw







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

Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that decode_str() did advance

[Michael Niedermayer]

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

On Sat, Apr 12, 2025 at 01:49:53AM +0000, softworkz . wrote:
> 
> 
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > Michael Niedermayer
> > Sent: Samstag, 12. April 2025 00:27
> > To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> > Subject: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
> > decode_str() did advance
> > 
> > Fixes infinite loop with unknown encodings
> > 
> > We could alternatively error out from decode_str() or consume all of
> > taglen
> > this would affect other callers though.
> > 
> > Fixes: 409819224/clusterfuzz-testcase-minimized-ffmpeg_dem_H261_fuzzer-
> > 6003527535362048
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavformat/id3v2.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c
> > index 90314583a74..e3f7f9e2a90 100644
> > --- a/libavformat/id3v2.c
> > +++ b/libavformat/id3v2.c
> > @@ -341,10 +341,13 @@ static void read_ttag(AVFormatContext *s,
> > AVIOContext *pb, int taglen,
> >      taglen--; /* account for encoding type byte */
> > 
> >      while (taglen > 1) {
> > +        int current_taglen = taglen;
> >          if (decode_str(s, pb, encoding, &dst, &taglen) < 0) {
> >              av_log(s, AV_LOG_ERROR, "Error reading frame %s,
> > skipped\n", key);
> >              return;
> >          }
> > +        if (current_taglen == taglen)
> > +            return;
> > 
> >          count++;
> > 
> > --
> > 2.49.0
> > 
> > _______________________________________________
> 
> Hi Michael,
> 
> this kind of conflicts with this patch that I had submitted recently:
> 
> https://patchwork.ffmpeg.org/project/ffmpeg/patch/pull.54.ffstaging.FFmpeg.1740873449247.ffmpegagent@gmail.com/
> 
> 
> I wonder whether my patch would still be prone to the issue your patch is addressing -

This already conflicts with rcombs patch in git master, i think
Applying: Fixes Trac ticket https://trac.ffmpeg.org/ticket/6949
Using index info to reconstruct a base tree...
M	libavformat/id3v2.c
Falling back to patching base and 3-way merge...
Auto-merging libavformat/id3v2.c
CONFLICT (content): Merge conflict in libavformat/id3v2.c
error: Failed to merge in the changes.
Patch failed at 0001 Fixes Trac ticket https://trac.ffmpeg.org/ticket/6949


> do you have a test file perhaps?

Will email you one, but the loop with a function that doesnt advance
is an issue even if the specific file doesnt trigger it in a different
implementation

also probaly a good idea if you contact rcombs as you seemed to work on
the same code

I was looking at teh ticket and saw a link to rcombs patch, looked at
the patch and applied it. I did not realize there where 2 patches

thx


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

The day soldiers stop bringing you their problems is the day you have stopped 
leading them. They have either lost confidence that you can help or concluded 
you do not care. Either case is a failure of leadership. - Colin Powell

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

Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that decode_str() did advance

[softworkz .]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Michael Niedermayer
> Sent: Dienstag, 15. April 2025 01:20
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
> decode_str() did advance
> 
> On Sat, Apr 12, 2025 at 01:49:53AM +0000, softworkz . wrote:
> >
> >
> > > -----Original Message-----
> > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > > Michael Niedermayer
> > > Sent: Samstag, 12. April 2025 00:27
> > > To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> > > Subject: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
> > > decode_str() did advance
> > >
> > > Fixes infinite loop with unknown encodings
> > >
> > > We could alternatively error out from decode_str() or consume all
> of
> > > taglen
> > > this would affect other callers though.
> > >
> > > Fixes: 409819224/clusterfuzz-testcase-minimized-
> ffmpeg_dem_H261_fuzzer-
> > > 6003527535362048
> > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > ---
> > >  libavformat/id3v2.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c
> > > index 90314583a74..e3f7f9e2a90 100644
> > > --- a/libavformat/id3v2.c
> > > +++ b/libavformat/id3v2.c
> > > @@ -341,10 +341,13 @@ static void read_ttag(AVFormatContext *s,
> > > AVIOContext *pb, int taglen,
> > >      taglen--; /* account for encoding type byte */
> > >
> > >      while (taglen > 1) {
> > > +        int current_taglen = taglen;
> > >          if (decode_str(s, pb, encoding, &dst, &taglen) < 0) {
> > >              av_log(s, AV_LOG_ERROR, "Error reading frame %s,
> > > skipped\n", key);
> > >              return;
> > >          }
> > > +        if (current_taglen == taglen)
> > > +            return;
> > >
> > >          count++;
> > >
> > > --
> > > 2.49.0
> > >
> > > _______________________________________________
> >
> > Hi Michael,
> >
> > this kind of conflicts with this patch that I had submitted
> recently:
> >
> >
> https://patchwork.ffmpeg.org/project/ffmpeg/patch/pull.54.ffstaging.FF
> mpeg.1740873449247.ffmpegagent@gmail.com/
> >
> >
> > I wonder whether my patch would still be prone to the issue your
> patch is addressing -
> 
> This already conflicts with rcombs patch in git master, i think
> Applying: Fixes Trac ticket https://trac.ffmpeg.org/ticket/6949
> Using index info to reconstruct a base tree...
> M	libavformat/id3v2.c
> Falling back to patching base and 3-way merge...
> Auto-merging libavformat/id3v2.c
> CONFLICT (content): Merge conflict in libavformat/id3v2.c
> error: Failed to merge in the changes.
> Patch failed at 0001 Fixes Trac ticket
> https://trac.ffmpeg.org/ticket/6949
> 
> 
> > do you have a test file perhaps?
> 
> Will email you one, but the loop with a function that doesnt advance
> is an issue even if the specific file doesnt trigger it in a different
> implementation
> 
> also probaly a good idea if you contact rcombs as you seemed to work
> on
> the same code
> 
> I was looking at teh ticket and saw a link to rcombs patch, looked at
> the patch and applied it. I did not realize there where 2 patches


Hi Michael,

I know the rcombs patch, but it has a - let's say - different behavior.
Let's look at an example where artist and genre have multiple values:


This was ffmpeg output unpatched:

  Metadata:
    title           : Infinite (Original Mix)
    artist          : B-Front
    track           : 1
    album           : Infinite
    date            : 2017
    genre           : Hardstyle
    TBPM            : 150
    compilation     : 0
    album_artist    : B-Front
    publisher       : Roughstate


This is what the rcombs patch does:

  Metadata:
    title           : Infinite (Original Mix)
    artist          : B-Front
    artist          : Second Artist Example
    track           : 1
    album           : Infinite
    date            : 2017
    genre           : Hardstyle
    genre           : Test
    genre           : Example
    genre           : Hard Dance
    TBPM            : 150
    compilation     : 0
    album_artist    : B-Front
    publisher       : Roughstate



My path does that:

  Metadata:
    title           : Infinite (Original Mix)
    artist          : B-Front;Second Artist Example
    track           : 1
    album           : Infinite
    date            : 2017
    genre           : Hardstyle;Test;Example;Hard Dance
    TBPM            : 150
    compilation     : 0
    album_artist    : B-Front
    publisher       : Roughstate



I'm not sure whether it is even allowed or intended that there are 
multiple metadata entries with the same key?

But in any case, what's happening with the rcombs patch is that 
when transcoding a file, the result looks like this:

  Metadata:
    title           : Infinite (Original Mix)
    artist          : B-Front
    track           : 1
    album           : Infinite
    date            : 2017
    genre           : Hardstyle
    TBPM            : 150
    compilation     : 0
    album_artist    : B-Front
    publisher       : Roughstate


Which means that the extra values all get lost. This doesn't happen
with the patch that I have submitted. Even though we should ideally
also modify the encoder to write the semicolon-delimited values with
null-separation, a semicolon is treated by most apps as a value 
delimiter as well - so it's a largely solid way and it avoids loss of
information when transcoding.

(which is why I had submitted the patch even though I knew about 
the one from rcombs)

Thanks
sw







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

Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that decode_str() did advance

[Ridley Combs via ffmpeg-devel]

> On Apr 15, 2025, at 08:59, softworkz . <softworkz-at-hotmail.com@ffmpeg.org> wrote:
> 
> 
> 
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org <mailto:ffmpeg-devel-bounces@ffmpeg.org>> On Behalf Of
>> Michael Niedermayer
>> Sent: Dienstag, 15. April 2025 01:20
>> To: FFmpeg development discussions and patches <ffmpeg-
>> devel@ffmpeg.org <mailto:devel@ffmpeg.org>>
>> Subject: Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
>> decode_str() did advance
>> 
>> On Sat, Apr 12, 2025 at 01:49:53AM +0000, softworkz . wrote:
>>> 
>>> 
>>>> -----Original Message-----
>>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
>>>> Michael Niedermayer
>>>> Sent: Samstag, 12. April 2025 00:27
>>>> To: FFmpeg development discussions and patches <ffmpeg-
>> devel@ffmpeg.org>
>>>> Subject: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
>>>> decode_str() did advance
>>>> 
>>>> Fixes infinite loop with unknown encodings
>>>> 
>>>> We could alternatively error out from decode_str() or consume all
>> of
>>>> taglen
>>>> this would affect other callers though.
>>>> 
>>>> Fixes: 409819224/clusterfuzz-testcase-minimized-
>> ffmpeg_dem_H261_fuzzer-
>>>> 6003527535362048
>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>>> ---
>>>> libavformat/id3v2.c | 3 +++
>>>> 1 file changed, 3 insertions(+)
>>>> 
>>>> diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c
>>>> index 90314583a74..e3f7f9e2a90 100644
>>>> --- a/libavformat/id3v2.c
>>>> +++ b/libavformat/id3v2.c
>>>> @@ -341,10 +341,13 @@ static void read_ttag(AVFormatContext *s,
>>>> AVIOContext *pb, int taglen,
>>>>     taglen--; /* account for encoding type byte */
>>>> 
>>>>     while (taglen > 1) {
>>>> +        int current_taglen = taglen;
>>>>         if (decode_str(s, pb, encoding, &dst, &taglen) < 0) {
>>>>             av_log(s, AV_LOG_ERROR, "Error reading frame %s,
>>>> skipped\n", key);
>>>>             return;
>>>>         }
>>>> +        if (current_taglen == taglen)
>>>> +            return;
>>>> 
>>>>         count++;
>>>> 
>>>> --
>>>> 2.49.0
>>>> 
>>>> _______________________________________________
>>> 
>>> Hi Michael,
>>> 
>>> this kind of conflicts with this patch that I had submitted
>> recently:
>>> 
>>> 
>> https://patchwork.ffmpeg.org/project/ffmpeg/patch/pull.54.ffstaging.FF
>> mpeg.1740873449247.ffmpegagent@gmail.com/
>>> 
>>> 
>>> I wonder whether my patch would still be prone to the issue your
>> patch is addressing -
>> 
>> This already conflicts with rcombs patch in git master, i think
>> Applying: Fixes Trac ticket https://trac.ffmpeg.org/ticket/6949
>> Using index info to reconstruct a base tree...
>> M	libavformat/id3v2.c
>> Falling back to patching base and 3-way merge...
>> Auto-merging libavformat/id3v2.c
>> CONFLICT (content): Merge conflict in libavformat/id3v2.c
>> error: Failed to merge in the changes.
>> Patch failed at 0001 Fixes Trac ticket
>> https://trac.ffmpeg.org/ticket/6949
>> 
>> 
>>> do you have a test file perhaps?
>> 
>> Will email you one, but the loop with a function that doesnt advance
>> is an issue even if the specific file doesnt trigger it in a different
>> implementation
>> 
>> also probaly a good idea if you contact rcombs as you seemed to work
>> on
>> the same code
>> 
>> I was looking at teh ticket and saw a link to rcombs patch, looked at
>> the patch and applied it. I did not realize there where 2 patches
> 
> 
> Hi Michael,
> 
> I know the rcombs patch, but it has a - let's say - different behavior.
> Let's look at an example where artist and genre have multiple values:
> 
> 
> This was ffmpeg output unpatched:
> 
>  Metadata:
>    title           : Infinite (Original Mix)
>    artist          : B-Front
>    track           : 1
>    album           : Infinite
>    date            : 2017
>    genre           : Hardstyle
>    TBPM            : 150
>    compilation     : 0
>    album_artist    : B-Front
>    publisher       : Roughstate
> 
> 
> This is what the rcombs patch does:
> 
>  Metadata:
>    title           : Infinite (Original Mix)
>    artist          : B-Front
>    artist          : Second Artist Example
>    track           : 1
>    album           : Infinite
>    date            : 2017
>    genre           : Hardstyle
>    genre           : Test
>    genre           : Example
>    genre           : Hard Dance
>    TBPM            : 150
>    compilation     : 0
>    album_artist    : B-Front
>    publisher       : Roughstate
> 
> 
> 
> My path does that:
> 
>  Metadata:
>    title           : Infinite (Original Mix)
>    artist          : B-Front;Second Artist Example
>    track           : 1
>    album           : Infinite
>    date            : 2017
>    genre           : Hardstyle;Test;Example;Hard Dance
>    TBPM            : 150
>    compilation     : 0
>    album_artist    : B-Front
>    publisher       : Roughstate
> 
> 
> 
> I'm not sure whether it is even allowed or intended that there are 
> multiple metadata entries with the same key?

It is indeed an intended feature of the AVDictionary system, and the metadata feature in particular.

> 
> But in any case, what's happening with the rcombs patch is that 
> when transcoding a file, the result looks like this:
> 
>  Metadata:
>    title           : Infinite (Original Mix)
>    artist          : B-Front
>    track           : 1
>    album           : Infinite
>    date            : 2017
>    genre           : Hardstyle
>    TBPM            : 150
>    compilation     : 0
>    album_artist    : B-Front
>    publisher       : Roughstate
> 
> 
> Which means that the extra values all get lost. This doesn't happen
> with the patch that I have submitted. Even though we should ideally
> also modify the encoder to write the semicolon-delimited values with
> null-separation, a semicolon is treated by most apps as a value 
> delimiter as well - so it's a largely solid way and it avoids loss of
> information when transcoding.

This is an issue that should be resolved by supporting multiple entries all the way through the relevant ffmpeg.c and muxer code paths, rather than adding new fragile usage of in-band signaling within metadata strings.

> 
> (which is why I had submitted the patch even though I knew about 
> the one from rcombs)
> 
> Thanks
> sw
> 
> 
> 
> 
> 
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org <mailto:ffmpeg-devel@ffmpeg.org>
> 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".

Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that decode_str() did advance

[softworkz .]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Ridley Combs via ffmpeg-devel
> Sent: Dienstag, 15. April 2025 02:03
> To: ffmpeg-devel <ffmpeg-devel@ffmpeg.org>
> Cc: Ridley Combs <rcombs@rcombs.me>
> Subject: Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
> decode_str() did advance
> 
> 
> 
> > On Apr 15, 2025, at 08:59, softworkz . <softworkz-at-
> hotmail.com@ffmpeg.org> wrote:
> >
> >
> >
> >> -----Original Message-----
> >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org <mailto:ffmpeg-
> devel-bounces@ffmpeg.org>> On Behalf Of
> >> Michael Niedermayer
> >> Sent: Dienstag, 15. April 2025 01:20
> >> To: FFmpeg development discussions and patches <ffmpeg-
> >> devel@ffmpeg.org <mailto:devel@ffmpeg.org>>
> >> Subject: Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
> >> decode_str() did advance
> >>
> >> On Sat, Apr 12, 2025 at 01:49:53AM +0000, softworkz . wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> >>>> Michael Niedermayer
> >>>> Sent: Samstag, 12. April 2025 00:27
> >>>> To: FFmpeg development discussions and patches <ffmpeg-
> >> devel@ffmpeg.org>
> >>>> Subject: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
> >>>> decode_str() did advance
> >>>>
> >>>> Fixes infinite loop with unknown encodings
> >>>>
> >>>> We could alternatively error out from decode_str() or consume all
> >> of
> >>>> taglen
> >>>> this would affect other callers though.
> >>>>
> >>>> Fixes: 409819224/clusterfuzz-testcase-minimized-
> >> ffmpeg_dem_H261_fuzzer-
> >>>> 6003527535362048
> >>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> >>>> ---
> >>>> libavformat/id3v2.c | 3 +++
> >>>> 1 file changed, 3 insertions(+)
> >>>>
> >>>> diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c
> >>>> index 90314583a74..e3f7f9e2a90 100644
> >>>> --- a/libavformat/id3v2.c
> >>>> +++ b/libavformat/id3v2.c
> >>>> @@ -341,10 +341,13 @@ static void read_ttag(AVFormatContext *s,
> >>>> AVIOContext *pb, int taglen,
> >>>>     taglen--; /* account for encoding type byte */
> >>>>
> >>>>     while (taglen > 1) {
> >>>> +        int current_taglen = taglen;
> >>>>         if (decode_str(s, pb, encoding, &dst, &taglen) < 0) {
> >>>>             av_log(s, AV_LOG_ERROR, "Error reading frame %s,
> >>>> skipped\n", key);
> >>>>             return;
> >>>>         }
> >>>> +        if (current_taglen == taglen)
> >>>> +            return;
> >>>>
> >>>>         count++;
> >>>>
> >>>> --
> >>>> 2.49.0
> >>>>
> >>>> _______________________________________________
> >>>
> >>> Hi Michael,
> >>>
> >>> this kind of conflicts with this patch that I had submitted
> >> recently:
> >>>
> >>>
> >>
> https://patchwork.ffmpeg.org/project/ffmpeg/patch/pull.54.ffstaging.FF
> >> mpeg.1740873449247.ffmpegagent@gmail.com/
> >>>
> >>>
> >>> I wonder whether my patch would still be prone to the issue your
> >> patch is addressing -
> >>
> >> This already conflicts with rcombs patch in git master, i think
> >> Applying: Fixes Trac ticket https://trac.ffmpeg.org/ticket/6949
> >> Using index info to reconstruct a base tree...
> >> M	libavformat/id3v2.c
> >> Falling back to patching base and 3-way merge...
> >> Auto-merging libavformat/id3v2.c
> >> CONFLICT (content): Merge conflict in libavformat/id3v2.c
> >> error: Failed to merge in the changes.
> >> Patch failed at 0001 Fixes Trac ticket
> >> https://trac.ffmpeg.org/ticket/6949
> >>
> >>
> >>> do you have a test file perhaps?
> >>
> >> Will email you one, but the loop with a function that doesnt
> advance
> >> is an issue even if the specific file doesnt trigger it in a
> different
> >> implementation
> >>
> >> also probaly a good idea if you contact rcombs as you seemed to
> work
> >> on
> >> the same code
> >>
> >> I was looking at teh ticket and saw a link to rcombs patch, looked
> at
> >> the patch and applied it. I did not realize there where 2 patches
> >
> >
> > Hi Michael,
> >
> > I know the rcombs patch, but it has a - let's say - different
> behavior.
> > Let's look at an example where artist and genre have multiple
> values:
> >
> >
> > This was ffmpeg output unpatched:
> >
> >  Metadata:
> >    title           : Infinite (Original Mix)
> >    artist          : B-Front
> >    track           : 1
> >    album           : Infinite
> >    date            : 2017
> >    genre           : Hardstyle
> >    TBPM            : 150
> >    compilation     : 0
> >    album_artist    : B-Front
> >    publisher       : Roughstate
> >
> >
> > This is what the rcombs patch does:
> >
> >  Metadata:
> >    title           : Infinite (Original Mix)
> >    artist          : B-Front
> >    artist          : Second Artist Example
> >    track           : 1
> >    album           : Infinite
> >    date            : 2017
> >    genre           : Hardstyle
> >    genre           : Test
> >    genre           : Example
> >    genre           : Hard Dance
> >    TBPM            : 150
> >    compilation     : 0
> >    album_artist    : B-Front
> >    publisher       : Roughstate
> >
> >
> >
> > My path does that:
> >
> >  Metadata:
> >    title           : Infinite (Original Mix)
> >    artist          : B-Front;Second Artist Example
> >    track           : 1
> >    album           : Infinite
> >    date            : 2017
> >    genre           : Hardstyle;Test;Example;Hard Dance
> >    TBPM            : 150
> >    compilation     : 0
> >    album_artist    : B-Front
> >    publisher       : Roughstate
> >
> >
> >
> > I'm not sure whether it is even allowed or intended that there are
> > multiple metadata entries with the same key?
> 
> It is indeed an intended feature of the AVDictionary system, and the
> metadata feature in particular.

Hi,

you meant to say that it is an intended change introduced by your patch? 😉 

Because we haven't had duplicate metadata keys before. I'm afraid but 
I think these changes should be reverted, because it also creates invalid
FFprobe output - like in case of JSON for example:

{
    "format": {
        "filename": "multiple_id3v2_4_values.mp3",
        "nb_streams": 1,
        "nb_programs": 0,
        "nb_stream_groups": 0,
        "format_name": "mp3",
        "format_long_name": "MP2/3 (MPEG audio layer 2/3)",
        "start_time": "0.011995",
        "duration": "20.035918",
        "size": "804936",
        "bit_rate": "321397",
        "probe_score": 51,
        "tags": {
            "title": "Infinite (Original Mix)",
            "artist": "B-Front",
            "artist": "Second Artist Example",
            "track": "1",
            "album": "Infinite",
            "date": "2017",
            "genre": "Hardstyle",
            "genre": "Test",
            "genre": "Example",
            "genre": "Hard Dance",
            "TBPM": "150",
            "compilation": "0",
            "album_artist": "B-Front",
            "publisher": "Roughstate",
            "encoder": "Lavf57.83.100",
        }
    }
}

Thanks,
sw



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

Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that decode_str() did advance

[Michael Niedermayer]

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

On Tue, Apr 15, 2025 at 12:17:32AM +0000, softworkz . wrote:
> 
> 
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > Ridley Combs via ffmpeg-devel
> > Sent: Dienstag, 15. April 2025 02:03
> > To: ffmpeg-devel <ffmpeg-devel@ffmpeg.org>
> > Cc: Ridley Combs <rcombs@rcombs.me>
> > Subject: Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
> > decode_str() did advance
> > 
> > 
> > 
> > > On Apr 15, 2025, at 08:59, softworkz . <softworkz-at-
> > hotmail.com@ffmpeg.org> wrote:
> > >
> > >
> > >
> > >> -----Original Message-----
> > >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org <mailto:ffmpeg-
> > devel-bounces@ffmpeg.org>> On Behalf Of
> > >> Michael Niedermayer
> > >> Sent: Dienstag, 15. April 2025 01:20
> > >> To: FFmpeg development discussions and patches <ffmpeg-
> > >> devel@ffmpeg.org <mailto:devel@ffmpeg.org>>
> > >> Subject: Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
> > >> decode_str() did advance
> > >>
> > >> On Sat, Apr 12, 2025 at 01:49:53AM +0000, softworkz . wrote:
> > >>>
> > >>>
> > >>>> -----Original Message-----
> > >>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > >>>> Michael Niedermayer
> > >>>> Sent: Samstag, 12. April 2025 00:27
> > >>>> To: FFmpeg development discussions and patches <ffmpeg-
> > >> devel@ffmpeg.org>
> > >>>> Subject: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
> > >>>> decode_str() did advance
> > >>>>
> > >>>> Fixes infinite loop with unknown encodings
> > >>>>
> > >>>> We could alternatively error out from decode_str() or consume all
> > >> of
> > >>>> taglen
> > >>>> this would affect other callers though.
> > >>>>
> > >>>> Fixes: 409819224/clusterfuzz-testcase-minimized-
> > >> ffmpeg_dem_H261_fuzzer-
> > >>>> 6003527535362048
> > >>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > >>>> ---
> > >>>> libavformat/id3v2.c | 3 +++
> > >>>> 1 file changed, 3 insertions(+)
> > >>>>
> > >>>> diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c
> > >>>> index 90314583a74..e3f7f9e2a90 100644
> > >>>> --- a/libavformat/id3v2.c
> > >>>> +++ b/libavformat/id3v2.c
> > >>>> @@ -341,10 +341,13 @@ static void read_ttag(AVFormatContext *s,
> > >>>> AVIOContext *pb, int taglen,
> > >>>>     taglen--; /* account for encoding type byte */
> > >>>>
> > >>>>     while (taglen > 1) {
> > >>>> +        int current_taglen = taglen;
> > >>>>         if (decode_str(s, pb, encoding, &dst, &taglen) < 0) {
> > >>>>             av_log(s, AV_LOG_ERROR, "Error reading frame %s,
> > >>>> skipped\n", key);
> > >>>>             return;
> > >>>>         }
> > >>>> +        if (current_taglen == taglen)
> > >>>> +            return;
> > >>>>
> > >>>>         count++;
> > >>>>
> > >>>> --
> > >>>> 2.49.0
> > >>>>
> > >>>> _______________________________________________
> > >>>
> > >>> Hi Michael,
> > >>>
> > >>> this kind of conflicts with this patch that I had submitted
> > >> recently:
> > >>>
> > >>>
> > >>
> > https://patchwork.ffmpeg.org/project/ffmpeg/patch/pull.54.ffstaging.FF
> > >> mpeg.1740873449247.ffmpegagent@gmail.com/
> > >>>
> > >>>
> > >>> I wonder whether my patch would still be prone to the issue your
> > >> patch is addressing -
> > >>
> > >> This already conflicts with rcombs patch in git master, i think
> > >> Applying: Fixes Trac ticket https://trac.ffmpeg.org/ticket/6949
> > >> Using index info to reconstruct a base tree...
> > >> M	libavformat/id3v2.c
> > >> Falling back to patching base and 3-way merge...
> > >> Auto-merging libavformat/id3v2.c
> > >> CONFLICT (content): Merge conflict in libavformat/id3v2.c
> > >> error: Failed to merge in the changes.
> > >> Patch failed at 0001 Fixes Trac ticket
> > >> https://trac.ffmpeg.org/ticket/6949
> > >>
> > >>
> > >>> do you have a test file perhaps?
> > >>
> > >> Will email you one, but the loop with a function that doesnt
> > advance
> > >> is an issue even if the specific file doesnt trigger it in a
> > different
> > >> implementation
> > >>
> > >> also probaly a good idea if you contact rcombs as you seemed to
> > work
> > >> on
> > >> the same code
> > >>
> > >> I was looking at teh ticket and saw a link to rcombs patch, looked
> > at
> > >> the patch and applied it. I did not realize there where 2 patches
> > >
> > >
> > > Hi Michael,
> > >
> > > I know the rcombs patch, but it has a - let's say - different
> > behavior.
> > > Let's look at an example where artist and genre have multiple
> > values:
> > >
> > >
> > > This was ffmpeg output unpatched:
> > >
> > >  Metadata:
> > >    title           : Infinite (Original Mix)
> > >    artist          : B-Front
> > >    track           : 1
> > >    album           : Infinite
> > >    date            : 2017
> > >    genre           : Hardstyle
> > >    TBPM            : 150
> > >    compilation     : 0
> > >    album_artist    : B-Front
> > >    publisher       : Roughstate
> > >
> > >
> > > This is what the rcombs patch does:
> > >
> > >  Metadata:
> > >    title           : Infinite (Original Mix)
> > >    artist          : B-Front
> > >    artist          : Second Artist Example
> > >    track           : 1
> > >    album           : Infinite
> > >    date            : 2017
> > >    genre           : Hardstyle
> > >    genre           : Test
> > >    genre           : Example
> > >    genre           : Hard Dance
> > >    TBPM            : 150
> > >    compilation     : 0
> > >    album_artist    : B-Front
> > >    publisher       : Roughstate
> > >
> > >
> > >
> > > My path does that:
> > >
> > >  Metadata:
> > >    title           : Infinite (Original Mix)
> > >    artist          : B-Front;Second Artist Example
> > >    track           : 1
> > >    album           : Infinite
> > >    date            : 2017
> > >    genre           : Hardstyle;Test;Example;Hard Dance
> > >    TBPM            : 150
> > >    compilation     : 0
> > >    album_artist    : B-Front
> > >    publisher       : Roughstate
> > >
> > >
> > >
> > > I'm not sure whether it is even allowed or intended that there are
> > > multiple metadata entries with the same key?
> > 
> > It is indeed an intended feature of the AVDictionary system, and the
> > metadata feature in particular.
> 
> Hi,
> 
> you meant to say that it is an intended change introduced by your patch? 😉 
> 
> Because we haven't had duplicate metadata keys before. I'm afraid but 
> I think these changes should be reverted, because it also creates invalid
> FFprobe output - like in case of JSON for example:
> 
> {
>     "format": {
>         "filename": "multiple_id3v2_4_values.mp3",
>         "nb_streams": 1,
>         "nb_programs": 0,
>         "nb_stream_groups": 0,
>         "format_name": "mp3",
>         "format_long_name": "MP2/3 (MPEG audio layer 2/3)",
>         "start_time": "0.011995",
>         "duration": "20.035918",
>         "size": "804936",
>         "bit_rate": "321397",
>         "probe_score": 51,
>         "tags": {
>             "title": "Infinite (Original Mix)",
>             "artist": "B-Front",
>             "artist": "Second Artist Example",
>             "track": "1",
>             "album": "Infinite",
>             "date": "2017",
>             "genre": "Hardstyle",
>             "genre": "Test",
>             "genre": "Example",
>             "genre": "Hard Dance",
>             "TBPM": "150",
>             "compilation": "0",
>             "album_artist": "B-Front",
>             "publisher": "Roughstate",
>             "encoder": "Lavf57.83.100",
>         }
>     }
> }

I think ffprobe should not generate invalid output if metadata given to it has
2 entries with the same key, independant of that being the correct way to export
metadata.

thx

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

Nations do behave wisely once they have exhausted all other alternatives. 
-- Abba Eban

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

Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that decode_str() did advance

[Nicolas George]

Michael Niedermayer (HE12025-04-15):
> I think ffprobe should not generate invalid output if metadata given to it has
> 2 entries with the same key, independant of that being the correct way to export
> metadata.

Good thing that “The JSON syntax […] does not require that name strings
be unique”.

https://ecma-international.org/publications-and-standards/standards/ecma-404/

Regards,

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

Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that decode_str() did advance

[softworkz .]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Nicolas George
> Sent: Dienstag, 15. April 2025 21:03
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
> decode_str() did advance
> 
> Michael Niedermayer (HE12025-04-15):
> > I think ffprobe should not generate invalid output if metadata given
> to it has
> > 2 entries with the same key, independant of that being the correct
> way to export
> > metadata.
> 
> Good thing that “The JSON syntax […] does not require that name
> strings
> be unique”.
> 
> https://ecma-international.org/publications-and-
> standards/standards/ecma-404/
> 

That's a good thing indeed, but it would be a breaking change for all
users of FFprobe output - and not only JSON but actually all formats.

sw




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

Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that decode_str() did advance

[Nicolas George]

softworkz . (HE12025-04-15):
>			      but it would be a breaking change

Please elaborate.

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

Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that decode_str() did advance

[softworkz .]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Nicolas George
> Sent: Dienstag, 15. April 2025 22:47
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
> decode_str() did advance
> 
> softworkz . (HE12025-04-15):
> >			      but it would be a breaking change
> 
> Please elaborate.

You are suggesting to change to an array [..] which breaks all usages.

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

Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that decode_str() did advance

[Nicolas George]

softworkz . (HE12025-04-15):
>						which breaks all usages.

Please elaborate.

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

Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that decode_str() did advance

[softworkz .]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Nicolas George
> Sent: Mittwoch, 16. April 2025 07:15
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
> decode_str() did advance
> 
> softworkz . (HE12025-04-15):
> >						which breaks all usages.
> 
> Please elaborate.
> 
> --
>   Nicolas George


Please show me widely used implementations which do not break.

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

Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that decode_str() did advance

[Nicolas George]

softworkz . (HE12025-04-16):
> Please show me widely used implementations which do not break.

No: you are making a strong claim, it is up to you to justify it: prove
that it breaks anything, let alone everything.

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

Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that decode_str() did advance

[softworkz .]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Michael Niedermayer
> Sent: Dienstag, 15. April 2025 20:33
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
> decode_str() did advance
> 
> On Tue, Apr 15, 2025 at 12:17:32AM +0000, softworkz . wrote:
> >
> >
> > > -----Original Message-----
> > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > > Ridley Combs via ffmpeg-devel
> > > Sent: Dienstag, 15. April 2025 02:03
> > > To: ffmpeg-devel <ffmpeg-devel@ffmpeg.org>
> > > Cc: Ridley Combs <rcombs@rcombs.me>
> > > Subject: Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
> > > decode_str() did advance
> > >
> > >
> > >
> > > > On Apr 15, 2025, at 08:59, softworkz . <softworkz-at-
> > > hotmail.com@ffmpeg.org> wrote:
> > > >
> > > >
> > > >
> > > >> -----Original Message-----
> > > >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org
> <mailto:ffmpeg-
> > > devel-bounces@ffmpeg.org>> On Behalf Of
> > > >> Michael Niedermayer
> > > >> Sent: Dienstag, 15. April 2025 01:20
> > > >> To: FFmpeg development discussions and patches <ffmpeg-
> > > >> devel@ffmpeg.org <mailto:devel@ffmpeg.org>>
> > > >> Subject: Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check
> that
> > > >> decode_str() did advance
> > > >>
> > > >> On Sat, Apr 12, 2025 at 01:49:53AM +0000, softworkz . wrote:
> > > >>>
> > > >>>
> > > >>>> -----Original Message-----
> > > >>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On
> Behalf Of
> > > >>>> Michael Niedermayer
> > > >>>> Sent: Samstag, 12. April 2025 00:27
> > > >>>> To: FFmpeg development discussions and patches <ffmpeg-
> > > >> devel@ffmpeg.org>
> > > >>>> Subject: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check
> that
> > > >>>> decode_str() did advance
> > > >>>>
> > > >>>> Fixes infinite loop with unknown encodings
> > > >>>>
> > > >>>> We could alternatively error out from decode_str() or consume
> all
> > > >> of
> > > >>>> taglen
> > > >>>> this would affect other callers though.
> > > >>>>
> > > >>>> Fixes: 409819224/clusterfuzz-testcase-minimized-
> > > >> ffmpeg_dem_H261_fuzzer-
> > > >>>> 6003527535362048
> > > >>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > >>>> ---
> > > >>>> libavformat/id3v2.c | 3 +++
> > > >>>> 1 file changed, 3 insertions(+)
> > > >>>>
> > > >>>> diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c
> > > >>>> index 90314583a74..e3f7f9e2a90 100644
> > > >>>> --- a/libavformat/id3v2.c
> > > >>>> +++ b/libavformat/id3v2.c
> > > >>>> @@ -341,10 +341,13 @@ static void read_ttag(AVFormatContext
> *s,
> > > >>>> AVIOContext *pb, int taglen,
> > > >>>>     taglen--; /* account for encoding type byte */
> > > >>>>
> > > >>>>     while (taglen > 1) {
> > > >>>> +        int current_taglen = taglen;
> > > >>>>         if (decode_str(s, pb, encoding, &dst, &taglen) < 0) {
> > > >>>>             av_log(s, AV_LOG_ERROR, "Error reading frame %s,
> > > >>>> skipped\n", key);
> > > >>>>             return;
> > > >>>>         }
> > > >>>> +        if (current_taglen == taglen)
> > > >>>> +            return;
> > > >>>>
> > > >>>>         count++;
> > > >>>>
> > > >>>> --
> > > >>>> 2.49.0
> > > >>>>
> > > >>>> _______________________________________________
> > > >>>
> > > >>> Hi Michael,
> > > >>>
> > > >>> this kind of conflicts with this patch that I had submitted
> > > >> recently:
> > > >>>
> > > >>>
> > > >>
> > >
> https://patchwork.ffmpeg.org/project/ffmpeg/patch/pull.54.ffstaging.FF
> > > >> mpeg.1740873449247.ffmpegagent@gmail.com/
> > > >>>
> > > >>>
> > > >>> I wonder whether my patch would still be prone to the issue
> your
> > > >> patch is addressing -
> > > >>
> > > >> This already conflicts with rcombs patch in git master, i think
> > > >> Applying: Fixes Trac ticket https://trac.ffmpeg.org/ticket/6949
> > > >> Using index info to reconstruct a base tree...
> > > >> M	libavformat/id3v2.c
> > > >> Falling back to patching base and 3-way merge...
> > > >> Auto-merging libavformat/id3v2.c
> > > >> CONFLICT (content): Merge conflict in libavformat/id3v2.c
> > > >> error: Failed to merge in the changes.
> > > >> Patch failed at 0001 Fixes Trac ticket
> > > >> https://trac.ffmpeg.org/ticket/6949
> > > >>
> > > >>
> > > >>> do you have a test file perhaps?
> > > >>
> > > >> Will email you one, but the loop with a function that doesnt
> > > advance
> > > >> is an issue even if the specific file doesnt trigger it in a
> > > different
> > > >> implementation
> > > >>
> > > >> also probaly a good idea if you contact rcombs as you seemed to
> > > work
> > > >> on
> > > >> the same code
> > > >>
> > > >> I was looking at teh ticket and saw a link to rcombs patch,
> looked
> > > at
> > > >> the patch and applied it. I did not realize there where 2
> patches
> > > >
> > > >
> > > > Hi Michael,
> > > >
> > > > I know the rcombs patch, but it has a - let's say - different
> > > behavior.
> > > > Let's look at an example where artist and genre have multiple
> > > values:
> > > >
> > > >
> > > > This was ffmpeg output unpatched:
> > > >
> > > >  Metadata:
> > > >    title           : Infinite (Original Mix)
> > > >    artist          : B-Front
> > > >    track           : 1
> > > >    album           : Infinite
> > > >    date            : 2017
> > > >    genre           : Hardstyle
> > > >    TBPM            : 150
> > > >    compilation     : 0
> > > >    album_artist    : B-Front
> > > >    publisher       : Roughstate
> > > >
> > > >
> > > > This is what the rcombs patch does:
> > > >
> > > >  Metadata:
> > > >    title           : Infinite (Original Mix)
> > > >    artist          : B-Front
> > > >    artist          : Second Artist Example
> > > >    track           : 1
> > > >    album           : Infinite
> > > >    date            : 2017
> > > >    genre           : Hardstyle
> > > >    genre           : Test
> > > >    genre           : Example
> > > >    genre           : Hard Dance
> > > >    TBPM            : 150
> > > >    compilation     : 0
> > > >    album_artist    : B-Front
> > > >    publisher       : Roughstate
> > > >
> > > >
> > > >
> > > > My path does that:
> > > >
> > > >  Metadata:
> > > >    title           : Infinite (Original Mix)
> > > >    artist          : B-Front;Second Artist Example
> > > >    track           : 1
> > > >    album           : Infinite
> > > >    date            : 2017
> > > >    genre           : Hardstyle;Test;Example;Hard Dance
> > > >    TBPM            : 150
> > > >    compilation     : 0
> > > >    album_artist    : B-Front
> > > >    publisher       : Roughstate
> > > >
> > > >
> > > >
> > > > I'm not sure whether it is even allowed or intended that there
> are
> > > > multiple metadata entries with the same key?
> > >
> > > It is indeed an intended feature of the AVDictionary system, and
> the
> > > metadata feature in particular.
> >
> > Hi,
> >
> > you meant to say that it is an intended change introduced by your
> patch? 😉
> >
> > Because we haven't had duplicate metadata keys before. I'm afraid
> but
> > I think these changes should be reverted, because it also creates
> invalid
> > FFprobe output - like in case of JSON for example:
> >
> > {
> >     "format": {
> >         "filename": "multiple_id3v2_4_values.mp3",
> >         "nb_streams": 1,
> >         "nb_programs": 0,
> >         "nb_stream_groups": 0,
> >         "format_name": "mp3",
> >         "format_long_name": "MP2/3 (MPEG audio layer 2/3)",
> >         "start_time": "0.011995",
> >         "duration": "20.035918",
> >         "size": "804936",
> >         "bit_rate": "321397",
> >         "probe_score": 51,
> >         "tags": {
> >             "title": "Infinite (Original Mix)",
> >             "artist": "B-Front",
> >             "artist": "Second Artist Example",
> >             "track": "1",
> >             "album": "Infinite",
> >             "date": "2017",
> >             "genre": "Hardstyle",
> >             "genre": "Test",
> >             "genre": "Example",
> >             "genre": "Hard Dance",
> >             "TBPM": "150",
> >             "compilation": "0",
> >             "album_artist": "B-Front",
> >             "publisher": "Roughstate",
> >             "encoder": "Lavf57.83.100",
> >         }
> >     }
> > }
> 
> I think ffprobe should not generate invalid output if metadata given
> to it has
> 2 entries with the same key, independant of that being the correct way
> to export
> metadata.

The problem is not at the side of FFprobe, it's the JSON format which
doesn't allow to express duplicate keys in that way.
(also applies to the upcoming YML output format)

The only thing that FFprobe could do is to suppress duplicate-key entries
but that would lead to an incomplete output of data. Since the code which
outputs the data is agnostic of the text formatter, the duplicate key entries
would be suppressed for all output formats. In the end, nothing would be
won because the output would be the same as without the rcombs patch.

In order to allow duplicate keys, the output format would need a general
change for metadata, like to an array of items (with key and value).
But this would be a breaking change for everybody who uses the metadata
information from FFprobe output in whatever way.

Best,
sw


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

Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that decode_str() did advance

[Nicolas George]

Ridley Combs via ffmpeg-devel (HE12025-04-15):
> This is an issue that should be resolved by supporting multiple
> entries all the way through the relevant ffmpeg.c and muxer code
> paths, rather than adding new fragile usage of in-band signaling
> within metadata strings.

Since it has been neglected in the ensuing discussion, I want to point
that I second that.

Regards,

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

Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that decode_str() did advance

[Andreas Rheinhardt]

Ridley Combs via ffmpeg-devel:
> 
> 
>> On Apr 15, 2025, at 08:59, softworkz . <softworkz-at-hotmail.com@ffmpeg.org> wrote:
>>
>>
>>
>>> -----Original Message-----
>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org <mailto:ffmpeg-devel-bounces@ffmpeg.org>> On Behalf Of
>>> Michael Niedermayer
>>> Sent: Dienstag, 15. April 2025 01:20
>>> To: FFmpeg development discussions and patches <ffmpeg-
>>> devel@ffmpeg.org <mailto:devel@ffmpeg.org>>
>>> Subject: Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
>>> decode_str() did advance
>>>
>>> On Sat, Apr 12, 2025 at 01:49:53AM +0000, softworkz . wrote:
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
>>>>> Michael Niedermayer
>>>>> Sent: Samstag, 12. April 2025 00:27
>>>>> To: FFmpeg development discussions and patches <ffmpeg-
>>> devel@ffmpeg.org>
>>>>> Subject: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
>>>>> decode_str() did advance
>>>>>
>>>>> Fixes infinite loop with unknown encodings
>>>>>
>>>>> We could alternatively error out from decode_str() or consume all
>>> of
>>>>> taglen
>>>>> this would affect other callers though.
>>>>>
>>>>> Fixes: 409819224/clusterfuzz-testcase-minimized-
>>> ffmpeg_dem_H261_fuzzer-
>>>>> 6003527535362048
>>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>>>> ---
>>>>> libavformat/id3v2.c | 3 +++
>>>>> 1 file changed, 3 insertions(+)
>>>>>
>>>>> diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c
>>>>> index 90314583a74..e3f7f9e2a90 100644
>>>>> --- a/libavformat/id3v2.c
>>>>> +++ b/libavformat/id3v2.c
>>>>> @@ -341,10 +341,13 @@ static void read_ttag(AVFormatContext *s,
>>>>> AVIOContext *pb, int taglen,
>>>>>     taglen--; /* account for encoding type byte */
>>>>>
>>>>>     while (taglen > 1) {
>>>>> +        int current_taglen = taglen;
>>>>>         if (decode_str(s, pb, encoding, &dst, &taglen) < 0) {
>>>>>             av_log(s, AV_LOG_ERROR, "Error reading frame %s,
>>>>> skipped\n", key);
>>>>>             return;
>>>>>         }
>>>>> +        if (current_taglen == taglen)
>>>>> +            return;
>>>>>
>>>>>         count++;
>>>>>
>>>>> --
>>>>> 2.49.0
>>>>>
>>>>> _______________________________________________
>>>>
>>>> Hi Michael,
>>>>
>>>> this kind of conflicts with this patch that I had submitted
>>> recently:
>>>>
>>>>
>>> https://patchwork.ffmpeg.org/project/ffmpeg/patch/pull.54.ffstaging.FF
>>> mpeg.1740873449247.ffmpegagent@gmail.com/
>>>>
>>>>
>>>> I wonder whether my patch would still be prone to the issue your
>>> patch is addressing -
>>>
>>> This already conflicts with rcombs patch in git master, i think
>>> Applying: Fixes Trac ticket https://trac.ffmpeg.org/ticket/6949
>>> Using index info to reconstruct a base tree...
>>> M	libavformat/id3v2.c
>>> Falling back to patching base and 3-way merge...
>>> Auto-merging libavformat/id3v2.c
>>> CONFLICT (content): Merge conflict in libavformat/id3v2.c
>>> error: Failed to merge in the changes.
>>> Patch failed at 0001 Fixes Trac ticket
>>> https://trac.ffmpeg.org/ticket/6949
>>>
>>>
>>>> do you have a test file perhaps?
>>>
>>> Will email you one, but the loop with a function that doesnt advance
>>> is an issue even if the specific file doesnt trigger it in a different
>>> implementation
>>>
>>> also probaly a good idea if you contact rcombs as you seemed to work
>>> on
>>> the same code
>>>
>>> I was looking at teh ticket and saw a link to rcombs patch, looked at
>>> the patch and applied it. I did not realize there where 2 patches
>>
>>
>> Hi Michael,
>>
>> I know the rcombs patch, but it has a - let's say - different behavior.
>> Let's look at an example where artist and genre have multiple values:
>>
>>
>> This was ffmpeg output unpatched:
>>
>>  Metadata:
>>    title           : Infinite (Original Mix)
>>    artist          : B-Front
>>    track           : 1
>>    album           : Infinite
>>    date            : 2017
>>    genre           : Hardstyle
>>    TBPM            : 150
>>    compilation     : 0
>>    album_artist    : B-Front
>>    publisher       : Roughstate
>>
>>
>> This is what the rcombs patch does:
>>
>>  Metadata:
>>    title           : Infinite (Original Mix)
>>    artist          : B-Front
>>    artist          : Second Artist Example
>>    track           : 1
>>    album           : Infinite
>>    date            : 2017
>>    genre           : Hardstyle
>>    genre           : Test
>>    genre           : Example
>>    genre           : Hard Dance
>>    TBPM            : 150
>>    compilation     : 0
>>    album_artist    : B-Front
>>    publisher       : Roughstate
>>
>>
>>
>> My path does that:
>>
>>  Metadata:
>>    title           : Infinite (Original Mix)
>>    artist          : B-Front;Second Artist Example
>>    track           : 1
>>    album           : Infinite
>>    date            : 2017
>>    genre           : Hardstyle;Test;Example;Hard Dance
>>    TBPM            : 150
>>    compilation     : 0
>>    album_artist    : B-Front
>>    publisher       : Roughstate
>>
>>
>>
>> I'm not sure whether it is even allowed or intended that there are 
>> multiple metadata entries with the same key?
> 
> It is indeed an intended feature of the AVDictionary system, and the metadata feature in particular.
> 

It is not how the lavf metadata AVDictionaries are supposed to be used:
"Keys are unique; there can never be 2 tags with the same key."

(I have already said this in my reply to the very first iteration of
this patchset:
https://patchwork.ffmpeg.org/project/ffmpeg/patch/20220620013711.91482-2-rcombs@rcombs.me/)

Side note: I too think that we should not merge values with the same
key, but export them as separate entries.

>>
>> But in any case, what's happening with the rcombs patch is that 
>> when transcoding a file, the result looks like this:
>>
>>  Metadata:
>>    title           : Infinite (Original Mix)
>>    artist          : B-Front
>>    track           : 1
>>    album           : Infinite
>>    date            : 2017
>>    genre           : Hardstyle
>>    TBPM            : 150
>>    compilation     : 0
>>    album_artist    : B-Front
>>    publisher       : Roughstate
>>
>>
>> Which means that the extra values all get lost. This doesn't happen
>> with the patch that I have submitted. Even though we should ideally
>> also modify the encoder to write the semicolon-delimited values with
>> null-separation, a semicolon is treated by most apps as a value 
>> delimiter as well - so it's a largely solid way and it avoids loss of
>> information when transcoding.
> 
> This is an issue that should be resolved by supporting multiple entries all the way through the relevant ffmpeg.c and muxer code paths, rather than adding new fragile usage of in-band signaling within metadata strings.
> 

And the commit to change this should have been in the same patchset as
the commit that allows lavf to export multiple entries with the same
key. (of course, it is even more complicated given that the lavf
metadata API is public.)

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

Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that decode_str() did advance

[Michael Niedermayer]

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

On Mon, Apr 14, 2025 at 11:59:02PM +0000, softworkz . wrote:
> 
> 
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > Michael Niedermayer
> > Sent: Dienstag, 15. April 2025 01:20
> > To: FFmpeg development discussions and patches <ffmpeg-
> > devel@ffmpeg.org>
> > Subject: Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
> > decode_str() did advance
> > 
> > On Sat, Apr 12, 2025 at 01:49:53AM +0000, softworkz . wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > > > Michael Niedermayer
> > > > Sent: Samstag, 12. April 2025 00:27
> > > > To: FFmpeg development discussions and patches <ffmpeg-
> > devel@ffmpeg.org>
> > > > Subject: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
> > > > decode_str() did advance
> > > >
> > > > Fixes infinite loop with unknown encodings
> > > >
> > > > We could alternatively error out from decode_str() or consume all
> > of
> > > > taglen
> > > > this would affect other callers though.
> > > >
> > > > Fixes: 409819224/clusterfuzz-testcase-minimized-
> > ffmpeg_dem_H261_fuzzer-
> > > > 6003527535362048
> > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > > ---
> > > >  libavformat/id3v2.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c
> > > > index 90314583a74..e3f7f9e2a90 100644
> > > > --- a/libavformat/id3v2.c
> > > > +++ b/libavformat/id3v2.c
> > > > @@ -341,10 +341,13 @@ static void read_ttag(AVFormatContext *s,
> > > > AVIOContext *pb, int taglen,
> > > >      taglen--; /* account for encoding type byte */
> > > >
> > > >      while (taglen > 1) {
> > > > +        int current_taglen = taglen;
> > > >          if (decode_str(s, pb, encoding, &dst, &taglen) < 0) {
> > > >              av_log(s, AV_LOG_ERROR, "Error reading frame %s,
> > > > skipped\n", key);
> > > >              return;
> > > >          }
> > > > +        if (current_taglen == taglen)
> > > > +            return;
> > > >
> > > >          count++;
> > > >
> > > > --
> > > > 2.49.0
> > > >
> > > > _______________________________________________
> > >
> > > Hi Michael,
> > >
> > > this kind of conflicts with this patch that I had submitted
> > recently:
> > >
> > >
> > https://patchwork.ffmpeg.org/project/ffmpeg/patch/pull.54.ffstaging.FF
> > mpeg.1740873449247.ffmpegagent@gmail.com/
> > >
> > >
> > > I wonder whether my patch would still be prone to the issue your
> > patch is addressing -
> > 
> > This already conflicts with rcombs patch in git master, i think
> > Applying: Fixes Trac ticket https://trac.ffmpeg.org/ticket/6949
> > Using index info to reconstruct a base tree...
> > M	libavformat/id3v2.c
> > Falling back to patching base and 3-way merge...
> > Auto-merging libavformat/id3v2.c
> > CONFLICT (content): Merge conflict in libavformat/id3v2.c
> > error: Failed to merge in the changes.
> > Patch failed at 0001 Fixes Trac ticket
> > https://trac.ffmpeg.org/ticket/6949
> > 
> > 
> > > do you have a test file perhaps?
> > 
> > Will email you one, but the loop with a function that doesnt advance
> > is an issue even if the specific file doesnt trigger it in a different
> > implementation
> > 
> > also probaly a good idea if you contact rcombs as you seemed to work
> > on
> > the same code
> > 
> > I was looking at teh ticket and saw a link to rcombs patch, looked at
> > the patch and applied it. I did not realize there where 2 patches
> 
> 
> Hi Michael,
> 
> I know the rcombs patch, but it has a - let's say - different behavior.
> Let's look at an example where artist and genre have multiple values:
> 
> 
> This was ffmpeg output unpatched:
> 
>   Metadata:
>     title           : Infinite (Original Mix)
>     artist          : B-Front
>     track           : 1
>     album           : Infinite
>     date            : 2017
>     genre           : Hardstyle
>     TBPM            : 150
>     compilation     : 0
>     album_artist    : B-Front
>     publisher       : Roughstate
> 
> 
> This is what the rcombs patch does:
> 
>   Metadata:
>     title           : Infinite (Original Mix)
>     artist          : B-Front
>     artist          : Second Artist Example
>     track           : 1
>     album           : Infinite
>     date            : 2017
>     genre           : Hardstyle
>     genre           : Test
>     genre           : Example
>     genre           : Hard Dance
>     TBPM            : 150
>     compilation     : 0
>     album_artist    : B-Front
>     publisher       : Roughstate
> 
> 
> 
> My path does that:
> 
>   Metadata:
>     title           : Infinite (Original Mix)
>     artist          : B-Front;Second Artist Example
>     track           : 1
>     album           : Infinite
>     date            : 2017
>     genre           : Hardstyle;Test;Example;Hard Dance
>     TBPM            : 150
>     compilation     : 0
>     album_artist    : B-Front
>     publisher       : Roughstate

Iam perfectly fine with either way
but i have to point out that the 2nd method has some problems too

for example checking if foo is an author becomes more difficult
theres also a question about scalability if there are many entries

And what exactly do you do if an Artist or Title itself contains a ;


Thx

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

While the State exists there can be no freedom; when there is freedom there
will be no State. -- Vladimir Lenin

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

Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that decode_str() did advance

[softworkz .]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Michael Niedermayer
> Sent: Dienstag, 15. April 2025 20:56
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
> decode_str() did advance
> 
> On Mon, Apr 14, 2025 at 11:59:02PM +0000, softworkz . wrote:
> >
> >
> > > -----Original Message-----
> > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > > Michael Niedermayer
> > > Sent: Dienstag, 15. April 2025 01:20
> > > To: FFmpeg development discussions and patches <ffmpeg-
> > > devel@ffmpeg.org>
> > > Subject: Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
> > > decode_str() did advance
> > >
> > > On Sat, Apr 12, 2025 at 01:49:53AM +0000, softworkz . wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf
> Of
> > > > > Michael Niedermayer
> > > > > Sent: Samstag, 12. April 2025 00:27
> > > > > To: FFmpeg development discussions and patches <ffmpeg-
> > > devel@ffmpeg.org>
> > > > > Subject: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
> > > > > decode_str() did advance
> > > > >
> > > > > Fixes infinite loop with unknown encodings
> > > > >
> > > > > We could alternatively error out from decode_str() or consume
> all
> > > of
> > > > > taglen
> > > > > this would affect other callers though.
> > > > >
> > > > > Fixes: 409819224/clusterfuzz-testcase-minimized-
> > > ffmpeg_dem_H261_fuzzer-
> > > > > 6003527535362048
> > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > > > ---
> > > > >  libavformat/id3v2.c | 3 +++
> > > > >  1 file changed, 3 insertions(+)
> > > > >
> > > > > diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c
> > > > > index 90314583a74..e3f7f9e2a90 100644
> > > > > --- a/libavformat/id3v2.c
> > > > > +++ b/libavformat/id3v2.c
> > > > > @@ -341,10 +341,13 @@ static void read_ttag(AVFormatContext
> *s,
> > > > > AVIOContext *pb, int taglen,
> > > > >      taglen--; /* account for encoding type byte */
> > > > >
> > > > >      while (taglen > 1) {
> > > > > +        int current_taglen = taglen;
> > > > >          if (decode_str(s, pb, encoding, &dst, &taglen) < 0) {
> > > > >              av_log(s, AV_LOG_ERROR, "Error reading frame %s,
> > > > > skipped\n", key);
> > > > >              return;
> > > > >          }
> > > > > +        if (current_taglen == taglen)
> > > > > +            return;
> > > > >
> > > > >          count++;
> > > > >
> > > > > --
> > > > > 2.49.0
> > > > >
> > > > > _______________________________________________
> > > >
> > > > Hi Michael,
> > > >
> > > > this kind of conflicts with this patch that I had submitted
> > > recently:
> > > >
> > > >
> > >
> https://patchwork.ffmpeg.org/project/ffmpeg/patch/pull.54.ffstaging.FF
> > > mpeg.1740873449247.ffmpegagent@gmail.com/
> > > >
> > > >
> > > > I wonder whether my patch would still be prone to the issue your
> > > patch is addressing -
> > >
> > > This already conflicts with rcombs patch in git master, i think
> > > Applying: Fixes Trac ticket https://trac.ffmpeg.org/ticket/6949
> > > Using index info to reconstruct a base tree...
> > > M	libavformat/id3v2.c
> > > Falling back to patching base and 3-way merge...
> > > Auto-merging libavformat/id3v2.c
> > > CONFLICT (content): Merge conflict in libavformat/id3v2.c
> > > error: Failed to merge in the changes.
> > > Patch failed at 0001 Fixes Trac ticket
> > > https://trac.ffmpeg.org/ticket/6949
> > >
> > >
> > > > do you have a test file perhaps?
> > >
> > > Will email you one, but the loop with a function that doesnt
> advance
> > > is an issue even if the specific file doesnt trigger it in a
> different
> > > implementation
> > >
> > > also probaly a good idea if you contact rcombs as you seemed to
> work
> > > on
> > > the same code
> > >
> > > I was looking at teh ticket and saw a link to rcombs patch, looked
> at
> > > the patch and applied it. I did not realize there where 2 patches
> >
> >
> > Hi Michael,
> >
> > I know the rcombs patch, but it has a - let's say - different
> behavior.
> > Let's look at an example where artist and genre have multiple
> values:
> >
> >
> > This was ffmpeg output unpatched:
> >
> >   Metadata:
> >     title           : Infinite (Original Mix)
> >     artist          : B-Front
> >     track           : 1
> >     album           : Infinite
> >     date            : 2017
> >     genre           : Hardstyle
> >     TBPM            : 150
> >     compilation     : 0
> >     album_artist    : B-Front
> >     publisher       : Roughstate
> >
> >
> > This is what the rcombs patch does:
> >
> >   Metadata:
> >     title           : Infinite (Original Mix)
> >     artist          : B-Front
> >     artist          : Second Artist Example
> >     track           : 1
> >     album           : Infinite
> >     date            : 2017
> >     genre           : Hardstyle
> >     genre           : Test
> >     genre           : Example
> >     genre           : Hard Dance
> >     TBPM            : 150
> >     compilation     : 0
> >     album_artist    : B-Front
> >     publisher       : Roughstate
> >
> >
> >
> > My path does that:
> >
> >   Metadata:
> >     title           : Infinite (Original Mix)
> >     artist          : B-Front;Second Artist Example
> >     track           : 1
> >     album           : Infinite
> >     date            : 2017
> >     genre           : Hardstyle;Test;Example;Hard Dance
> >     TBPM            : 150
> >     compilation     : 0
> >     album_artist    : B-Front
> >     publisher       : Roughstate
> 
> Iam perfectly fine with either way
> but i have to point out that the 2nd method has some problems too
> 
> for example checking if foo is an author becomes more difficult
> theres also a question about scalability if there are many entries
> 
> And what exactly do you do if an Artist or Title itself contains a ;

I think we need to take a look at the context and real-world use of 
music tagging via ID3 tags by various applications.

The representation of multi-values via null-separated strings is something
that has been added only to the 2.4 version of ID3 spec. It does not
exist in version 2.3 and earlier.

So, what did all the tools and players do before to handle multi-valued
metadata?

Two common practices exist: 

1. Slash Separation

genre           : Hardstyle / Test / Example / Hard Dance


2. Semicolon Separation


genre           : Hardstyle; Test; Example; Hard Dance


When we look at one of the most popular music tagging applications
(MusicBrainz Picard), then we see the following behaviors:

- When opening a file with v2.4 multivalue tags the application UI shows
  them like: Hardstyle; Test; Example; Hard Dance

- When saving with v2.3 tags (and semicolon separation configured)
  it saves them as single string: Hardstyle; Test; Example; Hard Dance

When we run FFprobe (without any patches!) on that v2.3 file, we see:

  Metadata:
    title           : Infinite (Original Mix)
    artist          : B-Front; Second Artist Example
    track           : 1
    album           : Infinite
    date            : 2017
    genre           : Hardstyle; Test; Example; Hard Dance


That means in turn:

- Semicolon-separated multi-values is not my personal "invention"

- This is a long-existing established standard for representation 
  of multi-values metadata entries

- Semicolon separation allows a version-independent round-tripping
  of multi-valued (music) metadata inside FFmpeg

A further improvement would be on the side of ID3 tag writing, to
convert semicolon separation to null-separation.


But even without that, it is the best possible way for us to go,
because:

- It will retrieve and show all values, no longer just the first

- Additional values don't get lost when transcoding 
  (as with the rcombs patch)

- The representation of multi-values - both, internally and when 
  outputting as probe data - is a de-facto standard

- FFprobe output remains valid


Best regards
sw
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that decode_str() did advance

[Michael Niedermayer]

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

On Tue, Apr 15, 2025 at 07:59:00PM +0000, softworkz . wrote:
[...]

> - The representation of multi-values - both, internally and when
>   outputting as probe data - is a de-facto standard

The external handling in formats is specified in the corresponing
specifications. ";" is certainly not correct for formats which
natively support multiple values per key.

Internally, if you have a data structure that represents multiple
authors, you certainly do not set it to one author and a string
with a bunch of semicolons seperating multiple authors

Title: "Smile ;)"
Author "Smily Face ;)"

is not 2 Titles and not 2 Authors and software that cannot handle that
should not be used as reference IMHO

That said, anything that works is fine with me,

But internally it will be better to use a representation that is
universal, generic and simple, ";" may seem to be that but only
as long as you do nothing with it and dont care about corner cases

Ill leave this ";" question to everyone else, i have a backlog
of quite a few things i need to work on

I do intend though to apply my bugfix, as i dont want to leave that
bug, even if that ends up reverted and replaced by some other solution

thx

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

it is not once nor twice but times without number that the same ideas make
their appearance in the world. -- Aristotle

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

Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that decode_str() did advance

[softworkz .]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Michael Niedermayer
> Sent: Mittwoch, 16. April 2025 00:50
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
> decode_str() did advance
> 
> On Tue, Apr 15, 2025 at 07:59:00PM +0000, softworkz . wrote:
> [...]
> 
> > - The representation of multi-values - both, internally and when
> >   outputting as probe data - is a de-facto standard
> 
> The external handling in formats is specified in the corresponing
> specifications. ";" is certainly not correct for formats which
> natively support multiple values per key.
> 
> Internally, if you have a data structure that represents multiple
> authors, you certainly do not set it to one author and a string
> with a bunch of semicolons seperating multiple authors
> 
> Title: "Smile ;)"
> Author "Smily Face ;)"
> 
> is not 2 Titles and not 2 Authors and software that cannot handle that
> should not be used as reference IMHO
> 
> That said, anything that works is fine with me,
> 
> But internally it will be better to use a representation that is
> universal, generic and simple, ";" may seem to be that but only
> as long as you do nothing with it and dont care about corner cases
> 
> Ill leave this ";" question to everyone else, i have a backlog
> of quite a few things i need to work on

This is not a great outcome, because "leaving everyone else" means 
nothing will happen.

At least revert the rcombs patch until there's a conclusion, because
it really makes things worse than better with regards to FFprobe 
output.
This will cause deserialization errors for many people in the world
who are processing FFprobe data.

Thanks
sw



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

Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that decode_str() did advance

[softworkz .]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> softworkz .
> Sent: Mittwoch, 16. April 2025 00:59
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
> decode_str() did advance
> 
> 
> 
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > Michael Niedermayer
> > Sent: Mittwoch, 16. April 2025 00:50
> > To: FFmpeg development discussions and patches <ffmpeg-
> > devel@ffmpeg.org>
> > Subject: Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
> > decode_str() did advance
> >
> > On Tue, Apr 15, 2025 at 07:59:00PM +0000, softworkz . wrote:
> > [...]
> >
> > > - The representation of multi-values - both, internally and when
> > >   outputting as probe data - is a de-facto standard
> >
> > The external handling in formats is specified in the corresponing
> > specifications. ";" is certainly not correct for formats which
> > natively support multiple values per key.
> >
> > Internally, if you have a data structure that represents multiple
> > authors, you certainly do not set it to one author and a string
> > with a bunch of semicolons seperating multiple authors
> >
> > Title: "Smile ;)"
> > Author "Smily Face ;)"
> >
> > is not 2 Titles and not 2 Authors and software that cannot handle
> that
> > should not be used as reference IMHO
> >
> > That said, anything that works is fine with me,
> >
> > But internally it will be better to use a representation that is
> > universal, generic and simple, ";" may seem to be that but only
> > as long as you do nothing with it and dont care about corner cases
> >
> > Ill leave this ";" question to everyone else, i have a backlog
> > of quite a few things i need to work on
> 
> This is not a great outcome, because "leaving everyone else" means
> nothing will happen.
> 
> At least revert the rcombs patch until there's a conclusion, because
> it really makes things worse than better with regards to FFprobe
> output.
> This will cause deserialization errors for many people in the world
> who are processing FFprobe data.
> 

Besides, the patch had been submitted 3 years ago, there hasn't been 
any review and the merge was totally unexpected.

Thanks
sw



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

Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that decode_str() did advance

[Michael Niedermayer]

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

On Tue, Apr 15, 2025 at 11:01:14PM +0000, softworkz . wrote:
[...]
> Besides, the patch had been submitted 3 years ago, there hasn't been 
> any review and the merge was totally unexpected.

no reply for 1 week means commit must be expected

"Send a patch to ffmpeg-devel. If no one answers within a reasonable
 time-frame (12h for build failures and security fixes, 3 days small changes,
 1 week for big patches) then commit your patch if you think it is OK.
 Also note, the maintainer can simply ask for more time to review!
"

besides, i reviewed the patch before applying it

and there was an open bug that alot of people wanted fixed.

thx

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

The misfortune of the wise is better than the prosperity of the fool.
-- Epicurus

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

Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that decode_str() did advance

[softworkz .]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Michael Niedermayer
> Sent: Mittwoch, 16. April 2025 02:54
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
> decode_str() did advance
> 
> On Tue, Apr 15, 2025 at 11:01:14PM +0000, softworkz . wrote:
> [...]
> > Besides, the patch had been submitted 3 years ago, there hasn't been
> > any review and the merge was totally unexpected.
> 
> no reply for 1 week means commit must be expected
> 
> "Send a patch to ffmpeg-devel. If no one answers within a reasonable
>  time-frame (12h for build failures and security fixes, 3 days small
> changes,
>  1 week for big patches) then commit your patch if you think it is OK.
>  Also note, the maintainer can simply ask for more time to review!
> "
> 
> besides, i reviewed the patch before applying it
> 
> and there was an open bug that alot of people wanted fixed.

But not in a way that breaks their deserialization or parsing.


Essentially, you've just exchanged this bug for another way more severe 
bug in the FFprobe output. Do you just want to await the reports
and complaints coming in?

No matter whether applying this patch was OK or not in the first place,
knowing that it causes a serious regression doesn't appear to me like 
something that could just be ignored, or do you think otherwise?

sw









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

Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that decode_str() did advance

[softworkz .]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Michael Niedermayer
> Sent: Mittwoch, 16. April 2025 02:54
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
> decode_str() did advance
> 
> On Tue, Apr 15, 2025 at 11:01:14PM +0000, softworkz . wrote:
> [...]
> > Besides, the patch had been submitted 3 years ago, there hasn't been
> > any review and the merge was totally unexpected.
> 
> no reply for 1 week means commit must be expected
> 
> "Send a patch to ffmpeg-devel. If no one answers within a reasonable
>  time-frame (12h for build failures and security fixes, 3 days small
> changes,
>  1 week for big patches) then commit your patch if you think it is OK.
>  Also note, the maintainer can simply ask for more time to review!
> "

Does that mean that I can apply any patch from the past few years without
prior notice when it hasn't received a reply?

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

Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that decode_str() did advance

[Nicolas George]

softworkz . (HE12025-04-16):
> Does that mean that I can apply any patch from the past few years without
> prior notice when it hasn't received a reply?

Are you already considering abusing your git access?

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

Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that decode_str() did advance

[Michael Niedermayer]

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

On Wed, Apr 16, 2025 at 02:39:44AM +0000, softworkz . wrote:
> 
> 
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > Michael Niedermayer
> > Sent: Mittwoch, 16. April 2025 02:54
> > To: FFmpeg development discussions and patches <ffmpeg-
> > devel@ffmpeg.org>
> > Subject: Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
> > decode_str() did advance
> > 
> > On Tue, Apr 15, 2025 at 11:01:14PM +0000, softworkz . wrote:
> > [...]
> > > Besides, the patch had been submitted 3 years ago, there hasn't been
> > > any review and the merge was totally unexpected.
> > 
> > no reply for 1 week means commit must be expected
> > 
> > "Send a patch to ffmpeg-devel. If no one answers within a reasonable
> >  time-frame (12h for build failures and security fixes, 3 days small
> > changes,
> >  1 week for big patches) then commit your patch if you think it is OK.
> >  Also note, the maintainer can simply ask for more time to review!
> > "
> 
> Does that mean that I can apply any patch from the past few years without
> prior notice when it hasn't received a reply?

you (and everyone else) has my support to review and then apply patches.
we have failed with timely reacting to some patches, so if you want
to pick some up and look into them, that is welcome!

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

"You are 36 times more likely to die in a bathtub than at the hands of a
terrorist. Also, you are 2.5 times more likely to become a president and
2 times more likely to become an astronaut, than to die in a terrorist
attack." -- Thoughty2


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

Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that decode_str() did advance

[softworkz .]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Michael Niedermayer
> Sent: Mittwoch, 16. April 2025 13:07
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
> decode_str() did advance
> 
> On Wed, Apr 16, 2025 at 02:39:44AM +0000, softworkz . wrote:
> >
> >
> > > -----Original Message-----
> > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > > Michael Niedermayer
> > > Sent: Mittwoch, 16. April 2025 02:54
> > > To: FFmpeg development discussions and patches <ffmpeg-
> > > devel@ffmpeg.org>
> > > Subject: Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
> > > decode_str() did advance
> > >
> > > On Tue, Apr 15, 2025 at 11:01:14PM +0000, softworkz . wrote:
> > > [...]
> > > > Besides, the patch had been submitted 3 years ago, there hasn't
> been
> > > > any review and the merge was totally unexpected.
> > >
> > > no reply for 1 week means commit must be expected
> > >
> > > "Send a patch to ffmpeg-devel. If no one answers within a
> reasonable
> > >  time-frame (12h for build failures and security fixes, 3 days
> small
> > > changes,
> > >  1 week for big patches) then commit your patch if you think it is
> OK.
> > >  Also note, the maintainer can simply ask for more time to review!
> > > "
> >
> > Does that mean that I can apply any patch from the past few years
> without
> > prior notice when it hasn't received a reply?
> 
> you (and everyone else) has my support to review and then apply
> patches.
> we have failed with timely reacting to some patches, so if you want
> to pick some up and look into them, that is welcome!


Great! (even though it's not what I meant)

Nonetheless, I can assure everybody that I will not apply something 
without posting about it before 😊

sw






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

Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that decode_str() did advance

[Michael Niedermayer]

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

On Tue, Apr 15, 2025 at 10:59:07PM +0000, softworkz . wrote:
> 
> 
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > Michael Niedermayer
> > Sent: Mittwoch, 16. April 2025 00:50
> > To: FFmpeg development discussions and patches <ffmpeg-
> > devel@ffmpeg.org>
> > Subject: Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
> > decode_str() did advance
> > 
> > On Tue, Apr 15, 2025 at 07:59:00PM +0000, softworkz . wrote:
> > [...]
> > 
> > > - The representation of multi-values - both, internally and when
> > >   outputting as probe data - is a de-facto standard
> > 
> > The external handling in formats is specified in the corresponing
> > specifications. ";" is certainly not correct for formats which
> > natively support multiple values per key.
> > 
> > Internally, if you have a data structure that represents multiple
> > authors, you certainly do not set it to one author and a string
> > with a bunch of semicolons seperating multiple authors
> > 
> > Title: "Smile ;)"
> > Author "Smily Face ;)"
> > 
> > is not 2 Titles and not 2 Authors and software that cannot handle that
> > should not be used as reference IMHO
> > 
> > That said, anything that works is fine with me,
> > 
> > But internally it will be better to use a representation that is
> > universal, generic and simple, ";" may seem to be that but only
> > as long as you do nothing with it and dont care about corner cases
> > 
> > Ill leave this ";" question to everyone else, i have a backlog
> > of quite a few things i need to work on
> 
> This is not a great outcome, because "leaving everyone else" means 
> nothing will happen.

I have a release to do, I have contracts that i should be working on,
as iam a few month behind some deadline, I have a backlog of fuzzer
issues i wanted to look at. I have a backlog of other things

And this ";" thing is close to the last thing i want to spend my time on ;)


> 
> At least revert the rcombs patch until there's a conclusion, because
> it really makes things worse than better with regards to FFprobe 
> output.

I guess thats reasonable.
I will revert it, and then stay out of this unless it reaches the TC
(where I as member will participate in finding a solution)


> This will cause deserialization errors for many people in the world
> who are processing FFprobe data.

As said, ffprobe should not produce troublesome output

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Modern terrorism, a quick summary: Need oil, start war with country that
has oil, kill hundread thousand in war. Let country fall into chaos,
be surprised about raise of fundamantalists. Drop more bombs, kill more
people, be surprised about them taking revenge and drop even more bombs
and strip your own citizens of their rights and freedoms. to be continued

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

Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that decode_str() did advance

[softworkz .]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Michael Niedermayer
> Sent: Mittwoch, 16. April 2025 03:22
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
> decode_str() did advance
> 
> On Tue, Apr 15, 2025 at 10:59:07PM +0000, softworkz . wrote:
> >
> >
> > > -----Original Message-----
> > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > > Michael Niedermayer
> > > Sent: Mittwoch, 16. April 2025 00:50
> > > To: FFmpeg development discussions and patches <ffmpeg-
> > > devel@ffmpeg.org>
> > > Subject: Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
> > > decode_str() did advance
> > >
> > > On Tue, Apr 15, 2025 at 07:59:00PM +0000, softworkz . wrote:
> > > [...]
> > >
> > > > - The representation of multi-values - both, internally and when
> > > >   outputting as probe data - is a de-facto standard
> > >
> > > The external handling in formats is specified in the corresponing
> > > specifications. ";" is certainly not correct for formats which
> > > natively support multiple values per key.
> > >
> > > Internally, if you have a data structure that represents multiple
> > > authors, you certainly do not set it to one author and a string
> > > with a bunch of semicolons seperating multiple authors
> > >
> > > Title: "Smile ;)"
> > > Author "Smily Face ;)"
> > >
> > > is not 2 Titles and not 2 Authors and software that cannot handle
> that
> > > should not be used as reference IMHO
> > >
> > > That said, anything that works is fine with me,
> > >
> > > But internally it will be better to use a representation that is
> > > universal, generic and simple, ";" may seem to be that but only
> > > as long as you do nothing with it and dont care about corner cases
> > >
> > > Ill leave this ";" question to everyone else, i have a backlog
> > > of quite a few things i need to work on
> >
> > This is not a great outcome, because "leaving everyone else" means
> > nothing will happen.
> 
> I have a release to do, I have contracts that i should be working on,
> as iam a few month behind some deadline, I have a backlog of fuzzer
> issues i wanted to look at. I have a backlog of other things
> 
> And this ";" thing is close to the last thing i want to spend my time
> on ;)

It's fine. You don't have to. I'm also not insisting on my patch.


> > At least revert the rcombs patch until there's a conclusion, because
> > it really makes things worse than better with regards to FFprobe
> > output.
> 
> I guess thats reasonable.

Perfect, thanks.


> > This will cause deserialization errors for many people in the world
> > who are processing FFprobe data.
> 
> As said, ffprobe should not produce troublesome output

As I said, it cannot be remedied on the FFprobe side without making a 
breaking change to its output format, that's what turns this small
thing into a big one.

Thank you
sw

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

Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that decode_str() did advance

[Michael Niedermayer]

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

On Wed, Apr 16, 2025 at 01:29:02AM +0000, softworkz . wrote:
[...]
> > > This will cause deserialization errors for many people in the world
> > > who are processing FFprobe data.
> > 
> > As said, ffprobe should not produce troublesome output
> 
> As I said, it cannot be remedied on the FFprobe side without making a 

If you want ffprobe to combine multiple author tags with ";", you
certainly can do that in ffprobe

thx

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

If you fake or manipulate statistics in a paper in physics you will never
get a job again.
If you fake or manipulate statistics in a paper in medicin you will get
a job for life at the pharma industry.

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

Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that decode_str() did advance

[softworkz .]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Michael Niedermayer
> Sent: Mittwoch, 16. April 2025 03:34
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
> decode_str() did advance
> 
> On Wed, Apr 16, 2025 at 01:29:02AM +0000, softworkz . wrote:
> [...]
> > > > This will cause deserialization errors for many people in the
> world
> > > > who are processing FFprobe data.
> > >
> > > As said, ffprobe should not produce troublesome output
> >
> > As I said, it cannot be remedied on the FFprobe side without making
> a
> 
> If you want ffprobe to combine multiple author tags with ";", you
> certainly can do that in ffprobe

That's not what I want to do, and if at all, it would be up to do
for the one who wants to introduce multi-key metadata.

This has a far wider scope and cannot be introduced in a drive-by
way via a bugfix patch.

I have already shown how the additional values get lost when transcoding
and that was just about ID3 tag writing. 

What about other formats (muxers)?

Can they deal with multi-key metadata?

Does it create regressions? 
(yet untested by FATE)


What I want to say is that this is a big change that needs discussion,
planning and evaluation of the consequences in all affected areas.
I can't be introduced by a bugfix which causes regressions and others
having to clean up the mess afterwards.

Thanks
sw










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

Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that decode_str() did advance

[softworkz .]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Michael Niedermayer
> Sent: Mittwoch, 16. April 2025 03:34
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
> decode_str() did advance
> 
> On Wed, Apr 16, 2025 at 01:29:02AM +0000, softworkz . wrote:
> [...]
> > > > This will cause deserialization errors for many people in the
> world
> > > > who are processing FFprobe data.
> > >
> > > As said, ffprobe should not produce troublesome output
> >
> > As I said, it cannot be remedied on the FFprobe side without making
> a
> 
> If you want ffprobe to combine multiple author tags with ";", you
> certainly can do that in ffprobe

Which by-the-way contradicts most of your earlier arguments against
semicolon delimited values.

I gain the impression that the actual reason for why you (seemingly)
want this, might be for having an actual use case for the duplicate key support
in AVMap..?  😊

It's at least an interesting coincidence in time...
I can understand that, don't mean it in a negative way, just don't think it should be done at the cost of regressing the probe output.

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

Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that decode_str() did advance

[Michael Niedermayer]

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

On Wed, Apr 16, 2025 at 02:31:58AM +0000, softworkz . wrote:
> 
> 
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > Michael Niedermayer
> > Sent: Mittwoch, 16. April 2025 03:34
> > To: FFmpeg development discussions and patches <ffmpeg-
> > devel@ffmpeg.org>
> > Subject: Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
> > decode_str() did advance
> > 
> > On Wed, Apr 16, 2025 at 01:29:02AM +0000, softworkz . wrote:
> > [...]
> > > > > This will cause deserialization errors for many people in the
> > world
> > > > > who are processing FFprobe data.
> > > >
> > > > As said, ffprobe should not produce troublesome output
> > >
> > > As I said, it cannot be remedied on the FFprobe side without making
> > a
> > 
> > If you want ffprobe to combine multiple author tags with ";", you
> > certainly can do that in ffprobe
> 
> Which by-the-way contradicts most of your earlier arguments against
> semicolon delimited values.

does it ?


> 
> I gain the impression that the actual reason for why you (seemingly)
> want this, might be for having an actual use case for the duplicate key support
> in AVMap..?  😊

the AVDictionary struct FFmpeg uses supports multiple
equal keys since over 9 years:

commit 4ebf0b109cdb4daa888d69e8294621948168c46c
Author: Thilo Borgmann <thilo.borgmann@mail.de>
Date:   Sat Mar 12 14:52:17 2016 +0100

    lavu/dict: Add new flag to allow multiple equal keys.


thx

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

Everything should be made as simple as possible, but not simpler.
-- Albert Einstein

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

Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that decode_str() did advance

[softworkz .]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Michael Niedermayer
> Sent: Mittwoch, 16. April 2025 12:42
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
> decode_str() did advance
> 
> On Wed, Apr 16, 2025 at 02:31:58AM +0000, softworkz . wrote:
> >
> >
> > > -----Original Message-----
> > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > > Michael Niedermayer
> > > Sent: Mittwoch, 16. April 2025 03:34
> > > To: FFmpeg development discussions and patches <ffmpeg-
> > > devel@ffmpeg.org>
> > > Subject: Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
> > > decode_str() did advance
> > >
> > > On Wed, Apr 16, 2025 at 01:29:02AM +0000, softworkz . wrote:
> > > [...]
> > > > > > This will cause deserialization errors for many people in
> the
> > > world
> > > > > > who are processing FFprobe data.
> > > > >
> > > > > As said, ffprobe should not produce troublesome output
> > > >
> > > > As I said, it cannot be remedied on the FFprobe side without
> making
> > > a
> > >
> > > If you want ffprobe to combine multiple author tags with ";", you
> > > certainly can do that in ffprobe
> >
> > Which by-the-way contradicts most of your earlier arguments against
> > semicolon delimited values.
> 
> does it ?
> 
> 
> >
> > I gain the impression that the actual reason for why you (seemingly)
> > want this, might be for having an actual use case for the duplicate
> key support
> > in AVMap..?  😊
> 
> the AVDictionary struct FFmpeg uses supports multiple
> equal keys since over 9 years:
> commit 4ebf0b109cdb4daa888d69e8294621948168c46c
> Author: Thilo Borgmann <thilo.borgmann@mail.de>
> Date:   Sat Mar 12 14:52:17 2016 +0100
> 
>     lavu/dict: Add new flag to allow multiple equal keys.
> 
> 

Hi Michael,

I think you already know what I would be responding to these things, and it might likely become just more repetitive. I feel that I've sufficiently expressed my view - maybe even a bit more than I think I should, so I'll step back and leave room for others to voice their opinion - if any.

Thanks and nevermind 😊

sw










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

Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that decode_str() did advance

[softworkz .]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Michael Niedermayer
> Sent: Mittwoch, 16. April 2025 03:34
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
> decode_str() did advance
> 
> On Wed, Apr 16, 2025 at 01:29:02AM +0000, softworkz . wrote:
> [...]
> > > > This will cause deserialization errors for many people in the
> world
> > > > who are processing FFprobe data.
> > >
> > > As said, ffprobe should not produce troublesome output

First of all, any patch MUST NOT introduce behavior that goes
against our own specifications.

From avformat.h:

/**
 * @defgroup metadata_api Public Metadata API
 * @{
 * @ingroup libavf
 * The metadata API allows libavformat to export metadata tags to a client
 * application when demuxing. Conversely it allows a client application to
 * set metadata when muxing.
 *
 * Metadata is exported or set as pairs of key/value strings in the 'metadata'
 * fields of the AVFormatContext, AVStream, AVChapter and AVProgram structs
 * using the @ref lavu_dict "AVDictionary" API. Like all strings in FFmpeg,
 * metadata is assumed to be UTF-8 encoded Unicode. Note that metadata
 * exported by demuxers isn't checked to be valid UTF-8 in most cases.
 *
 * Important concepts to keep in mind:
 * -  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.
 * -  Metadata is flat, not hierarchical; there are no subtags. If you
 *    want to store, e.g., the email address of the child of producer Alice
 *    and actor Bob, that could have key=alice_and_bobs_childs_email_address.
 * -  Several modifiers can be applied to the tag name. This is done by
 *    appending a dash character ('-') and the modifier name in the order
 *    they appear in the list below -- e.g. foo-eng-sort, not foo-sort-eng.
 *    -  language -- a tag whose value is localized for a particular language
 *       is appended with the ISO 639-2/B 3-letter language code.
 *       For example: Author-ger=Michael, Author-eng=Mike
 *       The original/default language is in the unqualified "Author" tag.
 *       A demuxer should set a default if it sets any translated tag.
 *    -  sorting  -- a modified version of a tag that should be used for
 *       sorting will have '-sort' appended. E.g. artist="The Beatles",
 *       artist-sort="Beatles, The".


Especially:

*    E.g., key=Author5, key=Author6. In this example, all authors must be
*    placed in the same tag.

I think, this tells very clearly how it's gotta be and how not.


Best,
sw




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

Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that decode_str() did advance

[Michael Niedermayer]

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

Hi softworkz

On Wed, Apr 16, 2025 at 02:52:21AM +0000, softworkz . wrote:
> 
> 
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > Michael Niedermayer
> > Sent: Mittwoch, 16. April 2025 03:34
> > To: FFmpeg development discussions and patches <ffmpeg-
> > devel@ffmpeg.org>
> > Subject: Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
> > decode_str() did advance
> > 
> > On Wed, Apr 16, 2025 at 01:29:02AM +0000, softworkz . wrote:
> > [...]
> > > > > This will cause deserialization errors for many people in the
> > world
> > > > > who are processing FFprobe data.
> > > >
> > > > As said, ffprobe should not produce troublesome output
> 
> First of all, any patch MUST NOT introduce behavior that goes
> against our own specifications.
> 
> From avformat.h:
> 
> /**
>  * @defgroup metadata_api Public Metadata API
>  * @{
>  * @ingroup libavf
>  * The metadata API allows libavformat to export metadata tags to a client
>  * application when demuxing. Conversely it allows a client application to
>  * set metadata when muxing.
>  *
>  * Metadata is exported or set as pairs of key/value strings in the 'metadata'
>  * fields of the AVFormatContext, AVStream, AVChapter and AVProgram structs
>  * using the @ref lavu_dict "AVDictionary" API. Like all strings in FFmpeg,
>  * metadata is assumed to be UTF-8 encoded Unicode. Note that metadata
>  * exported by demuxers isn't checked to be valid UTF-8 in most cases.
>  *
>  * Important concepts to keep in mind:
>  * -  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.
>  * -  Metadata is flat, not hierarchical; there are no subtags. If you
>  *    want to store, e.g., the email address of the child of producer Alice
>  *    and actor Bob, that could have key=alice_and_bobs_childs_email_address.
>  * -  Several modifiers can be applied to the tag name. This is done by
>  *    appending a dash character ('-') and the modifier name in the order
>  *    they appear in the list below -- e.g. foo-eng-sort, not foo-sort-eng.
>  *    -  language -- a tag whose value is localized for a particular language
>  *       is appended with the ISO 639-2/B 3-letter language code.
>  *       For example: Author-ger=Michael, Author-eng=Mike
>  *       The original/default language is in the unqualified "Author" tag.
>  *       A demuxer should set a default if it sets any translated tag.
>  *    -  sorting  -- a modified version of a tag that should be used for
>  *       sorting will have '-sort' appended. E.g. artist="The Beatles",
>  *       artist-sort="Beatles, The".
> 
> 
> Especially:
> 
> *    E.g., key=Author5, key=Author6. In this example, all authors must be
> *    placed in the same tag.
> 
> I think, this tells very clearly how it's gotta be and how not.

This is written by me 16 years ago
and it made sense at the time. Our APIs did not support multiple values per
key.
The API supports multiple values per key since 9 years. Using said API is
more convenient than having to parse and escape ";" around.
Thats for our code, its simpler for a muxer to iterate over a AVDictionary key
than parse a ";" seperated string (with undefined escaping rules).
and easier to build a ";" string from a multi-value AVDictionary than to assume
the internal ";" escaping rules (whatever they would be) match the target format.

commit 47146dfbf6bca94dd0706b4313cc5e26edaf18d4
Author: Michael Niedermayer <michaelni@gmx.at>
Date:   Sun Jan 4 18:48:37 2009 +0000

    Generic metadata API.
    avi is updated as example.
    No version bump, the API still might change slightly ...
    No update to ffmpeg.c as requested by aurel.

    Originally committed as revision 16424 to svn://svn.ffmpeg.org/ffmpeg/trunk

diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index e45f7d6dfb3..7038d2d2c41 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -400,6 +400,51 @@ enum SampleFormat {
  */
 #define FF_MIN_BUFFER_SIZE 16384

+
+/*
+ * public Metadata API.
+ * Important concepts, to keep in mind
+ * 1. keys are unique, there are never 2 tags with equal keys, this is also
+ *    meant semantically that is a demuxer should not knowingly produce
+ *    several keys that are litterally different but semantically identical,
+ *    like key=Author5, key=Author6.
+ *    All authors have to be placed in the same tag for the case of Authors.
+ * 2. Metadata is flat, there are no subtags, if you for whatever obscene
+ *    reason want to store the email address of the child of producer alice
+ *    and actor bob, that could have key=alice_and_bobs_childs_email_address.
+ * 3. A tag whichs value is translated has the ISO 639 3-letter language code
+ *    with a '-' between appended. So for example Author-ger=Michael, Author-eng=Mike
+ *    the original/default language is in the unqualified "Author"
+ *    A demuxer should set a default if it sets any translated tag.
+ */

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Freedom in capitalist society always remains about the same as it was in
ancient Greek republics: Freedom for slave owners. -- Vladimir Lenin

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

Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that decode_str() did advance

[Andreas Rheinhardt]

Michael Niedermayer:
> Hi softworkz
> 
> On Wed, Apr 16, 2025 at 02:52:21AM +0000, softworkz . wrote:
>>
>>
>>> -----Original Message-----
>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
>>> Michael Niedermayer
>>> Sent: Mittwoch, 16. April 2025 03:34
>>> To: FFmpeg development discussions and patches <ffmpeg-
>>> devel@ffmpeg.org>
>>> Subject: Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
>>> decode_str() did advance
>>>
>>> On Wed, Apr 16, 2025 at 01:29:02AM +0000, softworkz . wrote:
>>> [...]
>>>>>> This will cause deserialization errors for many people in the
>>> world
>>>>>> who are processing FFprobe data.
>>>>>
>>>>> As said, ffprobe should not produce troublesome output
>>
>> First of all, any patch MUST NOT introduce behavior that goes
>> against our own specifications.
>>
>> From avformat.h:
>>
>> /**
>>  * @defgroup metadata_api Public Metadata API
>>  * @{
>>  * @ingroup libavf
>>  * The metadata API allows libavformat to export metadata tags to a client
>>  * application when demuxing. Conversely it allows a client application to
>>  * set metadata when muxing.
>>  *
>>  * Metadata is exported or set as pairs of key/value strings in the 'metadata'
>>  * fields of the AVFormatContext, AVStream, AVChapter and AVProgram structs
>>  * using the @ref lavu_dict "AVDictionary" API. Like all strings in FFmpeg,
>>  * metadata is assumed to be UTF-8 encoded Unicode. Note that metadata
>>  * exported by demuxers isn't checked to be valid UTF-8 in most cases.
>>  *
>>  * Important concepts to keep in mind:
>>  * -  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.
>>  * -  Metadata is flat, not hierarchical; there are no subtags. If you
>>  *    want to store, e.g., the email address of the child of producer Alice
>>  *    and actor Bob, that could have key=alice_and_bobs_childs_email_address.
>>  * -  Several modifiers can be applied to the tag name. This is done by
>>  *    appending a dash character ('-') and the modifier name in the order
>>  *    they appear in the list below -- e.g. foo-eng-sort, not foo-sort-eng.
>>  *    -  language -- a tag whose value is localized for a particular language
>>  *       is appended with the ISO 639-2/B 3-letter language code.
>>  *       For example: Author-ger=Michael, Author-eng=Mike
>>  *       The original/default language is in the unqualified "Author" tag.
>>  *       A demuxer should set a default if it sets any translated tag.
>>  *    -  sorting  -- a modified version of a tag that should be used for
>>  *       sorting will have '-sort' appended. E.g. artist="The Beatles",
>>  *       artist-sort="Beatles, The".
>>
>>
>> Especially:
>>
>> *    E.g., key=Author5, key=Author6. In this example, all authors must be
>> *    placed in the same tag.
>>
>> I think, this tells very clearly how it's gotta be and how not.
> 
> This is written by me 16 years ago
> and it made sense at the time. Our APIs did not support multiple values per
> key.
> The API supports multiple values per key since 9 years. Using said API is
> more convenient than having to parse and escape ";" around.
> Thats for our code, its simpler for a muxer to iterate over a AVDictionary key
> than parse a ";" seperated string (with undefined escaping rules).
> and easier to build a ";" string from a multi-value AVDictionary than to assume
> the internal ";" escaping rules (whatever they would be) match the target format.

This is all true, but it does not change that we are bound by our API
guarantees.

> 
> commit 47146dfbf6bca94dd0706b4313cc5e26edaf18d4
> Author: Michael Niedermayer <michaelni@gmx.at>
> Date:   Sun Jan 4 18:48:37 2009 +0000
> 
>     Generic metadata API.
>     avi is updated as example.
>     No version bump, the API still might change slightly ...
>     No update to ffmpeg.c as requested by aurel.
> 
>     Originally committed as revision 16424 to svn://svn.ffmpeg.org/ffmpeg/trunk
> 
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index e45f7d6dfb3..7038d2d2c41 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -400,6 +400,51 @@ enum SampleFormat {
>   */
>  #define FF_MIN_BUFFER_SIZE 16384
> 
> +
> +/*
> + * public Metadata API.
> + * Important concepts, to keep in mind
> + * 1. keys are unique, there are never 2 tags with equal keys, this is also
> + *    meant semantically that is a demuxer should not knowingly produce
> + *    several keys that are litterally different but semantically identical,
> + *    like key=Author5, key=Author6.
> + *    All authors have to be placed in the same tag for the case of Authors.
> + * 2. Metadata is flat, there are no subtags, if you for whatever obscene
> + *    reason want to store the email address of the child of producer alice
> + *    and actor bob, that could have key=alice_and_bobs_childs_email_address.
> + * 3. A tag whichs value is translated has the ISO 639 3-letter language code
> + *    with a '-' between appended. So for example Author-ger=Michael, Author-eng=Mike
> + *    the original/default language is in the unqualified "Author"
> + *    A demuxer should set a default if it sets any translated tag.
> + */
> 
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that decode_str() did advance

[Michael Niedermayer]

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

Hi Andreas

On Wed, Apr 16, 2025 at 12:57:27PM +0200, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > Hi softworkz
> > 
> > On Wed, Apr 16, 2025 at 02:52:21AM +0000, softworkz . wrote:
> >>
> >>
> >>> -----Original Message-----
> >>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> >>> Michael Niedermayer
> >>> Sent: Mittwoch, 16. April 2025 03:34
> >>> To: FFmpeg development discussions and patches <ffmpeg-
> >>> devel@ffmpeg.org>
> >>> Subject: Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
> >>> decode_str() did advance
> >>>
> >>> On Wed, Apr 16, 2025 at 01:29:02AM +0000, softworkz . wrote:
> >>> [...]
> >>>>>> This will cause deserialization errors for many people in the
> >>> world
> >>>>>> who are processing FFprobe data.
> >>>>>
> >>>>> As said, ffprobe should not produce troublesome output
> >>
> >> First of all, any patch MUST NOT introduce behavior that goes
> >> against our own specifications.
> >>
> >> From avformat.h:
> >>
> >> /**
> >>  * @defgroup metadata_api Public Metadata API
> >>  * @{
> >>  * @ingroup libavf
> >>  * The metadata API allows libavformat to export metadata tags to a client
> >>  * application when demuxing. Conversely it allows a client application to
> >>  * set metadata when muxing.
> >>  *
> >>  * Metadata is exported or set as pairs of key/value strings in the 'metadata'
> >>  * fields of the AVFormatContext, AVStream, AVChapter and AVProgram structs
> >>  * using the @ref lavu_dict "AVDictionary" API. Like all strings in FFmpeg,
> >>  * metadata is assumed to be UTF-8 encoded Unicode. Note that metadata
> >>  * exported by demuxers isn't checked to be valid UTF-8 in most cases.
> >>  *
> >>  * Important concepts to keep in mind:
> >>  * -  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.
> >>  * -  Metadata is flat, not hierarchical; there are no subtags. If you
> >>  *    want to store, e.g., the email address of the child of producer Alice
> >>  *    and actor Bob, that could have key=alice_and_bobs_childs_email_address.
> >>  * -  Several modifiers can be applied to the tag name. This is done by
> >>  *    appending a dash character ('-') and the modifier name in the order
> >>  *    they appear in the list below -- e.g. foo-eng-sort, not foo-sort-eng.
> >>  *    -  language -- a tag whose value is localized for a particular language
> >>  *       is appended with the ISO 639-2/B 3-letter language code.
> >>  *       For example: Author-ger=Michael, Author-eng=Mike
> >>  *       The original/default language is in the unqualified "Author" tag.
> >>  *       A demuxer should set a default if it sets any translated tag.
> >>  *    -  sorting  -- a modified version of a tag that should be used for
> >>  *       sorting will have '-sort' appended. E.g. artist="The Beatles",
> >>  *       artist-sort="Beatles, The".
> >>
> >>
> >> Especially:
> >>
> >> *    E.g., key=Author5, key=Author6. In this example, all authors must be
> >> *    placed in the same tag.
> >>
> >> I think, this tells very clearly how it's gotta be and how not.
> > 
> > This is written by me 16 years ago
> > and it made sense at the time. Our APIs did not support multiple values per
> > key.
> > The API supports multiple values per key since 9 years. Using said API is
> > more convenient than having to parse and escape ";" around.
> > Thats for our code, its simpler for a muxer to iterate over a AVDictionary key
> > than parse a ";" seperated string (with undefined escaping rules).
> > and easier to build a ";" string from a multi-value AVDictionary than to assume
> > the internal ";" escaping rules (whatever they would be) match the target format.
> 
> This is all true, but it does not change that we are bound by our API
> guarantees.

yes, as i said i will revert the patch, iam just a bit behind with what
i want to do and it happening in reality

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

The worst form of inequality is to try to make unequal things equal.
-- Aristotle

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

Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that decode_str() did advance

[softworkz .]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Michael Niedermayer
> Sent: Mittwoch, 16. April 2025 12:53
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
> decode_str() did advance
> 
> Hi softworkz
> 
> On Wed, Apr 16, 2025 at 02:52:21AM +0000, softworkz . wrote:
> >
> >
> > > -----Original Message-----
> > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > > Michael Niedermayer
> > > Sent: Mittwoch, 16. April 2025 03:34
> > > To: FFmpeg development discussions and patches <ffmpeg-
> > > devel@ffmpeg.org>
> > > Subject: Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
> > > decode_str() did advance
> > >
> > > On Wed, Apr 16, 2025 at 01:29:02AM +0000, softworkz . wrote:
> > > [...]
> > > > > > This will cause deserialization errors for many people in
> the
> > > world
> > > > > > who are processing FFprobe data.
> > > > >
> > > > > As said, ffprobe should not produce troublesome output
> >
> > First of all, any patch MUST NOT introduce behavior that goes
> > against our own specifications.
> >
> > From avformat.h:
> >
> > /**
> >  * @defgroup metadata_api Public Metadata API
> >  * @{
> >  * @ingroup libavf
> >  * The metadata API allows libavformat to export metadata tags to a
> client
> >  * application when demuxing. Conversely it allows a client
> application to
> >  * set metadata when muxing.
> >  *
> >  * Metadata is exported or set as pairs of key/value strings in the
> 'metadata'
> >  * fields of the AVFormatContext, AVStream, AVChapter and AVProgram
> structs
> >  * using the @ref lavu_dict "AVDictionary" API. Like all strings in
> FFmpeg,
> >  * metadata is assumed to be UTF-8 encoded Unicode. Note that
> metadata
> >  * exported by demuxers isn't checked to be valid UTF-8 in most
> cases.
> >  *
> >  * Important concepts to keep in mind:
> >  * -  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.
> >  * -  Metadata is flat, not hierarchical; there are no subtags. If
> you
> >  *    want to store, e.g., the email address of the child of
> producer Alice
> >  *    and actor Bob, that could have
> key=alice_and_bobs_childs_email_address.
> >  * -  Several modifiers can be applied to the tag name. This is done
> by
> >  *    appending a dash character ('-') and the modifier name in the
> order
> >  *    they appear in the list below -- e.g. foo-eng-sort, not foo-
> sort-eng.
> >  *    -  language -- a tag whose value is localized for a particular
> language
> >  *       is appended with the ISO 639-2/B 3-letter language code.
> >  *       For example: Author-ger=Michael, Author-eng=Mike
> >  *       The original/default language is in the unqualified
> "Author" tag.
> >  *       A demuxer should set a default if it sets any translated
> tag.
> >  *    -  sorting  -- a modified version of a tag that should be used
> for
> >  *       sorting will have '-sort' appended. E.g. artist="The
> Beatles",
> >  *       artist-sort="Beatles, The".
> >
> >
> > Especially:
> >
> > *    E.g., key=Author5, key=Author6. In this example, all authors
> must be
> > *    placed in the same tag.
> >
> > I think, this tells very clearly how it's gotta be and how not.
> 
> This is written by me 16 years ago
> and it made sense at the time. Our APIs did not support multiple
> values per
> key.
> The API supports multiple values per key since 9 years. Using said API


But it wasn't enabled for metadata, only the recombs patch enabled that, no?

(but, see next message)
sw


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

Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that decode_str() did advance

[softworkz .]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Michael Niedermayer
> Sent: Dienstag, 15. April 2025 01:20
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
> decode_str() did advance
> 
> On Sat, Apr 12, 2025 at 01:49:53AM +0000, softworkz . wrote:
> >
> >
> > > -----Original Message-----
> > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > > Michael Niedermayer
> > > Sent: Samstag, 12. April 2025 00:27
> > > To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> > > Subject: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
> > > decode_str() did advance
> > >
> > > Fixes infinite loop with unknown encodings
> > >
> > > We could alternatively error out from decode_str() or consume all
> of
> > > taglen
> > > this would affect other callers though.
> > >
> > > Fixes: 409819224/clusterfuzz-testcase-minimized-
> ffmpeg_dem_H261_fuzzer-
> > > 6003527535362048
> > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > ---
> > >  libavformat/id3v2.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c
> > > index 90314583a74..e3f7f9e2a90 100644
> > > --- a/libavformat/id3v2.c
> > > +++ b/libavformat/id3v2.c
> > > @@ -341,10 +341,13 @@ static void read_ttag(AVFormatContext *s,
> > > AVIOContext *pb, int taglen,
> > >      taglen--; /* account for encoding type byte */
> > >
> > >      while (taglen > 1) {
> > > +        int current_taglen = taglen;
> > >          if (decode_str(s, pb, encoding, &dst, &taglen) < 0) {
> > >              av_log(s, AV_LOG_ERROR, "Error reading frame %s,
> > > skipped\n", key);
> > >              return;
> > >          }
> > > +        if (current_taglen == taglen)
> > > +            return;
> > >
> > >          count++;
> > >
> > > --
> > > 2.49.0
> > >
> > > _______________________________________________
> >
> > Hi Michael,
> >
> > this kind of conflicts with this patch that I had submitted
> recently:
> >
> >
> https://patchwork.ffmpeg.org/project/ffmpeg/patch/pull.54.ffstaging.FF
> mpeg.1740873449247.ffmpegagent@gmail.com/
> >
> >
> > I wonder whether my patch would still be prone to the issue your
> patch is addressing -
> 
> This already conflicts with rcombs patch in git master, i think
> Applying: Fixes Trac ticket https://trac.ffmpeg.org/ticket/6949
> Using index info to reconstruct a base tree...
> M	libavformat/id3v2.c
> Falling back to patching base and 3-way merge...
> Auto-merging libavformat/id3v2.c
> CONFLICT (content): Merge conflict in libavformat/id3v2.c
> error: Failed to merge in the changes.
> Patch failed at 0001 Fixes Trac ticket
> https://trac.ffmpeg.org/ticket/6949
> 
> 
> > do you have a test file perhaps?
> 
> Will email you one, but the loop with a function that doesnt advance
> is an issue even if the specific file doesnt trigger it in a different
> implementation


Thanks a lot for the test file. I was able to reproduce the eternal loop
that you were intending to fix, but I noticed that after removing the 
patches from rcombs, that endless loop doesn't happen in the first place.

Best,
sw












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

Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that decode_str() did advance

[Michael Niedermayer]

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

Hi

On Tue, Apr 15, 2025 at 01:37:56AM +0000, softworkz . wrote:
[...]
> > > do you have a test file perhaps?
> > 
> > Will email you one, but the loop with a function that doesnt advance
> > is an issue even if the specific file doesnt trigger it in a different
> > implementation
> 
> 
> Thanks a lot for the test file. I was able to reproduce the eternal loop
> that you were intending to fix, but I noticed that after removing the 
> patches from rcombs, that endless loop doesn't happen in the first place.

the patch was intended to fix that regression yes. But your patch contains
a very similarly looking loop to what caused that. Which is what i meant,
that because it looks similar it should be checked for that

thx

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

If a bugfix only changes things apparently unrelated to the bug with no
further explanation, that is a good sign that the bugfix is wrong.

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

Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that decode_str() did advance

[softworkz .]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Michael Niedermayer
> Sent: Dienstag, 15. April 2025 20:26
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
> decode_str() did advance
> 
> Hi
> 
> On Tue, Apr 15, 2025 at 01:37:56AM +0000, softworkz . wrote:
> [...]
> > > > do you have a test file perhaps?
> > >
> > > Will email you one, but the loop with a function that doesnt
> advance
> > > is an issue even if the specific file doesnt trigger it in a
> different
> > > implementation
> >
> >
> > Thanks a lot for the test file. I was able to reproduce the eternal
> loop
> > that you were intending to fix, but I noticed that after removing
> the
> > patches from rcombs, that endless loop doesn't happen in the first
> place.
> 
> the patch was intended to fix that regression yes. But your patch
> contains
> a very similarly looking loop to what caused that. Which is what i
> meant,
> that because it looks similar it should be checked for that

Of course, I've checked that and - albeit similar - it does not expose
this eternal-looping problem.

Thanks
sw


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