[FFmpeg-devel] [PATCH] VFW capture support

Rich Felker dalias
Mon Mar 3 07:44:04 CET 2008


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




More information about the ffmpeg-devel mailing list