[FFmpeg-cvslog] r25118 - in trunk: Changelog configure doc/filters.texi libavfilter/Makefile libavfilter/allfilters.c libavfilter/avfilter.h libavfilter/vf_libopencv.c

[Diego Biurrun]

On Thu, Sep 16, 2010 at 10:49:13AM +0200, Stefano Sabatini wrote:
> On date Thursday 2010-09-16 10:04:05 +0200, Diego Biurrun wrote:
> > On Tue, Sep 14, 2010 at 03:21:14PM +0200, stefano wrote:
> > > 
> > > Log:
> > > Implement libopencv smooth filter.
> > > 
> > > --- trunk/Changelog	Tue Sep 14 02:17:58 2010	(r25117)
> > > +++ trunk/Changelog	Tue Sep 14 15:21:13 2010	(r25118)
> > > @@ -35,6 +35,7 @@ version <next>:
> > >  - MMS-HTTP support
> > >  - G.722 ADPCM audio decoder
> > >  - R10k video decoder
> > > +- ocv_smooth filter
> > 
> > The readers of the Changelog should not have to guess what ocv_
> > stands for.  I would not have guessed correctly without reading
> > the commit.
> >  
> > > --- trunk/doc/filters.texi	Tue Sep 14 02:17:58 2010	(r25117)
> > > +++ trunk/doc/filters.texi	Tue Sep 14 15:21:13 2010	(r25118)
> > > @@ -112,6 +112,33 @@ input to the vflip filter.
> > >  
> > > +It accepts the following parameters:
> > 
> > s/It/The filter/
> > 
> > > + at var{type}:@var{param1}:@var{param2}:@var{param3}:@var{param4}.
> > > +
> > > + at var{type} is the type of smooth filter to apply, and can be one of
> > > +the following value: "blur", "blur_no_scale", "median", "gaussian",
> > 
> > valueS
> > 
> > > +These parameters corresponds to the parameters assigned to the
> > 
> > correspond
> > 
> > > +libopencv function @code{cvSmooth}. Refer the official libopencv
> > 
> > Refer to
> > 
> > Please doublecheck your plural/singular uses before committing.
> > 
> > > --- /dev/null	00:00:00 1970	(empty, because file is newly added)
> > > +++ trunk/libavfilter/vf_libopencv.c	Tue Sep 14 15:21:13 2010	(r25118)
> > > @@ -0,0 +1,156 @@
> > > +/*
> > > + * copyright Stefano Sabatini 2010
> > 
> > This looks slightly weird, compare the other copyright lines in the
> > code.
> > 
> > > +#include "opencv/cv.h"
> > > +#include "opencv/cxtypes.h"
> > 
> > System headers should use <> instead of "".
> > 
> > > +static void fill_iplimage_from_picref(IplImage *img, const AVFilterBufferRef *picref, enum PixelFormat pixfmt)
> > 
> > Here (and in lots of other places) you could easily break this long
> > line for extra readability.

Extra good karma for snipping quoted text - these are almost 60 lines
of noise...

> > > +#if CONFIG_OCV_SMOOTH_FILTER
> > 
> > This is pointless and thus harmful: The file is only ever compiled
> > if that condition is true...
> 
> Yes but my idea was to put here more than one ocv filters, so it's
> useful for enabling just one of these.

Then put the #ifdefs in place once they become necessary.  Experience
shows that all too often plans like these are never realized.  Keep the
code clean *now*, you can always make it ugly when you have to, but why
rush it?

> > > +typedef struct {
> > > +    int type;
> > > +    int    param1, param2;
> > > +    double param3, param4;
> > > +} SmoothContext;
> > 
> > What's with the typedef mania that everybody has?  Just write 'struct'...
> 
> Save some typing, you know FFmpeg devs are *lazy*...

So?  Then why not use single character var names?  Or use the preprocessor
to shorten common funcs to two chars?  Or just typedef the struct to SC?

The 'struct' keyword does IMO provide some useful information to the
programmer.

Compare what gregkh said "typedefs are evil":

http://www.linuxjournal.com/article/5780?page=0,2

Diego