| Message ID | 1394647711-25291-1-git-send-email-will@williammanley.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi William, Thank you for the patch. First of all, could you please CC me in the future for uvcvideo patches ? On Wednesday 12 March 2014 18:08:31 William Manley wrote: > The uvcvideo webcam driver exposes the v4l2 control "Exposure (Absolute)" > which allows the user to control the exposure time of the webcam, > essentially controlling the brightness of the received image. By default > the webcam automatically adjusts the exposure time automatically but the > if you set the control "Exposure, Auto"="Manual Mode" the user can fix > the exposure time. > > Unfortunately it seems that the Logitech C920 has a firmware bug where > it will forget that it's in manual mode temporarily during initialisation. > This means that the camera doesn't respect the exposure time that the user > requested if they request it before starting to stream video. They end up > with a video stream which is either too bright or too dark and must reset > the controls after video starts streaming. I've asked Logitech whether they can confirm this is a known issue. I'm not sure when I'll have a reply though. > This patch works around this camera bug by re-uploading the cached > controls to the camera immediately after initialising the camera. I'm a bit concerned about this. As you noticed UVC camera are often buggy, and small changes in the driver can fix problems with one model and break others. Sending a bunch of SET_CUR requests at once right after starting the stream is something that has the potential to crash firmwares (yes, they can be that fragile, unfortunately). I would like to get a better understanding of the problem first. As I don't have a C920, could you please perform two tests for me ? I would first like to know what the camera reports as its exposure time after starting the stream. If you get the exposure time at that point (by sending a GET_CUR request, bypassing the driver cache), do you get the value you had previously set (which, from your explanation, would be incorrect, as the exposure time has changed based on your findings), or a different value ? Does the camera change the exposure priority control autonomously as well, or just the exposure time ? Then, I would like to know whether the camera sends a control update event when you start the stream, or if it just changes the exposure time without notifying the driver. > Signed-off-by: William Manley <will@williammanley.net> > --- > drivers/media/usb/uvc/uvc_ctrl.c | 2 +- > drivers/media/usb/uvc/uvc_driver.c | 2 +- > drivers/media/usb/uvc/uvc_video.c | 5 +++++ > drivers/media/usb/uvc/uvcvideo.h | 2 +- > 4 files changed, 8 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c > b/drivers/media/usb/uvc/uvc_ctrl.c index a2f4501..f72d7eb 100644 > --- a/drivers/media/usb/uvc/uvc_ctrl.c > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > @@ -1795,7 +1795,7 @@ done: > * - Handle restore order (Auto-Exposure Mode should be restored before > * Exposure Time). > */ > -int uvc_ctrl_resume_device(struct uvc_device *dev) > +int uvc_ctrl_restore_values(struct uvc_device *dev) > { > struct uvc_control *ctrl; > struct uvc_entity *entity; > diff --git a/drivers/media/usb/uvc/uvc_driver.c > b/drivers/media/usb/uvc/uvc_driver.c index c3bb250..9f8a87e 100644 > --- a/drivers/media/usb/uvc/uvc_driver.c > +++ b/drivers/media/usb/uvc/uvc_driver.c > @@ -1981,7 +1981,7 @@ static int __uvc_resume(struct usb_interface *intf, > int reset) int ret = 0; > > if (reset) { > - ret = uvc_ctrl_resume_device(dev); > + ret = uvc_ctrl_restore_values(dev); > if (ret < 0) > return ret; > } > diff --git a/drivers/media/usb/uvc/uvc_video.c > b/drivers/media/usb/uvc/uvc_video.c index 3394c34..87cd57b 100644 > --- a/drivers/media/usb/uvc/uvc_video.c > +++ b/drivers/media/usb/uvc/uvc_video.c > @@ -1660,6 +1660,11 @@ static int uvc_init_video(struct uvc_streaming > *stream, gfp_t gfp_flags) } > } > > + /* The Logitech C920 temporarily forgets that it should not be > + adjusting Exposure Absolute during init so restore controls to > + stored values. */ > + uvc_ctrl_restore_values(stream->dev); > + > return 0; > } > > diff --git a/drivers/media/usb/uvc/uvcvideo.h > b/drivers/media/usb/uvc/uvcvideo.h index 9e35982..3b365a3 100644 > --- a/drivers/media/usb/uvc/uvcvideo.h > +++ b/drivers/media/usb/uvc/uvcvideo.h > @@ -676,7 +676,7 @@ extern int uvc_ctrl_add_mapping(struct uvc_video_chain > *chain, const struct uvc_control_mapping *mapping); > extern int uvc_ctrl_init_device(struct uvc_device *dev); > extern void uvc_ctrl_cleanup_device(struct uvc_device *dev); > -extern int uvc_ctrl_resume_device(struct uvc_device *dev); > +extern int uvc_ctrl_restore_values(struct uvc_device *dev); > > extern int uvc_ctrl_begin(struct uvc_video_chain *chain); > extern int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
On Thu, 13 Mar 2014, at 10:23, Laurent Pinchart wrote: > First of all, could you please CC me in the future for uvcvideo patches ? Will do. > On Wednesday 12 March 2014 18:08:31 William Manley wrote: > > The uvcvideo webcam driver exposes the v4l2 control "Exposure (Absolute)" > > which allows the user to control the exposure time of the webcam, > > essentially controlling the brightness of the received image. By default > > the webcam automatically adjusts the exposure time automatically but the > > if you set the control "Exposure, Auto"="Manual Mode" the user can fix > > the exposure time. > > > > Unfortunately it seems that the Logitech C920 has a firmware bug where > > it will forget that it's in manual mode temporarily during initialisation. > > This means that the camera doesn't respect the exposure time that the user > > requested if they request it before starting to stream video. They end up > > with a video stream which is either too bright or too dark and must reset > > the controls after video starts streaming. > > I've asked Logitech whether they can confirm this is a known issue. I'm > not > sure when I'll have a reply though. Great :) > > This patch works around this camera bug by re-uploading the cached > > controls to the camera immediately after initialising the camera. > > I'm a bit concerned about this. As you noticed UVC camera are often > buggy, and > small changes in the driver can fix problems with one model and break > others. > Sending a bunch of SET_CUR requests at once right after starting the > stream is > something that has the potential to crash firmwares (yes, they can be > that > fragile, unfortunately). Good point. I can add a quirk such that it only happens with the C920. > I would like to get a better understanding of the problem first. As I > don't > have a C920, could you please perform two tests for me ? > > I would first like to know what the camera reports as its exposure time > after > starting the stream. If you get the exposure time at that point (by > sending a > GET_CUR request, bypassing the driver cache), do you get the value you > had > previously set (which, from your explanation, would be incorrect, as the > exposure time has changed based on your findings), or a different value ? > Does > the camera change the exposure priority control autonomously as well, or > just > the exposure time ? It's a bit of a strange behaviour. I'd already tried littering the code with (uncached) GET_CUR requests. It seems that the value changes sometime during the call to usb_set_interface in uvc_init_video. Strangely enough though calling uvc_ctrl_restore_values immediately after uvc_init_video has no effect. It must be put after the usb_submit_urb loop to fix the problem. > Then, I would like to know whether the camera sends a control update > event > when you start the stream, or if it just changes the exposure time > without > notifying the driver. Wireshark tells me that it is sending a control update event, but it seems like uvcvideo ignores it. I had to add the flag UVC_CTRL_FLAG_AUTO_UPDATE to the uvc_control_info entry for "Exposure (Auto)" for the new value to be properly reported to userspace. Thanks Will -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Will, On Thursday 13 March 2014 10:48:20 Will Manley wrote: > On Thu, 13 Mar 2014, at 10:23, Laurent Pinchart wrote: > > First of all, could you please CC me in the future for uvcvideo patches ? > > Will do. Thank you. > > On Wednesday 12 March 2014 18:08:31 William Manley wrote: > > > The uvcvideo webcam driver exposes the v4l2 control "Exposure > > > (Absolute)" which allows the user to control the exposure time of the > > > webcam, essentially controlling the brightness of the received image. > > > By default the webcam automatically adjusts the exposure time > > > automatically but the if you set the control "Exposure, Auto"="Manual > > > Mode" the user can fix the exposure time. > > > > > > Unfortunately it seems that the Logitech C920 has a firmware bug where > > > it will forget that it's in manual mode temporarily during > > > initialisation. This means that the camera doesn't respect the exposure > > > time that the user requested if they request it before starting to > > > stream video. They end up with a video stream which is either too bright > > > or too dark and must reset the controls after video starts streaming. > > > > I've asked Logitech whether they can confirm this is a known issue. I'm > > not sure when I'll have a reply though. > > Great :) > > > > This patch works around this camera bug by re-uploading the cached > > > controls to the camera immediately after initialising the camera. > > > > I'm a bit concerned about this. As you noticed UVC camera are often buggy, > > and small changes in the driver can fix problems with one model and break > > others. Sending a bunch of SET_CUR requests at once right after starting > > the stream is something that has the potential to crash firmwares (yes, > > they can be that fragile, unfortunately). > > Good point. I can add a quirk such that it only happens with the C920. > > > I would like to get a better understanding of the problem first. As I > > don't have a C920, could you please perform two tests for me ? > > > > I would first like to know what the camera reports as its exposure time > > after starting the stream. If you get the exposure time at that point (by > > sending a GET_CUR request, bypassing the driver cache), do you get the > > value you had previously set (which, from your explanation, would be > > incorrect, as the exposure time has changed based on your findings), or a > > different value ? Does the camera change the exposure priority control > > autonomously as well, or just the exposure time ? > > It's a bit of a strange behaviour. I'd already tried littering the code with > (uncached) GET_CUR requests. It seems that the value changes sometime during > the call to usb_set_interface in uvc_init_video. I'll assume this means that the camera reports the updated exposure time in response to the GET_CUR request. Does the value of other controls (such as the exposure priority control for instance) change as well ? > Strangely enough though calling uvc_ctrl_restore_values immediately after > uvc_init_video has no effect. It must be put after the usb_submit_urb loop > to fix the problem. > > > Then, I would like to know whether the camera sends a control update > > event when you start the stream, or if it just changes the exposure time > > without notifying the driver. > > Wireshark tells me that it is sending a control update event, but it seems > like uvcvideo ignores it. I had to add the flag UVC_CTRL_FLAG_AUTO_UPDATE to > the uvc_control_info entry for "Exposure (Auto)" for the new value to be > properly reported to userspace. Could you send me the USB trace ?
On 13/03/14 17:03, Laurent Pinchart wrote: > On Thursday 13 March 2014 10:48:20 Will Manley wrote: >> On Thu, 13 Mar 2014, at 10:23, Laurent Pinchart wrote: >>> On Wednesday 12 March 2014 18:08:31 William Manley wrote: >>>> The uvcvideo webcam driver exposes the v4l2 control "Exposure >>>> (Absolute)" which allows the user to control the exposure time of the >>>> webcam, essentially controlling the brightness of the received image. >>>> By default the webcam automatically adjusts the exposure time >>>> automatically but the if you set the control "Exposure, Auto"="Manual >>>> Mode" the user can fix the exposure time. >>>> >>>> Unfortunately it seems that the Logitech C920 has a firmware bug where >>>> it will forget that it's in manual mode temporarily during >>>> initialisation. This means that the camera doesn't respect the exposure >>>> time that the user requested if they request it before starting to >>>> stream video. They end up with a video stream which is either too bright >>>> or too dark and must reset the controls after video starts streaming. >>> >>> I would like to get a better understanding of the problem first. As I >>> don't have a C920, could you please perform two tests for me ? >>> >>> I would first like to know what the camera reports as its exposure time >>> after starting the stream. If you get the exposure time at that point (by >>> sending a GET_CUR request, bypassing the driver cache), do you get the >>> value you had previously set (which, from your explanation, would be >>> incorrect, as the exposure time has changed based on your findings), or a >>> different value ? Does the camera change the exposure priority control >>> autonomously as well, or just the exposure time ? >> >> It's a bit of a strange behaviour. I'd already tried littering the code with >> (uncached) GET_CUR requests. It seems that the value changes sometime during >> the call to usb_set_interface in uvc_init_video. > > I'll assume this means that the camera reports the updated exposure time in > response to the GET_CUR request. That's right > Does the value of other controls (such as the > exposure priority control for instance) change as well ? No, I've not seen any of the other controls change. >> Strangely enough though calling uvc_ctrl_restore_values immediately after >> uvc_init_video has no effect. It must be put after the usb_submit_urb loop >> to fix the problem. >> >>> Then, I would like to know whether the camera sends a control update >>> event when you start the stream, or if it just changes the exposure time >>> without notifying the driver. >> >> Wireshark tells me that it is sending a control update event, but it seems >> like uvcvideo ignores it. I had to add the flag UVC_CTRL_FLAG_AUTO_UPDATE to >> the uvc_control_info entry for "Exposure (Auto)" for the new value to be >> properly reported to userspace. > > Could you send me the USB trace ? I've uploaded 3 traces: one with no patch (kernel 3.12) one with my "PATCH v2" applied and one with no patch but caching of gets disabled. You can get them here: http://williammanley.net/C920-no-patch.pcapng http://williammanley.net/C920-with-patch.pcapng http://williammanley.net/C920-no-caching.pcapng The process to generate them was: sudo dumpcap -i usbmon1 -w /tmp/C920-no-patch2.pcapng & # Plug USB cable in here v4l2-ctl -d /dev/v4l/by-id/usb-046d_HD_Pro_Webcam_C920_C833389F-video-index1 -cexposure_auto=1,exposure_auto_priority=0,exposure_absolute=152,white_balance_temperature_auto=0 ./streamon The relevant output seems to be (here from C920-no-patch.pcapng): > 10446 23.095874000 21:14:16.313257000 host -> 32.0 USB 64 SET INTERFACE Request > 10447 23.109379000 21:14:16.326762000 32.3 -> host USBVIDEO 70 URB_INTERRUPT in > 10448 23.109389000 21:14:16.326772000 host -> 32.3 USB 64 URB_INTERRUPT in > 10449 23.189448000 21:14:16.406831000 32.3 -> host USBVIDEO 73 URB_INTERRUPT in > 10450 23.189486000 21:14:16.406869000 host -> 32.3 USB 64 URB_INTERRUPT in > 10451 23.243314000 21:14:16.460697000 32.0 -> host USB 64 SET INTERFACE Response Taking a closer look at packet 10449: > Frame 10449: 73 bytes on wire (584 bits), 73 bytes captured (584 bits) on interface 0 > Interface id: 0 > Encapsulation type: USB packets with Linux header and padding (115) > Arrival Time: Mar 13, 2014 21:14:16.406831000 GMT > [Time shift for this packet: 0.000000000 seconds] > Epoch Time: 1394745256.406831000 seconds > [Time delta from previous captured frame: 0.080059000 seconds] > [Time delta from previous displayed frame: 0.080059000 seconds] > [Time since reference or first frame: 23.189448000 seconds] > Frame Number: 10449 > Frame Length: 73 bytes (584 bits) > Capture Length: 73 bytes (584 bits) > [Frame is marked: False] > [Frame is ignored: False] > [Protocols in frame: usb:usbvideo] > USB URB > URB id: 0xffff88081bf95240 > URB type: URB_COMPLETE ('C') > URB transfer type: URB_INTERRUPT (0x01) > Endpoint: 0x83, Direction: IN > 1... .... = Direction: IN (1) > .000 0011 = Endpoint value: 3 > Device: 32 > URB bus id: 1 > Device setup request: not relevant ('-') > Data: present (0) > URB sec: 1394745256 > URB usec: 406831 > URB status: Success (0) > URB length [bytes]: 9 > Data length [bytes]: 9 > [Request in: 10448] > [Time from request: 0.080059000 seconds] > [bInterfaceClass: Video (0x0e)] > .... 0001 = Status Type: VideoControl Interface (0x01) > Originator: 1 > Event: Control Change (0x00) > Control Selector: Exposure Time (Absolute) (0x04) > Change Type: Value (0x00) > Current value: 333 (0x0000014d) This is the notification of the change of the value. As you can see it happens between "SET INTERFACE Request" and "SET INTERFACE Response". The sources for "streamon" look like: #include <linux/videodev2.h> #include <stdio.h> #include <stdlib.h> #include <fcntl.h> #include <errno.h> #include <string.h> #include <unistd.h> #include <sys/ioctl.h> #define DEVICE "/dev/v4l/by-id/usb-046d_HD_Pro_Webcam_C920_C833389F-video-index1" int main() { int fd, type; fd = open(DEVICE, O_RDWR | O_CLOEXEC); if (fd < 0) { perror("Failed to open " DEVICE "\n"); return 1; } struct v4l2_requestbuffers reqbuf = { .type = V4L2_BUF_TYPE_VIDEO_CAPTURE, .memory = V4L2_MEMORY_MMAP, .count = 1, }; if (-1 == ioctl (fd, VIDIOC_REQBUFS, &reqbuf)) { perror("VIDIOC_REQBUFS"); return 1; } type = V4L2_BUF_TYPE_VIDEO_CAPTURE; if (ioctl (fd, VIDIOC_STREAMON, &type) != 0) { perror("VIDIOC_STREAMON"); return 1; } printf("VIDIOC_STREAMON\n"); usleep(100000); return 0; } Thanks Will -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi William, On Thursday 13 March 2014 22:08:36 William Manley wrote: > On 13/03/14 17:03, Laurent Pinchart wrote: > > On Thursday 13 March 2014 10:48:20 Will Manley wrote: > >> On Thu, 13 Mar 2014, at 10:23, Laurent Pinchart wrote: > >>> On Wednesday 12 March 2014 18:08:31 William Manley wrote: > >>>> The uvcvideo webcam driver exposes the v4l2 control "Exposure > >>>> (Absolute)" which allows the user to control the exposure time of the > >>>> webcam, essentially controlling the brightness of the received image. > >>>> By default the webcam automatically adjusts the exposure time > >>>> automatically but the if you set the control "Exposure, Auto"="Manual > >>>> Mode" the user can fix the exposure time. > >>>> > >>>> Unfortunately it seems that the Logitech C920 has a firmware bug where > >>>> it will forget that it's in manual mode temporarily during > >>>> initialisation. This means that the camera doesn't respect the exposure > >>>> time that the user requested if they request it before starting to > >>>> stream video. They end up with a video stream which is either too > >>>> bright or too dark and must reset the controls after video starts > >>>> streaming. > >>> > >>> I would like to get a better understanding of the problem first. As I > >>> don't have a C920, could you please perform two tests for me ? > >>> > >>> I would first like to know what the camera reports as its exposure time > >>> after starting the stream. If you get the exposure time at that point > >>> (by sending a GET_CUR request, bypassing the driver cache), do you get > >>> the value you had previously set (which, from your explanation, would be > >>> incorrect, as the exposure time has changed based on your findings), or > >>> a different value ? Does the camera change the exposure priority control > >>> autonomously as well, or just the exposure time ? > >> > >> It's a bit of a strange behaviour. I'd already tried littering the code > >> with (uncached) GET_CUR requests. It seems that the value changes > >> sometime during the call to usb_set_interface in uvc_init_video. > > > > I'll assume this means that the camera reports the updated exposure time > > in response to the GET_CUR request. > > That's right > > > Does the value of other controls (such as the > > exposure priority control for instance) change as well ? > > No, I've not seen any of the other controls change. > > >> Strangely enough though calling uvc_ctrl_restore_values immediately after > >> uvc_init_video has no effect. It must be put after the usb_submit_urb > >> loop to fix the problem. > >> > >>> Then, I would like to know whether the camera sends a control update > >>> event when you start the stream, or if it just changes the exposure time > >>> without notifying the driver. > >> > >> Wireshark tells me that it is sending a control update event, but it > >> seems like uvcvideo ignores it. I had to add the flag > >> UVC_CTRL_FLAG_AUTO_UPDATE to the uvc_control_info entry for "Exposure > >> (Auto)" for the new value to be properly reported to userspace. > > > > Could you send me the USB trace ? > > I've uploaded 3 traces: one with no patch (kernel 3.12) one with my > "PATCH v2" applied and one with no patch but caching of gets disabled. > You can get them here: > > http://williammanley.net/C920-no-patch.pcapng > http://williammanley.net/C920-with-patch.pcapng > http://williammanley.net/C920-no-caching.pcapng > > The process to generate them was: > > sudo dumpcap -i usbmon1 -w /tmp/C920-no-patch2.pcapng & > > # Plug USB cable in here > > v4l2-ctl -d > /dev/v4l/by-id/usb-046d_HD_Pro_Webcam_C920_C833389F-video-index1 > > -cexposure_auto=1,exposure_auto_priority=0,exposure_absolute=152,white_balan > ce_temperature_auto=0 > > ./streamon > > The relevant output seems to be (here from C920-no-patch.pcapng): > > 10446 23.095874000 21:14:16.313257000 host -> 32.0 USB 64 > > SET INTERFACE Request 10447 23.109379000 21:14:16.326762000 32.3 > > -> host USBVIDEO 70 URB_INTERRUPT in 10448 23.109389000 > > 21:14:16.326772000 host -> 32.3 USB 64 URB_INTERRUPT in > > 10449 23.189448000 21:14:16.406831000 32.3 -> host > > USBVIDEO 73 URB_INTERRUPT in 10450 23.189486000 21:14:16.406869000 > > host -> 32.3 USB 64 URB_INTERRUPT in 10451 23.243314000 > > 21:14:16.460697000 32.0 -> host USB 64 SET INTERFACE > > Response > Taking a closer look at packet 10449: > > Frame 10449: 73 bytes on wire (584 bits), 73 bytes captured (584 bits) on > > interface 0> > > Interface id: 0 > > Encapsulation type: USB packets with Linux header and padding (115) > > Arrival Time: Mar 13, 2014 21:14:16.406831000 GMT > > [Time shift for this packet: 0.000000000 seconds] > > Epoch Time: 1394745256.406831000 seconds > > [Time delta from previous captured frame: 0.080059000 seconds] > > [Time delta from previous displayed frame: 0.080059000 seconds] > > [Time since reference or first frame: 23.189448000 seconds] > > Frame Number: 10449 > > Frame Length: 73 bytes (584 bits) > > Capture Length: 73 bytes (584 bits) > > [Frame is marked: False] > > [Frame is ignored: False] > > [Protocols in frame: usb:usbvideo] > > > > USB URB > > > > URB id: 0xffff88081bf95240 > > URB type: URB_COMPLETE ('C') > > URB transfer type: URB_INTERRUPT (0x01) > > Endpoint: 0x83, Direction: IN > > > > 1... .... = Direction: IN (1) > > .000 0011 = Endpoint value: 3 > > > > Device: 32 > > URB bus id: 1 > > Device setup request: not relevant ('-') > > Data: present (0) > > URB sec: 1394745256 > > URB usec: 406831 > > URB status: Success (0) > > URB length [bytes]: 9 > > Data length [bytes]: 9 > > [Request in: 10448] > > [Time from request: 0.080059000 seconds] > > [bInterfaceClass: Video (0x0e)] > > > > .... 0001 = Status Type: VideoControl Interface (0x01) > > Originator: 1 > > Event: Control Change (0x00) > > Control Selector: Exposure Time (Absolute) (0x04) > > Change Type: Value (0x00) > > Current value: 333 (0x0000014d) > > This is the notification of the change of the value. As you can see it > happens between "SET INTERFACE Request" and "SET INTERFACE Response". Thank you for investigating this, and sorry for the late reply. I still haven't heard back from Logitech on this issue. I've pinged them, they might be busy at the moment. Given that the device notifies us that the control value changes, one possibly more clever fix would be to handle that even and set the old control value back when the auto control is disabled. However, that's probably an overengineered solution. I've still been wondering whether the quirk shouldn't restore only the control(s) that are known to be erroneously changed by the camera instead of restoring them all. Feel free to disagree, what's your opinion about that ?
On Mon, 14 Apr 2014, at 1:34, Laurent Pinchart wrote: > On Thursday 13 March 2014 22:08:36 William Manley wrote: > > On 13/03/14 17:03, Laurent Pinchart wrote: > > > On Thursday 13 March 2014 10:48:20 Will Manley wrote: > > >> On Thu, 13 Mar 2014, at 10:23, Laurent Pinchart wrote: > > >>> On Wednesday 12 March 2014 18:08:31 William Manley wrote: > > >>>> The uvcvideo webcam driver exposes the v4l2 control "Exposure > > >>>> (Absolute)" which allows the user to control the exposure time of the > > >>>> webcam, essentially controlling the brightness of the received image. > > >>>> By default the webcam automatically adjusts the exposure time > > >>>> automatically but the if you set the control "Exposure, Auto"="Manual > > >>>> Mode" the user can fix the exposure time. > > >>>> > > >>>> Unfortunately it seems that the Logitech C920 has a firmware bug where > > >>>> it will forget that it's in manual mode temporarily during > > >>>> initialisation. This means that the camera doesn't respect the exposure > > >>>> time that the user requested if they request it before starting to > > >>>> stream video. They end up with a video stream which is either too > > >>>> bright or too dark and must reset the controls after video starts > > >>>> streaming. > > >>> > > >>> I would like to get a better understanding of the problem first. As I > > >>> don't have a C920, could you please perform two tests for me ? > > >>> > > >>> I would first like to know what the camera reports as its exposure time > > >>> after starting the stream. If you get the exposure time at that point > > >>> (by sending a GET_CUR request, bypassing the driver cache), do you get > > >>> the value you had previously set (which, from your explanation, would be > > >>> incorrect, as the exposure time has changed based on your findings), or > > >>> a different value ? Does the camera change the exposure priority control > > >>> autonomously as well, or just the exposure time ? > > >> > > >> It's a bit of a strange behaviour. I'd already tried littering the code > > >> with (uncached) GET_CUR requests. It seems that the value changes > > >> sometime during the call to usb_set_interface in uvc_init_video. > > > > > > I'll assume this means that the camera reports the updated exposure time > > > in response to the GET_CUR request. > > > > That's right > > > > > Does the value of other controls (such as the > > > exposure priority control for instance) change as well ? > > > > No, I've not seen any of the other controls change. > > > > >> Strangely enough though calling uvc_ctrl_restore_values immediately after > > >> uvc_init_video has no effect. It must be put after the usb_submit_urb > > >> loop to fix the problem. > > >> > > >>> Then, I would like to know whether the camera sends a control update > > >>> event when you start the stream, or if it just changes the exposure time > > >>> without notifying the driver. > > >> > > >> Wireshark tells me that it is sending a control update event, but it > > >> seems like uvcvideo ignores it. I had to add the flag > > >> UVC_CTRL_FLAG_AUTO_UPDATE to the uvc_control_info entry for "Exposure > > >> (Auto)" for the new value to be properly reported to userspace. > > > > > > Could you send me the USB trace ? > > > > I've uploaded 3 traces: one with no patch (kernel 3.12) one with my > > "PATCH v2" applied and one with no patch but caching of gets disabled. > > You can get them here: > > > > http://williammanley.net/C920-no-patch.pcapng > > http://williammanley.net/C920-with-patch.pcapng > > http://williammanley.net/C920-no-caching.pcapng > > > > The process to generate them was: > > > > sudo dumpcap -i usbmon1 -w /tmp/C920-no-patch2.pcapng & > > > > # Plug USB cable in here > > > > v4l2-ctl -d > > /dev/v4l/by-id/usb-046d_HD_Pro_Webcam_C920_C833389F-video-index1 > > > > -cexposure_auto=1,exposure_auto_priority=0,exposure_absolute=152,white_balan > > ce_temperature_auto=0 > > > > ./streamon > > > > The relevant output seems to be (here from C920-no-patch.pcapng): > > > 10446 23.095874000 21:14:16.313257000 host -> 32.0 USB 64 > > > SET INTERFACE Request 10447 23.109379000 21:14:16.326762000 32.3 > > > -> host USBVIDEO 70 URB_INTERRUPT in 10448 23.109389000 > > > 21:14:16.326772000 host -> 32.3 USB 64 URB_INTERRUPT in > > > 10449 23.189448000 21:14:16.406831000 32.3 -> host > > > USBVIDEO 73 URB_INTERRUPT in 10450 23.189486000 21:14:16.406869000 > > > host -> 32.3 USB 64 URB_INTERRUPT in 10451 23.243314000 > > > 21:14:16.460697000 32.0 -> host USB 64 SET INTERFACE > > > Response > > Taking a closer look at packet 10449: > > > Frame 10449: 73 bytes on wire (584 bits), 73 bytes captured (584 bits) on > > > interface 0> > > > Interface id: 0 > > > Encapsulation type: USB packets with Linux header and padding (115) > > > Arrival Time: Mar 13, 2014 21:14:16.406831000 GMT > > > [Time shift for this packet: 0.000000000 seconds] > > > Epoch Time: 1394745256.406831000 seconds > > > [Time delta from previous captured frame: 0.080059000 seconds] > > > [Time delta from previous displayed frame: 0.080059000 seconds] > > > [Time since reference or first frame: 23.189448000 seconds] > > > Frame Number: 10449 > > > Frame Length: 73 bytes (584 bits) > > > Capture Length: 73 bytes (584 bits) > > > [Frame is marked: False] > > > [Frame is ignored: False] > > > [Protocols in frame: usb:usbvideo] > > > > > > USB URB > > > > > > URB id: 0xffff88081bf95240 > > > URB type: URB_COMPLETE ('C') > > > URB transfer type: URB_INTERRUPT (0x01) > > > Endpoint: 0x83, Direction: IN > > > > > > 1... .... = Direction: IN (1) > > > .000 0011 = Endpoint value: 3 > > > > > > Device: 32 > > > URB bus id: 1 > > > Device setup request: not relevant ('-') > > > Data: present (0) > > > URB sec: 1394745256 > > > URB usec: 406831 > > > URB status: Success (0) > > > URB length [bytes]: 9 > > > Data length [bytes]: 9 > > > [Request in: 10448] > > > [Time from request: 0.080059000 seconds] > > > [bInterfaceClass: Video (0x0e)] > > > > > > .... 0001 = Status Type: VideoControl Interface (0x01) > > > Originator: 1 > > > Event: Control Change (0x00) > > > Control Selector: Exposure Time (Absolute) (0x04) > > > Change Type: Value (0x00) > > > Current value: 333 (0x0000014d) > > > > This is the notification of the change of the value. As you can see it > > happens between "SET INTERFACE Request" and "SET INTERFACE Response". > > Thank you for investigating this, and sorry for the late reply. > > I still haven't heard back from Logitech on this issue. I've pinged them, > they > might be busy at the moment. Thanks for looking at my patch :). > Given that the device notifies us that the control value changes, one > possibly > more clever fix would be to handle that even and set the old control > value > back when the auto control is disabled. However, that's probably an > overengineered solution. > > I've still been wondering whether the quirk shouldn't restore only the > control(s) that are known to be erroneously changed by the camera instead > of > restoring them all. Feel free to disagree, what's your opinion about that > ? So that was my initial intention, but when I got into it it seemed like it was going to add a whole bunch of additional complexity (and lines of code) for questionable benefit. While uploading all the values is a bit of a sledgehammer it has the advantage that it's simple and dumb and exercises code that's already in use for suspend/resume. OTOH you could argue that a patch which explicitly contains code like: if (strcmp(param.name, "Exposure (Absolute)") == 0) { blah, blah, blah } or similar documents the quirk a little more explicitly. I still didn't think it was worth the extra complexity. I'm quite willing to be convinced otherwise though :). Another more marginal advantage is that the quirk may be more applicable to other hardware. Of course this is entirely theoretical at this point so probably can be discounted. Thanks Will -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Will, On Monday 14 April 2014 11:27:00 Will Manley wrote: > On Mon, 14 Apr 2014, at 1:34, Laurent Pinchart wrote: [snip] > > Thank you for investigating this, and sorry for the late reply. > > > > I still haven't heard back from Logitech on this issue. I've pinged them, > > they might be busy at the moment. > > Thanks for looking at my patch :). You're welcome. > > Given that the device notifies us that the control value changes, one > > possibly more clever fix would be to handle that even and set the old > > control value back when the auto control is disabled. However, that's > > probably an overengineered solution. > > > > I've still been wondering whether the quirk shouldn't restore only the > > control(s) that are known to be erroneously changed by the camera instead > > of restoring them all. Feel free to disagree, what's your opinion about > > that ? > > So that was my initial intention, but when I got into it it seemed like > it was going to add a whole bunch of additional complexity (and lines of > code) for questionable benefit. While uploading all the values is a bit > of a sledgehammer it has the advantage that it's simple and dumb and > exercises code that's already in use for suspend/resume. OTOH you could > argue that a patch which explicitly contains code like: > > if (strcmp(param.name, "Exposure (Absolute)") == 0) { > blah, blah, blah > } > > or similar documents the quirk a little more explicitly. I still didn't > think it was worth the extra complexity. I'm quite willing to be > convinced otherwise though :). > > Another more marginal advantage is that the quirk may be more applicable > to other hardware. Of course this is entirely theoretical at this point > so probably can be discounted. One of the things that bother me with restoring all controls right after starting the stream is that it might actually result in different unexpected behaviors. For instance, this would write the value of all manual controls even when the corresponding auto control is turned on. Some cameras might not be happy, and this could have an adverse effect from temporary glitches in the video to complete crashes. Of course the quirk should not be enabled for cameras that would then crash, so we could consider that it's good as-is for the C920. I'd like to hear from Logitech on this issue before taking a decision. I've pinged them, let's wait one more week if that's fine with you. In the meantime, there's another question that crossed my mind, have you checked whether the camera has a similar buggy behaviour for other auto controls (auto white balance for instance), or if it's limited to auto- exposure ?
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index a2f4501..f72d7eb 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -1795,7 +1795,7 @@ done: * - Handle restore order (Auto-Exposure Mode should be restored before * Exposure Time). */ -int uvc_ctrl_resume_device(struct uvc_device *dev) +int uvc_ctrl_restore_values(struct uvc_device *dev) { struct uvc_control *ctrl; struct uvc_entity *entity; diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c index c3bb250..9f8a87e 100644 --- a/drivers/media/usb/uvc/uvc_driver.c +++ b/drivers/media/usb/uvc/uvc_driver.c @@ -1981,7 +1981,7 @@ static int __uvc_resume(struct usb_interface *intf, int reset) int ret = 0; if (reset) { - ret = uvc_ctrl_resume_device(dev); + ret = uvc_ctrl_restore_values(dev); if (ret < 0) return ret; } diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c index 3394c34..87cd57b 100644 --- a/drivers/media/usb/uvc/uvc_video.c +++ b/drivers/media/usb/uvc/uvc_video.c @@ -1660,6 +1660,11 @@ static int uvc_init_video(struct uvc_streaming *stream, gfp_t gfp_flags) } } + /* The Logitech C920 temporarily forgets that it should not be + adjusting Exposure Absolute during init so restore controls to + stored values. */ + uvc_ctrl_restore_values(stream->dev); + return 0; } diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h index 9e35982..3b365a3 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -676,7 +676,7 @@ extern int uvc_ctrl_add_mapping(struct uvc_video_chain *chain, const struct uvc_control_mapping *mapping); extern int uvc_ctrl_init_device(struct uvc_device *dev); extern void uvc_ctrl_cleanup_device(struct uvc_device *dev); -extern int uvc_ctrl_resume_device(struct uvc_device *dev); +extern int uvc_ctrl_restore_values(struct uvc_device *dev); extern int uvc_ctrl_begin(struct uvc_video_chain *chain); extern int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
The uvcvideo webcam driver exposes the v4l2 control "Exposure (Absolute)" which allows the user to control the exposure time of the webcam, essentially controlling the brightness of the received image. By default the webcam automatically adjusts the exposure time automatically but the if you set the control "Exposure, Auto"="Manual Mode" the user can fix the exposure time. Unfortunately it seems that the Logitech C920 has a firmware bug where it will forget that it's in manual mode temporarily during initialisation. This means that the camera doesn't respect the exposure time that the user requested if they request it before starting to stream video. They end up with a video stream which is either too bright or too dark and must reset the controls after video starts streaming. This patch works around this camera bug by re-uploading the cached controls to the camera immediately after initialising the camera. Signed-off-by: William Manley <will@williammanley.net> --- drivers/media/usb/uvc/uvc_ctrl.c | 2 +- drivers/media/usb/uvc/uvc_driver.c | 2 +- drivers/media/usb/uvc/uvc_video.c | 5 +++++ drivers/media/usb/uvc/uvcvideo.h | 2 +- 4 files changed, 8 insertions(+), 3 deletions(-)