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

Michael Niedermayer michaelni
Thu Nov 19 17:38:14 CET 2009


On Wed, Nov 18, 2009 at 11:59:17PM +0100, Stefano Sabatini wrote:
> On date Wednesday 2009-11-18 23:04:38 +0100, Michael Niedermayer encoded:
> > On Wed, Nov 18, 2009 at 09:09:34PM +0100, Stefano Sabatini wrote:
> [...]
> > > 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
> > 
> [...] 
> > > 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)
> 
> For example having -list all would probably avoid the need for keeping
> show_formats (or at least it may significantly simplify it).

show_foo(){
    show_formats()
    show_codecs()
    show_bsfs()
}


> 
> But then I want to avoid to pollute the option space, having one
> extensible option is better than many ones.

i see no difference in the polution, -show codecs vs. -show_codecs
polutes similarly


>  
> > > 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
> 
> Applied.
> 
> > 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!
> 
> Tried this approach.

no, your patch does something quite different.
anyway, ive implemented it myself, this took less time than reviewing your
patches

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

If you really think that XML is the answer, then you definitly missunderstood
the question -- Attila Kinali
-------------- 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/20091119/3c2957b8/attachment.pgp>



More information about the ffmpeg-devel mailing list