[FFmpeg-devel] [PATCH] avutil/parseutils: only accept full us duration, do not accept mss duration

[Aurelien Jacobs]

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 at 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'.