[FFmpeg-devel] [PATCH] show-banner and show-license as cmdutils.c functions

Stefano Sabatini stefano.sabatini-lala
Thu Aug 9 01:10:18 CEST 2007


On date Wednesday 2007-08-08 19:53:18 +0200, Michael Niedermayer encoded:
> Hi
> 
> On Wed, Aug 08, 2007 at 10:57:29AM +0200, Stefano Sabatini wrote:
> > On date Tuesday 2007-08-07 22:34:29 +0200, Michael Niedermayer encoded:
> > [...]
> > > > Second attempt, regression test passed.
> > > > 
> > > > Suggested log: implement show_version and show_license as cmdutils.c functions.
> > [...]
> > > > @@ -3575,9 +3576,9 @@
> > > >  
> > > >  const OptionDef options[] = {
> > > >      /* main options */
> > > > -    { "L", 0, {(void*)show_license}, "show license" },
> > > > +    { "L", 0, {(void*)opt_show_license}, "show license" },
> > > >      { "h", 0, {(void*)show_help}, "show help" },
> > > > -    { "version", 0, {(void*)show_version}, "show version" },
> > > > +    { "version", 0, {(void*)opt_show_version}, "show version" },
> > > >      { "formats", 0, {(void*)show_formats}, "show available formats, codecs, protocols, ..." },
> > > 
> > > these changes do NOT belong in this patch, they are not needed for
> > > moving the code to cmdutils
> > 
> > No they belong to this patch, since I defined the symbols show_version
> > and show_license in cmdutils.o, so I can't use them for handling the
> > -license and -version options. 
> 
> A requires B
> B does not belong in this patch
> from that it follows that
> A does not belong in this patch
> 
> A= "using show_license, ... as global identifers in cmdutils.c even though
> they are already used for a different purpose in ffmpeg.c"

OK, got the point... in my case this means I have at least two
prerequisites patches before this can goes...
  
> > Anyway it's also better to prefix
> > opt_show_version and opt_show_license with "opt", to highlight that
> > they're option handler functions (and for consistency with the
> > prelevant use in ffmpeg.c).
> 
> renaming option parsing functions so that they all have a opt_ prefix is
> something seperate, it may or may not be a good idea. but it does not
> belong in this patch. furthermore this patch just renames a random subset
> of these functions any such change should rename all

I checked that, opt_ prefix for option handler functions in ffmpeg.c
and ffplay.c is the prelevant rule with these exceptions:

ffmpeg.c: show_help, show_version, show_license, show_formats
ffplay.c: show_help

so extending the opt_ prefixing rule doesn't seem such a bad idea.

show_help is an exception in another way too, since it's not only used
as an option handler function, but also to print help in case of
errors.  show_help has also another problem: it always returns 1, even
in the case the user just requested to print the help with ffmpeg -h.

The best solution seems to me to have two distinct functions:
show_help which simply shows help, without exiting and opt_show_help
which calls show_help and exits 0.

But anyway, it's another patch.

Cheers.
-- 
Stefano Sabatini
Linux user number 337176 (see http://counter.li.org)




More information about the ffmpeg-devel mailing list