[MPlayer-G2-dev] Re: About final cfg.h (config layer 0 discussion)

[Andriy N. Gritsenko]

    Hi, Arpi!

    I've seen your new cfg.h now and I want to say - I'm against of
including any function in it. You allow to create some unwanted
dependencies on config parsers by lazy developers later! I'm against
mpconfig_parse_*() very much! GUI won't have any parameter as string
anyway so you'll force GUI makers convert all parameters to string to
have compatibility with lazy programmers. It's ugly, sorry. Please,
remove any functions from cfg.h and place they in some cfg-l1.h if you
wish.

Sometime (on Sunday, May 25 at 15:18) I've received something...
>> 2. Do you still have something to argue about CONF_TYPE_LIST option type?

>isn't my CONF_TYPE_SELECT the same?

no.
CONF_TYPE_SELECT is to select _one_ option from list of available ones
(for example, combobox). CONF_TYPE_LIST is box for selection, remove, and
reorder options from list (box with two fields, for example - left is
selected list, right is available options list). These are too different
and CONF_TYPE_LIST may have items with different type (see my example
earlier).

>> 4. Let's change unsigned short type, version to unsigned short version,
>>   type in module_info_t definition to have compatibility forever. :)

>no
>i'm thinking about add type-specific versions
>so if we change vf API we don't have to force demuxer, stream, vo etc plugin
>authors/users to recompile their plugins just to get version number bumped.

Is there a difference between

unsigned short type, version;

and

unsigned short version, type;

anyway? This is anyway from this new API so I think it's easy. I'm
thinkig about module loader only - putting unsigned short version as
first member of struct will let us change that structure in the future
and still have version check available.

>>     Discussion about CONF_TYPE_GROUP isn't my concern right now since
>> that option is application level only and it's marked as TODO anyway. :)

>actually i've added CONF_TYPE_MODULE which does the same as you expected
>from CONF_TYPE_GROUP.

no. :)
I've explained that by examples earlier - submenus in GUI aren't modules,
are they? I'll try to explain a bit more - see the menu for video encoder
GUI:

  File  Video  Audio  Tools  About
+      ---------+
| Open...       |
| Save...       |
|---------------|+----------+
| Save stream...|  Audio... |
| File info     |+ Video... |
|---------------|| Text...  |
| Quit          |+----------+
+---------------+

You have menu group 'File' and subgroup in 'Save stream...' option in it.
I've explained good now? :)

Another example for using of CONF_TYPE_GROUP:

(*) constant quantizer [   ]
( ) variable quantizer ----------------------------
minimum quant [  ]   maximum quant [  ]
...................
---------------------------------------------------

so we have CONF_TYPE_SELECT with two choises:
1) CONF_TYPE_GROUP including 'vqscale' integer.
2) CONG_TYPE_GROUP including 'vqmin', 'vqmax' and other options.

When we select constant quantizer then second group will be disabled,
when we select second group then we cannot change value of 'vqscale'.
Config of vf_lavc must have hints for that. Simplest way for it is just
have "transparent" CONG_TYPE_GROUP type. Did you understand?

If we don't preserve that availability in config struct then GUI maker
will have to create own config struct that will allow that by copying all
from existing ve_lavc config but with changed a bit structure and then
keep it up to date forever. It's too bad and we wanted to avoid that,
aren't we?

BTW, this sample explaining why I wanted to have CONF_TYPE_SELECT as
subconig type too - config_select_list_t doesn't allow us to have choises
as CONG_TYPE_GROUP but these must be available for GUI. ;)

>>   for vf->control(). These requests will have void *data as parameters
>>   array, preallocated by caller: for VFCTRL_GET_PARAMS it's array with
>>   undefined values and filter has to set all changeable parameters there;
>>   VFCTRL_SET_PARAMS will ask the filter to apply changed parameters to
>>   the instance. Size of that array defined by config_size in module info.

>imho VFCTRL_GET_PARAMS is useless.
>config_data_t* (the struct containing the preallocated config vars)
>is part of the instance structs (vf_instance_t), and is used by the filter
>to keep the config data. (ie it's free()'d only at filter uninit).

>it's always readable by upper layer (so VFCTRL_GET_PARAMS is useless), but
>we should add VFCTRL_SET_PARAMS (or better name) to notify filter about
>config changes made by UI. i'm not completely sure about this one yet.
>(runtime config change is always a mess...)

    As you said yourself, runtime config change may be a mess and I don't
like to change parameters in running application - only filter may decide
if parameter may be applied or not. So I've proposed to get copy of
config_data_t* and let filter apply it instead of direct changing it,
changes in a copy are always safe. So application will have something
like:

data_copy=malloc(vf->info->config_size);
vf->control(vf, VFCTRL_GET_PARAMS, data_copy);
.....change parameters.....
vf->control(vf, VFCTRL_SET_PARAMS, data_copy);
free(data_copy);

Of course, we could

data_copy=malloc(vf->info->config_size);
memcpy(data_copy, vf->cfg, vf->info->config_size);
.....change parameters.....
vf->control(vf, VFCTRL_SET_PARAMS, data_copy);
free(data_copy);

but I don't like to trust developers if they will always use a copy but
don't change existing data. You know it yourself, many programmers are
too lazy to know all requirements and that may cause troubles.

    With best wishes.
    Andriy.