| Message ID | 20180306000248.31772-1-cus@passwd.hu |
|---|---|
| State | Accepted |
| Commit | 8d37dd6ed3bc3de6b6aa2bdc46f87e842ee19054 |
| Headers | show |
On Tue, 6 Mar 2018, Marton Balint wrote: > Accepting 'u' suffix for a time specification is neither intuitive nor > consistent (now that we don't accept m). Also there was a bug in the code > accepting an extra 's' even after 'ms'. > > Signed-off-by: Marton Balint <cus@passwd.hu> > --- > libavutil/parseutils.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) Will apply this soon. Regards, Marton > > diff --git a/libavutil/parseutils.c b/libavutil/parseutils.c > index 95274f564f..924c49d52c 100644 > --- a/libavutil/parseutils.c > +++ b/libavutil/parseutils.c > @@ -693,12 +693,11 @@ int av_parse_time(int64_t *timeval, const char *timestr, int duration) > suffix = 1000; > microseconds /= 1000; > q += 2; > - } else if (*q == 'u') { > + } else if (q[0] == 'u' && q[1] == 's') { > suffix = 1; > microseconds = 0; > - q++; > - } > - if (*q == 's') > + q += 2; > + } else if (*q == 's') > q++; > } else { > int is_utc = *q == 'Z' || *q == 'z'; > -- > 2.13.6 > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On Tue, Mar 06, 2018 at 01:02:48AM +0100, Marton Balint wrote: > Accepting 'u' suffix for a time specification is neither intuitive nor > consistent (now that we don't accept m). The 'm' SI prefix is still accepted in various time options, and the 'u' prefix is still accepted in those options even after your patch, so you can't really argue that this patch improve consistency. (eg. -black_min_duration 5ms is still accepted). So this will surprise nobody that I don't like this patch. > Also there was a bug in the code > accepting an extra 's' even after 'ms'. Indeed, removing support for the 'm' prefix alone introduced this bug and that needs to be fixed.
On Wed, 7 Mar 2018, Aurelien Jacobs wrote: > On Tue, Mar 06, 2018 at 01:02:48AM +0100, Marton Balint wrote: >> Accepting 'u' suffix for a time specification is neither intuitive nor >> consistent (now that we don't accept m). > > The 'm' SI prefix is still accepted in various time options, and the 'u' > prefix is still accepted in those options even after your patch, so you > can't really argue that this patch improve consistency. > (eg. -black_min_duration 5ms is still accepted). > So this will surprise nobody that I don't like this patch. This really is a cursed topic, I am not sure I follow, after the patch: 5ms is accepted 5us is accepted 5m is not accepted 5u is not accepted You really insist on accepting '5u'? If not, then I can push the patch as is, right? Thanks, Marton
On Wed, Mar 07, 2018 at 11:45:03PM +0100, Marton Balint wrote: > > > On Wed, 7 Mar 2018, Aurelien Jacobs wrote: > > > On Tue, Mar 06, 2018 at 01:02:48AM +0100, Marton Balint wrote: > > > Accepting 'u' suffix for a time specification is neither intuitive nor > > > consistent (now that we don't accept m). > > > > The 'm' SI prefix is still accepted in various time options, and the 'u' > > prefix is still accepted in those options even after your patch, so you > > can't really argue that this patch improve consistency. > > (eg. -black_min_duration 5ms is still accepted). > > So this will surprise nobody that I don't like this patch. > > This really is a cursed topic, I am not sure I follow, after the patch: > > 5ms is accepted > 5us is accepted > 5m is not accepted > 5u is not accepted That is only for AV_OPT_TYPE_DURATION. All other numeric options type still accept SI prefix without unit. This include various time options such as black_min_duration. So after the patch, for black_min_duration: 5m is accepted 5u is accepted > You really insist on accepting '5u'? I'm not insisting on this (even if I prefer it), but I'm saying that your patch is *not* removing '5u' support at least for *some* time options, so it is actually decreasing consistency.
On Thu, Mar 8, 2018 at 12:05 AM, Aurelien Jacobs <aurel@gnuage.org> wrote: > On Wed, Mar 07, 2018 at 11:45:03PM +0100, Marton Balint wrote: >> >> >> On Wed, 7 Mar 2018, Aurelien Jacobs wrote: >> >> > On Tue, Mar 06, 2018 at 01:02:48AM +0100, Marton Balint wrote: >> > > Accepting 'u' suffix for a time specification is neither intuitive nor >> > > consistent (now that we don't accept m). >> > >> > The 'm' SI prefix is still accepted in various time options, and the 'u' >> > prefix is still accepted in those options even after your patch, so you >> > can't really argue that this patch improve consistency. >> > (eg. -black_min_duration 5ms is still accepted). >> > So this will surprise nobody that I don't like this patch. >> >> This really is a cursed topic, I am not sure I follow, after the patch: >> >> 5ms is accepted >> 5us is accepted >> 5m is not accepted >> 5u is not accepted > > That is only for AV_OPT_TYPE_DURATION. > All other numeric options type still accept SI prefix without unit. > This include various time options such as black_min_duration. > So after the patch, for black_min_duration: > > 5m is accepted > 5u is accepted > Because those use AV_OPT_TYPE_DOUBLE, a generic type without any possiblity of a unit. Ideally those should all be transitioned to AV_OPT_TYPE_DURATION if they refer to a time for consistency, and not the actual time-type bend to reflect some generic type. Why would we have the duration type if its just a copy of the double type anyway? - Hendrik
On Thu, Mar 08, 2018 at 12:14:00AM +0100, Hendrik Leppkes wrote: > On Thu, Mar 8, 2018 at 12:05 AM, Aurelien Jacobs <aurel@gnuage.org> wrote: > > On Wed, Mar 07, 2018 at 11:45:03PM +0100, Marton Balint wrote: > >> > >> > >> On Wed, 7 Mar 2018, Aurelien Jacobs wrote: > >> > >> > On Tue, Mar 06, 2018 at 01:02:48AM +0100, Marton Balint wrote: > >> > > Accepting 'u' suffix for a time specification is neither intuitive nor > >> > > consistent (now that we don't accept m). > >> > > >> > The 'm' SI prefix is still accepted in various time options, and the 'u' > >> > prefix is still accepted in those options even after your patch, so you > >> > can't really argue that this patch improve consistency. > >> > (eg. -black_min_duration 5ms is still accepted). > >> > So this will surprise nobody that I don't like this patch. > >> > >> This really is a cursed topic, I am not sure I follow, after the patch: > >> > >> 5ms is accepted > >> 5us is accepted > >> 5m is not accepted > >> 5u is not accepted > > > > That is only for AV_OPT_TYPE_DURATION. > > All other numeric options type still accept SI prefix without unit. > > This include various time options such as black_min_duration. > > So after the patch, for black_min_duration: > > > > 5m is accepted > > 5u is accepted > > > > Because those use AV_OPT_TYPE_DOUBLE, a generic type without any > possiblity of a unit. Of course, I know this. > Ideally those should all be transitioned to AV_OPT_TYPE_DURATION if > they refer to a time for consistency, Of course that's what we ideally want in the end. But it is not that trivial to do. To avoid breaking ABI we can't just replace numeric options by AV_OPT_TYPE_DURATION. For each one we need to introduce a new option name using AV_OPT_TYPE_DURATION and deprecate the old numeric option. The point is, as long has this transition isn't fully done, end users have to deal with various time related options, some of which are AV_OPT_TYPE_DURATION and some AV_OPT_TYPE_DOUBLE or AV_OPT_TYPE_INT64... Right now, we can use '5u' with both black_min_duration and sbc_delay. If this patch is applied, users will have to know that black_min_duration is AV_OPT_TYPE_DOUBLE so they must use '5u' and that sbc_delay is AV_OPT_TYPE_DURATION so they must use '5us'.
On 08.03.2018 00:14, Hendrik Leppkes wrote: > On Thu, Mar 8, 2018 at 12:05 AM, Aurelien Jacobs <aurel@gnuage.org> wrote: >> On Wed, Mar 07, 2018 at 11:45:03PM +0100, Marton Balint wrote: >>> >>> >>> On Wed, 7 Mar 2018, Aurelien Jacobs wrote: >>> >>>> On Tue, Mar 06, 2018 at 01:02:48AM +0100, Marton Balint wrote: >>>>> Accepting 'u' suffix for a time specification is neither intuitive nor >>>>> consistent (now that we don't accept m). >>>> >>>> The 'm' SI prefix is still accepted in various time options, and the 'u' >>>> prefix is still accepted in those options even after your patch, so you >>>> can't really argue that this patch improve consistency. >>>> (eg. -black_min_duration 5ms is still accepted). >>>> So this will surprise nobody that I don't like this patch. >>> >>> This really is a cursed topic, I am not sure I follow, after the patch: >>> >>> 5ms is accepted >>> 5us is accepted >>> 5m is not accepted >>> 5u is not accepted >> >> That is only for AV_OPT_TYPE_DURATION. >> All other numeric options type still accept SI prefix without unit. >> This include various time options such as black_min_duration. >> So after the patch, for black_min_duration: >> >> 5m is accepted >> 5u is accepted >> > > Because those use AV_OPT_TYPE_DOUBLE, a generic type without any > possiblity of a unit. > Ideally those should all be transitioned to AV_OPT_TYPE_DURATION if > they refer to a time for consistency, and not the actual time-type > bend to reflect some generic type. Why would we have the duration type > if its just a copy of the double type anyway? As a user with technical background I find it logical that AV_OPT_TYPE_DURATION is a superset of AV_OPT_TYPE_DOUBLE. Any double-formatted string is treated as a value in seconds. Additionally AV_OPT_TYPE_DURATION accepts HH:MM:SS formatted strings. So I think Aurelien has a point here: Why should we enforce a unit suffix for AV_OPT_TYPE_DURATION and loose the superset property? Regards, Tobias
On Thu, Mar 8, 2018 at 9:47 AM, Tobias Rapp <t.rapp@noa-archive.com> wrote: > On 08.03.2018 00:14, Hendrik Leppkes wrote: >> >> On Thu, Mar 8, 2018 at 12:05 AM, Aurelien Jacobs <aurel@gnuage.org> wrote: >>> >>> On Wed, Mar 07, 2018 at 11:45:03PM +0100, Marton Balint wrote: >>>> >>>> >>>> >>>> On Wed, 7 Mar 2018, Aurelien Jacobs wrote: >>>> >>>>> On Tue, Mar 06, 2018 at 01:02:48AM +0100, Marton Balint wrote: >>>>>> >>>>>> Accepting 'u' suffix for a time specification is neither intuitive nor >>>>>> consistent (now that we don't accept m). >>>>> >>>>> >>>>> The 'm' SI prefix is still accepted in various time options, and the >>>>> 'u' >>>>> prefix is still accepted in those options even after your patch, so you >>>>> can't really argue that this patch improve consistency. >>>>> (eg. -black_min_duration 5ms is still accepted). >>>>> So this will surprise nobody that I don't like this patch. >>>> >>>> >>>> This really is a cursed topic, I am not sure I follow, after the patch: >>>> >>>> 5ms is accepted >>>> 5us is accepted >>>> 5m is not accepted >>>> 5u is not accepted >>> >>> >>> That is only for AV_OPT_TYPE_DURATION. >>> All other numeric options type still accept SI prefix without unit. >>> This include various time options such as black_min_duration. >>> So after the patch, for black_min_duration: >>> >>> 5m is accepted >>> 5u is accepted >>> >> >> Because those use AV_OPT_TYPE_DOUBLE, a generic type without any >> possiblity of a unit. >> Ideally those should all be transitioned to AV_OPT_TYPE_DURATION if >> they refer to a time for consistency, and not the actual time-type >> bend to reflect some generic type. Why would we have the duration type >> if its just a copy of the double type anyway? > > > As a user with technical background I find it logical that > AV_OPT_TYPE_DURATION is a superset of AV_OPT_TYPE_DOUBLE. Any > double-formatted string is treated as a value in seconds. Additionally > AV_OPT_TYPE_DURATION accepts HH:MM:SS formatted strings. > > So I think Aurelien has a point here: Why should we enforce a unit suffix > for AV_OPT_TYPE_DURATION and loose the superset property? > Because something like "3m" for a time property is ambigious and confusing. I would personally never expect that to mean "3ms", and as this thread has shown here, I would not be alone with that interpretation. - Hendrik
On Thu, 8 Mar 2018, Hendrik Leppkes wrote: > On Thu, Mar 8, 2018 at 9:47 AM, Tobias Rapp <t.rapp@noa-archive.com> wrote: >> On 08.03.2018 00:14, Hendrik Leppkes wrote: >>> >>> On Thu, Mar 8, 2018 at 12:05 AM, Aurelien Jacobs <aurel@gnuage.org> wrote: >>>> >>>> On Wed, Mar 07, 2018 at 11:45:03PM +0100, Marton Balint wrote: >>>>> >>>>> >>>>> >>>>> On Wed, 7 Mar 2018, Aurelien Jacobs wrote: >>>>> >>>>>> On Tue, Mar 06, 2018 at 01:02:48AM +0100, Marton Balint wrote: >>>>>>> >>>>>>> Accepting 'u' suffix for a time specification is neither intuitive nor >>>>>>> consistent (now that we don't accept m). >>>>>> >>>>>> >>>>>> The 'm' SI prefix is still accepted in various time options, and the >>>>>> 'u' >>>>>> prefix is still accepted in those options even after your patch, so you >>>>>> can't really argue that this patch improve consistency. >>>>>> (eg. -black_min_duration 5ms is still accepted). >>>>>> So this will surprise nobody that I don't like this patch. >>>>> >>>>> >>>>> This really is a cursed topic, I am not sure I follow, after the patch: >>>>> >>>>> 5ms is accepted >>>>> 5us is accepted >>>>> 5m is not accepted >>>>> 5u is not accepted >>>> >>>> >>>> That is only for AV_OPT_TYPE_DURATION. >>>> All other numeric options type still accept SI prefix without unit. >>>> This include various time options such as black_min_duration. >>>> So after the patch, for black_min_duration: >>>> >>>> 5m is accepted >>>> 5u is accepted >>>> >>> >>> Because those use AV_OPT_TYPE_DOUBLE, a generic type without any >>> possiblity of a unit. >>> Ideally those should all be transitioned to AV_OPT_TYPE_DURATION if >>> they refer to a time for consistency, and not the actual time-type >>> bend to reflect some generic type. Why would we have the duration type >>> if its just a copy of the double type anyway? >> >> >> As a user with technical background I find it logical that >> AV_OPT_TYPE_DURATION is a superset of AV_OPT_TYPE_DOUBLE. Any >> double-formatted string is treated as a value in seconds. Additionally >> AV_OPT_TYPE_DURATION accepts HH:MM:SS formatted strings. >> >> So I think Aurelien has a point here: Why should we enforce a unit suffix >> for AV_OPT_TYPE_DURATION and loose the superset property? >> > > Because something like "3m" for a time property is ambigious and > confusing. I would personally never expect that to mean "3ms", and as > this thread has shown here, I would not be alone with that > interpretation. Ok, it seems there is no consensus/decision, but keeping it how it is locks us in accepting the prefix-only variants because of compatibility. So I will push this soon unless I get an explicit reject from somebody. Regards, Marton
On Fri, 9 Mar 2018, Marton Balint wrote: > > > On Thu, 8 Mar 2018, Hendrik Leppkes wrote: > >> On Thu, Mar 8, 2018 at 9:47 AM, Tobias Rapp <t.rapp@noa-archive.com> wrote: >>> On 08.03.2018 00:14, Hendrik Leppkes wrote: >>>> >>>> On Thu, Mar 8, 2018 at 12:05 AM, Aurelien Jacobs <aurel@gnuage.org> > wrote: >>>>> >>>>> On Wed, Mar 07, 2018 at 11:45:03PM +0100, Marton Balint wrote: >>>>>> >>>>>> >>>>>> >>>>>> On Wed, 7 Mar 2018, Aurelien Jacobs wrote: >>>>>> >>>>>>> On Tue, Mar 06, 2018 at 01:02:48AM +0100, Marton Balint wrote: >>>>>>>> >>>>>>>> Accepting 'u' suffix for a time specification is neither intuitive > nor >>>>>>>> consistent (now that we don't accept m). >>>>>>> >>>>>>> >>>>>>> The 'm' SI prefix is still accepted in various time options, and the >>>>>>> 'u' >>>>>>> prefix is still accepted in those options even after your patch, so > you >>>>>>> can't really argue that this patch improve consistency. >>>>>>> (eg. -black_min_duration 5ms is still accepted). >>>>>>> So this will surprise nobody that I don't like this patch. >>>>>> >>>>>> >>>>>> This really is a cursed topic, I am not sure I follow, after the patch: >>>>>> >>>>>> 5ms is accepted >>>>>> 5us is accepted >>>>>> 5m is not accepted >>>>>> 5u is not accepted >>>>> >>>>> >>>>> That is only for AV_OPT_TYPE_DURATION. >>>>> All other numeric options type still accept SI prefix without unit. >>>>> This include various time options such as black_min_duration. >>>>> So after the patch, for black_min_duration: >>>>> >>>>> 5m is accepted >>>>> 5u is accepted >>>>> >>>> >>>> Because those use AV_OPT_TYPE_DOUBLE, a generic type without any >>>> possiblity of a unit. >>>> Ideally those should all be transitioned to AV_OPT_TYPE_DURATION if >>>> they refer to a time for consistency, and not the actual time-type >>>> bend to reflect some generic type. Why would we have the duration type >>>> if its just a copy of the double type anyway? >>> >>> >>> As a user with technical background I find it logical that >>> AV_OPT_TYPE_DURATION is a superset of AV_OPT_TYPE_DOUBLE. Any >>> double-formatted string is treated as a value in seconds. Additionally >>> AV_OPT_TYPE_DURATION accepts HH:MM:SS formatted strings. >>> >>> So I think Aurelien has a point here: Why should we enforce a unit suffix >>> for AV_OPT_TYPE_DURATION and loose the superset property? >>> >> >> Because something like "3m" for a time property is ambigious and >> confusing. I would personally never expect that to mean "3ms", and as >> this thread has shown here, I would not be alone with that >> interpretation. > > Ok, it seems there is no consensus/decision, but keeping it how it is > locks us in accepting the prefix-only variants because of compatibility. > So I will push this soon unless I get an explicit reject from somebody. Pushed. Regards, Marton
diff --git a/libavutil/parseutils.c b/libavutil/parseutils.c index 95274f564f..924c49d52c 100644 --- a/libavutil/parseutils.c +++ b/libavutil/parseutils.c @@ -693,12 +693,11 @@ int av_parse_time(int64_t *timeval, const char *timestr, int duration) suffix = 1000; microseconds /= 1000; q += 2; - } else if (*q == 'u') { + } else if (q[0] == 'u' && q[1] == 's') { suffix = 1; microseconds = 0; - q++; - } - if (*q == 's') + q += 2; + } else if (*q == 's') q++; } else { int is_utc = *q == 'Z' || *q == 'z';
Accepting 'u' suffix for a time specification is neither intuitive nor consistent (now that we don't accept m). Also there was a bug in the code accepting an extra 's' even after 'ms'. Signed-off-by: Marton Balint <cus@passwd.hu> --- libavutil/parseutils.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)