[FFmpeg-devel] [PATCH] VFW capture support

[Rich Felker]

On Mon, Mar 03, 2008 at 03:19:55AM -0300, Ramiro Polla wrote:
> >>I have also used "var == FALSE" and "var == NULL" checks. I prefer
> >>that instead of "!var" just to be closer to MSDN.
> >
> >Using "var == FALSE", and "var == TRUE" more so, not only not our
> >style, it's plain stupid.  Being closer to msdn is hardly a desirable
> >goal.
> 
> Hey, we never know if Microsoft someday decides that FALSE == 18 instead 
> of the expected 0...

This is akin to insisting on using PI instead of 3.1415926535897932
because the value of ? might change...

Seriously, Windows is so encrusted with legacy crap that they can't
even fix their long to be the right size even when moving to a new
arch (LP64). So of course they couldn't change FALSE to something
other than 0 even if they wanted. And moreover the value of false is
defined by the C language not by Windows..

> >>struct vfw_ctx {
> >>    HWND hwnd;
> >>    int grabbed;
> >>    AVPacket *pkt;
> >>};
> >>
> >>static int vfw_pixfmt( DWORD biCompression )
> >
> >Do we really have to use those dreadful windows typedefs and naming
> >conventions?
> 
> I find it best when writing an interface to an API that has 
> documentation, the same way you follow variable names from specs.

It's hideous style and IMO not acceptable. Not to mention the name
DWORD is _incorrect_!!!

> 
> >It is preferred to have no whitespace immediately inside () in FFmpeg.
> 
> This is my preferred style, and I'll maintain this file. You won't need 
> to look at it after it gets into SVN.

This is not the way to make people feel positive about accepting your
code...

> >>{
> >>    switch( biCompression ) {
> >>    case MKTAG( 'Y', 'U', 'Y', '2' ):
> >>        return PIX_FMT_YUYV422;
> >>    default:
> >>        return PIX_FMT_BGR24;
> >>    }
> >
> >Is this really complete and correct?
> 
> Complete? Most likely not.
> Correct? For me it is...

Surely not correct either.

> >>static void dump_bih( AVFormatContext *s, BITMAPINFO *bih )
> >>{
> >>    av_log( s, AV_LOG_DEBUG, " biSize:\t%d\n", (int) 
> >>    bih->bmiHeader.biSize );
> >>    av_log( s, AV_LOG_DEBUG, " biWidth:\t%d\n", (int) 
> >>    bih->bmiHeader.biWidth );
> >>    av_log( s, AV_LOG_DEBUG, " biHeight:\t%d\n", (int) 
> >>    bih->bmiHeader.biHeight );
> >>    av_log( s, AV_LOG_DEBUG, " biPlanes:\t%d\n", (int) 
> >>    bih->bmiHeader.biPlanes );
> >>    av_log( s, AV_LOG_DEBUG, " biBitCount:\t%d\n", (int) 
> >>    bih->bmiHeader.biBitCount );
> >>    av_log( s, AV_LOG_DEBUG, " biCompression:\t%.4s\n", (char*) 
> >>    &bih->bmiHeader.biCompression );
> >>    av_log( s, AV_LOG_DEBUG, " biSizeImage:\t%d\n", (int) 
> >>    bih->bmiHeader.biSizeImage );
> >>    av_log( s, AV_LOG_DEBUG, " biXPelsPerMeter:\t%d\n", (int) 
> >>    bih->bmiHeader.biXPelsPerMeter );
> >>    av_log( s, AV_LOG_DEBUG, " biYPelsPerMeter:\t%d\n", (int) 
> >>    bih->bmiHeader.biYPelsPerMeter );
> >>    av_log( s, AV_LOG_DEBUG, " biClrUsed:\t%d\n", (int) 
> >>    bih->bmiHeader.biClrUsed );
> >>    av_log( s, AV_LOG_DEBUG, " biClrImportant:\t%d\n", (int) 
> >>    bih->bmiHeader.biClrImportant );
> >
> >All those (int) casts are unnecessary.
> 
> warning: format '%d' expects type 'int', but argument 4 has type 'DWORD'

Indeed this should be fixed but I'm not sure the casts are the clean
way to do it..

> >If any of the fields are
> >unsigned, the corresponding format specifiers should be %u.
> 
> http://msdn2.microsoft.com/en-us/library/ms532290(VS.85).aspx
> They're defined as DWORDs and LONGs. Some allow negative numbers, the 
> rest I don't know but have never seen with signed numbers.
> It's just debugging information. We'll learn more with bug reports.

DWORD is an unsigned type on Windows, isn't it? Who knows.. with such
horrible naming conventions.

Rich