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

Michael Niedermayer michaelni
Wed Nov 18 23:04:38 CET 2009


On Wed, Nov 18, 2009 at 09:09:34PM +0100, Stefano Sabatini wrote:
> On date Wednesday 2009-11-18 00:43:40 +0100, Michael Niedermayer encoded:
> > On Sun, Nov 15, 2009 at 12:40:21PM +0100, Stefano Sabatini wrote:
> > > On date Sunday 2009-11-15 01:38:28 +0100, Michael Niedermayer encoded:
> > > > On Sun, Nov 15, 2009 at 12:51:56AM +0100, Stefano Sabatini wrote:
> > > > > 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:
> > > > [...]
> > > > > > > +
> > > > > > > +    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
> > > 
> > > To me it looks correct, 
> > 
> > > now it just implements the second part of the
> > > description " Exits from the application if the value in arg is not
> > > supported.".
> > 
> > thats like saying printf() is correctly implemented if it just returns
> 
> Would be acceptable to define the function only when defining the
> first handler? I avoided that in the first time because I was trying to
> minimize each patch.

The goal is not to minimize each patch, that way each patch would just add one
byte or remove one byte.
The goal is to sanely split patches so they can be reviewed with reasonable
effort.
One cannot review a function that does not work at all because most of its
code will be added in several later patches


> 
> > > > > 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.
> > > > 
> > > > iam ok with the idea of formats -> several lists
> > 
> > several lists like -list-codecs -list-...
> 
> What's the advantage of having many options rather than just one? Also

> having one option would allow tricks of the kind: -list help or -list
> formats+codecs -list all.

and for what would these tricks be good for? (except of course making the
thing much more complex)


> 
> Also if you're trying to keep the -formats option we can do like in
> the attached patch.
> 
> First patch makes the bitstreams listing in -formats show one entry per
> line, makes it more readable and consistent with the other listing.
> 
> Regards.
> -- 
> FFmpeg = Forgiving & Faithless Mystic Pitiless Eager Ghost

>  cmdutils.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 72784f900330aa0105d9d067b94d7a180be28e37  print-bsf-one-per-line.patch
> Index: ffmpeg/cmdutils.c
> ===================================================================
> --- ffmpeg.orig/cmdutils.c	2009-11-18 20:59:58.000000000 +0100
> +++ ffmpeg/cmdutils.c	2009-11-18 21:03:14.000000000 +0100
> @@ -532,7 +532,7 @@
>  
>      printf("Bitstream filters:\n");
>      while((bsf = av_bitstream_filter_next(bsf)))
> -        printf(" %s", bsf->name);
> +        printf("%s\n", bsf->name);
>      printf("\n");
>  
>      printf("Supported file protocols:\n");

ok

remainder rejected

look we have code like
function(){
    print bitstreams
    print codecs
}

your patch should look like:
-function(){
+foo(){
    print bitstreams
+}
+bar(){
    print codecs
}

anything else is rejected, please _try_ to change code in a way that is
reviewable, all these complete rewrites because you split a function in 2
are completely unacceptable and always rejected without review!

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

Let us carefully observe those good qualities wherein our enemies excel us
and endeavor to excel them, by avoiding what is faulty, and imitating what
is excellent in them. -- Plutarch
-------------- 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/20091118/110d2aca/attachment.pgp>



More information about the ffmpeg-devel mailing list