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

[Diego Biurrun]

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

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

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

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

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

> --- /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.

> +    for (cur = args; cur; cur = sep ? sep+1 : NULL) {

I'd put spaces around + here.

Diego