[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