Re: [Spca50x-devs] Adding gspca to the mainline kernel
- Date: Mon, 4 Jun 2007 16:04:31 -0400
- From: "Denver Gingerich" <denver@xxxxxxxxxx>
- Subject: Re: [Spca50x-devs] Adding gspca to the mainline kernel
On 6/4/07, Michel Xhaard <mxhaard@xxxxxxxx> wrote:
> > >1. Code accepted into the mainline kernel must meet certain quality
> > >standards.
> > What you mean when you say about "code quality standards" ?
I'm talking mainly about two things: coding style and more general
overall code structure. The gspca driver has poor coding style in a
number of ways:
- too many indentations make the code unreadable (see spca5xx_setMode,
- code that indents too many times should be moved into a separate function
- inconsistent indentations (see spca5xx_set_light_freq, which uses
spaces instead of tabs, like the rest of the code uses)
The gspca driver has poor overall code style as well:
- the USB IDs and names of webcams should be associated with their
specific driver code, not contained in a huge list in gspca_core.c; it
makes it much easier to figure out which camera uses which driver when
the USB IDs and names are in the same file as the code that handles
- the header file abuse I mentioned earlier (see below)
> > Denver, what you mean in this case ?
> > GSPCA work with multiple simultaneously-connected webcams pretty good.
> > Some problems can take place with USB bandwidth, but in this case GSPCA
> > driver has the certain advantages in comparison with other webcam drivers
> > owing to an opportunity dynamically allocated bandwidth for attached
> > devices...see attached screenshot. (it is my modular gspca branch but it
> > has on 100% identically working code with original gspca)
> look here:
My apologies. I should have looked into this a bit further. I had
tried to do this but it didn't work for me for some reason. I
incorrectly assumed it was the driver's fault, but clearly it is a
fault in the user.
> > >The third is the abuse of header files to add webcam-specific code.
> > >Header files should be used for function declarations, but NOT for
> > >function implementations, especially when these implementations are
> > >more than one line. The current .h files should be separated into .h
> > >and .c files so the .c files can be compiled into their own object
> > >files and then linked together to produce the gspca module.
> > It is not "code quality" problem. It is only "code style" issue.
> Sorry man, i like *.inc file, maybe because i work a lot with assembler :)
I think if you asked around (especially among kernel developers), you
would find that this is frowned upon for many reasons. I recall a
post from Linus on the LKML about this. I'll find it and post back.
> > >If we must continue to support v4l version 1, then we can use a method
> > >like the ov511 people chose: mainline what you can and keep the
> > >decompressor available out of the mainline for those who need it.
> Yes, i did not like this. My goal is to provided a working driver not to be
> happy with a really good code, but no one can used ! As i say here, gspca is
> "users centric" NOT "kernel centric" .
Are you saying we need to continue to support v4l1? Why? From what I
recall, most applications have moved to v4l2 already.
> > In my opinion the basic problems connected with low speed of transition
> > from v4l1 to v4l2 just are connected by that decompressor now should is in
> > userspace libraries, however it creates the certain problems with backward
> > compatibility.
> The fourcc method in v4l2 should never works for the ton of proprietary
> compressor, that is my opinion, after 5 years of people waiting this
> wonderfull userspace video library, i am afraid, the concept is wrong !
I'm not sure precisely what you mean, but I think you're referring to
some of the webcam data formats that are not quite standard JPEG so
they need a bit of processing before being passed to userspace. What
do the Windows driver do about this? Certainly they don't decompress,
process, then recompress the JPEG before delivering it to the
application. I think we need some more investigation into this.
> > >If you strongly feel the gspca driver should not be mainlined, please
> > >explain why.
> I received a lot of mail about that:
> What is wrong with GPL code outside the main line kernel ?
> Is there only one way togo ? If so what "free" mean ?
I'm not saying that maintaining a driver outside of the mainline
kernel is wrong. But I think there the benefits of moving the gspca
driver into the mainline kernel outweigh the costs, which is why we
should strongly consider doing that.
> > Denver, I believe that by the current moment nor gspca nor similar
> > subsystems in the mainline kernel have no harmonous, transparent general
> > architecture or model of such architecture. Entering gspca a code in a
> > kernel in the present kind in my opinion prematurely.
If there is no current subsystem, then we should make one. It seems
logical to have a webcam subsystem that you could register with to
utilize standard webcam functions.
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
Spca50x-devs mailing list