Web lists-archives.com

Re: [Spca50x-devs] Adding gspca to the mainline kernel




On 6/4/07, Serge A. Suchkov <Serge.A.S@xxxxxxxxx> wrote:
> В сообщении от 5 июня 2007 00:04 вы написали:
> > 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,
> > spca50x_move_data)
> >  - 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)
>
> I'm sorry ;) Just my IDE has such adjustments for my projects.
> In my IDE this code style looks pretty good.
> hint: Modern IDE allow to change indent style one command ;)

True, I could change my tab settings so that tabs represent 4 spaces
instead of 8.  But having poorly-indented code (or code that is
indented too much) goes against the kernel coding guidelines
(Documentation/CodingStyle in the kernel tree, available at
http://lxr.linux.no/source/Documentation/CodingStyle).  I think we
should stick to these since gspca is a kernel driver, even if it's not
in the kernel tree (yet).

> > 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;
>
> OK. See http://mgspca.e1.bmstu.ru
> Especially  mgspcav1-01.00.12-2 and  mgspcav2-02.00.20-1

That's an interesting project.  I haven't looked into it in detail,
but a cursory glance suggests it might be a good direction for the
main driver to go in.  Is there any reason why we're sticking with
gspca's fairly un-modular code instead of using Serge's more modular
code?

> > 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
> > them
> > - the header file abuse I mentioned earlier (see below)
>
> But all it has no attitude to "quality of a code" it concerns to style of the coding introduced by numerous authors of various parts gspca more likely

Are you suggesting we be accepting of new authors' different coding
styles in the code they submit to the gspca project?  Or are you
saying that because authors with different coding styles have already
committed to the gspca driver and so it's a combination of a variety
of styles that we should keep it that way?

In any case, I think it's a good idea to settle on one coding style
and stick with that for the entire driver.  Since it's a kernel
driver, I suggest we use the Linux kernel coding style guidelines,
which I mentioned above.

> > > > >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" .
> > >  http://www.theinquirer.net/default.aspx?article=39291
> >
> > Are you saying we need to continue to support v4l1?  Why?  From what I
> > recall, most applications have moved to v4l2 already.
>
> "Most" ??? Name please even 5 applications. AFAIK more ore less well with current versions v4l2 drivers for webcams work only luvcview by Michel and ekiga.

Off the top of my head I can't name any.  It's more an impression I
was under that suggested applications were moving in the v4l2
direction.

> In the others take place to be problems of this or that sort. The basic problem of compatibility with v4l1 consists in absence userspace libraries of proprietary
> decoders and here the layer of compatibility with v4l1 which will not help even is available in v4l2

For the webcams that provide information in modified JPEG format,
there should be some way to set the camera so it sends pure JPEG data.
 If not, then there is likely a way to do this that does not require
decoding the whole JPEG stream (I think this is what we're currently
doing for the PAC7311) because I can't see any logical reason for a
webcam to send its data in such a format if that couldn't be done.

For the webcams that pass information in pure JPEG format, no
proprietary decoder is required so this would not be a problem.

Are there other formats we have to deal with that I haven't mentioned?

> > > > 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 !
> +1
> >
> > 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.
>
> We work in this direction. Though speed of work probably not too impresses ;)
> In GSPCA in my opinion many architectural problems which were to be solved
> as much as possible effectively and elegantly and to be engaged cosmetic refactoring
>  not having solved these fundamental problems in my opinion waste of time and forces

Are you saying we should or should not be refactoring the code?
Certainly I agree that purely cosmetic code changes would be of
questionable usefulness.  But what I'm suggesting is refactoring for
the purpose of making the code more maintainable.

Denver
-------------------------------------------------------------------------
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.
http://sourceforge.net/powerbar/db2/
_______________________________________________
Spca50x-devs mailing list
Spca50x-devs@xxxxxxxxxxxxxxxxxxxxx
https://lists.sourceforge.net/lists/listinfo/spca50x-devs