[FFmpeg-devel] [PATCH] VFW capture support

[Ramiro Polla]

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.

>> 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...

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.

[...]

>> - 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.

>> 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.

[...]

>> 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 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.

>> {
>>     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...

We'll know more as users submit the output from
ffmpeg -f vfwcap -i 0 -v 99

>> }
>>
>> 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'

> 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.

[more predictable Windows bashing]

[Useless (LRESULT) casts]

Removed.

>>     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.

[yet some more predictable Windows bashing]

[...]

>>     SendMessage( ctx->hwnd, WM_CAP_SET_OVERLAY, 0, 0 );
>>     SendMessage( ctx->hwnd, WM_CAP_SET_PREVIEW, 0, 0 );
> 
> Error checking?

It doesn't really matter if they work or not. Overlay and Preview should 
be off by default.

>>     ret = SendMessage( ctx->hwnd, WM_CAP_SET_CALLBACK_FRAME, 0,
>>                        (LPARAM) FrameCallbackProc );
>>     if( ret == FALSE )
>>         return AVERROR_IO;
>>
>>     ret = SendMessage( ctx->hwnd, WM_CAP_SET_USER_DATA, 0, (LPARAM) ctx );
>>     if( ret == FALSE )
>>         return AVERROR_IO;
>>
>>     st = av_new_stream( s, 0 );
>>     if ( !st )
>>         return AVERROR_NOMEM;
>>
>>     ret = SendMessage( ctx->hwnd, WM_CAP_GET_VIDEOFORMAT, (WPARAM) 0,
>>                       (LPARAM) 0 );
>
> Useless casts.

Removed.

>>     if( !ret )
>>         return AVERROR_NOMEM;
>>     bih = av_malloc( ret );
> 
> Missing check of av_malloc() return value.

Added.

>>     SendMessage( ctx->hwnd, WM_CAP_GET_VIDEOFORMAT, (WPARAM) ret,
>>                  (LPARAM) bih );
> 
> Error checking?

Added.

>>     dump_bih( s, bih );
>>
>>     codec = st->codec;
>>     codec->time_base = ap->time_base;
>>     codec->codec_type = CODEC_TYPE_VIDEO;
>>     codec->width = bih->bmiHeader.biWidth;
>>     codec->height = bih->bmiHeader.biHeight;
>>     codec->codec_id = CODEC_ID_RAWVIDEO;
>>     codec->pix_fmt = vfw_pixfmt( bih->bmiHeader.biCompression );
>>
>>     av_set_pts_info( st, 32, 1, 1000 );
>>
>>     av_free( bih );
>>
>>     return 0;
>> }
>>
>> static int vfw_read_packet( AVFormatContext *s, AVPacket *pkt )
>> {
>>     struct vfw_ctx *ctx = s->priv_data;
>>     int ret;
>>
>>     ctx->pkt = pkt;
>>     ctx->grabbed = 0;
>>
>>     ret = SendMessage( ctx->hwnd, WM_CAP_GRAB_FRAME, 0, 0 );
>>     if( ret == FALSE || !ctx->grabbed )
>>         return AVERROR_IO;
>>
>>     return pkt->size;
>> }
>>
>> static int vfw_read_close( AVFormatContext *s )
>> {
>>     struct vfw_ctx *ctx = s->priv_data;
>>
>>     SendMessage( ctx->hwnd, WM_CAP_SET_CALLBACK_FRAME, 0, 0 );
>>     SendMessage( ctx->hwnd, WM_CAP_DRIVER_DISCONNECT, 0, 0 );
> 
> Doesn't the window need to be destroyed?

Destroyed.

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