[MPlayer-G2-dev] Re: g2 config - restart...

[Alban Bedel]

Hi Arpi,

on Wed, 14 May 2003 22:27:29 +0200 you wrote:

> Hi,
> 
> > >     BTW, did you wrote new config_t and module_info_t structures (I
> > >     mean
> > > API layer 0) yet? If you didn't then there is a draft here (from
> > > yours API description, of course - you really fine generalized all).
> > > :)  Also I typed here some #define because they have values of
> > > fields of config_t. I hope I did all correctly now. :)  BTW, I'm not
> > > sure if filename must be in module_info_t - plugin subsystem can
> > > have it in internal arrays. That subsystem anyway has to keep bad
> > > filenames list to avoid trying its (in cache also, I think) further.
> > > Bad filenames - if plugins are in plugin's subdirectory but are not
> > > plugins or have incompatible version.
> > 
> > I like the cache idea but imho the it should use more than the
> > filename to identify a file. Smthg like filename + date (a hash would
> > be better but that's probaly overkill) so we could detect if the file
> > changed since last run.
> 
> Of course filename is not enough, maybe filename+date also not, but
> dunno. But these infos don't belong to the module_info_t at all.
Sure that it doesn't belong to the module_info_t at all. I just a thought
to this while reading this few lines about caching.

> The filename is there for the dlopen() only, to allow any filename, but
> i'm not sure we really want to allow it. Esp., that CLI interfaces would
> load plugins by filename, without scanning the plugin dir first.
> 
> 
> > > ------------------------- begin of cfg.h
> > > #ifndef __CFG_H
> > > #define __CFG_H 1
> > > 
> > > typedef struct {
> > >   char *name;
> > >   void *p; /* variable pointer, type specific */
> > >   int type;
> > >   unsigned int flags;
> > >   float min, max; /* range */
> > >   void *priv; /* additional pointer, type specific */
> > >   char *desc; /* short description for help and gui */
> > > } config_t;
> > 
> > I liked the idea you had of putting some hint for GUI widget type.
> 
> Yes, I also like that idea.
> 
> > It would avoid having different options type wich are in fact the
> > same C type and it would also make things nicer for GUI writers.
> > Also type specific flags would be nice. Ok there is priv but
> > if a type only need a few flags priv is overkill. We could either
> > add another flag field or use the lower bit for common flags and
> > the higher for type specific ones.
> 
> Yes, we could add more flags, to more exactly describe how it should
> look like on the GUI. Including step units (for sliders), and others.
> So a clever GUI coder could write an 'interpreter' which dinamically
> build gui panels based on these info. I've seen such thing in xine, they
> build Xv setup panel runtime based on the attribute list returned by
> xvinfo.
You'r right that for a true dynamic interface building we need more than
a few flags. Another field to put a pointer to some (optional) other
struct wich fully discribe the 'recommended' widget ?

> > > /* note: version has to be first in ModuleInfo struct in loadable
> > > module
> > >    to allow us always do validation/version check of module */
> > > typedef struct {
> > >   unsigned char version[4]; // { PLUGIN_VERSION_MAJOR,
> > >   PLUGIN_VERSION_MINOR, 'M', 'P' } 
> > >   int type; // for loadable module identification
> > >   char *name;
> > >   char *desc; // short description for help and gui
> > >   char *maintainer;
> > >   char *author;
> > >   char *comment;
> > >   config_t *opts;
> > >   void *param_defaults;
> > >   size_t *param_size;
> > >   char *filename; // filled by plugin subsystem, NULL for internal
> > > } module_info_t;
> > > 
> > > /* type (menu field)	->p		->min	->max	->priv */
> > > enum {
> > > // flag (checkbox)	int*		reset	set	-
> > >   CONF_TYPE_FLAG=0,
> > > // integer (slider)	int*		min	max	-
> > >   CONF_TYPE_INT,
> > > // float (slider)	float*		min	max	-
> > >   CONF_TYPE_FLOAT,
> > > // position (slider)	off_t*		min	max	-
> > >   CONF_TYPE_POSITION,
> > > // string (input box)	char**		min	max	-
> > >   CONF_TYPE_STRING,
> > > // string as list - must be obsoleted to CONF_TYPE_LIST
> > >   CONF_TYPE_STRING_LIST,
> > Obsolated stuff in a draft ??????
> 
> I can't see why is string list is obsolete? It's a good thing and is
> used a lot by g1 and in g2 too.
Well for GUI in some case it's perhaps a bit too generic, but i agree
that it's should stay.

> > 
> > > // function		cfg_func*	-	-	revert-cfg_func*
> > >   CONF_TYPE_FUNC,
> > >   CONF_TYPE_FUNC_PARAM,
> > >   CONF_TYPE_FUNC_FULL,
> > I think we should avoid callback based stuff. These were nice in the
> > old days for very specific stuff. But now the only place in G1 where
> > they are really needed is for the -aa* options of aalib. But as in G2
> > each module instance have it's own context it doesn't really make
> > sense as options parsing append before instanciation.
> > For options global to a layer that would make some sense but then the
> > callback also need to get the struct/context on wich it should
> > operate. So this context should be static otherwise you can't put
> > it in the config_t->priv field. But we don't want static stuff so
> > for G2 lib that's anyway completly unusefull.
> > We may let that for app level stuff but i doubt it's really usefull.
> 
> it's usefull. think of an external plugin (wirtten by third party) which
> requires special parsing for some parameters. Ok, they could receive
> string and do parsing at open(), but implementing a parsing function is
> better, as the UI can notice syntax errors before getting to open().
Yes, you'r right.

> but imho we don't need 3 of them, only one such type is enough.
> but we should define how to pass struct offset/pointer and number of
> args.
I agree too, only having the "full" is more than enought unless we
want to kill everything gcc warnings ;)

> > > // text (none)		char**		-	-	-
> > >   CONF_TYPE_PRINT,
> > That's also only usefull at app level.
> 
> this one is really obsolete, imho.
> it was used in g1 to abort UI at parsing level with error... (for
> obsolete parameters or when requested not compiled-in things)
> 
> or dunno, maybe it's still usefull for something...
> but should be handled differently.
Imho the only place where it's usefull is for the top-level app
like it's used in G1. We can provide it for the apps but in G2
libs it's really uneeded afaict.

[...]

> > > /* macro to get relative address of x.y in struct x */
> > > #define STRUCT_OFF(x,y)		((void *)(&x.y-x))
> > I doubt that this macro really work. And even if it work it need an
> > instance to apply it to. Why don't you just take the one in m_struct.h
> > (wich is from glib.h) wich just need the declaration of the struct.
> 
> agree
> 
> btw i defined in my layer 0 draft, that you should make a static stuct,
> filled with the defaults. it also serevs as target of pointers in
> config_t, but that static instance is read-only, must not be modified.

That's why when discribing struct fields config_t->p should store the
offset of the field in the struct and not a normal pointer. The offset is
easy to get with such macro :

// From glib.h (modified ;-)
#define M_ST_OFF(struct_type, member)    \
    ((void*) &((struct_type*) 0)->member)
#define M_ST_MB_P(struct_p, struct_offset)   \
    ((void*) (struct_p) + (unsigned long) (struct_offset))
#define M_ST_MB(member_type, struct_p, struct_offset)   \
    (*(member_type*) M_ST_MB_P ((struct_p), (struct_offset)))

Then it's no prob. to set a specific field of the struct after having
allocated some mem for it.

> but you're maybe right, if defaulting everything to zero is ok, then we
> could left this out (optinally).

Imho the 'default struct' is needed bcs in many case you need default
values wich are not zero and there is also cases where zero is a fully
valid value but it shouldn't be the default (the evil case ;)

Until now imho there is no big problem with implementing such stuff.
The only problematic thing that i can see is the current main problem
of the 'new' config code in G1 : dynamicly allocated stuff vs. mem
leak vs. static values.
If we strictly deal with struct that are create by (properly) copying
the default stuct it's not a problem as we will only have dynamiclly
allocted stuff.
But if we also want to operate on 'normal' vars (we probably want)
then it's a problem.
In current G1 it's solved with a hack (when options are registered
in the context saving system those who use dynamicly allocted
mem (like string) have their initial value replaced by a copy of it
so there is then only dynamic stuff). If somebody know how one can
tell if a mem area was dynamiclly allocated or not then it's easy
to solve. Othewise i really can't think of smthg clean.

	Albeu