GNOME Bugzilla – Bug 546868
PATCH: Various issues with cheese after installing a tv card
Last modified: 2008-09-07 20:25:24 UTC
Short intro: I'm a long time Linux developer who is currently working on improving webcam support under Linux, see: https://fedoraproject.org/wiki/Features/BetterWebcamSupport As part of getting the gspca usb webcam driver framework into the kernel I've written a userspace library which does all the conversion from proprietary cam formats that gspca used todo in kernel space, see: http://hansdegoede.livejournal.com/3636.html A couple of days ago I've installed a Bt878 tv card in my machine to mske sure my libary (which until then was only tested with webcams) did not break tv card support. With this tv card installed, I'm having several issues with cheese: 1) It segfaulted each time I went to the preferences dialog, I know a patch for this went in for the scenario where no devices were present, but in this case for some reason a gconf key was not set and there was no check for this not being set, the attached patch fixes this crash. I can reproduce this 100% by removing cheese's gconf settings. With the tv-card present, I now have 4 v4l devices: /dev/video0 tv-card video capture /dev/vbi0 tv-card vertical blank information (teletext) /dev/radio0 tv-card fm radio on tv-card /dev/video1 a webcam (doesn't matter which one for the problems I see) 2) With 4 devices present the scanning by cheese to determine with a device is a v4l1 or v4l2 device takes ages, and if I go to the preferences menu while it is still scanning all devices but the first one are shown as "null (/dev/xxx)". If I make any choices other then the first device at this time cheese crashes. 3) If I wait for the scanning to finish, the /dev/radio0 device still shows with a <null> description as gstreamer does not know what todo with it so neither using the v4l1 source nor the v4l2 source will work. 4) If I select /dev/radio0 cheese crashes (and annoyingly remembers, so I need to remove the gconf settings otherwise it crashes at start all the time). I think this can be easily solved by writing a different (blindingly fast) probing routine, simply open all the devices found by hall and then do a VIDIOC_QUERYCAP ioctl, if this succeeds its a v4l2 device and the v4l2 source should be used. The result of the ioctl can then also be used to determine of the device is of any interest, iow if it has capture capability and if it does not the device should then be removed from the list built by hal (such as for example /dev/radio0). If VIDIOC_QUERYCAP fails its most likely a v4l1 device, but you can make sure by doing a VIDIOCGCAP ioctl. If you want I can write a patch for this, let me know if you're interested in this. 5) When I select the tv-card as input for cheese all I get is a black screen (yes the card is properly tuned) and very slow refreshes, as if gstreamer is taking ages to get one (black) frame and cheese is blocked during all the time gstreamer is busy. During this time the terminal from which cheese was started fills up with these messages: (cheese:5208): GLib-GObject-WARNING **: IA__g_object_notify: object class `GstV4l2Src' has no property named `norm' (cheese:5208): GStreamer-WARNING **: Name photo_save_bin is not unique in bin pipeline, not adding repeated constantantly at what I guess is the refresh rate / respone rate to keyboard / mouse actions that I'm seeing. Do you have any clue whats going one here? p.s. I'm going on vaction for a week starting right about now (I wanted to make a brain dump on this before I left), so don't expect any response from me for 7 days.
Created attachment 116101 [details] [review] PATCH: fixing a crash when no device is set in gconf
Created attachment 116126 [details] [review] ioctl device probing Attaching a patch that probe device with ioctl as Hans suggested. Not sure it's all correct because I only have a v4l2 device here, could anybody test it with a more complex scenario (several devices v4l2 v4l radios etc)? Anyway we still have to set up a pipeline to get supported videoformats so I don't believe that the speed increase would be that much. I'm not sure if that's achievable without gstreamer, I'll take a look at v4l?src code. I cannot test the gconf patch since there is no easy way here to remove a gconf key. If I unset it simply gets resetted to the one in the schema file. Anyway I dont't anything against committing that fix. Daniel? Some thoughts. Is this ioctl thing really up to cheese? Shouldn't we demand all hardware low level operation to hal? Does hal list vbi and radio devices too when we ask for video4linux capability? Is there a way to filter out them? I'm ok with doing ioctl in cheese, it's not that hard and I don't see any "risk" with that but I think a solution totally based upon hal would be the best.
Created attachment 116127 [details] [review] ioctl probing patch (there was a typo in the previous) Sorry, there was an error in the previous patch
>Not sure it's all correct because I only have a v4l2 device here, could anybody >test it with a more complex scenario (several devices v4l2 v4l radios etc)? Little update. I now have a cheap v4l logitech camera and I can confirm that the ioctl patch works with v4l devices too. Anyone willing to test with a device with no capture capability? I'm pretty sure it works with that too but a confirm would be better.
regarding point 2) outlined by Hans, what about setting preferences action insensitive while device probing is running? no clue about point 5), a complete cheese -v output would probably be useful, I think it has something to do with setting maximum available framerate.
Hi, Thanks for the patch! I'm back from my vacation and I'm looking into this currently on a system with video devices without capture capability it crashes. This is because of the following: @@ -559,6 +594,15 @@ } g_print ("\n"); } + return; + +cleanup: + g_free (webcam_device->hal_udi); + g_free (webcam_device->video_device); + g_free (webcam_device); + g_free (device); + priv->num_webcam_devices = priv->num_hal_devices - 1; + return; } static void Here you free the webcam_device, but that was not allocated on its own its part of an array! I'm currently working on an alternative patch fixing this, I will attach that when its done.
Ok, I've been putting a lot of time in to this, as there are several issues which needs solving to make cheese work properly with more complex video4linux setups like my system with both a tv/radio card and a webcam attached. So I've written the following patches sofar: cheese-2.23.90-dont-use-non-capture-devices.patch: A new version of ioctl.diff which does the ioctl checking in cheese_webcam_get_video_devices_from_hal and does not add them to the webcam_devices array if it is not a suitable device, so that we don't have to remove things from the array later which leads to all kind of issues. cheese-2.23.90-supported-resolutions-per-device.patch: The supported_resolutions hashtable contains pointers to entries of the video_formats table in CheeseWebcamDevice, but the supported_resolutions hashtable was global, so looking up the current resolution there could result in a format which was not valid on the current webcam, iow it resulted in a pointer to a member of the video_formats table of another webcam then the current. This patch changes the supported_resolutions hashtable to be per CheeseWebcamDevice. With this patch added I can finally switch between my 2 video4linux devices without having to restart cheese. cheese-2.23.90-let-gstreamer-choose-yuv-or-rgb.patch: My bttv card can generate video data for all supported resolutions in 18 different video formats of which 16 are either rgb or yuv variations! This leads to all resolutions being listed 16 times in the resolution drop down list. While looking at ways to fix this I've come to the conclusion that the current code where the resolution gets looked up into the hashtable and whatever format happens to be first (rgb or yuv) in the hashtable for a certain resolution gets used is wrong. We should really only specify resolution and framerate and let gstreamer decide whether its more efficient to use yuv or rgb (in cases where the v4l device supports both). This patch implements this as a first step to get rid of each resolution being listed 16 times! ### Note that with the first 2 patches applied all my issues from the original report are fixed, the each resolution being listed 16 times isue is a new one. As for issue 5) of my original report, my tv card still won't work without some manual tweaking, that is I need to change the resolution to 384x256 before it will work. Which leads me to another issue, currently cheese remembers the last set resolution, not a resolution per device, so each time I switch device I need to change the resolution to get my tv input to work. But I'll leave writing a patch for that to someone else. I'm still looking into getting rid of the resolutions being listed multiple times issue.
Created attachment 117722 [details] [review] Patch: don't use non capture devices
Created attachment 117723 [details] [review] Patch: make supported_resolutions hashtable per device
Created attachment 117724 [details] [review] Patch: let gstreamer decide wether it wants yuv or rgb
hans, thanks for your tremendous effort on those patches. we will have a look at those in the next days, thanks!
(In reply to comment #6) > Hi, > > Thanks for the patch! > + priv->num_webcam_devices = priv->num_hal_devices - 1; That's wrong too :P. I was aware of a couple of issues with this patch, it was just a quick try I did to see if ioctl was a viable path. I had no time to work on it lately and I was waiting for you to come back since I don't have a good testing environment (just a v4l and a v4l2 devices both with capture cap). > I'm currently working on an alternative patch fixing this, I will attach that > when its done. I see :D! Thank you for all the work you did (another comment following about those).
(In reply to comment #7) > cheese-2.23.90-let-gstreamer-choose-yuv-or-rgb.patch: > My bttv card can generate video data for all supported resolutions in 18 > different video formats of which 16 are either rgb or yuv variations! This > leads to all resolutions being listed 16 times in the resolution drop down > list. > > While looking at ways to fix this I've come to the conclusion that the current > code where the resolution gets looked up into the hashtable and whatever format > happens to be first (rgb or yuv) in the hashtable for a certain resolution gets > used is wrong. We should really only specify resolution and framerate and let > gstreamer decide whether its more efficient to use yuv or rgb (in cases where > the v4l device supports both). This patch implements this as a first step to > get rid of each resolution being listed 16 times! > > > ### > > Note that with the first 2 patches applied all my issues from the original > report are fixed, the each resolution being listed 16 times isue is a new one. Not a new one, take a look at bug #547144 and bug #547140 (would you mind attaching those patches there?). I don't think gstreamer has a smart way to chose between yuv and rgb, I think it just picks the first listed in the caps. It's ok for me to let gstreamer chose which format is better between different yuv or rgb formats (like different fourccs or rgb pixelformats). I think though that we should let the user chose between rgb and yuv because there are some drivers having issues with one of those. e.g. I have a logitech camera supported by gspca drivers that has broken yuv mode (everything is blue) and I'm aware of other webcams (ps2 eyetoy) having a similar issue with rgb. > As for issue 5) of my original report, my tv card still won't work without some > manual tweaking, that is I need to change the resolution to 384x256 before it > will work. > > Which leads me to another issue, currently cheese remembers the last set > resolution, not a resolution per device, so each time I switch device I need to > change the resolution to get my tv input to work. But I'll leave writing a > patch for that to someone else. I'm still looking into getting rid of the > resolutions being listed multiple times issue. Sure, another known issue, my patch on bug #547144 should solve that too (if saved format is not supported by current device try to pick the nearest one). The best way to solve this should be to have per webcam settings. About issue 5, still no clue but this warning: (cheese:5208): GLib-GObject-WARNING **: IA__g_object_notify: object class `GstV4l2Src' has no property named `norm' has been fixed in current gstreamer (bug #549090). About the patches: I'm on vacation now :), I might be able to take a look at them the next week. Anyway thanks for all this work you're doing.
(In reply to comment #13) > (In reply to comment #7) > > cheese-2.23.90-let-gstreamer-choose-yuv-or-rgb.patch: > > My bttv card can generate video data for all supported resolutions in 18 > > different video formats of which 16 are either rgb or yuv variations! This > > leads to all resolutions being listed 16 times in the resolution drop down > > list. > > > > While looking at ways to fix this I've come to the conclusion that the current > > code where the resolution gets looked up into the hashtable and whatever format > > happens to be first (rgb or yuv) in the hashtable for a certain resolution gets > > used is wrong. We should really only specify resolution and framerate and let > > gstreamer decide whether its more efficient to use yuv or rgb (in cases where > > the v4l device supports both). This patch implements this as a first step to > > get rid of each resolution being listed 16 times! > > > > > > ### > > > > Note that with the first 2 patches applied all my issues from the original > > report are fixed, the each resolution being listed 16 times isue is a new one. > > Not a new one, take a look at bug #547144 Ok, I'll attach my patches to bug 547144. > I don't think gstreamer has a smart way to chose between yuv and rgb, I think > it just picks the first listed in the caps. I don't know how smart gstreamer is, but atleast the part about it choosing the first one in the caps is not true, the caps order with regards to yuv versus rgb is different for my webcam versus my tv-card, yet in both cases with my patch applied gstreamer chooses yuv. Which makes sense assuming it is using an xvimage sync, as xv usually works best with yuv. > It's ok for me to let gstreamer > chose which format is better between different yuv or rgb formats (like > different fourccs or rgb pixelformats). Good, as I really think we should not try to outsmart it. > I think though that we should let the > user chose between rgb and yuv because there are some drivers having issues > with one of those. e.g. I have a logitech camera supported by gspca drivers > that has broken yuv mode (everything is blue) and I'm aware of other webcams > (ps2 eyetoy) having a similar issue with rgb. > Then the drivers for those should be fixed to either not report that format, or to not be buggy in that format, without this the resolution list would look something like this (webcam example, tvcard is too long): 160x120 (rgb) 160x120 (yuv) 176x144 (rgb) 176x144 (yuv) 320x240 (rgb) 320x240 (yuv) 352x288 (rgb) 352x288 (yuv) And we cannot make the rgb/yuv a seperate choice as not all modes may be available in both rgb and yuv. Things get even more confusing if some modes also support multiple framerates and you want to add those too. Ask yourself how will this look from an ui / end user pov? The end user will wonder: WTF is rgb / yuv ?? Trust me if cam does not work in yuv or rgb but claims it does we need to fix the driver. The problem is the quality of Linux webcam drivers so far has been rather poor, but I'm working hard on improving this: https://fedoraproject.org/wiki/Features/BetterWebcamSupport For this I've written dozens of patches to webcam drivers, helped getting gspca ported to v4l2 and merged into the mainline kernel (will show up in 2.6.27) and written a format conversion library. I've a patch pending for gstreamer-plugins-good to use this library, see bug 545033. With gstreamer using this conversion library all cams will claim to support both yuv and rgb, making this problem all the more prudent. This may sound bad, but its a good thing, as this will make cheese work with cams which use a variety of proprietary video formats (custom compressed raw bayer) as well as with jpeg cams (which with it currently does not work). I noticed talk about exporting more knobs to the end user in bug 547144 such as for example framerates, but this whole gstreamer framerate thing really is bullocks for webcams, and given that cheese's main use is webcams we really should not export framerates to the end user! framerate is a bogus thing for a webcam, as with cmos sensors (anything but the most expensive cams) the framerate is limited by the needed exposure time, iow the darker the environment (and bright artificial light is already dark in this context) the lower the framerate. Proper written drivers will adjust exposure automatically to the lighting conditions, thus changing the framerate! Phew, long story, but the moral is exporting the resolution and only the resolution to the end user is all we should do, and one could question even that. Things really should just work, and when they don't we need to fix the drivers and / or gstreamer and / or cheese not give the users some knobs to fix things for himself.
Created attachment 117822 [details] [review] Patch: don't use non capture devices My previous version of the ioctl patch didn't close the device after probing it, which worked fine with the 2 devices I tested which had drivers which support multiple opens, but didn't work when I tried another cam with a driver which didn't support multiple opens. This versions adds the missing close() calls.
Created attachment 117864 [details] [review] Patch: don't use non capture devices And one last version of the ioctl patch, this time with a missing #include for close() added.
hans, you totally rock! i will do some testing in the next days, but i totally want to get this patches into cheese 2.24 (which means there are two weeks left to accomplish that) i just miss one thing in your patches: the indenting style. as you probably can tell, we have a different style, which we use throughout cheese than you use. if you dont want to use that style for yourself, we have a script in tools/ which uses the program uncrustify to reformat the source code to our style. you might also want to add yourself to the AUTHORS file and to the header of the specific source files, as you did some great improvements in several places. thanks!
(In reply to comment #17) > hans, you totally rock! i will do some testing in the next days, but i totally > want to get this patches into cheese 2.24 (which means there are two weeks left > to accomplish that) > Thanks, does that include the ones I attached to bug 547144, I would also really like to see those get added. > i just miss one thing in your patches: the indenting style. Erm, I tried to stick to the indenting style of the files (I always do I don't really have a stule of my own) the only exception is the ioctl patch, where I copied parts of the code from the original patch attached here and those seem to have a different style. Anyways feel free to change the identation in anyway you want!
(In reply to comment #18) > (In reply to comment #17) > > hans, you totally rock! i will do some testing in the next days, but i totally > > want to get this patches into cheese 2.24 (which means there are two weeks left > > to accomplish that) > > > > Thanks, does that include the ones I attached to bug 547144, I would also > really like to see those get added. of course! i just need some time to test them ;) > > > i just miss one thing in your patches: the indenting style. > > Erm, I tried to stick to the indenting style of the files (I always do I don't > really have a stule of my own) the only exception is the ioctl patch, where I > copied parts of the code from the original patch attached here and those seem > to have a different style. > > Anyways feel free to change the identation in anyway you want! yeah, we will run the script afterwards
committed "PATCH: fixing a crash when no device is set in gconf" to trunk
Created attachment 117881 [details] [review] ioctl probing patch with indenting changes my isight webcam does not get detected anymore with the " ioctl probing patch (there was a typo in the previous)" patch. i had to add it manually as we made the indenting changes and therefore patch isnt able to add it automatically. if i didnt fuck up doing that, your patch has a problem ;) i updated the patch which should apply to recent trunk
somehow im lost, could you please clarify which patches should be applied in which order? e.g. i think "PATCH: fixing a crash when no device is set in gconf" is the older version of "Patch: don't use non capture devices", is that correct? you can mark the old patches as obsolete in the box down there.
Sorry about the confusion, the "ioctl probing patch (there was a typo in the previous)" patch is obsolete its replaced by: cheese-2.23.90-dont-use-non-capture-devices.patch I expect that using that version instead will fix your detection problems, the original is rather buggy. I didn't have the rights to mark it as obsolete as I didn't attach it. Given that: PATCH: fixing a crash when no device is set in gconf Has already been applied the following patches should still be applied (listed in the order to apply them): Patch: don't use non capture devices (latest version) Patch: make supported_resolutions hashtable per device (has fuzz due to changes to previous patch) Patch: let gstreamer decide wether it wants yuv or rgb Patch: shuffle some code in preparation for fixing the resolutions being listed multiple times issue (from bug 547144) Patch: only list resolutions once (from bug 547144) Patch: sort the list of resolutions (from bug 547144) Note that your patch with indenting changes is based on the obsolete "ioctl probing patch (there was a typo in the previous)" patch and thus is obsolete itself.
(In reply to comment #18) > copied parts of the code from the original patch attached here and those seem > to have a different style. I'm sorry, I wrote it before to fix the configure emacs to use spaces instead of tabs.
(In reply to comment #14) > Then the drivers for those should be fixed to either not report that format, or > to not be buggy in that format, without this the resolution list would look > something like this (webcam example, tvcard is too long): > > 160x120 (rgb) > 160x120 (yuv) > 176x144 (rgb) > 176x144 (yuv) > 320x240 (rgb) > 320x240 (yuv) > 352x288 (rgb) > 352x288 (yuv) > > And we cannot make the rgb/yuv a seperate choice as not all modes may be > available in both rgb and yuv. Things get even more confusing if some modes > also support multiple framerates and you want to add those too. > > Ask yourself how will this look from an ui / end user pov? The end user will > wonder: WTF is rgb / yuv ?? > > Trust me if cam does not work in yuv or rgb but claims it does we need to fix > the driver. The problem is the quality of Linux webcam drivers so far has been > rather poor, but I'm working hard on improving this: > https://fedoraproject.org/wiki/Features/BetterWebcamSupport Completely agree, I thinked about this lately and I definitely agree that we should let gstreamer choose the correct video format for us. > I noticed talk about exporting more knobs to the end user in bug 547144 such as > for example framerates, but this whole gstreamer framerate thing really is > bullocks for webcams, and given that cheese's main use is webcams we really > should not export framerates to the end user! framerate is a bogus thing for a > webcam, as with cmos sensors (anything but the most expensive cams) the > framerate is limited by the needed exposure time, iow the darker the > environment (and bright artificial light is already dark in this context) the > lower the framerate. Proper written drivers will adjust exposure automatically > to the lighting conditions, thus changing the framerate! Agree here too, as you can see in the other bug, I don't think we should never change framerate, I think we should let gstreamer choose that too. Why we have all that code for framerate retrieving and for finding the highest one if gstreamer can do this with less effort? Just to add some info, my uvcvideo based webcam (microdia) lists as available framerates 10 20 and 30 fps but only works with the maximum one (doesn't output anything with 10 and 20fps). When I set 30 fps it still works at a lower framerate (usually 15fps). So I agree that framerate is not a thing we should try to manage. > Phew, long story, but the moral is exporting the resolution and only the > resolution to the end user is all we should do, and one could question even > that. Things really should just work, and when they don't we need to fix the > drivers and / or gstreamer and / or cheese not give the users some knobs to fix > things for himself. Sure, user doesn't need to change something that works! I like this KISS approach.
(In reply to comment #25) > Agree here too, as you can see in the other bug, I don't think we should never > change framerate, I think we should let gstreamer choose that too. Why we have > all that code for framerate retrieving and for finding the highest one if > gstreamer can do this with less effort? > Just to add some info, my uvcvideo based webcam (microdia) lists as available > framerates 10 20 and 30 fps but only works with the maximum one (doesn't output > anything with 10 and 20fps). When I set 30 fps it still works at a lower > framerate (usually 15fps). So I agree that framerate is not a thing we should > try to manage. > I've been thinking along the same line, but in my series of patches kept the select highest framerate (upto 30 fps) behavior to avoid causing regressions. But I agree, we should consider not telling gstreamer what framerate to use at all, but thats probably 2.25.x material.
> Patch: don't use non capture devices (latest version) > Patch: make supported_resolutions hashtable per device (has fuzz due to changes > to previous patch) > Patch: let gstreamer decide wether it wants yuv or rgb Hi Hans I gave a quick try (still on vacation :P) to those patches. They seem fine to me, but I have some remark: - given that you say the patches must be applied in the right order why not to group them in a single one? - they don't apply to current svn trunk.. would you mind updating them? - with the last patch the mimetype field in CheeseVideoFormat is no more useful.. why to leave all that unneeded code? (I think most of the whole CheeseVideoFormat thing is no more useful if we let gstreamer choose the framerate too).
(In reply to comment #27) > > Patch: don't use non capture devices (latest version) > > Patch: make supported_resolutions hashtable per device (has fuzz due to changes > > to previous patch) > > Patch: let gstreamer decide wether it wants yuv or rgb > > Hi Hans I gave a quick try (still on vacation :P) to those patches. They seem > fine to me, but I have some remark: > - given that you say the patches must be applied in the right order why not to > group them in a single one? To allow for easier review, I especially splitted some of my work for that purpose. > - they don't apply to current svn trunk.. would you mind updating them? I'll be updating them soonish to apply to 2.23.91 as I will be putting them in the Fedora cheese package, so that the Fedora-10 beta coming up will have a better cheese. > - with the last patch the mimetype field in CheeseVideoFormat is no more > useful.. why to leave all that unneeded code? Actually there isn't all that much code touching the mimetype, I left it there in case we want to use it in the future, but you are right it can be removed.
Created attachment 117961 [details] [review] Patch: don't use non capture devices New version against 2.23.91, I'll also attach new version of the other 2 patches.
Created attachment 117962 [details] [review] Patch: make supported_resolutions hashtable per device
Created attachment 117963 [details] [review] Patch: let gstreamer decide wether it wants yuv or rgb
"Patch: don't use non capture devices" seems to work for me, but i just have connected one webcam and nothing more. "Patch: make supported_resolutions hashtable per device" seems ok and works for me "Patch: let gstreamer decide wether it wants yuv or rgb" seems ok too. if filippo agrees, i would like to commit those 3 patches?
committed to svn, thanks a lot for your awesome work, hans! keep on rocking!
(In reply to comment #32) > if filippo agrees, i would like to commit those 3 patches? Hey, you did not wait for me ;)! Anyway I'm back from vacation and I'm ok with those patches (most of the ioctl code still comes from my first buggy patch, thanks Hans for fixing it). Hans, do you know if is it possible to obtain some more info from a v4l device? At the moment we just query name and device type. V4l2 gives more infos (driver version, bus path..) is it possible to have at least driver name and version for v4l1 too? That will really help with bug triaging.
well, you(In reply to comment #34) > (In reply to comment #32) > > if filippo agrees, i would like to commit those 3 patches? > > Hey, you did not wait for me ;)! Anyway I'm back from vacation and I'm ok with > those patches (most of the ioctl code still comes from my first buggy patch, > thanks Hans for fixing it). well you were not that fast ;) > > Hans, do you know if is it possible to obtain some more info from a v4l device? > At the moment we just query name and device type. V4l2 gives more infos (driver > version, bus path..) is it possible to have at least driver name and version > for v4l1 too? That will really help with bug triaging. >