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

[Stefano Sabatini]

On date Sunday 2007-08-05 16:25:37 +0200, Michael Niedermayer encoded:
[...]
> On Mon, Jul 16, 2007 at 01:46:50PM +0200, Stefano Sabatini wrote:
> > Hi all,
> > 
> > this patch implements the various show-banner and show-license
> > functions in ffmpeg.c, ffserver.c and ffplay.c as calls to
> > corresponding cmdutils.c:show_ffmpeg_license() and
> > cmdutils.c:show_ffmpeg_banner() functions.
> > 
> > Reduce code duplication in the command line utils programs.
> > 
> > Note that it also changes the exit code of ffserver -L and ffmpeg -L
> > >from 1 to 0, which seems to me the correct behaviour.
> 
> agree but this should be in a seperate patch

OK.

[...]
> [...]
> > Index: ffplay.c
> > ===================================================================
> > --- ffplay.c	(revision 9682)
> > +++ ffplay.c	(working copy)
> > @@ -2478,8 +2478,8 @@
> >  
> >  void show_help(void)
> >  {
> > -    printf("ffplay version " FFMPEG_VERSION ", Copyright (c) 2003-2007 Fabrice Bellard, et al.\n"
> > -           "usage: ffplay [options] input_file\n"
> > +    show_ffmpeg_banner(stderr);
> > +    printf("usage: ffplay [options] input_file\n"
> >             "Simple media player\n");
> 
> this changes stdout to stderr in half the code

See below.
 
> [...]
> > @@ -3780,6 +3781,11 @@
> >  }
> >  #endif
> >  
> > +/* required for the inclusion of cmdutils.h */
> > +void parse_arg_file(const char *filename)
> > +{
> > +}
> > +
> 
> ugly hack

Agree, but I couldn't find a better way to manage it.
What about to make cmdutils.c:parse_options like this:

void parse_options(int argc, char **argv, const OptionDef *options,(void (*)(const char *))parse_arg_function)
 
when parse_arg_function specifyies which function has to be executed
on a bare arg, and manage the case when is parse_arg_function ==
NULL (as would be in the ffserver case)?

> >  static int parse_ffconfig(const char *filename)
> >  {
> >      FILE *f;
> > @@ -4441,14 +4447,9 @@
> >          return 0;
> >  }
> >  
> > -static void show_banner(void)
> > -{
> > -    printf("ffserver version " FFMPEG_VERSION ", Copyright (c) 2000-2006 Fabrice Bellard, et al.\n");
> > -}
> > -
> >  static void show_help(void)
> >  {
> > -    show_banner();
> > +    show_ffmpeg_banner(stderr);
> 
> stdout->stderr change
> 
> also ffmpeg != ffserver, the code does not display the same thing after this
[...]

In all the ff* applications the banner shows FFMPEG_VERSION, in the
case of ffmpeg it also displays the libav[cfu] versions, so it seems
safe to me to have just a common banner function that shows them all: or
eventually we can have an argument inside ffmpeg_show_banner that
takes the name of the application as argument, like this:

void show_ffmpeg_banner(FILE *stream, const char* program_name)
{
    fprintf(stream, "%s version " FFMPEG_VERSION ", Copyright (c) 2000-2007 Fabrice Bellard, et al.\n", program_name);
    fprintf(stream, "  configuration: " FFMPEG_CONFIGURATION "\n");
    fprintf(stream, "  libavutil version: " AV_STRINGIFY(LIBAVUTIL_VERSION) "\n");
    fprintf(stream, "  libavcodec version: " AV_STRINGIFY(LIBAVCODEC_VERSION) "\n");
    fprintf(stream, "  libavformat version: " AV_STRINGIFY(LIBAVFORMAT_VERSION) "\n");
    fprintf(stream, "  built on " __DATE__ " " __TIME__);
#ifdef __GNUC__
    fprintf(stream, ", gcc: " __VERSION__ "\n");
#else
    fprintf(stream, ", using a non-gcc compiler\n");
#endif
}

Where to print the banner?

Current situation:
application | banner/version output destination |
=================================================
ffplay      | stdout
ffserver    | stdout
ffmpeg      | stderr

I prefer to print it on stderr, but on stdout when it is explicitly
requested the version of the program (for example by a "-version"
option), anyway the behaviour should be consistent across the various
ff* tools.

For what regards the license, it also seems safe to me to have just
one common function (which changes according to --enable-gpl), since the
license of the various ff* depends on the compiled library license
(but maybe I'm wrong here).

BTW: why to bother at all about this stuff?
I'd like to add to FFmpeg a simple diagnostic tool (named ffprobe) and
I'd like to avoid to rewrite again the various banner and license functions.

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