[FFmpeg-devel] [RFC] New option system

Stefano Sabatini stefano.sabatini-lala
Tue Jul 21 01:12:39 CEST 2009


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.

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

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

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

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.

Finally, while the old system could surely be extended to fix *some*
of the problems, I simply find *easier* to rewrite it from scratch,
also I'm trying to preserve as much code / as many ideas as possible
from the old one, at least the parsing code will be re-used and I
don't think there will be any regression in the features supported if
the new system is accepted.

Regards.
-- 
FFmpeg = Foolish and Frenzy Mortal Patchable Extended Geek



More information about the ffmpeg-devel mailing list