[FFmpeg-devel] parseutils: accept only full "ms" and "us" prefixes

Submitted by Rostislav Pehlivanov on March 3, 2018, 8:19 p.m.

Details

Message ID 20180303201943.15000-1-atomnuker@gmail.com
State New
Headers show

Commit Message

Rostislav Pehlivanov March 3, 2018, 8:19 p.m.
The commit which added those was pushed prematurely before anyone could object
to illogical suffixes like just m for milliseconds. Without this, we'd be locked
into never being able to implement the "m" suffix for minutes.

Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com>
---
 libavutil/parseutils.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

Comments

Bodecs Bela March 3, 2018, 8:46 p.m.
2018.03.03. 21:19 keltezéssel, Rostislav Pehlivanov írta:
> The commit which added those was pushed prematurely before anyone could object
> to illogical suffixes like just m for milliseconds. Without this, we'd be locked
> into never being able to implement the "m" suffix for minutes.
>
> Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com>
> ---
>   libavutil/parseutils.c | 10 ++++------
>   1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/libavutil/parseutils.c b/libavutil/parseutils.c
> index 44c845577a..1b81757aab 100644
> --- a/libavutil/parseutils.c
> +++ b/libavutil/parseutils.c
> @@ -689,17 +689,15 @@ int av_parse_time(int64_t *timeval, const char *timestr, int duration)
>   
>       if (duration) {
>           t = dt.tm_hour * 3600 + dt.tm_min * 60 + dt.tm_sec;
> -        if (*q == 'm') {
> +        if (q[0] == 'm' && q[1] == 's') {
>               suffix = 1000;
>               microseconds /= 1000;
> -            q++;
> -        } else if (*q == 'u') {
> +            q += 2;
> +        } else if (q[0] == 'u' && q[1] == 's') {
>               suffix = 1;
>               microseconds = 0;
> -            q++;
> +            q += 2;
>           }
> -        if (*q == 's')
> -            q++;
>       } else {
>           int is_utc = *q == 'Z' || *q == 'z';
>           int tzoffset = 0;
I read the other thread about this problem.
I think the problem is that SI prefixes u (micro) m (mili) h (hecto) d 
(deci) k (kilo) M (mega) etc may overlap with time period names: s 
(second) m (minutes) h (hour) d (day) etc.
Because only second can have prefixes (mili sec, micro sec, nano sec ) 
among time period names, thus your solution is good workaround.
But it will be complete only if we use this logic for nano sec also. But 
I am not sure if ffmpeg uses nano sec anywhere.

bb
Rostislav Pehlivanov March 4, 2018, 10:40 p.m.
On 3 March 2018 at 20:19, Rostislav Pehlivanov <atomnuker@gmail.com> wrote:

> The commit which added those was pushed prematurely before anyone could
> object
> to illogical suffixes like just m for milliseconds. Without this, we'd be
> locked
> into never being able to implement the "m" suffix for minutes.
>
> Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com>
> ---
>  libavutil/parseutils.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/libavutil/parseutils.c b/libavutil/parseutils.c
> index 44c845577a..1b81757aab 100644
> --- a/libavutil/parseutils.c
> +++ b/libavutil/parseutils.c
> @@ -689,17 +689,15 @@ int av_parse_time(int64_t *timeval, const char
> *timestr, int duration)
>
>      if (duration) {
>          t = dt.tm_hour * 3600 + dt.tm_min * 60 + dt.tm_sec;
> -        if (*q == 'm') {
> +        if (q[0] == 'm' && q[1] == 's') {
>              suffix = 1000;
>              microseconds /= 1000;
> -            q++;
> -        } else if (*q == 'u') {
> +            q += 2;
> +        } else if (q[0] == 'u' && q[1] == 's') {
>              suffix = 1;
>              microseconds = 0;
> -            q++;
> +            q += 2;
>          }
> -        if (*q == 's')
> -            q++;
>      } else {
>          int is_utc = *q == 'Z' || *q == 'z';
>          int tzoffset = 0;
> --
> 2.16.2
>
>
I'll push this tomorrow if there are no objections
Aurelien Jacobs March 5, 2018, 4:47 p.m.
On Sat, Mar 03, 2018 at 08:19:43PM +0000, Rostislav Pehlivanov wrote:
> The commit which added those was pushed prematurely before anyone could object
> to illogical suffixes like just m for milliseconds.

What you call illogical is following the same convention as pretty much
all numeric parameters in ffmpeg. (bitrate, bufsize, framerate...)
So it is at least consistent.

> Without this, we'd be locked
> into never being able to implement the "m" suffix for minutes.

I will always be against using "m" for minute, but you can use "min" if
you want.

> Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com>
> ---
>  libavutil/parseutils.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/libavutil/parseutils.c b/libavutil/parseutils.c
> index 44c845577a..1b81757aab 100644
> --- a/libavutil/parseutils.c
> +++ b/libavutil/parseutils.c
> @@ -689,17 +689,15 @@ int av_parse_time(int64_t *timeval, const char *timestr, int duration)
>  
>      if (duration) {
>          t = dt.tm_hour * 3600 + dt.tm_min * 60 + dt.tm_sec;
> -        if (*q == 'm') {
> +        if (q[0] == 'm' && q[1] == 's') {
>              suffix = 1000;
>              microseconds /= 1000;
> -            q++;
> -        } else if (*q == 'u') {
> +            q += 2;
> +        } else if (q[0] == 'u' && q[1] == 's') {
>              suffix = 1;
>              microseconds = 0;
> -            q++;
> +            q += 2;
>          }
> -        if (*q == 's')
> -            q++;
>      } else {
>          int is_utc = *q == 'Z' || *q == 'z';
>          int tzoffset = 0;

Why do you remove support for using just 's' as a unit without prefix ?
I've seen no one complaining about it and I can't see any issue with it.

Also I don't like that this patch makes AV_OPT_TYPE_DURATION
inconsistant with every other numeric parameters, that all accept a SI
prefix without unit.
(that include opus_delay for which "3.5" and "3500m" are equivalent)
Rostislav Pehlivanov March 5, 2018, 8:45 p.m.
On 5 March 2018 at 16:47, Aurelien Jacobs <aurel@gnuage.org> wrote:

> On Sat, Mar 03, 2018 at 08:19:43PM +0000, Rostislav Pehlivanov wrote:
> > The commit which added those was pushed prematurely before anyone could
> object
> > to illogical suffixes like just m for milliseconds.
>
> What you call illogical is following the same convention as pretty much
> all numeric parameters in ffmpeg. (bitrate, bufsize, framerate...)
> So it is at least consistent.
>
> > Without this, we'd be locked
> > into never being able to implement the "m" suffix for minutes.
>
> I will always be against using "m" for minute, but you can use "min" if
> you want.
>
> > Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com>
> > ---
> >  libavutil/parseutils.c | 10 ++++------
> >  1 file changed, 4 insertions(+), 6 deletions(-)
> >
> > diff --git a/libavutil/parseutils.c b/libavutil/parseutils.c
> > index 44c845577a..1b81757aab 100644
> > --- a/libavutil/parseutils.c
> > +++ b/libavutil/parseutils.c
> > @@ -689,17 +689,15 @@ int av_parse_time(int64_t *timeval, const char
> *timestr, int duration)
> >
> >      if (duration) {
> >          t = dt.tm_hour * 3600 + dt.tm_min * 60 + dt.tm_sec;
> > -        if (*q == 'm') {
> > +        if (q[0] == 'm' && q[1] == 's') {
> >              suffix = 1000;
> >              microseconds /= 1000;
> > -            q++;
> > -        } else if (*q == 'u') {
> > +            q += 2;
> > +        } else if (q[0] == 'u' && q[1] == 's') {
> >              suffix = 1;
> >              microseconds = 0;
> > -            q++;
> > +            q += 2;
> >          }
> > -        if (*q == 's')
> > -            q++;
> >      } else {
> >          int is_utc = *q == 'Z' || *q == 'z';
> >          int tzoffset = 0;
>
> Why do you remove support for using just 's' as a unit without prefix ?
> I've seen no one complaining about it and I can't see any issue with it.
>
> Also I don't like that this patch makes AV_OPT_TYPE_DURATION
> inconsistant with every other numeric parameters, that all accept a SI
> prefix without unit.
> (that include opus_delay for which "3.5" and "3500m" are equivalent)
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

Dropped the "us" requirement and applied the patch.
Keeping the SI requirement where it doesn't make sense (and preventing us
from extending the option to something that does make sense) is worse than
being inconsistent.

Patch hide | download patch | download mbox

diff --git a/libavutil/parseutils.c b/libavutil/parseutils.c
index 44c845577a..1b81757aab 100644
--- a/libavutil/parseutils.c
+++ b/libavutil/parseutils.c
@@ -689,17 +689,15 @@  int av_parse_time(int64_t *timeval, const char *timestr, int duration)
 
     if (duration) {
         t = dt.tm_hour * 3600 + dt.tm_min * 60 + dt.tm_sec;
-        if (*q == 'm') {
+        if (q[0] == 'm' && q[1] == 's') {
             suffix = 1000;
             microseconds /= 1000;
-            q++;
-        } else if (*q == 'u') {
+            q += 2;
+        } else if (q[0] == 'u' && q[1] == 's') {
             suffix = 1;
             microseconds = 0;
-            q++;
+            q += 2;
         }
-        if (*q == 's')
-            q++;
     } else {
         int is_utc = *q == 'Z' || *q == 'z';
         int tzoffset = 0;