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

[Stefano Sabatini]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fix-format-filter-doc.patch
Type: text/x-diff
Size: 1453 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20091028/6a32cc9d/attachment.patch>