diff mbox

[FFmpeg-devel,01/16] doc/filters: document the unstability of the shorthand options notation.

Message ID 20170810114642.26779-1-george@nsup.org
State New
Headers show

Commit Message

Nicolas George Aug. 10, 2017, 11:46 a.m. UTC
It does not mean that we intend to break the order of options
at a whim, but it gives us more freedom to make necessary
changes without extra unnecessary burden while giving stability
to users that require it.

Signed-off-by: Nicolas George <george@nsup.org>
---
 Changelog        | 3 +++
 doc/filters.texi | 5 +++++
 2 files changed, 8 insertions(+)


Another argument that I did not think to bring up earlier: with this change,
it becomse similar to GNU long options or Vim commands: users can shorten
the commands or options, as long as it is not ambiguous; but new options or
commands can make it ambiguous, requiring a longer prefix, so long-term
scripts must be written with the full name. Users are perfectly aware about
it and do not fuss over it because they understand this small price is
necessary for evolution.

Comments

Michael Niedermayer Aug. 10, 2017, 2:05 p.m. UTC | #1
On Thu, Aug 10, 2017 at 01:46:27PM +0200, Nicolas George wrote:
> It does not mean that we intend to break the order of options
> at a whim, but it gives us more freedom to make necessary
> changes without extra unnecessary burden while giving stability
> to users that require it.
> 
> Signed-off-by: Nicolas George <george@nsup.org>
> ---
>  Changelog        | 3 +++
>  doc/filters.texi | 5 +++++
>  2 files changed, 8 insertions(+)
> 
> 
> Another argument that I did not think to bring up earlier: with this change,
> it becomse similar to GNU long options or Vim commands: users can shorten
> the commands or options, as long as it is not ambiguous; but new options or
> commands can make it ambiguous, requiring a longer prefix, so long-term
> scripts must be written with the full name. Users are perfectly aware about
> it and do not fuss over it because they understand this small price is
> necessary for evolution.
> 
> 
> diff --git a/Changelog b/Changelog
> index c797d68a36..78286e3606 100644
> --- a/Changelog
> +++ b/Changelog
> @@ -32,6 +32,9 @@ version <next>:
>  - unpremultiply video filter
>  - tlut2 video filter
>  - floodfill video filter
> +- The order of options in filters is no longer considered stable. If
> +  long-term stability is necessary (i.e. for scripts), use named options
> +  (e.g. overlay=50:100 -> overlay=x=50:y=100).
>  
>  version 3.3:
>  - CrystalHD decoder moved to new decode API
> diff --git a/doc/filters.texi b/doc/filters.texi
> index eedc7b5896..470ffa60a1 100644
> --- a/doc/filters.texi
> +++ b/doc/filters.texi
> @@ -161,6 +161,11 @@ follow the same constraints order of the previous point. The following
>  
>  @end itemize
>  
> +Future evolutions of filters may require inserting new options or changing
> +their order, especially for the non-essential options, and that would break
> +options given without their name. For that reason, uses that require
> +stability should favor the @var{key=value} notation.

Please limit the notes in filters.texi and Changelog to the filters and
options you intend to change.

thanks

[...]
Nicolas George Aug. 10, 2017, 2:15 p.m. UTC | #2
Le tridi 23 thermidor, an CCXXV, Michael Niedermayer a écrit :
> Please limit the notes in filters.texi and Changelog to the filters and
> options you intend to change.

That would defeat the purpose. Doubly so:

- Being free to change the options as need requires. That means any
  filter and any option. Not all, not many, but any.

- Having a rule that is simple to remember instead of a long list.

Please bring new arguments to the discussion if you want to continue it.

Regards,
Marton Balint Aug. 10, 2017, 7:37 p.m. UTC | #3
On Thu, 10 Aug 2017, Nicolas George wrote:

> Le tridi 23 thermidor, an CCXXV, Michael Niedermayer a écrit :
>> Please limit the notes in filters.texi and Changelog to the filters and
>> options you intend to change.
>
> That would defeat the purpose. Doubly so:
>
> - Being free to change the options as need requires. That means any
>  filter and any option. Not all, not many, but any.
>
> - Having a rule that is simple to remember instead of a long list.
>
> Please bring new arguments to the discussion if you want to continue it.

I suggest you push the patch series without this patch, Michael can fix 
the overlay and blend/tblend parameter order. If later the needed 
additional compatibility code becomes too much of a burden, we can discuss 
this further.

Regards,
Marton
Michael Niedermayer Aug. 10, 2017, 11:56 p.m. UTC | #4
On Thu, Aug 10, 2017 at 04:15:55PM +0200, Nicolas George wrote:
> Le tridi 23 thermidor, an CCXXV, Michael Niedermayer a écrit :
> > Please limit the notes in filters.texi and Changelog to the filters and
> > options you intend to change.
> 
> That would defeat the purpose. Doubly so:
> 
> - Being free to change the options as need requires. That means any
>   filter and any option. Not all, not many, but any.
> 
> - Having a rule that is simple to remember instead of a long list.
> 
> Please bring new arguments to the discussion if you want to continue it.

well
First, I dont think a single developer should declare a whole class of
interfaces spaning the areas other developers work on unstable against
one or more objections from them. Or if one has that right then everyone
else should as well.

I maintain several filters and clearly stated that this doc-text does not
apply to them. None of them would benefit from breaking the
order or inserting options in the middle.
In fact with a unstable order there is no benefit from adding options
in the middle, noone could reliably use the new order.

as in if you link to libavfilter 99.123 you can write a:b:c , if you
link to libavfilter 99.124 you can write a:c:b. Update your distro
package and it could change in otherwise unchanged applications.

and none of this in the example above is neccesarily a syntax error
and gives an error message. the different order could just give
different results. thats bad design

I like to re iterate, i do not agree to declaring the option order
of the filters i maintain as unstable.

Even if we could reorder them without disadvantages,
there is no benefit in doing so.

Its trivial to change your docs/changeog patch to avoid my concern, and
its trivial to change MAINTAINERs as well if people value declaring the
interfaces as unstable more than a maintainer who fanatically insist on
maintaining a stable interface in the filters he maintains.

Also there is no long list, just a entry in the changelog and in the
documentation of the specific filter which was changed.
I think we both agree that this is a rare event. And i would argue
it is NOT a event specific to the shorthand interface. Am i not
correct that you would similarly change the named interface if a
cleanup you do benefits from it ?

Thanks
Paul B Mahol Aug. 11, 2017, 8:31 a.m. UTC | #5
On 8/11/17, Michael Niedermayer <michael@niedermayer.cc> wrote:
> On Thu, Aug 10, 2017 at 04:15:55PM +0200, Nicolas George wrote:
>> Le tridi 23 thermidor, an CCXXV, Michael Niedermayer a écrit :
>> > Please limit the notes in filters.texi and Changelog to the filters and
>> > options you intend to change.
>>
>> That would defeat the purpose. Doubly so:
>>
>> - Being free to change the options as need requires. That means any
>>   filter and any option. Not all, not many, but any.
>>
>> - Having a rule that is simple to remember instead of a long list.
>>
>> Please bring new arguments to the discussion if you want to continue it.
>
> well
> First, I dont think a single developer should declare a whole class of
> interfaces spaning the areas other developers work on unstable against
> one or more objections from them. Or if one has that right then everyone
> else should as well.
>
> I maintain several filters and clearly stated that this doc-text does not
> apply to them. None of them would benefit from breaking the
> order or inserting options in the middle.
> In fact with a unstable order there is no benefit from adding options
> in the middle, noone could reliably use the new order.
>
> as in if you link to libavfilter 99.123 you can write a:b:c , if you
> link to libavfilter 99.124 you can write a:c:b. Update your distro
> package and it could change in otherwise unchanged applications.
>
> and none of this in the example above is neccesarily a syntax error
> and gives an error message. the different order could just give
> different results. thats bad design
>
> I like to re iterate, i do not agree to declaring the option order
> of the filters i maintain as unstable.
>
> Even if we could reorder them without disadvantages,
> there is no benefit in doing so.
>
> Its trivial to change your docs/changeog patch to avoid my concern, and
> its trivial to change MAINTAINERs as well if people value declaring the
> interfaces as unstable more than a maintainer who fanatically insist on
> maintaining a stable interface in the filters he maintains.
>
> Also there is no long list, just a entry in the changelog and in the
> documentation of the specific filter which was changed.
> I think we both agree that this is a rare event. And i would argue
> it is NOT a event specific to the shorthand interface. Am i not
> correct that you would similarly change the named interface if a
> cleanup you do benefits from it ?

What about keeping old options intact?
Nicolas George Aug. 11, 2017, 8:50 a.m. UTC | #6
Le quartidi 24 thermidor, an CCXXV, Michael Niedermayer a écrit :
> First, I dont think a single developer should declare a whole class of
> interfaces spaning the areas other developers work on unstable against
> one or more objections from them. Or if one has that right then everyone
> else should as well.

So they post a patch on the mailing-list and everybody can discuss it.
That is the way it is supposed to work, and that is exactly what I did.
Are you accusing me of perverting the process or something, or is this
paragraph here only to muddy things more?

> I maintain several filters and clearly stated that this doc-text does not
> apply to them.

Well, good for you. Keep the options on your filter in order. I do not
intend to prevent you from doing so. What are you fussing about?

> order or inserting options in the middle.
> In fact with a unstable order there is no benefit from adding options
> in the middle, noone could reliably use the new order.
> 
> as in if you link to libavfilter 99.123 you can write a:b:c , if you
> link to libavfilter 99.124 you can write a:c:b. Update your distro
> package and it could change in otherwise unchanged applications.

Everybody is already aware of that.

> and none of this in the example above is neccesarily a syntax error
> and gives an error message. the different order could just give
> different results. thats bad design

Yes, that is bad design. And that applies to somebody misremembering
a:c:b when it should have been a:b:c: the whole shorthand notation was
bad design from the start. Thanks for making my point for me.

> I like to re iterate, i do not agree to declaring the option order
> of the filters i maintain as unstable.

You re-iterate, but you do not bring new arguments to the discussion. I
have brought many and you have not addressed half of them. I can
re-state them if you have forgotten.

> Also there is no long list, just a entry in the changelog and in the
> documentation of the specific filter which was changed.

You fail to see from a user's point of view. Try, for one second, to
imagine you are a simple user learning how to use that tool. You read in
the documentation that this filter has stable shorthand notation, and
this one not, this other yes, these two no, and these five give no
information at all. What will you remember? Which filter is stable and
which is not? No, you will remember not to use the shorthand notation,
because writing "x=", "y=" even when it is not necessary is easier than
remembering when it is necessary and when it is not.

Having exceptions and categories is worse than having everything
declared unstable.

> I think we both agree that this is a rare event. And i would argue
> it is NOT a event specific to the shorthand interface. Am i not
> correct that you would similarly change the named interface if a
> cleanup you do benefits from it ?

No, you are not correct at all. Quite the contrary, I have already
explained why it is not true.

The users want stability, I know that. They have it: the named options.
Since they have something that will always work, I can propose to break
something that is just a convenience to type a few less keys. I would
not propose to break the named interface, because there is no backup
stable interface.

Regards,
Nicolas George Aug. 11, 2017, 8:51 a.m. UTC | #7
Le tridi 23 thermidor, an CCXXV, Marton Balint a écrit :
> I suggest you push the patch series without this patch, Michael can fix the
> overlay and blend/tblend parameter order. If later the needed additional
> compatibility code becomes too much of a burden, we can discuss this
> further.

I could do that if people agree, but wouldn't breaking then repairing be
akin to breaking twice, and as such worse than breaking once?

Regards,
Nicolas George Aug. 11, 2017, 8:52 a.m. UTC | #8
Le quartidi 24 thermidor, an CCXXV, Paul B Mahol a écrit :
> What about keeping old options intact?

That requires glue code and more importantly testing. I do not intend to
do it, and it blocks progress.

Regards,
Clément Bœsch Aug. 11, 2017, 8:57 a.m. UTC | #9
On Thu, Aug 10, 2017 at 01:46:27PM +0200, Nicolas George wrote:
> It does not mean that we intend to break the order of options
> at a whim, but it gives us more freedom to make necessary
> changes without extra unnecessary burden while giving stability
> to users that require it.
[...]

I'd rather make such changes justified and documented as exceptional in
the Changelog (or in APIchanges) when we can't get around it cleanly, than
documenting a free for all area.

You're saying documenting the risk or potential changes helps us make
changes more easily with more transparency, but no matter the wording, to
me it looks like it's going to be a slippery slope where developers
interpret it differently and will abuse that declared rule whenever
possible. Indeed, it's the perfect defense against users and other
developers when breaking the interface.

So yeah, I'm in favor of "no API breakage" (of course, major version bumps
allow to bypass this), whatever the form, and we can always make
documented exceptions for obscures options after a discussion.
Nicolas George Aug. 11, 2017, 9:12 a.m. UTC | #10
Le quartidi 24 thermidor, an CCXXV, Clement Boesch a écrit :
> I'd rather make such changes justified and documented as exceptional in
> the Changelog (or in APIchanges) when we can't get around it cleanly, than
> documenting a free for all area.
> 
> You're saying documenting the risk or potential changes helps us make
> changes more easily with more transparency, but no matter the wording, to
> me it looks like it's going to be a slippery slope where developers
> interpret it differently and will abuse that declared rule whenever
> possible. Indeed, it's the perfect defense against users and other
> developers when breaking the interface.

Can you explain what you mean exactly? I am very doubtful about
"slippery slope" arguments, they usually have at their core the fallacy
of trusting your now-self more than your later-self, while actually your
later-self will have more information and thus be able of better
decisions.

> So yeah, I'm in favor of "no API breakage" (of course, major version bumps
> allow to bypass this), whatever the form, and we can always make
> documented exceptions for obscures options after a discussion.

This would be nice, in principle. But that does not tell me what to do
in practice.

Will you implement and test the compatibility code, soon so that the
patch series can be pushed, fixing the bugs it is fixing and allowing
work to continue?

Will you do that in the future when filters are converted to the new
options system?

Will you go over all the filter's documentation to fix the places where
the order does not match?

The last paragraph is one of my strongest arguments, and nobody
addressed it yet: the shorthand notation is not stable. Right now. We
have on occasion inserted options out of order, and the order is in fact
not documented since the documentation does not match the
implementation. This patch is not changing the policy, it is documenting
it.

Regards,
Clément Bœsch Aug. 11, 2017, 9:52 a.m. UTC | #11
On Fri, Aug 11, 2017 at 11:12:32AM +0200, Nicolas George wrote:
> Le quartidi 24 thermidor, an CCXXV, Clement Boesch a écrit :
> > I'd rather make such changes justified and documented as exceptional in
> > the Changelog (or in APIchanges) when we can't get around it cleanly, than
> > documenting a free for all area.
> > 
> > You're saying documenting the risk or potential changes helps us make
> > changes more easily with more transparency, but no matter the wording, to
> > me it looks like it's going to be a slippery slope where developers
> > interpret it differently and will abuse that declared rule whenever
> > possible. Indeed, it's the perfect defense against users and other
> > developers when breaking the interface.
> 
> Can you explain what you mean exactly? I am very doubtful about
> "slippery slope" arguments, they usually have at their core the fallacy
> of trusting your now-self more than your later-self, while actually your
> later-self will have more information and thus be able of better
> decisions.
> 

I'm afraid of the situation where a developer will feel like the order of
the options is not ideal, or an option could be renamed for consistency
with other filters, and will take the easy way out "oh well, we documented
it's unstable, users can only blame themselves for relying on it, not even
mentioning the new order/name is much better for everyone".

> > So yeah, I'm in favor of "no API breakage" (of course, major version bumps
> > allow to bypass this), whatever the form, and we can always make
> > documented exceptions for obscures options after a discussion.
> 
> This would be nice, in principle. But that does not tell me what to do
> in practice.
> 
> Will you implement and test the compatibility code, soon so that the
> patch series can be pushed, fixing the bugs it is fixing and allowing
> work to continue?

If you're referring to this current patch serie, can you list here all the
filters that are affected by an API break and to what degree?

We can discuss here if it's an acceptable change or not, and should
require a compatibility code (maybe it will be for one or two filters
only).

> Will you do that in the future when filters are converted to the new
> options system?

We could consider an API to deprecate an AVOption for future similar
issue. I think we already had such discussion in the past (maybe that was
around the time we changed "user-agent" into "user_agent"), involving the
presence of a flag.

> Will you go over all the filter's documentation to fix the places where
> the order does not match?

I consider that one a long standing and deeper issue we haven't solved
yet. My position here is that I do believe all the long-description of
option belongs in the source code itself (under NULL_IF_CONFIG_SMALL()
maybe) and the final documentation should be generated from it.

That way, we won't have to worry about option orders or inconsistencies
between code and documentation. It will also allow having a cleaner
documentation wrt option types (mainly thinking about bool values here).

That's a large project, but I don't think it's hard.

> The last paragraph is one of my strongest arguments, and nobody
> addressed it yet: the shorthand notation is not stable. Right now. We
> have on occasion inserted options out of order, and the order is in fact
> not documented since the documentation does not match the
> implementation.

I think we've been careful about avoiding such mistakes. I have the
feeling this may only affect options at the end of already long options
list, so the impact is mitigated.

> This patch is not changing the policy, it is documenting it.

And my position is that it's currently not that bad handled and should be
improved.

In the past, we've been breaking the C API and ABI several times by
"mistakes". Our documentation never was perfect either, and still isn't.
Does that mean we should document that users should never expect any API
stability from the FFmpeg project because we're just shitty at it?
Nicolas George Aug. 11, 2017, 10:33 a.m. UTC | #12
Le quartidi 24 thermidor, an CCXXV, Clement Boesch a écrit :
> I'm afraid of the situation where a developer will feel like the order of
> the options is not ideal, or an option could be renamed for consistency
> with other filters, and will take the easy way out "oh well, we documented
> it's unstable, users can only blame themselves for relying on it, not even
> mentioning the new order/name is much better for everyone".

In my mind, even with this documentation commit, changing the order of
an option would still require posting a patch on the mailing-list to
discuss it. It could be stated explicitly, but I do not know where (the
user documentation is not the place).

As for renaming options, this is absolutely not allowed.

> If you're referring to this current patch serie, can you list here all the
> filters that are affected by an API break and to what degree?
> 
> We can discuss here if it's an acceptable change or not, and should
> require a compatibility code (maybe it will be for one or two filters
> only).

All the filters that use framesync2.h and have any of the "shortest",
"eof_action" and "repeatlast" options lose the possibility to set these
options by shorthand notation. I.e. you can no longer put 1 in the fifth
or sixth position in the arguments list and hope it will fall on
"shortest".

> We could consider an API to deprecate an AVOption for future similar
> issue. I think we already had such discussion in the past (maybe that was
> around the time we changed "user-agent" into "user_agent"), involving the
> presence of a flag.

Yes, we could, and that takes time that could be dedicated to fixing
actual bugs or implementing new features. Ask any user: "if you agree to
always write x=, y= in your overlay filters, I can fix your bug faster,
what do you prefer?", I know what they will answer.

> I consider that one a long standing and deeper issue we haven't solved
> yet.

Yes, but it still makes my point: the shorthand notation is currently
broken, and we might as well take the benefits from it.

>      My position here is that I do believe all the long-description of
> option belongs in the source code itself (under NULL_IF_CONFIG_SMALL()
> maybe) and the final documentation should be generated from it.
> 
> That way, we won't have to worry about option orders or inconsistencies
> between code and documentation. It will also allow having a cleaner
> documentation wrt option types (mainly thinking about bool values here).
> 
> That's a large project, but I don't think it's hard.

I agree. In fact, a few months ago I posted a detailed plan of an API to
replace the AVOption system that included that and many other things,
including getting rid of escaping hell.

> I think we've been careful about avoiding such mistakes. I have the
> feeling this may only affect options at the end of already long options
> list, so the impact is mitigated.

Yes, but when it comes to trust, it is all or nothing: right now, users
cannot trust the order given in the documentation, and they are not
warned of it.

> And my position is that it's currently not that bad handled and should be
> improved.
> 
> In the past, we've been breaking the C API and ABI several times by
> "mistakes". Our documentation never was perfect either, and still isn't.
> Does that mean we should document that users should never expect any API
> stability from the FFmpeg project because we're just shitty at it?

No, but that means that when a specific has been in fact unstable and
people did not complain about it, we can get rid of it with less hassle.

Regards,
Clément Bœsch Aug. 11, 2017, 4:57 p.m. UTC | #13
On Fri, Aug 11, 2017 at 12:33:25PM +0200, Nicolas George wrote:
> Le quartidi 24 thermidor, an CCXXV, Clement Boesch a écrit :
> > I'm afraid of the situation where a developer will feel like the order of
> > the options is not ideal, or an option could be renamed for consistency
> > with other filters, and will take the easy way out "oh well, we documented
> > it's unstable, users can only blame themselves for relying on it, not even
> > mentioning the new order/name is much better for everyone".
> 
> In my mind, even with this documentation commit, changing the order of
> an option would still require posting a patch on the mailing-list to
> discuss it. It could be stated explicitly, but I do not know where (the
> user documentation is not the place).
> 
> As for renaming options, this is absolutely not allowed.
> 
> > If you're referring to this current patch serie, can you list here all the
> > filters that are affected by an API break and to what degree?
> > 
> > We can discuss here if it's an acceptable change or not, and should
> > require a compatibility code (maybe it will be for one or two filters
> > only).
> 
> All the filters that use framesync2.h and have any of the "shortest",
> "eof_action" and "repeatlast" options lose the possibility to set these
> options by shorthand notation. I.e. you can no longer put 1 in the fifth
> or sixth position in the arguments list and hope it will fall on
> "shortest".
> 

That looks like it has a marginal effect in this particular case. I'd
agree with just documenting it in the Changelog and still not making it
"the rule" (that is, NAK on the doc patch). Also please bump minor or
micro.

If other people insist on compatibility, I'd suggest the following:

In ff_framesync2_init(), run av_opt_find() with "eof_action", "shortest"
and "repeatlast" on parent (AVFilterContext); if you can't find the option
you fallback on the framesync opts values. Then you wrap the fields and
options in the filters with ifdefery, and announce the future breakage in
doc/APIchanges.

I don't think that's worth the hassle (even though I don't think that's
much more work), but people may disagree.

> > We could consider an API to deprecate an AVOption for future similar
> > issue. I think we already had such discussion in the past (maybe that was
> > around the time we changed "user-agent" into "user_agent"), involving the
> > presence of a flag.
> 
> Yes, we could, and that takes time that could be dedicated to fixing
> actual bugs or implementing new features. Ask any user: "if you agree to
> always write x=, y= in your overlay filters, I can fix your bug faster,
> what do you prefer?", I know what they will answer.

You know the answer of the user with the bug, not the hundreds others who
don't have the bug, rely on the shorthand, read a SO/reddit/forum post
with a now non-working example, or execute a random plugin of an app which
suddenly stop working.

But your patch doesn't seem to affect the x and y of overlay, so everything is
fine, right? :)

[...]

BTW, now that I think about it, how about this:

We add a flag to AVOption such as AV_OPT_FLAG_FILTER_STABLE_SHORTHAND and
flag all the options that we believe won't need to move in the future and
for which we can always rely on (typically x and y in overlay, or w and h
in scale).

Then we simply warn the user for every shorthand use of other options
("using short-hand form with '<option>' may cause issue in the future as
it could be swapped around").

That way, we:

- give clear visibility of the instability of (some of) the shorthands
- affect only marginal uses (that is, late options because we are
  generally going to flag the few first ones)
- gain flexibility to indeed swap around the options at will in the future
  (we initially wait for a release or two such that the users gets aware
  of their potential misuses with the introduced warning)
- keep shorthands useful for the most essential cases
- provide trust on the current use of shorthands.

Regards,
Nicolas George Aug. 11, 2017, 9:34 p.m. UTC | #14
Le quartidi 24 thermidor, an CCXXV, Clement Boesch a écrit :
> That looks like it has a marginal effect in this particular case. I'd

I would not have proposed something worse.

> agree with just documenting it in the Changelog and still not making it
> "the rule" (that is, NAK on the doc patch). Also please bump minor or
> micro.

Ok.

> If other people insist on compatibility, I'd suggest the following:
> 
> In ff_framesync2_init(), run av_opt_find() with "eof_action", "shortest"
> and "repeatlast" on parent (AVFilterContext); if you can't find the option
> you fallback on the framesync opts values. Then you wrap the fields and
> options in the filters with ifdefery, and announce the future breakage in
> doc/APIchanges.
> 
> I don't think that's worth the hassle (even though I don't think that's
> much more work), but people may disagree.

I will not oppose if somebody does it, but I think it would be a waste
of time.

> BTW, now that I think about it, how about this:
> 
> We add a flag to AVOption such as AV_OPT_FLAG_FILTER_STABLE_SHORTHAND and
> flag all the options that we believe won't need to move in the future and
> for which we can always rely on (typically x and y in overlay, or w and h
> in scale).
> 
> Then we simply warn the user for every shorthand use of other options
> ("using short-hand form with '<option>' may cause issue in the future as
> it could be swapped around").
> 
> That way, we:
> 
> - give clear visibility of the instability of (some of) the shorthands
> - affect only marginal uses (that is, late options because we are
>   generally going to flag the few first ones)
> - gain flexibility to indeed swap around the options at will in the future
>   (we initially wait for a release or two such that the users gets aware
>   of their potential misuses with the introduced warning)
> - keep shorthands useful for the most essential cases
> - provide trust on the current use of shorthands.

It looks nice on principle, but I think you forget one consideration
about convenience: learning which option is stable and which is not
requires more effort than just putting the names of the options always.
So as soon as we acknowledge that some options are not stable with
shorthand, it becomes more convenient to use the names, and any
mitigation scheme that can be implemented will just be mostly unused.

Think of it that way: if we had designed the filters with an
AVOption-based system from the start instead of going for a simple
string and a custom parser, same as every time, would we have
implemented the shorthand system? No, we would just have used the
standard key=value:key=value... parser. The shorthand system only exists
because we thought we could get away with parsing the filters options
with sscanf(opt_str, "%d:%d", &w, &h) and we did not have the
clairvoyance to make a clean break when switching to AVOption.

Regards,
Alexander Strasser Aug. 11, 2017, 10:56 p.m. UTC | #15
Hi all,

sorry for jumping into the discussion late. I think it is very
important though. Please pardon me if I missed anything from
the previous discussions on this topic.


On 2017-08-11 23:34 +0200, Nicolas George wrote:
> Le quartidi 24 thermidor, an CCXXV, Clement Boesch a écrit :

[...]

> > BTW, now that I think about it, how about this:
> > 
> > We add a flag to AVOption such as AV_OPT_FLAG_FILTER_STABLE_SHORTHAND and
> > flag all the options that we believe won't need to move in the future and
> > for which we can always rely on (typically x and y in overlay, or w and h
> > in scale).
> > 
> > Then we simply warn the user for every shorthand use of other options
> > ("using short-hand form with '<option>' may cause issue in the future as
> > it could be swapped around").

Besides AV_OPT_FLAG_FILTER_STABLE_SHORTHAND being an awfully long name
for it, I think it is a good idea.

Maybe even explicitly marking the options that can be used without name
and have a stable order and not allowing the others to be used without
name would be even better. Documentation should clearly indicate for
which options the name could be omitted. I guess we would have to come 
up with a clean notation for that, but that shouldn't be too hard.


> > That way, we:
> > 
> > - give clear visibility of the instability of (some of) the shorthands
> > - affect only marginal uses (that is, late options because we are
> >   generally going to flag the few first ones)
> > - gain flexibility to indeed swap around the options at will in the future
> >   (we initially wait for a release or two such that the users gets aware
> >   of their potential misuses with the introduced warning)
> > - keep shorthands useful for the most essential cases
> > - provide trust on the current use of shorthands.
> 
> It looks nice on principle, but I think you forget one consideration
> about convenience: learning which option is stable and which is not
> requires more effort than just putting the names of the options always.
> So as soon as we acknowledge that some options are not stable with
> shorthand, it becomes more convenient to use the names, and any
> mitigation scheme that can be implemented will just be mostly unused.

I do not think so. For many use cases *not* using named options will
clearly be more attractive, e.g. only one stupid and simple example is
vf fps.

I think if it is not allowed (like I suggested above) to give some
arguments with the shorthand notation, this will support the use cases
where using the shorthand notation is most useful and convenient.


> Think of it that way: if we had designed the filters with an
> AVOption-based system from the start instead of going for a simple
> string and a custom parser, same as every time, would we have
> implemented the shorthand system? No, we would just have used the
> standard key=value:key=value... parser. The shorthand system only exists
> because we thought we could get away with parsing the filters options
> with sscanf(opt_str, "%d:%d", &w, &h) and we did not have the
> clairvoyance to make a clean break when switching to AVOption.

To me this reads like a huge over simplification, though it is
partially correct. But I strongly believe the shorthand notation
was always much more than a side effect of choosing the wrong
way to implement things.


  Alexander
Nicolas George Aug. 20, 2017, 8:45 a.m. UTC | #16
Le tridi 23 thermidor, an CCXXV, Marton Balint a écrit :
> I suggest you push the patch series without this patch, Michael can fix the
> overlay and blend/tblend parameter order. If later the needed additional
> compatibility code becomes too much of a burden, we can discuss this
> further.

I will do that in a few days unless somebody opposes explicitly and with
arguments to continue the discussion. I have reworded the Changelog
entry in the last commit to:

- Some video filters with several inputs now use a common set of options:
  blend, libvmaf, lut3d, overlay, psnr, ssim.
  They must always be used by name.

Regards,
diff mbox

Patch

diff --git a/Changelog b/Changelog
index c797d68a36..78286e3606 100644
--- a/Changelog
+++ b/Changelog
@@ -32,6 +32,9 @@  version <next>:
 - unpremultiply video filter
 - tlut2 video filter
 - floodfill video filter
+- The order of options in filters is no longer considered stable. If
+  long-term stability is necessary (i.e. for scripts), use named options
+  (e.g. overlay=50:100 -> overlay=x=50:y=100).
 
 version 3.3:
 - CrystalHD decoder moved to new decode API
diff --git a/doc/filters.texi b/doc/filters.texi
index eedc7b5896..470ffa60a1 100644
--- a/doc/filters.texi
+++ b/doc/filters.texi
@@ -161,6 +161,11 @@  follow the same constraints order of the previous point. The following
 
 @end itemize
 
+Future evolutions of filters may require inserting new options or changing
+their order, especially for the non-essential options, and that would break
+options given without their name. For that reason, uses that require
+stability should favor the @var{key=value} notation.
+
 If the option value itself is a list of items (e.g. the @code{format} filter
 takes a list of pixel formats), the items in the list are usually separated by
 @samp{|}.