| 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" | expand |
| 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 |
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".
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".
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".
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 --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;
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(-)