[FFmpeg-devel] [PATCH] Implement a cmdutils functions for listing available elements

[Stefano Sabatini]

On date Sunday 2009-11-15 00:10:22 +0100, Michael Niedermayer encoded:
> On Sat, Nov 14, 2009 at 05:35:28PM +0100, Stefano Sabatini wrote:
> > On date Saturday 2009-11-14 11:53:30 +0100, Michael Niedermayer encoded:
[...]
> Your trivial messup with mixing av_log() and prinf() and stderr and stdout
> shows this nicely, its just something that doesnt happen with incremental
> changes to existing code.

The use of stderr for the "informative" messages was deliberate, when
the user does -list things | grep foo then she wants only to filter
out the elements, not the legend.

[...] 
> [...]
> >  cmdutils.c |   12 ++++++++++++
> >  cmdutils.h |    9 +++++++++
> >  2 files changed, 21 insertions(+)
> > ab7f1da8f730348bc6429969574dc44602de2dd0  impl-opt-list.patch
> > Index: ffmpeg/cmdutils.c
> > ===================================================================
> > --- ffmpeg.orig/cmdutils.c	2009-11-14 17:02:35.000000000 +0100
> > +++ ffmpeg/cmdutils.c	2009-11-14 17:12:21.000000000 +0100
> > @@ -221,6 +221,18 @@
> >      return 0;
> >  }
> >  
> > +int opt_list(const char *opt, const char *arg)
> > +{
> > +    {
> > +        fprintf(stderr, "Unknown argument '%s' for option '%s'. Possible values are "
> > +                "\n",
> > +                arg, opt);
> > +        exit(1);
> > +    }
> 
> the {} are unneeded and you arent returning so returning int makes no sense

That was to simplify the second patch, also the opt_list function is
supposed to be a func2_arg OptionDef callback, so it requires that
signature.

> besides this function makes no sense

That was to simplify the second patch.
 
> > +
> > +    return 0;
> > +}
> > +
> >  int opt_loglevel(const char *opt, const char *arg)
> >  {
> >      const struct { const char *name; int level; } log_levels[] = {
> > Index: ffmpeg/cmdutils.h
> > ===================================================================
> > --- ffmpeg.orig/cmdutils.h	2009-11-14 17:09:02.000000000 +0100
> > +++ ffmpeg/cmdutils.h	2009-11-14 17:09:25.000000000 +0100
> > @@ -56,6 +56,15 @@
> >  int opt_loglevel(const char *opt, const char *arg);
> >  
> >  /**
> > + * Lists available elements. Exits from the application if the
> > + * value in arg is not supported.
> > + *
> > + * @param opt the name of the option associated to the function
> > + * @param arg the name of the elements to show
> > + */
> > +int opt_list(const char *opt, const char *arg);
> 
> This function does not do what you describe

Furthermore this trial-and-error process has proved frustrating and
time-consumming, and you still didn't make clear if you are going to
accept the overall change (having a -list option replacing the
-formats one, rationale in the first mail), if we don't agree on that
then there is no point for me into continuing to push patches.

Regards.
-- 
FFmpeg = Fast and Faithless Merciless Patchable Excellent Geek