[MPlayer-dev-eng] Re: stupid option handling.. again???

[Alban Bedel]

Andriy N. Gritsenko wrote:
>     Hi, Alban Bedel!
> 
>     Sorry if my last letter offended you. I really want to get rid of all
> misunderstaindigs and reach of consent. Let's talk about it and find good
> solution that will be good for both sides, ok?
> 
> Sometime (on Thursday, April 10 at 13:24) I've received something...
> 
>>I really don't want to stop you from doing anything. Here is a copy of the
>>mail you never received (I'm really sorry for forgeting to retry to send it).
> 
> 
> [.......]
> 
>>I'm sorry if you take that against you but that's rather against your
>>patch. I wrote the first version of the config system (so most of the code
>>in cfgparser.[ch]). In the first system i wanted to let each part of
>>mplayer register his options in order to minimize the number of global
>>vars. In your patch you also use registering quiet a lot.
> 
> 
>     I did that to let each filter register itself so it can be visible as
> soon as it developed. No changes need at filters at all, exempt config
> structure (but you did it too, isn't it?). Changes for new config scheme
> are even less than yours.
Yes, but drawbacks are much bigger with your solution.
>     Ok, we can let filters be undefined and parsed on run. But then we
> cannot reach and configure them from Gui. One way to do it is to describe
> it in Gui explicitly. Another point of registering filters is to allow
> set its parameters in config file. Now you can (I didn't chacked if even
> that is possible, may be not) only describe full -vf (or -vop) line
> there. It's bad for me, I don't like that way. You can solve both these
> problems (and don't mentioned yet a problem with modularized filters that
> may come in the near future) by registering from vf.c. What's wrong with
> it?
Dependency. stfa.

>     BTW, I wonder where is that registering stuff you mentioned above? I
> couldn't find it, point me, please. May be it was in too old version of
> mplayer/mencoder so i din't see it?
Yes it was in old version, browse the cvs (there where somes in 
libmpdemux iirc).

>>Later it turned out to be a bad idea as by registering you introduce an
>>uneeded dependency. It's just the same (and is better) to simply have a
>>config variable wich is used by the main part of mplayer.
> 
> 
> Ok. I think just as developer. My reasons:
> 1) when I add new filter/etc. then why I want to know every point where
>   that filter is mentioned? I want just add the file and include it in
>   main list. It's a little problem and that is why some filters are
>   missed in documentation yet. Sorry, but it's inconvenient.
Since when do you have to do more than putting the filter in the main 
list to add a filter ?????

> 2) let's think if I want to modularize that stuff. In current project I
>   cannot do it at all - I have all my code linked. If I do anything like
>   a loader then I cannot get any configuration for any modules since it
>   needs static links to main config.

There is really no advantage. Again f we use registering then the 
register function (and all his deps) have to be linked in. If we just 
export a var the host app can chose what to do.
If it want to use this stuff it can by using the exported vars (as
mplayer is doing). If it doesn't care then it just have to ignore these
vars.

>>Another thing that i don't like is all the changes you did in m_config.c.
>>Do be honest i don't really understand why you need these changes. My idea
>>with the new system was to maximize the use of specialized options and
>>minimize the change on the config code (wich is really the sensitive part).
> 
> 
>     I've even simplified the config parser code, which is really the
> sensitive part. Now it's shorter:
> 
> -rw-r--r--    1 andrej   staff        9174 Mar 15 20:00 m_config.c
> -rw-r--r--    1 andrej   staff       14682 Apr  9 19:30 m_config.c.new
> -rw-r--r--    1 andrej   staff       32663 Mar 15 22:49 m_option.c
> -rw-r--r--    1 andrej   staff       24965 Apr 10 14:27 m_option.c.new

If that's the same as the patch you sent, you can forget that. You just
hack m_config, adding a lot of stuff there and removing stuff you think
unuseful from m_option.

> And it's even with a new function m_config_print_config() in m_config.c
> to exclude any parsing m_config_t* outside of parser code! :)  Correct me
> if I don't get right what you meant.
The size in line isn't a what i would call a measurement ;) I think *I*
still didn't get what you want to achive.

>>Finnaly concerning the way you handle the vf options, i'm sorry to say that
>>but you totaly missed the point. Each filter should get his OWN options,
>>using statics vars wich are set just before the open call is no more than a
>>hack. It work bcs mplayer is not a multithread program but it's not an
>>acceptable solution imho.
> 
> 
>     Thank you, I really missed something in it, now (for week now) I'm
> working on it. I'll discuss it with you when it ready, ok? :)
>     Anyway if you use multithreaded program something may be wrong if
> someone will change config of running filter, isn't it? I think if we get
> MPlayer/MEncoder multithreaded then we have to rethink it all. :)
MPlayer/MEncoder will NEVER be threaded as it's one of the core feature.
But some parts (like libmpdemux) are meant to be usable without (most
of) the mplayer code base. So these could happend to be used in a
multithreaded prog (hence it would be nice to have them fully
reentrant).
Also note that m_config is only meant to configure stuff at startup not
while mplayer/mencoder is running.
We may latter add support for runtime options settings in filter but
that don't have anything to do with the m_config stuff.

>>Now, i must say that your patch add some intersting things but imho not in
>>the 'right way'. Concerning the 'possibility of integration of config into
>>GUI' i don't think that it's so easy. I think we need some new kind of
>>options type like key-value pair where the internal value can be of any
>>type (so we can attach 'nice names' to any kind of options wich have
>>'unfriendly' arguments). Otherwise we can also add some options flags as
>>hints for the widget type to use (as the option kind alone won't be enouth
>>for all iho). Later if it's really envolve on the gui side we migth needed
>>more changes like a 'gui widget' field in the option to hold a real widget
>>desc.
> 
> 
>     As I said before, new extra "desc" field is for "nice names" for Gui,
> also it give you a nice commandline help. About 'widget hints' above: Gui
> already know which widget you have to use - see my previous letter how
> field type has to be interpreted in Gui. :)  The main goal of my patch
> was to give Gui all need hints
I have nothing aginst such thing but then add only one field for help
msg, gui hints, etc. As you probably noticed adding fields to the 
m_option struct isn't a so good idea. So if possible this time should
be the last time ;)
>>For the codecs configuration my plan is to use the same thing as for the vf.
> 
> 
>     Agree.
> 
> 
>>-vc, -ac are alredy lists so switching to vf like syntax will do no arm. The
>>currents existings options will stay (but as global option) and set only
>>the defaults. But first i'd like to finish the vf. Imho the vf should
>>get there 'config struct' via the arg param, so my plan is to first
>>make all filters ready (as there authors don't seems to be very likely
>>to do it) and then change the argument type on the open call.
> 
> 
>     Heh. I don't like to push all authors - since my way doesn't touch a
> syntax (what I don't like at most in your current way, sorry, and I think
> most of users and developers also hate changes of syntax), it's not need
> to wait, all filters will use the same syntax now as before, even if you
> will move parsing from sscanf() to config parser. :)
Again i didn't changed the syntax. Pls point me to a vop string wich was
working and have now a different behaviour (except Gabu
>>If you think that's needed we can also add some options to setup the
>>defaults values of the filters options (so you can put them in the
>>config file). But it's perhaps better to add sufix support to the
>>'object list' type. By this a mean supporting -vf-add f1,f2 and
>>-vf-del 2 (index is needed as the same filter name may appear several
>>times). It this way one can have a default filter chain in his config
>>file and modify it when needed.
> 
> 
>     If you don't like a war too then just wait a little, I'll send you
> all my proposals as a new m_* files by private mail, ok? I'd like to get
> an agreement at that all.
>     My way is to let filters have the same syntax, let them save it in
> config file and let config to be browsed by Gui. It's all without any
> further complication of syntax, of course.
> 
> [.......]
>     About -vf - I missed something in it too but it's resolved now, I'm
> still working on it.
> 
> 
>>I understand that it probaly take you a lot of time for this patch but i
>>can't accept it. Also the parts in libvo, libao, etc where you register
>>the options won't either be accepted by the maintainers of these parts
>>of mplayer (bcs of the unneeded dependency it introduce).
> 
> 
>     Let's ask the maintainers of these parts? :)  BTW, what is dependency
> that you said about? You meant m_config* stuff? But you already changed
> many in libmpcodecs, I don't think it's a problem with any other stuff.
It depend only on m_option/m_struct wich is imho 'ok'.
	Albeu