[FFmpeg-devel] [RFC] New option system

Tue Jul 21 03:40:39 CEST 2009

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:
> > On Sun, Jul 19, 2009 at 10:12:15PM +0200, Stefano Sabatini wrote:
> > > On date Monday 2009-07-13 19:27:09 +0200, Michael Niedermayer encoded:
> > > > On Sun, Jul 12, 2009 at 11:06:32PM +0200, Stefano Sabatini wrote:
> > > > > On date Sunday 2009-07-12 01:52:49 +0200, Michael Niedermayer encoded:
> > > [...]
> > > > > > Some of the individual parts you describe probably are a good idea, others
> > > > > > seem more obscure ...
> > > > > > 
> > > > > > If the current (primitive) set of parsing functions is insufficient, that can
> > > > > > be extended (color and whatever ...)
> > > > > 
> > > > > The problem with the current opt.[hc] is that it cannot be extended
> > > > > with ad-hoc functions, for example to implement a color option we
> > > > > would need to import the colorutils in libavcodec.
> > > > > 
> > > > > In order to keep opt.[hc] general and useful even outside FFmpeg we
> > > > > need to make it as generic as possible, then let the application
> > > > > eventually define which type of options to use and how to support
> > > > > them.
> > > > > 
> > > > 
> > > > > For example we could have:
> > > > > libavutil/opt.[hc]
> > > > > 
> > > > > then have some general purpose handlers (to set in the context options
> > > > > with type int, flags, double, ratio, string):
> > > > > libavutil/opt_handlers.[hc]
> > > > > 
> > > > > and for most specific functions:
> > > > > libavcodec/opt_handlers[hc] (e.g. size, framerates, profiles)
> > > > > libavfilter/opt_handlers.[hc] (e.g. colors, notes)
> > > > 
> > > > This does sound acceptable t first sight ...
> > > >
> > > > > 
> > > > > > supporting funny equations that set more than one field at a time dont seem
> > > > > > like they are worth the mess.
> > > > > > if you want
> > > > > >     out_h = 4/3 * in_h : out_w = out_h 
> > > > > > just do:
> > > > > >     out_h = 4/3 * in_h : out_w = 4/3 * in_h
> > > > > >
> > > > > > also the second is clearer
> > > > > 
> > > > > If out_h has a rather complicated expression then the first
> > > > > form would be nicer. 
> > > > > 
> > > > > Also to be able to define expressions exactly in the same order as
> > > > > they are passed to the user is in general an useful feature.
> > > > > 
> > > > > Consider for example:
> > > > > 
> > > > > pad = out_h=in_h+32 : out_w=in_w*out_h/in_h
> > > > > 
> > > > > (enlarge height of 32 pixel, then enlarge width keeping the same ratio
> > > > > as in input).
> > > > > 
> > > > > If we fix the order of evaluation:
> > > > > out_w, out_h
> > > > > 
> > > > > the above options are not valid, since when we evaluate out_w the
> > > > > value for out_h has not still been evaluated.
> > > > 
> > > > my oppinion ATM is pretty much that refering to any output in an
> > > > expression leads to undefined behavior.
> > > > Iam not convinced that this has any disadvantages
> > > > 
> > > > that said iam not against maintining the order but only as long as
> > > > it does not have disadvantages like code complexity or such
> > > 
> > > In attachment an elaboration of what I originally
> > > proposed. Preliminary patches (move eval and error codes to lavu) have
> > > been already posted.
> > > 
> > > I'm posting the opt.[hc] patches for letting people to comment on the
> > > new API, while the opt_handlers.[hc] are provided for showing a
> > > concrete example of its usage.
> > 
> > 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, 

> 
> * 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

> 
> * 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.

> 
> * 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, ...
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 ...

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

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

The educated differ from the uneducated as much as the living from the
dead. -- Aristotle 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090721/31765e20/attachment.pgp>