[MPlayer-dev-eng] [PATCH] tv config in file (load/save)

[Vladimir Voroshilov]

Hi, Otvos.

2007/9/11, Ötvös Attila <oattila at chello.hu>:
> 2007. szeptember 10. 20.35 dátummal Vladimir Voroshilov ezt írta:
>
> Hi Vladimir Voroshilov!
>
> > Goo work, Otvos.
> > Here are some issues (i did not make full review yet).
> >
> > >+#define ST_OFF(f) M_ST_OFF(tv_param_t,f)
> > >+static m_option_t tv_global_opts_fields[] = {
> > >+    {"immediatemode", ST_OFF(immediate), CONF_TYPE_INT,CONF_RANGE, 0, 1,
> >
> > NULL},
> >
> > >+    {"noaudio", ST_OFF(noaudio), CONF_TYPE_FLAG, 0, 0, 1, NULL},
> > >[...]
> > >+ {"saveall", ST_OFF(saveall), CONF_TYPE_FLAG, 0, 0, 1, NULL},
> > >+ { NULL, NULL, 0, 0, 0, 0, NULL }
> > >+};
> >
> > Is this copy of structure defined in сfg-common.h?
> > Is it poosible to reuse those definition?
> > I'm against this code duplication if it is possible to avoid it.
>
> tv_global_opts_fields[] : allow in tv config file (global section)

IMHO, global section should allow ALL options which can be passed in
~/.mplayer/config.

> tv_global_opts_fields_mix[]: disallow in tv config file (global section)

"scan","scan_threshold","scan_period" is not "-tv" options. it is
"-tvscan" related options.

Other options are descussable.

"tvconfigfile" can be global option, like "mplayer -tvconfig <myconfig>"
"tvconfigfilesave" can be tvconfgifile+".new" - no additional option needed.

notvconfig=(tvconfigfile==NULL)
notvconfigsave: discussable.


> > >+static void parse_channels_priv(tvi_handle_t *tvh)
> > >+{
> >
> > What is this supposed to do?
>
> convert from channel_priv_t to tv_channels_t

I suggest extending tv_channels_t to include all desired info (freq
offset, norm, etc)
This will avoid  unnecessary code duplication.

> > >+ int notvconfig; ///< don't use tv.conf
> > >+ char *tvconfigfile; ///< config file name
> > >+ char *tvconfigfilesave; ///< config file name with save
> > >+ int tvconfigsave; ///< save config file if close
> > >+ int saveall; ///< save all fields
> > >+ void *channels_priv;
> > >} tv_param_t;
> >
> > Is notvconfig really needed?
> > Why not "notvconfig<=>(configfile==NULL)" ?
>
> If exist tv.conf (default config file) in mplayer home dir
> (~/.mplayer/tv.conf) is use this. The notvconfig options disable usage defult
> file if exist.

if tv.conf exists, mplayer must use it. If user don't want to use
tv.conf he should either remove it or pass tvconfig without argument
(discussable).

> > What about putting config file parsing code (or at least for "global"
> > section) into
> > stream_open of stream/stream_tv.c (before stream->priv assignement) ?
> > In this case both default and assigned values for options will be available
> > and duplicated structure definitions will not be needed..
>
> Problem similar -gui options.
> If order is:
> 1. parse ~/.mplayer/config
> 2. parse mplayer arguments - overwrite config
> 3. parse ~/.mplayer/tv.conf or other if set configfile parameter - overwrite
> config and mplayer arguments
>
> User can't specity arguments in this case because overwrite tv.conf.

User CAN do this:
1. parse ~/.mplayer/config
2. parse mplayer arguments - overwrite config
3. parse ~/.mplayer/tv.conf or other if set configfile parameter - overwrite
config and mplayer arguments ONLY if them still equals to default
values (in stream_open() you will
have access to both default values through stream_tv_defaults variable
and values got from ~/.mplayer.conf through
opts parameter. your can compare them and take decision about options
from tv.c).

In this case we will have these priorities:
1. options in command line
2. options in ~/.mplayer/config
3. options in ~/.mplayer/.tv.conf
4. defaults (as last resort)

To enable options in tv.conf user have to remove them from ~/.mplayer/config
(Perhaps mplayer should otput apropriate message)

> If order is:
> 1. parse ~/.mplayer/config
> 2. parse ~/.mplayer/tv.conf - overwrite config
> 3. parse mplayer arguments - overwrite config and tv.conf
>
> User can't specify tv config file in this case because loadtvconfig() after
> parsing mplayer arguments where specify tv config file.


Agree.

> This patch:
> 1. parse ~/.mplayer/config to stream_tv_defaults[]
> 2. parse mplayer arguments to stream_tv_defaults[]
> 3. parse tv config file to tv_param_cfg[]
> 4. mix stream_tv_default[] + tv_param_cfg[] in mix_tv_param():
>
> if stream_tv_defaults.opt == empty_tv_defaults.opt and
> tv_param_cfg.opt == empty_tv_defaults.opt:
> no specify config/arguments/tv.conf -> overwrite tv_defaults.opt
>
> if stream_tv_defaults.opt == empty_tv_defaults.opt and
> tv_param_cfg.opt != empty_tv_defaults.opt:
> specify only tv.conf -> overwrite tv_param_cfg.opt
>
> if stream_tv_defaults.opt != empty_tv_defaults.opt:
> specify config/argumnets -> drop specify in tv.conf


This is the same  behaviour as i desribed above, but without three
additional structures.

First look shows that you need only one additional structure: for
options used in
"channels" section of tv.conf, but this will need more previse look.


-- 
Regards,
Vladimir Voroshilov     mailto:voroshil at gmail.com
JID: voroshil at gmail.com, voroshil at jabber.ru
ICQ: 95587719