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

[Sebastien Raveau]

On Thu, Apr 23, 2009 at 3:10 PM, Reimar Döffinger
<Reimar.Doeffinger at gmx.de> wrote:
>> +     case MP_CMD_SCREENSHOT_PATH:
>> +     case MP_CMD_SCREENSHOT_NAME:
>
> Obviously unrelated.

So... I should send that in a second patch? Didn't want to double ML
traffic for so little.


> I disagree. You should check if you can link against imaq library.

There has to be a misunderstanding: my code does not link against the
imaq library, it uses it if available at runtime. That way people with
NI cards don't have to compile MPlayer to get NI support (or worse:
compile it again just because they installed/updated their NI hardware
afterwards).


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

I'm sorry, I don't understand you there.


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

Good idea indeed :-)


>> +#include <windows.h>
> Should not be needed if you link directly against the dll

Please see why I went through the hassle of not linking directly
against the dll above.


>> +#include <stdio.h>
> Why?

Sorry, a leftover :-)


>> +    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?

NI provides a (non-standalone) file called niimaq.h in their SDK,
which is not necessarily installed along with the hardware. Again, I
wanted to spare users the troubles of installing SDKs, Cygwin and
extra packages just to be able to use their NI card with MPlayer.

Google shows a (rogue I guess) copy of this file at
http://odin.fi-b.unam.mx/software/lii/PC/ArmyDBManager/Source/niimaq.h


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

Not exactly: they mostly use custom typedef's. I spent time searching
for the underlying types and calling conventions and tested them
thoroughly. One could argue that maybe some day they will change their
underlying types, but I doubt they will change their mind every other
day about using uint32_t for types that are mostly booleans and IDs
anyway.


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

Sorry, a bad C99 habit ;-)


>> +    if (NULL == (tvh = new_handle()))
>> +        return NULL;
>
> I really think
>> tvh = new_handle();
>> if (!tvh)
>
> is _a lot_ more readable.

And I would say relying on NULL being all bits zero is bad (see
http://c-faq.com/null/machnon0.html ) but as you wish...


>> +    priv = (priv_t *)tvh->priv;
> The cast should not be necessary.

You're right. Then it should be removed from tvi_init_bsdbt848 also.


>> +        if (0 != (ret = (*priv->imgInterfaceQueryNames)(0, buf))) {
> The "0 !=" is completely pointless,

Obviously we have a different point of view on readability :-)
And obviously compilers ignore such syntactic sugar, so I wouldn't be
so fast to call such things pointless.
I explicit "0 !=" on functions whose return values are contrary to
what we would expect. For example, if (!InitSomething()) would make
more sense if it meant checking for initialization failure, not the
opposite.


> and why the * and () instead of just
> priv->imgInterfaceQueryNames(0, buf)
> ?

Also a way of expliciting a function pointer, i.e. it has to be
initialized manually.


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

http://zone.ni.com/reference/en-XX/help/370161G-01/imaqfr/imginterfacequerynames/
"name: pointer to a character array to receive the interface name. The
array must contain at least 256 elements."

http://zone.ni.com/reference/en-XX/help/370161G-01/imaqfr/imgshowerror/
"text: pointer to an array of at least 256 bytes."


>> +        priv->device = strdup(buf);
> It seems this is never freed...

Forgot to do that indeed, sorry :-)
Come to think about it, it's not needed anymore as I call
imgInterfaceOpen in the same function.


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

Can it? All code paths leading to it do assign funcname...

> and NULL + %s = undefined behaviour.

Neither in the GNU C Library nor in the Microsoft one, both output "(null)".
But if you really really want to waste some memory I can always assign
funcname a default, pointless message...


>> +    FreeLibrary(priv->dll);
>> +fail1:
>> +    free_handle(tvh);
>> +    return NULL;
>
> Please use only a single fail: goto target, it should be possible.

I factorized the failure code, using several goto targets, for better
readability and ease of maintenance. The other way would imply having
duplicate code all over the place or extra variables just to know what
to do in the failure code...


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

Ah ok... I was misguided by the tvi_dummy.c code, sorry.


Best regards,

-- 
Sebastien Raveau