diff mbox series

[FFmpeg-devel] Revert "avutil/timecode: fix sscanf format string with garbage at the end"

Message ID 20210116084942.13945-1-cus@passwd.hu
State Accepted
Commit 16766bf8a8e477da09337801b96f732740a0b279
Headers show
Series [FFmpeg-devel] Revert "avutil/timecode: fix sscanf format string with garbage at the end"
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Marton Balint Jan. 16, 2021, 8:49 a.m. UTC
This reverts commit 6696a07ac62bfec49dd488510a719367918b9f7a.

It is wrong to restrict timecodes to always contain leading zeros or for hours
or frames to be 2 chars only.

Signed-off-by: Marton Balint <cus@passwd.hu>
---
 libavutil/timecode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Limin Wang Jan. 17, 2021, 12:16 a.m. UTC | #1
On Sat, Jan 16, 2021 at 09:49:42AM +0100, Marton Balint wrote:
> This reverts commit 6696a07ac62bfec49dd488510a719367918b9f7a.
> 
> It is wrong to restrict timecodes to always contain leading zeros or for hours
> or frames to be 2 chars only.
Sorry, I think I was misunderstood by the following syntax description:
syntax: hh:mm:ss[:;.]ff

maybe it's better to change it to hour:minute:second[:;.]frame? 

After revisiting the code, I think we may support more valid syntax for the timecode
string:
ss
ss[:;.]ff
mm:ss
mm:ss[:;.]ff
hh:mm:ss
hh:mm:ss[:;.]ff


> 
> Signed-off-by: Marton Balint <cus@passwd.hu>
> ---
>  libavutil/timecode.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavutil/timecode.c b/libavutil/timecode.c
> index 5106f642b9..c1fa445d31 100644
> --- a/libavutil/timecode.c
> +++ b/libavutil/timecode.c
> @@ -252,7 +252,7 @@ int av_timecode_init_from_string(AVTimecode *tc, AVRational rate, const char *st
>      char c;
>      int hh, mm, ss, ff, flags;
>  
> -    if (sscanf(str, "%02u:%02u:%02u%c%02u", &hh, &mm, &ss, &c, &ff) != 5) {
> +    if (sscanf(str, "%d:%d:%d%c%d", &hh, &mm, &ss, &c, &ff) != 5) {
>          av_log(log_ctx, AV_LOG_ERROR, "Unable to parse timecode, "
>                                        "syntax: hh:mm:ss[:;.]ff\n");
>          return AVERROR_INVALIDDATA;
> -- 
> 2.26.2
> 
> _______________________________________________
> 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".
Marton Balint Jan. 17, 2021, 2:30 a.m. UTC | #2
On Sun, 17 Jan 2021, lance.lmwang@gmail.com wrote:

> On Sat, Jan 16, 2021 at 09:49:42AM +0100, Marton Balint wrote:
>> This reverts commit 6696a07ac62bfec49dd488510a719367918b9f7a.
>> 
>> It is wrong to restrict timecodes to always contain leading zeros or for hours
>> or frames to be 2 chars only.
> Sorry, I think I was misunderstood by the following syntax description:
> syntax: hh:mm:ss[:;.]ff
>
> maybe it's better to change it to hour:minute:second[:;.]frame?

That would better reflect on what the code did before the patch.

>
> After revisiting the code, I think we may support more valid syntax for the timecode
> string:
> ss
> ss[:;.]ff
> mm:ss
> mm:ss[:;.]ff
> hh:mm:ss
> hh:mm:ss[:;.]ff

I don't like this idea much, it is good if we are strict about the 
timecode format (e.g request all components to be present and no garbage 
after the parsed string), the main reasons I suggested the revert are 
because timecode has to support > 100 fps and >= 100 hours because our 
timecode API also has support for such timecodes.

Regards,
Marton

>
>
>> 
>> Signed-off-by: Marton Balint <cus@passwd.hu>
>> ---
>>  libavutil/timecode.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/libavutil/timecode.c b/libavutil/timecode.c
>> index 5106f642b9..c1fa445d31 100644
>> --- a/libavutil/timecode.c
>> +++ b/libavutil/timecode.c
>> @@ -252,7 +252,7 @@ int av_timecode_init_from_string(AVTimecode *tc, AVRational rate, const char *st
>>      char c;
>>      int hh, mm, ss, ff, flags;
>> 
>> -    if (sscanf(str, "%02u:%02u:%02u%c%02u", &hh, &mm, &ss, &c, &ff) != 5) {
>> +    if (sscanf(str, "%d:%d:%d%c%d", &hh, &mm, &ss, &c, &ff) != 5) {
>>          av_log(log_ctx, AV_LOG_ERROR, "Unable to parse timecode, "
>>                                        "syntax: hh:mm:ss[:;.]ff\n");
>>          return AVERROR_INVALIDDATA;
>> -- 
>> 2.26.2
>> 
>> _______________________________________________
>> 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".
>
> -- 
> Thanks,
> Limin Wang
> _______________________________________________
> 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".
Limin Wang Jan. 17, 2021, 11:27 a.m. UTC | #3
On Sun, Jan 17, 2021 at 03:30:15AM +0100, Marton Balint wrote:
> 
> On Sun, 17 Jan 2021, lance.lmwang@gmail.com wrote:
> 
> > On Sat, Jan 16, 2021 at 09:49:42AM +0100, Marton Balint wrote:
> > > This reverts commit 6696a07ac62bfec49dd488510a719367918b9f7a.
> > > 
> > > It is wrong to restrict timecodes to always contain leading zeros or for hours
> > > or frames to be 2 chars only.
> > Sorry, I think I was misunderstood by the following syntax description:
> > syntax: hh:mm:ss[:;.]ff
> > 
> > maybe it's better to change it to hour:minute:second[:;.]frame?
> 
> That would better reflect on what the code did before the patch.
> 
> > 
> > After revisiting the code, I think we may support more valid syntax for the timecode
> > string:
> > ss
> > ss[:;.]ff
> > mm:ss
> > mm:ss[:;.]ff
> > hh:mm:ss
> > hh:mm:ss[:;.]ff
> 
> I don't like this idea much, it is good if we are strict about the timecode
> format (e.g request all components to be present and no garbage after the
> parsed string), the main reasons I suggested the revert are because timecode
> has to support > 100 fps and >= 100 hours because our timecode API also has
> support for such timecodes.

Sure, please revert it anyway.

> 
> Regards,
> Marton
> 
> > 
> > 
> > > 
> > > Signed-off-by: Marton Balint <cus@passwd.hu>
> > > ---
> > >  libavutil/timecode.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/libavutil/timecode.c b/libavutil/timecode.c
> > > index 5106f642b9..c1fa445d31 100644
> > > --- a/libavutil/timecode.c
> > > +++ b/libavutil/timecode.c
> > > @@ -252,7 +252,7 @@ int av_timecode_init_from_string(AVTimecode *tc, AVRational rate, const char *st
> > >      char c;
> > >      int hh, mm, ss, ff, flags;
> > > 
> > > -    if (sscanf(str, "%02u:%02u:%02u%c%02u", &hh, &mm, &ss, &c, &ff) != 5) {
> > > +    if (sscanf(str, "%d:%d:%d%c%d", &hh, &mm, &ss, &c, &ff) != 5) {
> > >          av_log(log_ctx, AV_LOG_ERROR, "Unable to parse timecode, "
> > >                                        "syntax: hh:mm:ss[:;.]ff\n");
> > >          return AVERROR_INVALIDDATA;
> > > -- 
> > > 2.26.2
> > > 
> > > _______________________________________________
> > > 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".
> > 
> > -- 
> > Thanks,
> > Limin Wang
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > 
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Marton Balint Jan. 23, 2021, 7:07 p.m. UTC | #4
On Sun, 17 Jan 2021, lance.lmwang@gmail.com wrote:

> On Sun, Jan 17, 2021 at 03:30:15AM +0100, Marton Balint wrote:
>> 
>> On Sun, 17 Jan 2021, lance.lmwang@gmail.com wrote:
>> 
>> > On Sat, Jan 16, 2021 at 09:49:42AM +0100, Marton Balint wrote:
>> > > This reverts commit 6696a07ac62bfec49dd488510a719367918b9f7a.
>> > > 
>> > > It is wrong to restrict timecodes to always contain leading zeros or for hours
>> > > or frames to be 2 chars only.
>> > Sorry, I think I was misunderstood by the following syntax description:
>> > syntax: hh:mm:ss[:;.]ff
>> > 
>> > maybe it's better to change it to hour:minute:second[:;.]frame?
>> 
>> That would better reflect on what the code did before the patch.
>> 
>> > 
>> > After revisiting the code, I think we may support more valid syntax for the timecode
>> > string:
>> > ss
>> > ss[:;.]ff
>> > mm:ss
>> > mm:ss[:;.]ff
>> > hh:mm:ss
>> > hh:mm:ss[:;.]ff
>> 
>> I don't like this idea much, it is good if we are strict about the timecode
>> format (e.g request all components to be present and no garbage after the
>> parsed string), the main reasons I suggested the revert are because timecode
>> has to support > 100 fps and >= 100 hours because our timecode API also has
>> support for such timecodes.
>
> Sure, please revert it anyway.

Ok, thanks, reverted.

Regards,
Marton
diff mbox series

Patch

diff --git a/libavutil/timecode.c b/libavutil/timecode.c
index 5106f642b9..c1fa445d31 100644
--- a/libavutil/timecode.c
+++ b/libavutil/timecode.c
@@ -252,7 +252,7 @@  int av_timecode_init_from_string(AVTimecode *tc, AVRational rate, const char *st
     char c;
     int hh, mm, ss, ff, flags;
 
-    if (sscanf(str, "%02u:%02u:%02u%c%02u", &hh, &mm, &ss, &c, &ff) != 5) {
+    if (sscanf(str, "%d:%d:%d%c%d", &hh, &mm, &ss, &c, &ff) != 5) {
         av_log(log_ctx, AV_LOG_ERROR, "Unable to parse timecode, "
                                       "syntax: hh:mm:ss[:;.]ff\n");
         return AVERROR_INVALIDDATA;