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

[Michael Niedermayer]

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:
> [...]
> > patch rejected due to 100% code duplication, we have exitsing code in
> > ffmpeg.c that does that, improve it and or move it around in seperate
> > patches if you like but this is far too much work to verify to be not
> > fully broken. Just your obvious messup of mixing printf with av_log
> > shows how broken the code was. I spotted that one but how many did
> > i miss?
> 
> Check the attached patch and tell if you prefer this approach, or
> suggest how to do otherwise.
> 
> > Also, just to make it clear, as maybe theres a misunderstanding between
> > us, i reject ALL unjustfied rewrites. Thats not limited to AVOptions and
> > this patch, i will reject every future one as well.
> > Iam sure we do have code that is crap enough to justify a full rewrite
> > but neiter AVOptions nor show_formats() in ffmpeg.c are ugly written.
> > The way you make changes even breaks binary search of svn, if your code
> > is buggy such search would point to the removial of the old code not
> > the addition of the (in that case) broken code.
> 
> The conversion process is not always easy or even possible and
> developers has time / motivation constraints, sometimes you're in the
> position that you have either to accept some compromise and entirely
> replace old code, or to keep forever with the old code.

My time is limited as well, and reviewing rewrites is much more difficult
than changes to existing code.
And if you have not enough time to change existing code then you
absolutely have not enough to write the whole from scratch better than how it
was. The testing alone will take time
Its well possible that if you start with very poor written and buggy code that
you can rewrite it quickly in a way that its better but the code you
rewrite is not in that category i think.

if you just move code out and change some than this is
well verifyable. But if you just throw it away and redo it from scratch
noone knows what all might be buggy, there is a lot more that needs testing
in case of a rewrite.
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.

Its like "spot the difference in 2 pictures" vs. watching the artist
paint and erase each single feature one by one.
One is much easier the other is just asking for something to be missed


[...]
>  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
besides this function makes no sense


> +
> +    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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No great genius has ever existed without some touch of madness. -- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20091115/920c6da5/attachment.pgp>