Re: [linux-usb-devel] Re: About the vicam driver(s)
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)
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)
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)
-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)
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)
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)
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)
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)
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
