| Message ID | 20180303201943.15000-1-atomnuker@gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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
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)
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.
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;
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(-)