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

[Sebastien Raveau]

On Thu, Apr 23, 2009 at 10:29 PM, Reimar Döffinger
<Reimar.Doeffinger at gmx.de> wrote:
>> There has to be a misunderstanding: my code does not link against the
>> imaq library
>
> I am saying I think you should.

And what would be the point of contributing a patch to the MPlayer
trunk if it requires a complex recompilation?
My intention was to help other people, and to that you are opposing
code aesthetics and code size. Come on...


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

Complex? It's simple, straightforward dynamic library loading.
Furthermore, of a Windows library under Windows:  how can you call it
complex next to the win32codecs code for example, that loads Windows
libraries under non-Windows OSs?


> Windows-dependent

As I explained in my first email, there are unfortunately currently
only Windows drivers for NI cards, so there is (at the moment perhaps)
no point in making this code portable.


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

Are we seriously debating binary size of a couple of function calls
over user benefit?
And you call that reasonable?!

It wouldn't even occur to me to complain about the half-megabyte that
all the video filters that I never ever use occupy on my disk!


> The question was more if it will probably work on e.g. 64 bit systems.
> Looking at the niimaq.h it seems ok.

I did verify that :-)


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

My point was that assuming that MPlayer will never be compiled with a
compiler that negates pointers just like any other type is unsafe...
But as my intent is to help MPlayer users, I don't really mind
complying with whatever code-aesthetics request you have.


>> >> +        if (0 != (ret = (*priv->imgInterfaceQueryNames)(0, buf))) {
> 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.

* scratches head *

Where did you see that ret>0 is for warnings? In the API documentation
( http://zone.ni.com/reference/en-XX/help/370161G-01/imaqfr/imginterfacequerynames/
) I read that "This function returns 0 on success. On failure, this
function returns an error code. "


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

Different coding consensus I guess... But alright, I'll replace it
with a comment :-)


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

Well in a structure it can only be a "dynamic" function pointer
indeed. What I meant was that (*foobar)(1,2) makes it clear that
foobar is a function pointer, not a function statically defined
somewhere else. Again, different software engineering education, and
as I said I'll change as you wish.


> Hm, ok. Scary API.

Indeed :-)

And since they only support Windows, I recommend against buying NI
cards... You have to believe me, all I want to do is help people stuck
with such hardware.


> Right, I didn't look carefully enough.

No worries :-)


> Undefined by the standard I mean.

I know, I was disappointed that I couldn't link you to opengroup.org ;-)


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

Ok I'll look into it.


Kind regards,

-- 
Sebastien Raveau