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

[Uoti Urpala]

On Thu, 2009-04-23 at 17:00 +0200, Sebastien Raveau wrote:
> On Thu, Apr 23, 2009 at 3:10 PM, Reimar Döffinger
> <Reimar.Doeffinger at gmx.de> wrote:
> > 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.

He meant that it's better to only compile this code when the library is
present, rather than add hacks to compile it everywhere "in case the
user adds NI hardware afterwards" which very few users will do.

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

What does that have to do with C99? There's no more reason to use those
types in this context under C99 than there was under any old C version.

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

That does not in any way rely on the representation of a null pointer.
You should read the standard better yourself.


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

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

"Explicitizing"? Doesn't the code work without those? Or what are you
talking about?


Also you have a typedef named 'priv_t'. '*_t' is reserved namespace in
POSIX. Just use 'struct priv'.