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

Tue Aug 8 00:33:53 EEST 2017

On Mon, Aug 07, 2017 at 09:25:27AM +0200, Marton Balint wrote:
> 
> 
> On Mon, 7 Aug 2017, Michael Niedermayer wrote:
> 
> >On Sun, Aug 06, 2017 at 10:37:35AM +0200, Nicolas George wrote:
> >>L'octidi 18 thermidor, an CCXXV, James Almer a écrit :
> >>>What do you mean? What i suggested would be done each time an option is
> >>>removed or added anywhere but at the end, both of which afaik are
> >>>uncommon cases.
> >>>It's not something that requires a rewrite of the current codebase.
> >>
> >>I mean that since I consider the break bearable (somebody upgrades a
> >>piece of software, they MUST test the scripts that depend on it, and
> >>fixing the issue once and for all is easy), I am not willing to spend my
> >>time implementing (and testing, for this kind of thing that takes time)
> >>the deprecation-warning-and-backward-compatibility dance.
> >
> >Lets take a step back and look at this
> >
> >There are some rarely used options in multi input filters like
> >overlay which break.
> >Noone even noticed except me
> >
> >And you propose to declare the most used syntax from every filter
> >unstable.
> >
> >This just doesnt add up, its like shooting the patient in the head as
> >a treatment for a cold
> 
> Do you like this text better?

No

> 
> Future evolutions of filters may require inserting new options or
> changing their order,

This is not true.
like future evolutions of the C standard dont require reordering
function arguments or standard POSIX tools dont require
renaming options.
There simply is no such requirement
We should be honest in the text.
"People working on future evolutions of libavfilter _WANT_ to insert
options or change their order"

If we write that its required some will realize that this is unlikely
and look deeper and it would make FFmpeg as a whole look
unprofessional.

> especially for the non-essential options, and
> while we make reasonable effort to keep the order so the options
> given without their name would not break,

So far there was effort toward changing the order not toward keeping it.
If effort is directed towards keeping it then we probably do not need
this text.

Yes we may hit a case once in a while where theres a majority prefering
to break some rarely used case for code cleanliness but this doesnt
require declaring the whole of the interface as unstable.
It is basically the whole as everyone and everything uses the shorthand
notation including our and others docs, examples and tutorials.

> sometimes that is not
> feasable. Therefore users should generally favor the @var{key=value}
> notation,  [...]

I do not agree as a maintainer of several filters of libavfilter.

I do try to keep my code compatible
and if someone would inform me that theres a break in the interface of
a filter of mine i would fix that if it is possible.
[if there are such issues in filters i maintain, please directly
 mail me or ping me on IRC or CC me on tickets]

I do care about users of my code, be that end users of tools or
developers using the libs.
And to me a stable interface and stable public API is a essential part
in (practically) usable software.

Will i fix every interface break in every filter? i do not know.
I do know though that iam interrested in helping with that as long
as the shorthand is part of the stable interface and help from me is
welcome. So far ive the feeling that my help is VERY unwelcome.
And this patch removes the shorthand from the stable interface.
Removing any and all reasons why i would be interrested in working on
it.

> 
> >
> >Please correct me if iam wrong, but isnt all that is needed to just
> >not remove the options from the AVOption array from the tiny number
> >of filters affected?
> >Or declare the tiny number of moved/changed options as removed/not
> >supported in shorthand notation.
> 
> Yeah, what is needed for compatibility? Only a single AVOption line,
> or additional code as well?

There are 2 ways at least, one is:

One line for each removed option, in fact the removed line, and
possbly one line to copy it to the new place if this cannot be
directly referred to from the AVOption line.
If the factored struct is referenceable by offset from the context
that is either is part of the other then only a single line is needed

Heres the removed and added options from the patches:
As you can see its the same options, only OFFSET would have to
refer to the new place

-    { "eof_action", "Action to take when encountering EOF from secondary input ",
-        OFFSET(eof_action), AV_OPT_TYPE_INT, { .i64 = EOF_ACTION_REPEAT },
-        EOF_ACTION_REPEAT, EOF_ACTION_PASS, .flags = FLAGS, "eof_action" },
-        { "repeat", "Repeat the previous frame.",   0, AV_OPT_TYPE_CONST, { .i64 = EOF_ACTION_REPEAT }, .flags = FLAGS, "eof_action" },
-        { "endall", "End both streams.",            0, AV_OPT_TYPE_CONST, { .i64 = EOF_ACTION_ENDALL }, .flags = FLAGS, "eof_action" },
-        { "pass",   "Pass through the main input.", 0, AV_OPT_TYPE_CONST, { .i64 = EOF_ACTION_PASS },   .flags = FLAGS, "eof_action" },
...
-    { "shortest", "force termination when the shortest input terminates", OFFSET(opt_shortest), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, FLAGS },
...
-    { "repeatlast", "repeat overlay of the last overlay frame", OFFSET(opt_repeatlast), AV_OPT_TYPE_BOOL, {.i64=1}, 0, 1, FLAGS },

+static const AVOption framesync_options[] = {
+    { "eof_action", "Action to take when encountering EOF from secondary input ",
+        OFFSET(opt_eof_action), AV_OPT_TYPE_INT, { .i64 = EOF_ACTION_REPEAT },
+        EOF_ACTION_REPEAT, EOF_ACTION_PASS, .flags = FLAGS, "eof_action" },
+        { "repeat", "Repeat the previous frame.",   0, AV_OPT_TYPE_CONST, { .i64 = EOF_ACTION_REPEAT }, .flags = FLAGS, "eof_action" },
+        { "endall", "End both streams.",            0, AV_OPT_TYPE_CONST, { .i64 = EOF_ACTION_ENDALL }, .flags = FLAGS, "eof_action" },
+        { "pass",   "Pass through the main input.", 0, AV_OPT_TYPE_CONST, { .i64 = EOF_ACTION_PASS },   .flags = FLAGS, "eof_action" },
+    { "shortest", "force termination when the shortest input terminates", OFFSET(opt_shortest), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, FLAGS },
+    { "repeatlast", "repeat overlay of the last overlay frame", OFFSET(opt_repeatlast), AV_OPT_TYPE_BOOL, { .i64 = 1 }, 0, 1, FLAGS },
+    { NULL }
+};

The other is a list of shorthand options (one line per affected filter)
That way options are moveable, as the order in the shorthand list
defines order and which are shorthand options.
This would only be needed for filters where this differs from the default.
It would require some code to handle that shorthand list but would
make cleanup easier and clearly delimiting what is part of the shorthand
and what is not alot clearer.

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

Its not that you shouldnt use gotos but rather that you should write
readable code and code with gotos often but not always is less readable
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170807/99581cfa/attachment.sig>