Re: [linux-usb-devel] Re: About the vicam driver(s)

2002-10-21 Thread Joe Burks
At 10:06 PM 10/17/2002 -0700, you wrote:

The use of the dev_set_drvdata family of calls. I'm not sure why, but those
seem to keep the driver from being inserted, removed, and reinserted.
Replacing those with the use of private_data field in the usb_interface
struct seems to fix that (I removed the usb_put_device call along with
these, so that my be to blame instead).


I'm not sure why those are a problem either.  The dev_set_drvdata calls 
were used because the cpia driver uses them.  I've used the cpia driver as 
my example of a webcam driver that works.  I've probably misused something 
in the process.

The control message allocation and async urbs, as previously mentioned.

The is_initialized and needsDummyRead variables are unneeded as the things
protected by those variables can/should just be done when the camera is
initialized.


You're actually looking at a bug there.  The proc interface for the shutter 
(which you don't seem to like) should be setting needsDummyRead.  As the 
code says, the camera doesn't take changes in shutter speed until the frame 
after the one you capture.  Any time shutter speed changes, you need to do 
a dummy read to get a properly exposed image.

The disconnect handling is wrong for the same reasons mine was. There is no
synchronization with the open/release routines, no making sure that no
communication with the camera is going on, and the cam structure is freed
without checking if users still have the camera open.

framebuf is allocated in both open and mmap calls. Not necessarily wrong...
however, it is possible that if the pointer was overwritten that the memory
would just be reallocated/leaked instead of oopsing, which would indicate a
problem.


In both places, framebuf is only allocated if it is not already 
allocated.  I think in both places the allocation is protected by a 
semaphore.  Maybe it is wrong, could you give you more info?

There are some other, more subjective things that I take issue with such as
the read function being unnecesarily complicated, the proc interface not
being necessary, raw_image being allocated in open rather than probe, etc.


The read function is written as such because the early usblib-based camera 
control software was heavily used and I wanted to make it possible for the 
already existing scripts to work as they had before.  Thus the complexity 
of my read function is there so that users could do:

cp /dev/video/video0 mypic.raw

and it would work.  The raw file could then be sent through ImageMagick to 
convert it into something usable.

I don't mind the proc interface going provided that there is a way to 
change the shutter speed independently for every camera connected.  I have 
a lot of experience with users of this camera.  Some have the camera up a 
tree (looking at scenes that vary in lighting throughout the day) and some 
have many cameras connected to a security monitoring system (with different 
cameras being under different lighting conditions).  The gain control does 
not have enough range to cope with this, and gain degrades image quality 
anyway.

Some of these things are easy, some are a bit more complicated. My biggest
issue is that the driver that I submitted is a superset of the one that was
merged. I did a lot of work to write/test/debug/etc. that code, and my doing
that again would be a step backwards, in my mind.


Here's why I disagree:

Shutter speed.  I'm not sure why, your code to set up the frame request 
packet looks a lot like my code to do the same, but you only included the 
slow shutter speed calculation.  You also have a shutter speed bug, the 
camera can't actually capture anything slower than 1/4 sec. (it will take 
the value, but not give you the exposure).

Superior Color decode.  Your color decoder looks like a lightly touched 
version of Andy Armstrong's original color decoder.  In my driver I decided 
not to use that one, opting instead for Monroe Williams' better color decoder.

Removed V4L bug.  Your V4L code looks like a modified copy of my 3comhc.c 
driver, right down to a bug in VIDIOCMCAPTURE.

In any case, I will assume responsibility for merging and debugging if you 
are not interested.  I was of the opinion that if my driver were modified 
to include proper disconnect protection and asynchronous captures the 
driver would be feature complete until we reverse engineer the formats 
other than 512x242.

-Joe



---
This sf.net emial is sponsored by: Influence the future of 
Java(TM) technology. Join the Java Community Process(SM) (JCP(SM)) 
program now. http://ad.doubleclick.net/clk;4699841;7576298;k?
http://www.sun.com/javavote
___
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel


Re: [linux-usb-devel] Re: About the vicam driver(s)

2002-10-21 Thread Joe Burks
At 11:34 AM 10/21/2002 -0700, John Tyner wrote:

 You're actually looking at a bug there.  The proc interface for the shutter
 (which you don't seem to like) should be setting needsDummyRead.  As the

Well, that makes more sense. I'm not against what the proc interface is
trying to achieve so much as the way it is achieved. It takes quite a bit
of code (setup, parsing, etc) to create an interface that would probably
be better provided through an ioctl if at all possible.


Mine wasn't the first V4L driver to support this type of 
functionality.  Using the current mechanism, a user needs nothing more than 
echo to change the shutter speed.  Using a custom IOCTL, the user would 
require a stand alone vicam-specific executable to change the shutter 
speed.  I didn't think that was a reasonable expectation.

 In both places, framebuf is only allocated if it is not already
 allocated.  I think in both places the allocation is protected by a
 semaphore.  Maybe it is wrong, could you give you more info?

It's not wrong, per se. IMO, it's bad style. The memory should be
allocated in one place when the camera is initialized (or perhaps even in
the mmap call since it may not be needed until there (mine needs it before
then)). The fact that you allocated it twice could lead to a memory leak if
you were to ever accidentially overwrite your pointer.


The memory should definitely not be allocated when the camera is 
initialized.  I think there are a few who would throw a fit if your driver 
was allocating 320*240*3*2 bytes of data (nearly half a megabyte) any time 
the camera was plugged in but only got used if a V4L application started 
up.  The only appropriate single place for this I would think in open 
(assuming that mmap cannot possibly be called before open).   Thus 
scrubbing the allocation from mmap would clear up this issue.

Two things, the V4L API states that each successive call to read should
return the next image from the device. There isn't really a need to
support multiple reads over the same frame (though it is nice to the user
:) ). Second, I understand what you're trying to achieve and still think
that read is more complicated than it needs to be. I can submit a patch
showing what I think it should be.


The V4L API also says that applications should provide a suitably sized 
read buffer.  An application which does this, will receive the expected V4L 
behavior from my driver.  An application which does not, would break on any 
other camera.  I'd be happy to see read replaced with something simpler 
provided that it allows a user using simple shell commands to capture 
images from the camera.  I think it is a benefit to users to be able to 
capture images with a shell script from the webcam and not be required to 
use a V4L tool.

If we're really concerned about well behaved V4L drivers, both of our 
drivers should be discarded.  They both violate at least two of the well 
behaved requirements.

 I don't mind the proc interface going provided that there is a way to
 change the shutter speed independently for every camera connected.  I have

Is it possible to add it to an ioctl or perhaps one of the existing ones?
(This is probably a V4L issue.)


There are no V4L ioctl's that map to shutter speed.  I only mapped gain 
to brightness because everybody else does to the point that some 
applications (such as gqcam) use this in an attempt to auto adjust 
exposure.  The range on the shutter is 4 to 31500, not many applications 
have controls with that much range if I were to map it to something like 
contrast.

It is a V4L issue that is sorted out in V4L2.  But I still rather like the 
fact that without using any V4L apps, a user can write a shell script and 
use Image Magick to create a viewable gif/jpg/png/whatever.

I can send you the latest version of my working copy (I still play with
it some as a hobby), and you're welcome to use it as a guide to the async
urbs if you so desire. I'll also try to assist if I can provide insight
into what I did to make it happen. I just can't be the one to take the
lead on this issue.


Sounds reasonable to me.


Oliver hammered me pretty hard :) to get my disconnect handling into
shape. As time allows, I think I'd be a little more willing to look at and
try to fix that part up.


Any patches are welcome.




---
This sf.net emial is sponsored by: Influence the future 
of  Java(TM) technology. Join the Java Community 
Process(SM) (JCP(SM)) program now. 
http://ad.doubleclick.net/clk;4699841;7576298;k?http://www.sun.com/javavote
___
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel


Re: [linux-usb-devel] Re: About the vicam driver(s)

2002-10-21 Thread John Tyner
 require a stand alone vicam-specific executable to change the shutter
 speed.  I didn't think that was a reasonable expectation.

Is a MODULE_PARM too unreasonable? I agree that the proc interface
provides a simple way to change the shutter speed, but I still think that
it's a lot of code to do a small amount of work. Maybe a standalone app is
too much to ask, but [personally] I'd like something other than proc.

 The memory should definitely not be allocated when the camera is
 initialized.  I think there are a few who would throw a fit if your driver
 was allocating 320*240*3*2 bytes of data (nearly half a megabyte) any time
 the camera was plugged in but only got used if a V4L application started

If you don't intend to use the camera, why plug it in? If you want to plug
it in, why not leave the driver as an unloaded module? cp and dd are not
V4L applications, but they would cause this memory to be allocated.

 The V4L API also says that applications should provide a suitably sized
 read buffer.  An application which does this, will receive the expected V4L
 behavior from my driver.  An application which does not, would break on any
 other camera.  I'd be happy to see read replaced with something simpler
 provided that it allows a user using simple shell commands to capture

I submitted a patch earlier to show what I meant. I can submit another
which will probably break cp but will work with any application that
supplies a big enough buffer (dd works with it). Is that sufficient?

 It is a V4L issue that is sorted out in V4L2.  But I still rather like the

I'm not familiar with V4L2, unfortunately.

 fact that without using any V4L apps, a user can write a shell script and
 use Image Magick to create a viewable gif/jpg/png/whatever.

And I'm not opposed to it. But I think that the driver's primary
responsibility should be to V4L apps, and if we can make shell utils work
as a side effect, cool. But they should take a back seat to V4L.

I'm willing to back off of some of this stuff if others think I'm wrong,
but so far no one has spoken up.

John



---
This sf.net emial is sponsored by: Influence the future 
of  Java(TM) technology. Join the Java Community 
Process(SM) (JCP(SM)) program now. 
http://ad.doubleclick.net/clk;4699841;7576298;k?http://www.sun.com/javavote
___
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel



Re: [linux-usb-devel] Re: About the vicam driver(s)

2002-10-21 Thread Brad Hards
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On Tue, 22 Oct 2002 08:55, John Tyner wrote:
  The memory should definitely not be allocated when the camera is
  initialized.  I think there are a few who would throw a fit if your
  driver was allocating 320*240*3*2 bytes of data (nearly half a megabyte)
  any time the camera was plugged in but only got used if a V4L application
  started

 If you don't intend to use the camera, why plug it in? If you want to plug
 it in, why not leave the driver as an unloaded module? cp and dd are not
 V4L applications, but they would cause this memory to be allocated.

You are thinking like a developer, not like a user :-)

Developer: Why do you have the camera plugged in if you aren't using it right 
now?
User: I just leave the camera plugged in all the time. No real need to unplug 
it, because I don't need the port for anything else, and I'll probably use 
the camera tomorrow night
Developer: Then why do you have the module loaded?
User: What's a module? I just plug in the camera and it works...

Users shouldn't need to understand concepts like kernel memory is not 
swappable. Just do the allocation on open(), and free it on close().

Brad

- -- 
http://linux.conf.au. 22-25Jan2003. Perth, Aust. I'm registered. Are you?
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.0.6 (GNU/Linux)
Comment: For info see http://www.gnupg.org

iD8DBQE9tItwW6pHgIdAuOMRAtJzAKCgRJebEJqhdhPE42KgHSMBkunnRgCgmG/6
8ZwalkeKwKaW3yOlCRRxRZM=
=7zzp
-END PGP SIGNATURE-



---
This sf.net emial is sponsored by: Influence the future 
of  Java(TM) technology. Join the Java Community 
Process(SM) (JCP(SM)) program now. 
http://ad.doubleclick.net/clk;4699841;7576298;k?http://www.sun.com/javavote
___
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel



Re: [linux-usb-devel] Re: About the vicam driver(s)

2002-10-21 Thread Joe Burks
At 03:55 PM 10/21/2002 -0700, John Tyner wrote:

Is a MODULE_PARM too unreasonable? I agree that the proc interface
provides a simple way to change the shutter speed, but I still think that
it's a lot of code to do a small amount of work. Maybe a standalone app is
too much to ask, but [personally] I'd like something other than proc.


A MODULE_PARM is not too unreasonable, provided it:

1) Can be set at any time while the camera is plugged in.  We can be a 
little flexible on that, but it absolutely without question must be 
alterable while the device is open and being used by an application.
2) A different shutter speed can be specified for each camera connected.  I 
have four cameras.  Other people have reported using as many as 8.  We 
cannot have one parm affecting shutter speed on all 8 of those 
cameras.  I'd also not like to limit the number of cameras that can be 
plugged in just for sake of shutter speed adjustment.
3) Can easily be set by the user without having to use any hex/binary math.

If you don't intend to use the camera, why plug it in? If you want to plug
it in, why not leave the driver as an unloaded module? cp and dd are not
V4L applications, but they would cause this memory to be allocated.


As previously mentioned, the average user will leave the camera plugged in 
whether they are using it or not unless they have some reason to unplug it 
(like needing the USB port).  Their distribution will automatically insert 
the driver for the device upon detection.  It is unreasonable to require 
half a meg per camera plugged in if that camera is not in use.  It is 
reasonable for cp and dd to have that memory allocated because they are 
using the camera.  Is it reasonable for cp/dd to allocate and free that 
memory each time they run?  I think it is, but I'm not sure I free the 
memory on close.  If not, I should.

I submitted a patch earlier to show what I meant. I can submit another
which will probably break cp but will work with any application that
supplies a big enough buffer (dd works with it). Is that sufficient?


I haven't had a chance to look at your patch yet, but I promise I will look 
at your latest one within 24 hours.

And I'm not opposed to it. But I think that the driver's primary
responsibility should be to V4L apps, and if we can make shell utils work
as a side effect, cool. But they should take a back seat to V4L.


I think a reasonable set of requirements for read is:

1) Applications that read in a number of bytes equal to one whole frame, 
will receive subsequent frames on each read.  This is keeping with the V4L 
spec.
2) Applications that read less than one whole frame's worth of bytes will 
receive fragments of a single frame until it has read one whole 
frame.  Thus `cat /dev/video0 mypic` will work.  (I think I may have been 
wrong about using cp, my posts on sourceforge only indicate cat working)

I believe the current read function does this.  Many months ago the first 
incarnation of the driver had a V4L bug that prevented mmap from working, 
so some applications like gqcam ended up reading from /dev/video.  It 
worked perfectly and the read code hasn't changed since then so I assume it 
still does.  There may be more efficient ways of accomplishing the same 
thing, again I'll look at your patch.

I'm willing to back off of some of this stuff if others think I'm wrong,
but so far no one has spoken up.


I think most just see vicam in the subject and skip the content :)  I 
know I only read a fraction of all the messages flying across all the linux 
development lists I'm subscribed to.  There just aren't enough hours in a 
day to read and think about them all.

-Joe



---
This sf.net emial is sponsored by: Influence the future 
of  Java(TM) technology. Join the Java Community 
Process(SM) (JCP(SM)) program now. 
http://ad.doubleclick.net/clk;4699841;7576298;k?http://www.sun.com/javavote
___
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel


Re: [linux-usb-devel] Re: About the vicam driver(s)

2002-10-18 Thread John Tyner
 Yes, but Joe's was there first, based on the original driver, that's why
 I accepted his patch.

I understand that. My code is even based partially on it and wouldn't exist
without the reverse engineering/color decoding information that I found at
his site.

 Could you point out the problems in the existing driver?  It sounds like
 some things would be easy to fix, such as control message allocation for
 an example.

The use of the dev_set_drvdata family of calls. I'm not sure why, but those
seem to keep the driver from being inserted, removed, and reinserted.
Replacing those with the use of private_data field in the usb_interface
struct seems to fix that (I removed the usb_put_device call along with
these, so that my be to blame instead).

The control message allocation and async urbs, as previously mentioned.

The is_initialized and needsDummyRead variables are unneeded as the things
protected by those variables can/should just be done when the camera is
initialized.

The disconnect handling is wrong for the same reasons mine was. There is no
synchronization with the open/release routines, no making sure that no
communication with the camera is going on, and the cam structure is freed
without checking if users still have the camera open.

framebuf is allocated in both open and mmap calls. Not necessarily wrong...
however, it is possible that if the pointer was overwritten that the memory
would just be reallocated/leaked instead of oopsing, which would indicate a
problem.

There are some other, more subjective things that I take issue with such as
the read function being unnecesarily complicated, the proc interface not
being necessary, raw_image being allocated in open rather than probe, etc.

Some of these things are easy, some are a bit more complicated. My biggest
issue is that the driver that I submitted is a superset of the one that was
merged. I did a lot of work to write/test/debug/etc. that code, and my doing
that again would be a step backwards, in my mind.

John



---
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf
___
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel



Re: [linux-usb-devel] Re: About the vicam driver(s)

2002-10-17 Thread John Tyner
Sorry to resurrect this thread, but...

 On Mon, 14 Oct 2002, Joe Burks wrote:

 I'd hope for keeping just one version in the kernel, otherwise the
 companies making distros are going to have a terrible time with
 it...  There are a couple important things my driver does that John's
 doesn't

What are these things? Based on the last time I looked at the sourceforge
website, I would have agreed; however, I've begun working on the driver and
I'm finding that what got merged last week has alot (if not all) of those
things stripped out.

With those things missing, the only thing that the proc interface provides
is variable shutter speed (since the gain/brightness can be set with an
ioctl), and this seems like an awful lot of code bloat for not much payoff.
It also, from what I can see, makes the two drivers the same except that
mine provides the asynchronous urbs, handles disconnect correctly, doesn't
have to reallocate memory for every control message, and doesn't crash when
trying to reinsert after removal of the module (a problem I've been
experiencing with Joe's driver).

Which brings me to the point of the email (sorry it took so long to get
here, heh). In trying to add the features from mine to the one that got
merged, I'm finding that not only am I removing a lot of debug code and even
some bugs, I'm also having to re-debug my own code in order to add it to the
kernel. This, in my opinion, is a waste of my time since I've already
submitted a working, debugged driver.

I realize that this sounds like I'm upset that my driver got rejected or
that I'm trying to take my ball and go home. This is not the case at all.
Rather, I've done all of this work once (and submitted as a completed,
stable, working driver) and don't want to duplicate my own efforts. If
someone else would like to take up the job of trying to merge the two
drivers, I'm happy to give them my code.

If not, then I ask that we revisit the idea of having both drivers coexist
so that people can see/try the both of them. That is, unless a better idea
is suggested.

Thanks,
John



---
This sf.net email is sponsored by: viaVerio will pay you up to
$1,000 for every account that you consolidate with us.
http://ad.doubleclick.net/clk;4749864;7604308;v?
http://www.viaverio.com/consolidator/osdn.cfm
___
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel



Re: [linux-usb-devel] Re: About the vicam driver(s)

2002-10-17 Thread Greg KH
On Thu, Oct 17, 2002 at 08:12:14PM -0700, John Tyner wrote:
 
 Which brings me to the point of the email (sorry it took so long to get
 here, heh). In trying to add the features from mine to the one that got
 merged, I'm finding that not only am I removing a lot of debug code and even
 some bugs, I'm also having to re-debug my own code in order to add it to the
 kernel. This, in my opinion, is a waste of my time since I've already
 submitted a working, debugged driver.

Yes, but Joe's was there first, based on the original driver, that's why
I accepted his patch.  We all end up reworking already working code in
order to get it accepted.  This is just another form of that.

 I realize that this sounds like I'm upset that my driver got rejected or
 that I'm trying to take my ball and go home. This is not the case at all.
 Rather, I've done all of this work once (and submitted as a completed,
 stable, working driver) and don't want to duplicate my own efforts. If
 someone else would like to take up the job of trying to merge the two
 drivers, I'm happy to give them my code.
 
 If not, then I ask that we revisit the idea of having both drivers coexist
 so that people can see/try the both of them. That is, unless a better idea
 is suggested.

Could you point out the problems in the existing driver?  It sounds like
some things would be easy to fix, such as control message allocation for
an example.

thanks,

greg k-h


---
This sf.net email is sponsored by: viaVerio will pay you up to
$1,000 for every account that you consolidate with us.
http://ad.doubleclick.net/clk;4749864;7604308;v?
http://www.viaverio.com/consolidator/osdn.cfm
___
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel



Re: [linux-usb-devel] Re: About the vicam driver(s)

2002-10-14 Thread Greg KH

On Mon, Oct 14, 2002 at 04:38:12PM -0700, John Tyner wrote:
 On Mon, 14 Oct 2002, Joe Burks wrote:
 
  I'd hope for keeping just one version in the kernel, otherwise the
  companies making distros are going to have a terrible time with
  it...  There are a couple important things my driver does that John's
  doesn't, but there is one very important thing that John's driver did that
  mine doesn't: asynchronous frame grabs.
 
 Sounds like we're in agreement here on both counts.

Good, glad to see it.

  If John has the inclination, a patch to my 2.5.41 driver to do asynchronous
  streaming rather than synchronous streaming would be a great boon to whole
  community.
 
 Like I said, I'd rather try to merge my stuff into the sourceforge version
 (now in-kernel) than try to compete with it. Unfortunately, I'm too lazy
 to diff against anything other than an official kernel version, so it'll
 have to wait till Linus puts out a version with the updated vicam driver.
 That is, unless these kind of changes run up agains the Oct. 20 deadline...
 I'll need some clarification there.

No, it's ok to do driver stuff like this after Oct. 20 or Oct 31.
It's big core kernel features (like LSM, aio, and others) that are not
allowed after that date.

I think you can grab the -bk patches from kernel.org if you want to see
the latest driver merge, or you can wait a few days and get the next
release.

Either one is fine with me.

Thanks again for working together, I appreciate it.

greg k-h


---
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf
___
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel