[MPlayer-dev-eng] [PATCH] Support for video capture from National Instruments cards

[Reimar Döffinger]

On Thu, Apr 23, 2009 at 01:49:32PM +0200, Sebastien Raveau wrote:
> diff -urN mplayer-export-2009-04-23/command.c mplayer/command.c
> --- mplayer-export-2009-04-23/command.c	2009-01-31 02:09:49.000000000 +0100
> +++ mplayer/command.c	2009-04-23 10:11:22.000000000 +0200
> @@ -2970,6 +2970,28 @@
>  	    }
>  	    break;
>  
> +	case MP_CMD_SCREENSHOT_PATH:
> +	    if (vo_config_count) {
> +		mp_msg(MSGT_CPLAYER, MSGL_INFO, "sending VFCTRL_SCREENSHOT_PATH!\n");
> +		if (CONTROL_OK !=
> +		    ((vf_instance_t *) sh_video->vfilter)->
> +		    control(sh_video->vfilter, VFCTRL_SCREENSHOT_PATH,
> +			    cmd->args[0].v.s))
> +		    mp_msg(MSGT_CPLAYER, MSGL_INFO, "failed (forgot -vf screenshot?)\n");
> +	    }
> +	    break;
> +
> +	case MP_CMD_SCREENSHOT_NAME:
> +	    if (vo_config_count) {
> +		mp_msg(MSGT_CPLAYER, MSGL_INFO, "sending VFCTRL_SCREENSHOT_NAME!\n");
> +		if (CONTROL_OK !=
> +		    ((vf_instance_t *) sh_video->vfilter)->
> +		    control(sh_video->vfilter, VFCTRL_SCREENSHOT_NAME,
> +			    cmd->args[0].v.s))
> +		    mp_msg(MSGT_CPLAYER, MSGL_INFO, "failed (forgot -vf screenshot?)\n");
> +	    }
> +	    break;
> +
>  	case MP_CMD_VF_CHANGE_RECTANGLE:
>              if (!sh_video)
>                  break;

Obviously unrelated.

> @@ -7483,6 +7486,23 @@
>  echores "$_tv_dshow"
>  
>  
> +echocheck "National Instruments Image Acquisition"
> +if test "$_tv_ni" = auto ; then
> +  _tv_ni=no
> +  if test "$_tv" = yes && win32 ; then
> +    _tv_ni=yes
> +  fi
> +fi
> +if test "$_tv_ni" = yes ; then
> +  _inputmodules="tv-ni $_inputmodules"
> +  def_tv_ni='#define CONFIG_TV_NI 1'
> +else
> +  _noinputmodules="tv-ni $_noinputmodules"
> +  def_tv_ni='#undef CONFIG_TV_NI'
> +fi
> +echores "$_tv_ni"
> +

I disagree. You should check if you can link against imaq library.
In addition to getting rid of quite a bit of code, not including code
by default that 99.99% of users will never use it should also mean
that you will not need any Windows-specific code.

> diff -urN mplayer-export-2009-04-23/stream/tvi_ni.c mplayer/stream/tvi_ni.c
> --- mplayer-export-2009-04-23/stream/tvi_ni.c	1970-01-01 01:00:00.000000000 +0100
> +++ mplayer/stream/tvi_ni.c	2009-04-23 10:31:52.000000000 +0200
> @@ -0,0 +1,235 @@
> +/*
> + * National Instruments video capture

Help people find the documentation for it, e.g. add
(NI-IMAQ, see http://www.ni.com/pdf/manuals/370160a.pdf)

> +#include <windows.h>

Should not be needed if you link directly against the dll

> +#include <stdio.h>

Why?

> +    int32_t __stdcall (*imgClose)(uint32_t void_id, uint32_t freeResources);

Does NI provide an official header? Can it be used? If not, can they be
convinced to make it usable? If not, do these types match exactly theirs?
(I somewhat doubt it since Visual C which they support does not have
uint32_t etc. types, btw. you should include at least stdint.h for
these).

> +    uint32_t width;
> +    uint32_t height;
> +    uint32_t bytes_per_pixel;

Don't use such specific types as uint32_t unless you really _need_ them,
use int or unsigned otherwise.

> +    if (NULL == (tvh = new_handle()))
> +        return NULL;

I really think
> tvh = new_handle();
> if (!tvh)

is _a lot_ more readable.

> +    priv = (priv_t *)tvh->priv;

The cast should not be necessary.

> +        if (0 != (ret = (*priv->imgInterfaceQueryNames)(0, buf))) {

The "0 !=" is completely pointless, and why the * and () instead of just
priv->imgInterfaceQueryNames(0, buf)
?
Also, for imgInterfaceQueryNames and imgShowError I could not find any
documentation and those are passed a fixed-size buffer but not the size
of a buffer, this gives me an _extremely_ uncomfortable feeling...

> +        priv->device = strdup(buf);

It seems this is never freed...

> +fail4:
> +    (*priv->imgClose)(priv->interface_id, 1);
> +fail3:
> +    free(priv->device);
> +fail2:
> +    (*priv->imgShowError)(ret, buf);
> +    mp_msg(MSGT_TV, MSGL_ERR, "tvi_ni: %s: %s\n", funcname, buf);

This might crash or worse since funcname can be NULL and NULL + %s = undefined
behaviour.

> +    FreeLibrary(priv->dll);
> +fail1:
> +    free_handle(tvh);
> +    return NULL;

Please use only a single fail: goto target, it should be possible.

> +static double grab_video_frame(priv_t *priv, char *buffer, int len)
> +{
> +    return !(*priv->imgGrab)(priv->session_id, &buffer, 0);

returning 0/1 as double (particularly since that double is supposed to
be a time stamp) does not make much sense.