[Ffmpeg-devel] [PATCH] video4linux2 input

[Diego Biurrun]

On Sun, Feb 05, 2006 at 05:00:29PM +0100, Aurelien Jacobs wrote:
> On Sun, 5 Feb 2006 15:41:53 +0100
> Diego Biurrun <diego at biurrun.de> wrote:
> 
> > On Wed, Feb 01, 2006 at 03:15:02PM +0100, Michael Niedermayer wrote:
> > > 
> > > 3. simply unneeded comments which make reading more difficult:
> > > 
> > > h261.c:    // QCIF
> > > h261.c-    if (width == 176 && height == 144)
> > > h261.c-        return 0;
> > > h261.c:    // CIF
> > > h261.c-    else if (width == 352 && height == 288)
> > > h261.c-        return 1;
> > > h261.c:    // ERROR
> > > h261.c-    else
> > > h261.c-        return -1;
> > 
> > The last comment is redundant but I would disagree about the other two,
> > not everybody remembers CIF/QCIF resolutions offhand...  IMO this is a
> > good example of how two small comments make the code more readable.  If
> > you care about vertical space you can put them at the end of the line...
> 
> I disagree.
> It should probably read something like this:
> 
> typedef enum {
>   QCIF = 0,
>   CIF = 1,
> } res_desc_t;
> 
> if (width == 176 && height == 144)
>   return QCIF;
> else if (width == 352 && height == 288)
>   return CIF;
> else
>   return -1;

I think this is orthogonal to what I wrote, your solution is more
elegant, but without the enum I believe the comments help.

Diego