[FFmpeg-devel] [PATCH] VFW capture support

Ramiro Polla ramiro
Mon Mar 3 17:59:03 CET 2008


M?ns Rullg?rd wrote:
> Ramiro Polla <ramiro at lisha.ufsc.br> writes:
> 
>> Hello,
>>
>> M?ns Rullg?rd wrote:
>> [...]
>>>> - I tried to make the code look as much as possible as the code given
>>>> in MSDN, so that means I used "LRESULT CALLBACK" without even knowing
>>>> what it does or if it can be removed.
>>> I'm afraid that's unacceptable in FFmpeg.  Please find out what it
>>> does.
>> CALLBACK changed to __stdcall.
> 
> OK, I see what it does, and it should be left as CALLBACK, in case
> it's not always defined to __stdcall.

Changed back.

>>>> 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...
> 
> Fair point.
> 
>> You even say it yourself a few lines down from here:
>>> Are these values guaranteed to be correct for all Windows versions?
>> Anyways, I don't really care that much: changed.
>>
>>>> - Some defines are missing from MinGW include headers. I'll try to get
>>>> them integrated into MinGW. After that, we can properly check for
>>>> MinGW versions to define them, or just plain remove the definitions
>>>> here and require a newer version of MinGW. In the meantime, I think
>>>> there's no problem in defining them here.
>>> Are these values guaranteed to be correct for all Windows versions?
>> Probably. If it's not correct, we'll know as soon as someone submits a
>> bug report or a patch.
> 
> I guess it's acceptable, even if I don't like it.  Then again, I don't
> like the idea of Windows in the first place.

As Rich pointed out, there's too much legacy stuff in Windows. A VFW 
program compiled for Windows 98 also works on Windows XP. Not sure about 
WinCE though, never used it.

>>>> - If no "-r" parameter is given, way too many frames are read. I
>>>> thought FFmpeg was supposed to read the first few frames and decide
>>>> the rate based on that. The capture blocks, so the timestamps are
>>>> correct, and if some frames were captured and then the frame rate
>>>> calculated, it should find the correct frame rate, right?
>>> FFmpeg does not estimate frame rate based on elapsed real time,
>>> AFAIK.
>> The question is if it estimates frame rate based on pts. Real time or
>> not is irrelevant here.
> 
> Of course you're right.  I don't know what I was thinking.  That said,
> can't you set the frame rate in read_header()?

I could, but I thought FFmpeg could be smarter than VFW and find the 
real frame rate by calculating from the pts.

>>>> Please review.
>>> I am shocked an appalled by the ugly casting required by the win32
>>> API, but there's of course nothing we can do about that.
>> Please refrain from your predictable Windows bashing in the subsequent
>> reviews. Thank you.
> 
> I'll bash it all I want.  It'll still be getting far less than it
> deserves.

Whatever turns you on. Just don't mix real reviews and bashing. Please 
reply separately. Thank you.

>>>> 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.
> 
> This function isn't part of any API.

DWORD biCompression is part of the documentation.

>>> 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.
> 
> FFmpeg should use a consistent style.

I see no harm in having this style in a file that will most likely only 
be read by Windows coders, which are used to whitespaces immediately 
inside (). I find this file an exception. Of course, if ever I submit a 
(de)muxer or codec or whatever I'll use no whitespaces.

But, of course, I'll change it if someone authoritative asks me to.

>>>> {
>>>>     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...
> 
> I'm quite suspicious about the default case.  Would it not be better
> to list the known values explicitly, and report an error for unknown
> values.

Good idea.

>>>> }
>>>>
>>>> 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'
> 
> That's because DWORD is 'unsigned long' (at least on 32-bit), so the
> format specifier should be %lu.

Thanks for clarifying. Changed and added some more info.

[...]

>>>>     pkt->pts = GetTickCount( );
>>>>     memcpy( pkt->data, vdhdr->lpData, vdhdr->dwBytesUsed );
>>> Is it really not possible to capture without this memcpy()?
>> That's the best I found. The suggested way of capturing VFW frames
>> from MSDN involves copying to the clipboard and then to whatever you
>> need it for. Maybe someday someone will find some dirty hack that
>> avoids it.
> 
> Video... clipboard... words do not exist to express my disgust.

Then don't.

[...]

>> static int vfw_read_header( AVFormatContext *s, AVFormatParameters *ap )
>> {
>>     struct vfw_ctx *ctx = s->priv_data;
>>     AVCodecContext *codec;
>>     AVStream *st;
>>     int devnum, ret;
>>     BITMAPINFO *bih;
>>
>>     if( s->flags & AVFMT_FLAG_NONBLOCK ) {
>>         av_log( s, AV_LOG_ERROR, "Non blocking capture not yet implemented.\n" );
>>         return AVERROR_IO;
>>     }

Changed to "patch welcome".

>>
>>     ctx->hwnd = capCreateCaptureWindow( NULL, 0, 0, 0, 0, 0, HWND_MESSAGE, 0 );
>>     if( !ctx->hwnd ) {
>>         av_log( s, AV_LOG_ERROR, "Could not create capture window.\n" );
>>         return AVERROR_IO;
>>     }

I don't know how this can fail. Left as is.

>>     /* If atoi fails, devnum==0 and the default device is used */
>>     devnum = atoi( s->filename );
>>
>>     ret = SendMessage( ctx->hwnd, WM_CAP_DRIVER_CONNECT, devnum, 0 );
>>     if( !ret ) {
>>         av_log( s, AV_LOG_ERROR, "Could not connect to device.\n" );
>>         return AVERROR_IO;
>>     }
> 
> I'm not sure AVERROR_IO is the proper error code for those failures.

Other grab devices mostly return EIO.

>>     SendMessage( ctx->hwnd, WM_CAP_SET_OVERLAY, 0, 0 );
>>     SendMessage( ctx->hwnd, WM_CAP_SET_PREVIEW, 0, 0 );
>>
>>     ret = SendMessage( ctx->hwnd, WM_CAP_SET_CALLBACK_FRAME, 0,
>>                        (LPARAM) FrameCallbackProc );
>>     if( !ret )
>>         return AVERROR_IO;
>>
>>     ret = SendMessage( ctx->hwnd, WM_CAP_SET_USER_DATA, 0, (LPARAM) ctx );
>>     if( !ret )
>>         return AVERROR_IO;
>>
>>     st = av_new_stream( s, 0 );
>>     if ( !st )
>>         return AVERROR_NOMEM;
>>
>>     ret = SendMessage( ctx->hwnd, WM_CAP_GET_VIDEOFORMAT, 0, 0 );
>>     if( !ret )
>>         return AVERROR_IO;
>>     bih = av_malloc( ret );
>>     if ( !bih )
>>         return AVERROR_NOMEM;
>>     ret = SendMessage( ctx->hwnd, WM_CAP_GET_VIDEOFORMAT, ret, (LPARAM) bih );
>>     if( !ret )
>>         return AVERROR_IO;
> 
> Need to free bih in case of error here.

Freed.

Ramiro Polla
-------------- next part --------------
A non-text attachment was scrubbed...
Name: vfwcap.c
Type: text/x-csrc
Size: 6792 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080303/68f6b920/attachment.c>



More information about the ffmpeg-devel mailing list