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

[Reimar Döffinger]

On Thu, Apr 23, 2009 at 05:00:04PM +0200, Sebastien Raveau wrote:
> 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.

Discussing two unrelated things in a single thread only leads to chaos.

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

I am saying I think you should.

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

And that way the code is a lot more complex, Windows-dependent and the vast
majority of users will have code in their MPlayer binary that they never use.
Requiring a special MPlayer build admittedly is a bit of an annoyance but
given the small number of users it seems like the more reasonable compromise.

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

Changing the types means breaking ABI and would break all exisiting
applications, so nothing we have to worry about.
The question was more if it will probably work on e.g. 64 bit systems.
Looking at the niimaq.h it seems ok.

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

Uh, did you actually read what you linked to?
The "NULL" pointer is always 0, that has nothing at all to
do with what bit-representation it has.

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

Well, first I think your check is wrong anyway, ret > 0 seems to be used for
"warnings", so the check should be for < 0 anyway.
Secondly there is only a readability advantage if everyone knows and is aware
that "0 !=" is used to implicitly document something - otherwise it is just
additional code you have to read and understand.
Since the whole API uses this, instead of an implicit documentation via using
"0 !=" it would make more sense to document via an ordinary comment if necessary.

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

I don't see the difference between
> (*priv->imgInterfaceQueryNames)(0, buf)
and
> priv->imgInterfaceQueryNames(0, buf)
except that the first is really ugly.


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

Hm, ok. Scary API.

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

Right, I didn't look carefully enough.

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

Undefined by the standard I mean.

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

I don't think you need extra variables. Only the interface_id might need
a special case if you do not have a known invalid value you can initialize
it to. The problem with your approach is that (besides being ugly) it can
break easily when code is rearranged and the order changed.