[FFmpeg-devel] [RFC] New option system

[Stefano Sabatini]

On date Wednesday 2009-07-22 03:46:09 +0200, Michael Niedermayer encoded:
> On Wed, Jul 22, 2009 at 12:46:55AM +0200, Stefano Sabatini 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).
> 
> you already wrote a patch doing the same to the current API, you just didnt
> follow up on the issues raised in the review now you rewrite it all, do
> you think that would pass review quicker or with less work?!

I considered that route (patching the current system), but I came to
the conclusion that a new system written from scratch would require
less work and less pain.

Also in this way we don't need to broke backward compatibility, the
new options system would simply replace the old one internally, while
libavcodec/opt.h would get eventually deprecated and dropped at the
next major bump.

As for the stability of the new system, I have no problem at providing
a reasonable amount of tests for all the main corner cases.

> > 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).
> 
> change the default from double to char * and parse it and you have your
> arbitrary default support in the current system

Looks like a good idea, indeed if we define a set_string_handler() then
there is no need for having the default value not expressed like a
string.

> > 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, 
> 
> > setting its default value, 
> 
> thats not a clean solution it rather changes one limitation into a worse
> see above on how to do it properly
> 
> 
> > setting it from
> > a string representing a value, 
> 
> yes we could have sucha handler, it was the subject of that patch you didnt
> follow up on ...
> 
> > showing a description of what the
> > option does), 
> 
> for that just the description itself is fine ...

My idea was to use such an handler for describing the various options
values, for example there are named constants which are not defined as
named constants in the usual way. For example such an handler for a
pixfmt may list all the string representing a pixfmt.
  
> > and containing a pointer to an opaque struct containing
> > the information required to register a specific option.
> 
> hmm
> 
> 
> > 
> > > 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,
> 
> i dont see any design shortcomings that could not be trivially fixed,

What if I want to define an option which sets more than one value (for
example to set size -> w, h)?

Currently there is space for just one offset in the AVOption
definition, while an option may define more than one field in the
context. Similarly, the min/max constraints may not be adeguate for
each option, that's the reason for which I'd like to have an opaque
struct with all what is needed to set one particular option, that's
also the reason for the need of a register function, as such opaque
struct containing that information needs to be allocated.

> > also most of the patches I proposed for addressing those issues have
> > been rejected.
> 
> the so called rejected patches where
> A. not rejected but rather you did not fix the issues pointed at
> B. lacked any advantage (faster is nice but in what case does it matter
>    for avoptions? all speed relevant code should access things directly
>    given that clarity seems more important to me)

My problem has never been with the speed (a bin-search can be
implemented also with the current system, I should have a patch for
that somewhere, the main problem was that it resulted in having the
named constants intermixed with the options names - resulting in them
being scattered through the array), but with the lack of features /
difficulty to extend it.

Regards.
-- 
FFmpeg = Fierce and Forgiving Minimalistic Proud Eccentric Guru