typo in m_config.c

[jose@…]

minor bug due to a typo in the configuration parsing. logical vs bitwise AND ...

this is from the SVN HEAD and is in some releases, too.

proposed patch:

--- m_config.c.orig Sat Oct 7 10:10:01 2006
+++ m_config.c Sat Oct 7 10:10:43 2006
@@ -124,7 +124,7 @@

continue;

if(co->opt->flags & (M_OPT_GLOBAL|M_OPT_NOSAVE))

continue;

• if((co->opt->flags & M_OPT_OLD) && !(co->flags && M_CFG_OPT_SET))

+ if((co->opt->flags & M_OPT_OLD) && !(co->flags & M_CFG_OPT_SET))

continue;

if(co->flags & M_CFG_OPT_ALIAS)

continue;

[reimar]

Do you have have a sample command line that this patch will fix? I'm a bit too
lazy to try to understand that code.
Of course the current code is not as it was intended, but unfortunately that is
no guarantee that "fixing" it doesn't actually break stuff.

[jose@…]

i do not, no. i don't use mplayer, i don't know the code, etc ... i found this
bug by using google codesearch and some simple REs to find this sort of typo.

i took some time to read the code (and to try and understand it better), and i'm
not entirely sure what it's supposed to do. it looks like something about user
requested configuration options (screenh and screenw) and the "old style" of
options (piecing together comments in the header files).

i wonder if it's related to this quirk in mencoder.c:

543 /* HACK, for some weird reason, push() has to be called twice,
544 otherwise options are not saved correctly */
545 m_config_push(mconfig);
546 play_next_file:
547 m_config_push(mconfig);
548 m_entry_set_options(mconfig,&filelist[curfile]);
549 filename = filelist[curfile].name;

the function this bug's code occurs in (m_config_push()) is also called in
play_tree_iter_push_params() (playtree.c:452):

438 static void
439 play_tree_iter_push_params(play_tree_iter_t* iter) {
440 int n;
441 play_tree_t* pt;
442 #ifdef MP_DEBUG
443 assert(iter != NULL);
444 assert(iter->config != NULL);
445 assert(iter->tree != NULL);
446 #endif
447
448 pt = iter->tree;
449
450 We always push a config because we can set some option
451 while playing
452 m_config_push(iter->config);

like i said i don't use mplayer, i don't claim to know much about the code, but
i did try and make sense of it. i'm not entirely sure of the options paths that
would lead to this code getting called and evaluated incorrectly.

the option being (appearantly mistakenly) evaluated here aliased to CONF_OLD,
which is set in screenh and screenw in the options structure mplayer_opts in
cfg-mplayer.h:

cross definition:
m_option.h:308:#define M_OPT_OLD (1<<6)
m_option.h:321:#define CONF_OLD M_OPT_OLD

use:

212 {"screenw", &vo_screenwidth, CONF_TYPE_INT, CONF_RANGE|CONF_OLD,

0, 4096, NULL},

213 {"screenh", &vo_screenheight, CONF_TYPE_INT,

CONF_RANGE|CONF_OLD, 0, 4096, NULL},

CONF_OLD (or M_OPT_OLD) appears nowhere else. again, i haven't taken too much
time to evaluate the set of options and codepaths that would get this evaluated.
but i hope this helps you determine what may be afoot here.

[reimar]

Patch applied, though I didn't find the exact effects either. Maybe it motivates
somebody to cleanup and comment some code :-)