[FFmpeg-devel] [RFC] New option system

[Michael Niedermayer]

On Sun, Aug 09, 2009 at 01:49:19AM +0200, Stefano Sabatini wrote:
> 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.

you can do the same with svn cp and some renaming


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

the current system is tested, i see no reason to replace it


[...]
> 
> > > 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 wheres the point?
you decide that you ommit the constants, then you use that to justify
why you need to add more complex mess to display the constants that arent
there?
in a month you realize that your 
"reasonable amount of tests for all the main corner cases" isnt that
reasoable as no single GUI can use the new options
after all they dont know the constants or wait, you will add another
handler callback, remotely executed, java database that returns the
constants that you ommit

I do not approve this code not as a rewrite nor as individual change.
Besides i do not approve any rewrite unless there is a strong justification,
and in this case there is not.


>   
> > > 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)?

if you want that, you first have to provide use cases that need/benefit
from it.
After that it can be discussed, we can see what options are available to
implement it and we can compare them and pick the best.
This does not belong in a big rewrite.


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

i should replace main() by a callback to a user provided function that gets a
opaque user provided context. That will solve all bugs

also one of the uses of AVOptions is for applications to use them, opaque
stuff makes it tricky
like for example the application that reads through AVOptions and converts
them to its own internal command line or GUI parameter structs


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

ehh you just said you have no problem with the speed so
no problem -> no need for a patch


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

you overshoot things a little
no problem -> we need no patch
if we ever do want to improve speed there are hashtables or alternatively
a sorted list of pointers but again we have no issue to fix in this respect.
please dont create a new issue by fixing a issue that doesnt exist.
besides this one is another great example why a rewrite is so bad, things
like that just get missed too easily if one doesnt look at them one by one
but instead has a big chunk of totally new code ...

summary: patch rejected

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

Breaking DRM is a little like attempting to break through a door even
though the window is wide open and the only thing in the house is a bunch
of things you dont want and which you would get tomorrow for free anyway
-------------- 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/20090810/ad30eed8/attachment.pgp>