[FFmpeg-devel] [RFC] New option system

[Stefano Sabatini]

On date Tuesday 2009-07-21 03:40:39 +0200, Michael Niedermayer encoded:
> On Tue, Jul 21, 2009 at 01:12:39AM +0200, Stefano Sabatini wrote:
> > On date Monday 2009-07-20 19:56:06 +0200, Michael Niedermayer encoded:
[...]
> > > this rewrite is rejected
> > > i repeat that i see no problem with improving the existing system, and
> > > adding handlers for various types if you want to do that.
> > > But simply rewriting the system with no comments about why or what is
> > > changed is unacceptable,
> > 
> > Well I think that I already tried to make my point, and attached an
> > implementation to show how this can be done, but well I have no
> > problem at all at explaing why I don't like the current option system
> > and I contrived a new one.
> > 
> > > as is one would have to compare the proposal against what exists to find
> > > out what is changed if some features are lost and then guess from that
> > > why it was done like that ...
> > 
> > Problems with the old system:
> > 
> > * only a limited set of options is actually supported. Note that if I
> >   would like to extend the actual option system in order to make it
> >   support e.g. colors, I would need to import in lavc what is
> >   currently implemented in lavfi, the same for any other option which
> >   uses some data defined in a particular lib.
> > 
> >   Similarly, if I want to extend the option system for using it with a
> >   foobar external application / lib, the only way is to import the
> >   required features into lavc - i.e. it is not possible at all.
> > 
> >   The new system is as generic as possible, allowing option handlers
> >   to be defined in the module which uses it.
> 
> let me be more direct
> you can add a pointer to a convertion function into AVOptions, you dont
> rather you rewrite it.
> thats not a justifcation for a rewrite, 

I already proposed that:
http://thread.gmane.org/gmane.comp.video.ffmpeg.devel/79767/

and got it rejected.

> > * the way the current opt.c deals with named values... looks pretty
> >   messy imo, and I always have an hard time when I have to read that
> >   code, also there are some known "glitches" which may leads to rather
> >   obscure behavior. 
> >   For example the current code will broke if a named constant is defined
> >   *before* a parameter with the same name.
> 
> if there are glitches they should be fixed

That has a cost in complexity / time (developer time I mean), also I'm
not motivated into fixing such a limited system.

> > 
> > * API incomplete.
> >   For example there is no way to set the value for an option if the
> >   option pointer is known, the only way is to use av_set_XXX()
> >   which iterates once again the options array.
> 
> if you have a pointer then you can dereference it, see the C spec please
> we dont need an API for that.

No I mean this:

http://thread.gmane.org/gmane.comp.video.ffmpeg.devel/68166/

which has been already rejected.

What I mean is that if I have a pointer to an option, I need to use
the option name to set it, thus re-iterating through the options array
rather than directly access to the option through the pointer (very
minor issue anyway).

> > 
> > * the current system is quite inflexible, in many cases the fields in
> >   AVOption are not sufficient to describe an option, in other cases it
> >   contains useless fields (min, max). Also it doesn't allow for
> >   example to define more than one field at once (BTW this could be
> >   extended using an opaque field, but again this leads to such many
> >   changes that is easier to rewrite it from scratch).
> 
> your rewrite is rejected
> The first and foremost reason is that you did not explain
> what it does better or how it does it. You just post a patch and
> expect me to reverse engeneer what it does and what features it has
> how it does that, what its differences are, ...

Well I never meant you to reverse engineer the code, just to read the
doxy/API, also a working patch + test / example code is always better
than an explanation, and the core of the new option system is rather
trivial / short in lenght, not to mention the other lenghty replies I
wrote.

Anyway, resuming what I already wrote:

1) what the new option system does better with respect to the old one?

It allows to be easily extended without to touch the core, that is the
option handlers are implemented in the module which uses, so that
extensions - i.e. options handlers hook functions - cannot be
implemented / linked only where they are needed, without the need to
pollute the core (which is rather trivial).

Also it allows for a more flexible way to set options, for example it
allows for setting default values in strings, to set multiple values
in a context with a single option and with no limitation on the size
of the default value to be set (which is limited to the size of
"double" with the old system).

2) how is this implemented?

This is implemented making the new AVOpt struct contain the handlers
for the various hooks required for performing the various operations
on the options (setting it, settinh its default value, setting it from
a string representing a value, showing a description of what the
option does), and containing a pointer to an opaque struct containing
the information required to register a specific option.

> The second is obviously that the issues oyu point at a very trivial
> to fix or so obscure that it really isnt worth it, that said i dont
> mind seeing them fixed still ...

No they're not trivial to fix as they depend on design shortcomings,
also most of the patches I proposed for addressing those issues have
been rejected.
 
> > 
> > Problems with the new system:
> > 
> > * options need to be registered at run-time, with the previous one
> >   they were statically defined in an array, this doesn't look like a
> >   big problem anyway as this operation is performed just one time
> >   usually during the context initialization.
> > 
> > * still not supported the flags field, that's not a problem as I don't
> >   see any problem into adding it to the new option system.
> > 
> > * the find operation should be possibly O(1), that's also a problem
> >   with the old system and as above, I don't think there are serious
> >   problems into using a binary search for that.
> 
> Its very trivial to use a bsearch with the current system
> 
> > 
> > Finally, while the old system could surely be extended to fix *some*
> > of the problems, I simply find *easier* to rewrite it from scratch,
> 
> I cannot help you with that.

...
-- 
FFmpeg = Forgiving Fast Murdering Plastic Everlasting Glue