Hello, Sometimes it is usefull no ignore system and user-specific config files and use only command line options. Especially this is usefull on debugging or figuring out what is wrong with playback. Of course, it possible to override all config file options on the command line, but with large and rather complicated configs (e.g. as mine one) it is a pain. Usually I just temporary rename my mplayer.conf or ~/.mplayer/config to somewhat other name, but this is incovenient too. So I propose a patch which adds appropriate -noconfig command line option to both mplayer and mencoder. It will disable config files parsing at all. The first patch adds functionality described above. Note that option must be checked before configs parsing in the same way as -really-quiet. The second patch is just cosmetics required after first patch applying. Anyone can figure out that there is some code duplication in mplayer.c and mencoder.c. I dislike code duplication, so the third patch is intended to fix this by moving common code (related to the first patch) from these files to mpcommon.[ch]. This patch requires the first patch to be applied. And finally the fourth patch adds new option description to the manual page.
Hi, On Friday 11 April 2008 18:08, Andrew Savchenko wrote: [...]
And finally the fourth patch adds new option description to the manual page.
I have just remembered that options must be in alphabetic order inside a group in the man page. So please use the following patch instead of noconfig_manual.patch.
On Fri, 11 Apr 2008 18:08:37 +0400 Andrew Savchenko <Bircoph@list.ru> wrote:
Hello,
The first patch adds functionality described above. Note that option must be checked before configs parsing in the same way as -really-quiet.
Good idea.
int m_config_parse_config_file(m_config_t* config, char *conffile) { + if (noconfig) + return 1;
I don't like that very much. Having a global simply disable a function is ugly. Better check before looking for config files. It's a bit more work but you could also allow finer disabling (to only use the user's config file for ex.).
--- mplayer_base/mp_msg.h 2008-03-08 02:46:16.000000000 +0300 +++ mplayer/mp_msg.h 2008-04-11 17:30:24.000000000 +0400 @@ -3,6 +3,7 @@
// defined in mplayer.c and mencoder.c extern int verbose; +extern int noconfig;
This has nothing to do in mp_msg.h.
The second patch is just cosmetics required after first patch applying.
No need to split that lonely blank line. Added code is allowed to contain black lines :)
Anyone can figure out that there is some code duplication in mplayer.c and mencoder.c. I dislike code duplication, so the third patch is intended to fix this by moving common code (related to the first patch) from these files to mpcommon.[ch]. This patch requires the first patch to be applied.
+void preparse_command_line(int argc, char* argv[]) +{ + int i; + + for(i=1; i<argc; i++) { + if(!strcmp(argv[i], "-really-quiet")) + verbose= -10; + if(!strcmp(argv[i], "-noconfig")) + noconfig= 1; + } +}
With respect to this patch, this is fine. But it is quiet a hack that will catch stuff which would not normally be treated as command line options. Ideally this should latter be replaced by something robuster. Albeu
Hello, and thank you for reply. On Saturday 12 April 2008 12:57, Alban Bedel wrote:
int m_config_parse_config_file(m_config_t* config, char *conffile) { + if (noconfig) + return 1;
I don't like that very much. Having a global simply disable a function is ugly. Better check before looking for config files. It's a bit more work but you could also allow finer disabling (to only use the user's config file for ex.).
Done. Some code lookup shows that there is 5 different types of config files: 1) system-wide; 2) user-specific; 3) include; 4) user-filedir; 5) gui. So I introduced 4 new options: -noconfig, -disable-system-conf, -disable-user-conf, and -disable-gui-conf (gui only). Option -noconfig is equivalent to all -disable-*-conf specified simaltaneously. Behaviour of options -include and -user-filedir-conf is not changed. The reason is simple: if they are specified in the config file and -noconfig is used, they will be obviously ignored; if these options are specified in the command line together with -noconfig, than I assume that user knows what he/she is doing. This behaviour is documented in the manual.
// defined in mplayer.c and mencoder.c extern int verbose; +extern int noconfig;
This has nothing to do in mp_msg.h.
Fixed.
The second patch is just cosmetics required after first patch applying.
No need to split that lonely blank line. Added code is allowed to contain black lines :)
Very good. I am aware that mplayer developers are very strict about any cosmetics, so I was afraid newline will be treated as cosmetics.
+void preparse_command_line(int argc, char* argv[]) +{ + int i; + + for(i=1; i<argc; i++) { + if(!strcmp(argv[i], "-really-quiet")) + verbose= -10; + if(!strcmp(argv[i], "-noconfig")) + noconfig= 1; + } +}
With respect to this patch, this is fine. But it is quiet a hack that will catch stuff which would not normally be treated as command line options. Ideally this should latter be replaced by something robuster.
I do not understand you. What do you want me to do? The problem is that some command line options must be analyzed before configuration files will be. If I'll just move command line parsing before config files one, it will break overriding precedence (sysmem<-user<-CLI). Yes, currently used approach is not the best. Moreover it creates overhead: these options are parsed twice: in preparse and in normal parse routines, though second parsing is absolutely useless, but without it mplayer will complain about unknown options. Do you have some ideas how to fix this? I can see three different posibilities: 1) Leave it as is. 2) Parse all CLI options before configs, and either : 2a) save them in temporary storage and reassign after config files parsing; 2b) mark these options somehow and modify config files parser in order to not override marked options. 3) Modify parse subsystem to parse some subset of CLI options before config files and forbid their further modification whilst config files are parsed. Variant 2b) sounds as the best for me, but even it will create too much overhead. So perhaps our best is to leave things as is? Well, lets talk about new patches. The first patch moves some common code from mplayer.c and mencoder.c to mpcommon.[ch]. The second patch fixes a bug or, precisely speaking, missed feature of mencoder. Look at the man page: it says that mencoder read both system-wide and user-specific config files, but currently it reads only user-specific one. Anyone can easy check this by creating /etc/mplayer/mencoder.conf (or whatever is your's MPLAYER_CONFDIR). The reason is easly seen in comparison between parse_cfgfiles() in mplayer.c and mencoder.c. The last one use only get_path() which checks only for home directory. The second patch fixes this, it may be applied with or without the first patch. The third patch adds new functionality as described above. It requires both previous patches to be applied. The fourth patch is cosmetics required after previous patches, real cosmetics: not newlines, but indentation shifts. And finally the fifth patch is the manual page update. With best regards, Andrew
On Sat, Apr 12, 2008 at 08:17:41PM +0400, Andrew Savchenko wrote:
--- mplayer_old/mpcommon.c 2008-04-12 19:25:17.000000000 +0400 +++ mplayer/mpcommon.c 2008-04-12 19:39:52.000000000 +0400 @@ -213,11 +213,45 @@
+int disable_system_conf=0; +int disable_user_conf=0; +#ifdef HAVE_NEW_GUI + extern int disable_gui_conf; +#endif /* HAVE_NEW_GUI */
Useless #ifdef around extern declaration.
--- mplayer_base/DOCS/man/en/mplayer.1 2008-04-03 12:37:36.000000000 +0400 +++ mplayer/DOCS/man/en/mplayer.1 2008-04-12 18:05:52.000000000 +0400 @@ -632,6 +632,18 @@ . .TP +.B \-disable-gui-conf (GUI only)
GUI
@@ -692,6 +704,12 @@ . .TP +.B \-noconfig +Do not parse system, user, and gui (if enabled) configuration files
GUI
+at all. But if \-include or \-use\-filedir\-conf options are
New sentences on new lines in the manual page. Diego
Diego Biurrun wrote:
On Sat, Apr 12, 2008 at 08:17:41PM +0400, Andrew Savchenko wrote:
--- mplayer_base/DOCS/man/en/mplayer.1 2008-04-03 12:37:36.000000000 +0400 +++ mplayer/DOCS/man/en/mplayer.1 2008-04-12 18:05:52.000000000 +0400 @@ -632,6 +632,18 @@ . .TP +.B \-disable-gui-conf (GUI only)
GUI
"GUI" is already capitalized in the descriptive parenthetical comment. Do you really want it to be capitalized in the option name? That would make it more difficult to type... and would certainly require changing it in the actual code as well as the man page. -- The Wanderer My usual .sig is on vacation while I adjust to my new computer
On Sun, Apr 13, 2008 at 07:34:34AM -0400, The Wanderer wrote:
Diego Biurrun wrote:
On Sat, Apr 12, 2008 at 08:17:41PM +0400, Andrew Savchenko wrote:
--- mplayer_base/DOCS/man/en/mplayer.1 2008-04-03 12:37:36.000000000 +0400 +++ mplayer/DOCS/man/en/mplayer.1 2008-04-12 18:05:52.000000000 +0400 @@ -632,6 +632,18 @@ . .TP +.B \-disable-gui-conf (GUI only)
GUI
"GUI" is already capitalized in the descriptive parenthetical comment. Do you really want it to be capitalized in the option name? That would make it more difficult to type... and would certainly require changing it in the actual code as well as the man page.
No, I mistakenly removed one line too much from the quoted patch: +Do not parse gui configuration file. Here it should have been GUI. Diego
On Sat, Apr 12, 2008 at 7:17 PM, Andrew Savchenko <Bircoph@list.ru> wrote:
Hello, and thank you for reply.
On Saturday 12 April 2008 12:57, Alban Bedel wrote:
int m_config_parse_config_file(m_config_t* config, char *conffile) { + if (noconfig) + return 1;
I don't like that very much. Having a global simply disable a function is ugly. Better check before looking for config files. It's a bit more work but you could also allow finer disabling (to only use the user's config file for ex.).
Done. Some code lookup shows that there is 5 different types of config files: 1) system-wide; 2) user-specific; 3) include; 4) user-filedir; 5) gui.
So I introduced 4 new options: -noconfig, -disable-system-conf, -disable-user-conf, and -disable-gui-conf (gui only). Option -noconfig is equivalent to all -disable-*-conf specified simaltaneously.
How about having only one option -noconfig that takes combination of parameters "system","user","include","filedir","gui", "all" ?
On Sun, Apr 13, 2008 at 02:28:04PM +0300, Ivan Kalvachev wrote:
On Sat, Apr 12, 2008 at 7:17 PM, Andrew Savchenko <Bircoph@list.ru> wrote:
On Saturday 12 April 2008 12:57, Alban Bedel wrote:
int m_config_parse_config_file(m_config_t* config, char *conffile) { + if (noconfig) + return 1;
I don't like that very much. Having a global simply disable a function is ugly. Better check before looking for config files. It's a bit more work but you could also allow finer disabling (to only use the user's config file for ex.).
Done. Some code lookup shows that there is 5 different types of config files: 1) system-wide; 2) user-specific; 3) include; 4) user-filedir; 5) gui.
So I introduced 4 new options: -noconfig, -disable-system-conf, -disable-user-conf, and -disable-gui-conf (gui only). Option -noconfig is equivalent to all -disable-*-conf specified simaltaneously.
How about having only one option -noconfig that takes combination of parameters "system","user","include","filedir","gui", "all" ?
I agree. Just add one global option that skips parsing any config file. Everything else is far too specialised to warrant adding another option. Diego
On Sun, 13 Apr 2008 13:37:41 +0200 Diego Biurrun <diego@biurrun.de> wrote:
On Sun, Apr 13, 2008 at 02:28:04PM +0300, Ivan Kalvachev wrote:
On Sat, Apr 12, 2008 at 7:17 PM, Andrew Savchenko <Bircoph@list.ru> wrote:
On Saturday 12 April 2008 12:57, Alban Bedel wrote:
int m_config_parse_config_file(m_config_t* config, char *conffile) { + if (noconfig) + return 1;
I don't like that very much. Having a global simply disable a function is ugly. Better check before looking for config files. It's a bit more work but you could also allow finer disabling (to only use the user's config file for ex.).
Done. Some code lookup shows that there is 5 different types of config files: 1) system-wide; 2) user-specific; 3) include; 4) user-filedir; 5) gui.
So I introduced 4 new options: -noconfig, -disable-system-conf, -disable-user-conf, and -disable-gui-conf (gui only). Option -noconfig is equivalent to all -disable-*-conf specified simaltaneously.
How about having only one option -noconfig that takes combination of parameters "system","user","include","filedir","gui", "all" ?
I agree. Just add one global option that skips parsing any config file. Everything else is far too specialised to warrant adding another option.
I agree too, but the current ultra-trivial command line pre-parser won't be able to handle such non-trivial options. The normal option parser should be used instead, I'll take a look at that. Albeu
Hi, On Sunday 13 April 2008 15:37, Diego Biurrun wrote:
On Sun, Apr 13, 2008 at 02:28:04PM +0300, Ivan Kalvachev wrote:
On Sat, Apr 12, 2008 at 7:17 PM, Andrew Savchenko <Bircoph@list.ru> wrote:
On Saturday 12 April 2008 12:57, Alban Bedel wrote:
int m_config_parse_config_file(m_config_t* config, char *conffile) { + if (noconfig) + return 1;
I don't like that very much. Having a global simply disable a function is ugly. Better check before looking for config files. It's a bit more work but you could also allow finer disabling (to only use the user's config file for ex.).
Done. Some code lookup shows that there is 5 different types of config files: 1) system-wide; 2) user-specific; 3) include; 4) user-filedir; 5) gui.
So I introduced 4 new options: -noconfig, -disable-system-conf, -disable-user-conf, and -disable-gui-conf (gui only). Option -noconfig is equivalent to all -disable-*-conf specified simaltaneously.
How about having only one option -noconfig that takes combination of parameters "system","user","include","filedir","gui", "all" ?
I agree. Just add one global option that skips parsing any config file. Everything else is far too specialised to warrant adding another option.
Done, thanks to the new preparser it is much easier to implement ;-). But -include and -user-filedir-conf are left untouched as described in my earlier mail: On Saturday 12 April 2008 20:17, Andrew Savchenko wrote: [...]
Behaviour of options -include and -user-filedir-conf is not changed. The reason is simple: if they are specified in the config file and -noconfig is used, they will be obviously ignored; if these options are specified in the command line together with -noconfig, than I assume that user knows what he/she is doing. This behaviour is documented in the manual.
The first patch is a bugfix for mencoder: system-wide config file reading. The second patch is -noconfig option itself, requires 1st path. The third patch is cosmetics required after previous patches. And the fourth patch is a manual page update. With best regards, Andrew
On Mon, Apr 14, 2008 at 11:17:40AM +0400, Andrew Savchenko wrote:
The second patch is -noconfig option itself, requires 1st path.
And the fourth patch is a manual page update.
Note: Please never separate documentation updates from the rest of a patch. It is just an inconvenience since it has to be applied at the same time anyway. Diego
Hi, On Monday 14 April 2008 13:53, Diego Biurrun wrote:
On Mon, Apr 14, 2008 at 11:17:40AM +0400, Andrew Savchenko wrote:
The second patch is -noconfig option itself, requires 1st path.
And the fourth patch is a manual page update.
Note: Please never separate documentation updates from the rest of a patch. It is just an inconvenience since it has to be applied at the same time anyway.
Ok, I'll take it into account.
On Mon, 14 Apr 2008 11:17:40 +0400 Andrew Savchenko <Bircoph@list.ru> wrote:
On Saturday 12 April 2008 20:17, Andrew Savchenko wrote: [...]
Behaviour of options -include and -user-filedir-conf is not changed. The reason is simple: if they are specified in the config file and -noconfig is used, they will be obviously ignored; if these options are specified in the command line together with -noconfig, than I assume that user knows what he/she is doing. This behaviour is documented in the manual.
The first patch is a bugfix for mencoder: system-wide config file reading.
Applied with a fix, declaration are not allowed in the middle of code blocks (it break gcc 2.95.3).
The second patch is -noconfig option itself, requires 1st path.
Applied with a few fixes: preparsed option _must_ be global and the noconfig_all function can be made static.
The third patch is cosmetics required after previous patches.
Committed
And the fourth patch is a manual page update.
Committed, although I forgot to commit it along with the code as was pointed out by Diego. Albeu
Hi, On Monday 14 April 2008 15:54, Alban Bedel wrote: [...]
The first patch is a bugfix for mencoder: system-wide config file reading.
Applied with a fix, declaration are not allowed in the middle of code blocks (it break gcc 2.95.3).
Yes, I must remember at some point that mplayer must be compatible with gcc-2.95...
The second patch is -noconfig option itself, requires 1st path.
Applied with a few fixes: preparsed option _must_ be global
I agree with this.
and the noconfig_all function can be made static.
Well, I do not object against this. But what is the reason to make it static? It has no arguments neither variable declarations, thus I can see no benefits nor disadvantages of making it static. Sincerely, Andrew
On Tue, 15 Apr 2008 23:40:56 +0400 Andrew Savchenko <Bircoph@list.ru> wrote:
and the noconfig_all function can be made static.
Well, I do not object against this. But what is the reason to make it static? It has no arguments neither variable declarations, thus I can see no benefits nor disadvantages of making it static.
I don't see what you mean, perhaps you are confusing things with static methods in c++. A static function is just local to the compilation unit, so other compilation unit can't use it directly. It's generally a good habit to make any function which is not "public" static. Albeu
On Sat, 12 Apr 2008 20:17:41 +0400 Andrew Savchenko <Bircoph@list.ru> wrote:
+void preparse_command_line(int argc, char* argv[]) +{ + int i; + + for(i=1; i<argc; i++) { + if(!strcmp(argv[i], "-really-quiet")) + verbose= -10; + if(!strcmp(argv[i], "-noconfig")) + noconfig= 1; + } +}
With respect to this patch, this is fine. But it is quiet a hack that will catch stuff which would not normally be treated as command line options. Ideally this should latter be replaced by something robuster.
I do not understand you. What do you want me to do?
The problem is that some command line options must be analyzed before configuration files will be. If I'll just move command line parsing before config files one, it will break overriding precedence (sysmem<-user<-CLI).
Sure we still need an extra pass before parsing the config files.
Yes, currently used approach is not the best. Moreover it creates overhead: these options are parsed twice: in preparse and in normal parse routines, though second parsing is absolutely useless, but without it mplayer will complain about unknown options.
That doesn't matter much. There is rarely more than a few dozen parameters on the command line, and as it happen only at startup performance is not a problem.
Do you have some ideas how to fix this? I can see three different posibilities: 1) Leave it as is. 2) Parse all CLI options before configs, and either : 2a) save them in temporary storage and reassign after config files parsing; 2b) mark these options somehow and modify config files parser in order to not override marked options. 3) Modify parse subsystem to parse some subset of CLI options before config files and forbid their further modification whilst config files are parsed.
Variant 2b) sounds as the best for me, but even it will create too much overhead. So perhaps our best is to leave things as is?
Again overhead doesn't matter here. What I'm thinking about is a mix of 2b and 3. Basically flag some option as preparsed option, modify the command line preparsing to use the normal option parsing routines and the config system to ignore preparsed options afterward.
Well, lets talk about new patches.
The first patch moves some common code from mplayer.c and mencoder.c to mpcommon.[ch].
The second patch fixes a bug or, precisely speaking, missed feature of mencoder. Look at the man page: it says that mencoder read both system-wide and user-specific config files, but currently it reads only user-specific one. Anyone can easy check this by creating /etc/mplayer/mencoder.conf (or whatever is your's MPLAYER_CONFDIR).
The reason is easly seen in comparison between parse_cfgfiles() in mplayer.c and mencoder.c. The last one use only get_path() which checks only for home directory. The second patch fixes this, it may be applied with or without the first patch.
The third patch adds new functionality as described above. It requires both previous patches to be applied.
The fourth patch is cosmetics required after previous patches, real cosmetics: not newlines, but indentation shifts.
And finally the fifth patch is the manual page update.
That look alright to me, except perhaps the if(a) if(b) that would better be replaced by if(a && b). Anyway I'll look at fixing the preparsing correctly and we get back to this. Albeu
Hi, On Sunday 13 April 2008 16:57, Alban Bedel wrote: [...]
That look alright to me, except perhaps the if(a) if(b) that would better be replaced by if(a && b). Anyway I'll look at fixing the preparsing correctly and we get back to this.
Fixed.
participants (5)
-
Alban Bedel -
Andrew Savchenko -
Diego Biurrun -
Ivan Kalvachev -
The Wanderer