[FFmpeg-devel] [RFC] New option system

Wed Jul 22 03:46:09 CEST 2009

On Wed, Jul 22, 2009 at 12:46:55AM +0200, Stefano Sabatini wrote:
> 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.

it added an alternative parsing system with no plans to remove one of
these then redundant systems.
I did not reject the handlers just having 2 alternative code paths
you never corrected this, the patch thus is not rejected just waiting
for the rquested changes to be done

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

Do you think newly written code is bug free?
your code surely is not bug free, the time needed to fix its bugs and all
limitations and glitches it likely has should not be ignored.
One rewrites code if its too bad to do anything else, one doesnt rewrite
code because one has too little time to fix it ...

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

because it is pointless and your last reply in that thread does not seem
to disagree that much about it

If you need speed you MUST access a variable directly ...
If you want just a highlevel way to access fields the search should not
matter.
I dont mind at all if the code is made faster but either
A. it isnt more messy / less readable
or
B. there is some code that benefits from the faster AVOptions, Iam just
against making "av_cold" code obfuscted to make it a little faster.
Please first explain in what case its speed matters ...

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

I see what you mean but unless you explain where this matters it trades
compexity against speedup where we dont need speed.

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

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

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

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

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

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

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

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

The greatest way to live with honor in this world is to be what we pretend
to be. -- Socrates
-------------- 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/20090722/317ed3ac/attachment.pgp>