[FFmpeg-devel] [PATCH] Add format and noformat filters

[Stefano Sabatini]

On date Wednesday 2009-10-28 22:41:57 +0100, Stefano Sabatini encoded:
> On date Wednesday 2009-10-28 18:23:48 +0100, Diego Biurrun encoded:
> > On Sun, Oct 25, 2009 at 04:50:49PM +0100, Stefano Sabatini wrote:
> > > 
> > > --- ffmpeg.orig/doc/libavfilter.texi	2009-10-25 15:50:05.000000000 +0100
> > > +++ ffmpeg/doc/libavfilter.texi	2009-10-25 16:44:52.000000000 +0100
> > > @@ -111,6 +111,40 @@
> > >  
> > > +Convert the input video to one of the specified pixel formats.
> > > +Libavfilter will try to pick one that is supported as the input to
> > > +the next filter.
> > 
> > Now the pixel format is the input to the next filter?  I thought it was
> > being fed video...
> 
> What about "*for* the input to the next filter"?
> 
> I was trying not to make the expression too long.
> 
> > > +The filter takes as argument a list of pixel format names, separated
> > > +by ``:'', for example ``yuv420p:monow:rgb24''.
> > 
> > Please delete the expression "takes as argument" from your active
> > vocabulary.  Even when correctly used, it sounds clumsy.  Just "accept"
> > is much better :)
> 
> MMh I don't say, honestly I find the verb "accept" too generic in this
> context, changed anyway.
> 
> > > + at example
> > > +./ffmpeg -i in.avi -vfilters "noformat=yuv420p, vflip" out.avi
> > > + at end example
> > > +
> > > +will make libavfilter use for the input of the filter next to the
> > > +noformat filter, a format different from ``yuv420p''.
> > 
> > Garbled sentence structure is making a (probably) simple thing
> > complicated.  What where you trying to say?
> 
> I modified that before to commit, it should be clearer than this.
> 
> > > --- ffmpeg.orig/libavfilter/Makefile	2009-10-25 15:49:37.000000000 +0100
> > > +++ ffmpeg/libavfilter/Makefile	2009-10-25 15:59:31.000000000 +0100
> > > @@ -12,6 +12,8 @@
> > >  
> > >  OBJS-$(CONFIG_CROP_FILTER)    += vf_crop.o
> > > +OBJS-$(CONFIG_FORMAT_FILTER)  += vf_format.o
> > > +OBJS-$(CONFIG_NOFORMAT_FILTER) += vf_format.o
> > 
> > Does the separation make sense?
> 
> Why not? After all someone may want a format but not a no format, note
> that currently this doesn't make difference, we could add some ifdef
> in the file to avoid to compile only the selected filters, even if the
> advantage would be minimal since they share most (95%+) of the code.
> 
> > > --- ffmpeg.orig/libavfilter/allfilters.c	2009-10-25 15:49:37.000000000 +0100
> > > +++ ffmpeg/libavfilter/allfilters.c	2009-10-25 15:52:24.000000000 +0100
> > > @@ -35,6 +35,8 @@
> > >  
> > >      REGISTER_FILTER (CROP,crop,vf);
> > > +    REGISTER_FILTER (FORMAT,format,vf);
> > > +    REGISTER_FILTER (NOFORMAT,noformat,vf);
> > >      REGISTER_FILTER (NULL,null,vf);
> > >      REGISTER_FILTER (VFLIP,vflip,vf);
> > 
> > Ugh, when did spaces after commas go out of style?
> 
> I keep the original, now I don't know what is better, to put a single
> space or try to vertical align the entries, but they will inevitably
> end-up misaligned when we'll add the first long-named filter.
> 
> > > --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> > > +++ ffmpeg/libavfilter/vf_format.c	2009-10-25 16:46:45.000000000 +0100
> > > @@ -0,0 +1,152 @@
> > > +
> > > +/**
> > > + * @file libavfilter/vf_format.c
> > > + * video format and noformat filters
> > 
> > You have this weird way of splitting up words that makes things
> > unnecessarily hard to understand.  I'll try to explain.
> > 
> > The thing we are talking about here is primarily a video filter, not a
> > video cropping filter.  The latter is a more specific description.
> 
> Uh? So are you suggesting:
> 
> "format and noformat video filters"?
> 
> Yes it sounds better, but I for sure couldn't say why.
> 
> > > +    for (cur = args; cur; cur = sep ? sep+1 : NULL) {
> > 
> > I'd put spaces around + here.
> 
> I prefer to keep them attached to help to visualize the underlying
> structure (X ? Y : Z), rather than to make the reader parse the single
> components, but feel free to change it if it bothers you.
> 
> Attached patch.
> 
> And Diego, I really appreciate your corrections and I'm trying hard at
> fixing them in this poor head, but feel free to directly commit these
> changes if it will take less of your time, I won't get offended ;-).
> 
> Regards.
> -- 
> FFmpeg = Faithful and Fierce Martial Programmable Elitist Geek

> Index: doc/libavfilter.texi
> ===================================================================
> --- doc/libavfilter.texi	(revision 20390)
> +++ doc/libavfilter.texi	(working copy)
> @@ -114,11 +114,11 @@
>  @section format
>  
>  Convert the input video to one of the specified pixel formats.
> -Libavfilter will try to pick one that is supported as the input to
> +Libavfilter will try to pick one that is supported for the input to
>  the next filter.
>  
> -The filter takes as argument a list of pixel format names, separated
> -by ``:'', for example ``yuv420p:monow:rgb24''.
> +The filter accepts a list of pixel format names, separated by ``:'',
> +for example ``yuv420p:monow:rgb24''.
>  
>  The following command:
>  
> @@ -130,11 +130,11 @@
>  
>  @section noformat
>  
> -Force libavfilter not to use any of the specified pixel formats as the
> +Force libavfilter not to use any of the specified pixel formats for the
>  input to the next filter.
>  
> -The filter takes as argument a list of pixel format names, separated
> -by ``:'', for example ``yuv420p:monow:rgb24''.
> +The filter accepts a list of pixel format names, separated by ``:'',
> +for example ``yuv420p:monow:rgb24''.
>  
>  The following command:
>  
> @@ -142,7 +142,7 @@
>  ./ffmpeg -i in.avi -vfilters "noformat=yuv420p, vflip" out.avi
>  @end example
>  
> -will make libavfilter use a format different from ``yuv420p'' as the
> +will make libavfilter use a format different from ``yuv420p'' for the
>  input to the vflip filter.
>  
>  @section null

Ping.
-- 
FFmpeg = Fantastic & Foolish Moronic Powered Educated Geisha