Re: [FFmpeg-devel] [PATCH] parseutils: add support for ms and us suffix for AV_OPT_TYPE_DURATION

[Aurelien Jacobs]

On Fri, Mar 02, 2018 at 11:59:33PM +0100, Marton Balint wrote:
> 
> On Fri, 2 Mar 2018, Hendrik Leppkes wrote:
> 
> > On Fri, Mar 2, 2018 at 11:19 PM, Aurelien Jacobs  wrote:
> > > On Fri, Mar 02, 2018 at 10:02:58PM +, Rostislav Pehlivanov wrote:
> > > > On 2 March 2018 at 21:57, Aurelien Jacobs  wrote:
> > > > 
> > > > > On Fri, Mar 02, 2018 at 09:39:48PM +0100, Michael Niedermayer wrote:
> > > > > > On Thu, Mar 01, 2018 at 09:41:20PM +0100, Aurelien Jacobs wrote:
> > > > > > > supported suffixes are:
> > > > > > > - s: seconds (default when no suffix specified)
> > > > > > > - m or ms: milliseconds
> > > > > > > - u or us: microseconds
> 
> I don't see much benefit in accepting the SI prefixes without actual
> unit,

Maybe for conciseness... And because the unit is already implied by the
option name. When setting a time the unit is second, when setting a
bitrate the unit is bit per seconds so specifying the unit after the
prefix might be considered redundant.
Not that I personnaly really care. I wouldn't mind writting 128kb/s
rather than 128k.

> the purpose of this whoule patch is intuitive and readable command line, and
> an SI prefix alone, without a unit is not intuitive. So I'd drop the
> SI-prefix-only variants.

I mostly agree, but I allowed the use of SI prefix alone for consistency
with other options (bitrate for example).
I would suggest that all examples in documentations or tutorials should
use full SI prefix + unit for improved readability.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] parseutils: add support for ms and us suffix for AV_OPT_TYPE_DURATION

[Aurelien Jacobs]

On Fri, Mar 02, 2018 at 11:28:29PM +0100, Hendrik Leppkes wrote:
> On Fri, Mar 2, 2018 at 11:19 PM, Aurelien Jacobs  wrote:
> > On Fri, Mar 02, 2018 at 10:02:58PM +, Rostislav Pehlivanov wrote:
> >> On 2 March 2018 at 21:57, Aurelien Jacobs  wrote:
> >>
> >> > On Fri, Mar 02, 2018 at 09:39:48PM +0100, Michael Niedermayer wrote:
> >> > > On Thu, Mar 01, 2018 at 09:41:20PM +0100, Aurelien Jacobs wrote:
> >> > > > supported suffixes are:
> >> > > > - s: seconds (default when no suffix specified)
> >> > > > - m or ms: milliseconds
> >> > > > - u or us: microseconds
> >> > > > ---
> >> > > >  libavutil/parseutils.c | 15 +--
> >> > > >  1 file changed, 13 insertions(+), 2 deletions(-)
> >> > >
> >> > > can some of this and the si_prefixes related code in eval.c be
> >> > > factored ?
> >> >
> >> > I've had a look at this, but this would increase code complexity
> >> > in both eval.c and parseutils.c with no advantage, so I deceided not to.
> >> > Anyway, 'u' and 'm' SI prefix are the only ones that make any sense for
> >> > AV_OPT_TYPE_DURATION and supporting more prefix than those "thanks" to
> >> > common code, might add confusion for end user.
> >> >
> >> > BTW, while looking at si_prefixes, I noticed that both 'k' and 'K' are
> >> > supported for meaning "kilo", which is wrong in the SI. Only 'k' is
> >> > a prefix for "kilo", 'K' is a unit for "Kelvin".
> >> > Not sure if there is actually a good reason for having 'K', if it is
> >> > needed for backward compatibility, or if it should be removed...
> >> >
> >> > > either way this LGTM
> >> >
> >> > Great. Applied.
> >>
> >> I think that was a bit premature, I really think "m" should be for minutes
> >> while "ms" should be for milliseconds.
> >
> > What you (or I) think is not relevant here. We are simply implementing
> > an internationnal standard that define precisely what those letters mean
> > so that people all around the world can communicate without ambiguity.
> >
> 
> Why is it not relevant what we think?

Because what is relevant is what the majority of users will understand
the best, whatever their language, culture or education is.

> Who said this has to follow strict SI rules?

Nobody said this, but the fact the SI rules are already used in other
parts of ffmpeg and that nobody suggested any other widely accepted
rules is already a good start.
The fact that SI rules is by far the most widely used unit and prefix
system around the world, and that it is unambiguously defined, makes
it a strong contender I think.

> It should use postfixes which are the most logical and intuitive,

Provided that you can strictly define "logical" and "intuitive" in a
worldwide context...

> when someone reads "5m" he is the most likely to assume minutes, not
> milliseconds.

Not sure what makes you able to know what everybody is gonna assume ?
Personnaly when I read "5m" out of context, I read 5 meters.
With ffmpeg, when I first set SBC delay, I used 0.005 but right after
this I tried 5m by analogy with -b:a 128k where we only use the SI
prefix without unit (ie. we don't write -b:a 128kb/s).
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] parseutils: add support for ms and us suffix for AV_OPT_TYPE_DURATION

[Marton Balint]

On Fri, 2 Mar 2018, Hendrik Leppkes wrote:


On Fri, Mar 2, 2018 at 11:19 PM, Aurelien Jacobs  wrote:

On Fri, Mar 02, 2018 at 10:02:58PM +, Rostislav Pehlivanov wrote:

On 2 March 2018 at 21:57, Aurelien Jacobs  wrote:

> On Fri, Mar 02, 2018 at 09:39:48PM +0100, Michael Niedermayer wrote:
> > On Thu, Mar 01, 2018 at 09:41:20PM +0100, Aurelien Jacobs wrote:
> > > supported suffixes are:
> > > - s: seconds (default when no suffix specified)
> > > - m or ms: milliseconds
> > > - u or us: microseconds


I don't see much benefit in accepting the SI prefixes without actual unit, 
the purpose of this whoule patch is intuitive and readable command line, 
and an SI prefix alone, without a unit is not intuitive. So I'd drop the 
SI-prefix-only variants.


Thanks,
Marton
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] parseutils: add support for ms and us suffix for AV_OPT_TYPE_DURATION

[Hendrik Leppkes]

On Fri, Mar 2, 2018 at 11:19 PM, Aurelien Jacobs  wrote:
> On Fri, Mar 02, 2018 at 10:02:58PM +, Rostislav Pehlivanov wrote:
>> On 2 March 2018 at 21:57, Aurelien Jacobs  wrote:
>>
>> > On Fri, Mar 02, 2018 at 09:39:48PM +0100, Michael Niedermayer wrote:
>> > > On Thu, Mar 01, 2018 at 09:41:20PM +0100, Aurelien Jacobs wrote:
>> > > > supported suffixes are:
>> > > > - s: seconds (default when no suffix specified)
>> > > > - m or ms: milliseconds
>> > > > - u or us: microseconds
>> > > > ---
>> > > >  libavutil/parseutils.c | 15 +--
>> > > >  1 file changed, 13 insertions(+), 2 deletions(-)
>> > >
>> > > can some of this and the si_prefixes related code in eval.c be
>> > > factored ?
>> >
>> > I've had a look at this, but this would increase code complexity
>> > in both eval.c and parseutils.c with no advantage, so I deceided not to.
>> > Anyway, 'u' and 'm' SI prefix are the only ones that make any sense for
>> > AV_OPT_TYPE_DURATION and supporting more prefix than those "thanks" to
>> > common code, might add confusion for end user.
>> >
>> > BTW, while looking at si_prefixes, I noticed that both 'k' and 'K' are
>> > supported for meaning "kilo", which is wrong in the SI. Only 'k' is
>> > a prefix for "kilo", 'K' is a unit for "Kelvin".
>> > Not sure if there is actually a good reason for having 'K', if it is
>> > needed for backward compatibility, or if it should be removed...
>> >
>> > > either way this LGTM
>> >
>> > Great. Applied.
>> > ___
>> > ffmpeg-devel mailing list
>> > ffmpeg-devel@ffmpeg.org
>> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> >
>>
>> I think that was a bit premature, I really think "m" should be for minutes
>> while "ms" should be for milliseconds.
>
> What you (or I) think is not relevant here. We are simply implementing
> an internationnal standard that define precisely what those letters mean
> so that people all around the world can communicate without ambiguity.
>

Why is it not relevant what we think? Who said this has to follow
strict SI rules?

It should use postfixes which are the most logical and intuitive, and
when someone reads "5m" he is the most likely to assume minutes, not
milliseconds.

- Hendrik
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] parseutils: add support for ms and us suffix for AV_OPT_TYPE_DURATION

[Aurelien Jacobs]

On Fri, Mar 02, 2018 at 10:02:58PM +, Rostislav Pehlivanov wrote:
> On 2 March 2018 at 21:57, Aurelien Jacobs  wrote:
> 
> > On Fri, Mar 02, 2018 at 09:39:48PM +0100, Michael Niedermayer wrote:
> > > On Thu, Mar 01, 2018 at 09:41:20PM +0100, Aurelien Jacobs wrote:
> > > > supported suffixes are:
> > > > - s: seconds (default when no suffix specified)
> > > > - m or ms: milliseconds
> > > > - u or us: microseconds
> > > > ---
> > > >  libavutil/parseutils.c | 15 +--
> > > >  1 file changed, 13 insertions(+), 2 deletions(-)
> > >
> > > can some of this and the si_prefixes related code in eval.c be
> > > factored ?
> >
> > I've had a look at this, but this would increase code complexity
> > in both eval.c and parseutils.c with no advantage, so I deceided not to.
> > Anyway, 'u' and 'm' SI prefix are the only ones that make any sense for
> > AV_OPT_TYPE_DURATION and supporting more prefix than those "thanks" to
> > common code, might add confusion for end user.
> >
> > BTW, while looking at si_prefixes, I noticed that both 'k' and 'K' are
> > supported for meaning "kilo", which is wrong in the SI. Only 'k' is
> > a prefix for "kilo", 'K' is a unit for "Kelvin".
> > Not sure if there is actually a good reason for having 'K', if it is
> > needed for backward compatibility, or if it should be removed...
> >
> > > either way this LGTM
> >
> > Great. Applied.
> > ___
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> 
> I think that was a bit premature, I really think "m" should be for minutes
> while "ms" should be for milliseconds.

What you (or I) think is not relevant here. We are simply implementing
an internationnal standard that define precisely what those letters mean
so that people all around the world can communicate without ambiguity.

Minute is not an SI unit but still accepted for use along with SI units,
and the standard symbol for minutes is 'min'.
'm' is the prefix for milli and nothing else.

This is detailed here:
https://www.bipm.org/en/publications/si-brochure/table6.html

If you think having support for minute is really useful, I can have a
look at supporting 'min'.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] parseutils: add support for ms and us suffix for AV_OPT_TYPE_DURATION

[Rostislav Pehlivanov]

On 2 March 2018 at 21:57, Aurelien Jacobs  wrote:

> On Fri, Mar 02, 2018 at 09:39:48PM +0100, Michael Niedermayer wrote:
> > On Thu, Mar 01, 2018 at 09:41:20PM +0100, Aurelien Jacobs wrote:
> > > supported suffixes are:
> > > - s: seconds (default when no suffix specified)
> > > - m or ms: milliseconds
> > > - u or us: microseconds
> > > ---
> > >  libavutil/parseutils.c | 15 +--
> > >  1 file changed, 13 insertions(+), 2 deletions(-)
> >
> > can some of this and the si_prefixes related code in eval.c be
> > factored ?
>
> I've had a look at this, but this would increase code complexity
> in both eval.c and parseutils.c with no advantage, so I deceided not to.
> Anyway, 'u' and 'm' SI prefix are the only ones that make any sense for
> AV_OPT_TYPE_DURATION and supporting more prefix than those "thanks" to
> common code, might add confusion for end user.
>
> BTW, while looking at si_prefixes, I noticed that both 'k' and 'K' are
> supported for meaning "kilo", which is wrong in the SI. Only 'k' is
> a prefix for "kilo", 'K' is a unit for "Kelvin".
> Not sure if there is actually a good reason for having 'K', if it is
> needed for backward compatibility, or if it should be removed...
>
> > either way this LGTM
>
> Great. Applied.
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

I think that was a bit premature, I really think "m" should be for minutes
while "ms" should be for milliseconds.
Could you post a patch which does that?
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] parseutils: add support for ms and us suffix for AV_OPT_TYPE_DURATION

[Aurelien Jacobs]

On Fri, Mar 02, 2018 at 09:39:48PM +0100, Michael Niedermayer wrote:
> On Thu, Mar 01, 2018 at 09:41:20PM +0100, Aurelien Jacobs wrote:
> > supported suffixes are:
> > - s: seconds (default when no suffix specified)
> > - m or ms: milliseconds
> > - u or us: microseconds
> > ---
> >  libavutil/parseutils.c | 15 +--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> can some of this and the si_prefixes related code in eval.c be
> factored ?

I've had a look at this, but this would increase code complexity
in both eval.c and parseutils.c with no advantage, so I deceided not to.
Anyway, 'u' and 'm' SI prefix are the only ones that make any sense for
AV_OPT_TYPE_DURATION and supporting more prefix than those "thanks" to
common code, might add confusion for end user.

BTW, while looking at si_prefixes, I noticed that both 'k' and 'K' are
supported for meaning "kilo", which is wrong in the SI. Only 'k' is
a prefix for "kilo", 'K' is a unit for "Kelvin".
Not sure if there is actually a good reason for having 'K', if it is
needed for backward compatibility, or if it should be removed...

> either way this LGTM

Great. Applied.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] parseutils: add support for ms and us suffix for AV_OPT_TYPE_DURATION

[Michael Niedermayer]

On Thu, Mar 01, 2018 at 09:41:20PM +0100, Aurelien Jacobs wrote:
> supported suffixes are:
> - s: seconds (default when no suffix specified)
> - m or ms: milliseconds
> - u or us: microseconds
> ---
>  libavutil/parseutils.c | 15 +--
>  1 file changed, 13 insertions(+), 2 deletions(-)

can some of this and the si_prefixes related code in eval.c be
factored ?

either way this LGTM

thanks

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

The greatest way to live with honor in this world is to be what we pretend
to be. -- Socrates


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] parseutils: add support for ms and us suffix for AV_OPT_TYPE_DURATION

[Aurelien Jacobs]

supported suffixes are:
- s: seconds (default when no suffix specified)
- m or ms: milliseconds
- u or us: microseconds
---
 libavutil/parseutils.c | 15 +--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/libavutil/parseutils.c b/libavutil/parseutils.c
index 7ca07b37a1..44c845577a 100644
--- a/libavutil/parseutils.c
+++ b/libavutil/parseutils.c
@@ -590,7 +590,7 @@ int av_parse_time(int64_t *timeval, const char *timestr, 
int duration)
 int64_t t, now64;
 time_t now;
 struct tm dt = { 0 }, tmbuf;
-int today = 0, negative = 0, microseconds = 0;
+int today = 0, negative = 0, microseconds = 0, suffix = 100;
 int i;
 static const char * const date_fmt[] = {
 "%Y - %m - %d",
@@ -689,6 +689,17 @@ 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') {
+suffix = 1000;
+microseconds /= 1000;
+q++;
+} else if (*q == 'u') {
+suffix = 1;
+microseconds = 0;
+q++;
+}
+if (*q == 's')
+q++;
 } else {
 int is_utc = *q == 'Z' || *q == 'z';
 int tzoffset = 0;
@@ -724,7 +735,7 @@ int av_parse_time(int64_t *timeval, const char *timestr, 
int duration)
 if (*q)
 return AVERROR(EINVAL);
 
-t *= 100;
+t *= suffix;
 t += microseconds;
 *timeval = negative ? -t : t;
 return 0;
-- 
2.16.2

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel