Thanks for the diff Laurie - See my comments inline in your diff. Marcus
On Mon, 13 Jul 2020 17:19:41 +0100 Laurence Tratt <[email protected]> wrote: > This patch adds the ability to control the white balance temperature > on webcams in video(1). This is bound to keys W/w in steps of +/-10K > (because changing the temperature 1K at a time is tortuously slow > given a typical range of 2800-6500K, and +/-10 seems to roughly be > the minimum difference that my eyes can detect). > > At least on my Logitech C920, one can only change the white balance > temperature once auto-white balance is turned off (if it's not off, > the C920 refuses to change anything and the ioctl returns an error). > This patch thus turns auto-white balance on/off when you change/reset > the white balance. This is required by the USB video spec which > doesn't seem too surprising until you realise that the spec also says > that the contrast value can't be changed without auto-contrast being > turned off. However, I can't see any code anywhere which does this: > perhaps auto-contrast is off on some, or all, webcams by default? > > > Laurie > > > Index: video.c > =================================================================== > RCS file: /cvs/xenocara/app/video/video.c,v > retrieving revision 1.30 > diff -u -r1.30 video.c > --- video.c 1 Jul 2020 06:45:24 -0000 1.30 > +++ video.c 13 Jul 2020 16:08:05 -0000 > @@ -114,7 +114,10 @@ > { "gamma", 0, V4L2_CID_GAMMA, 0, 0, 0, 0, 0 }, > #define CTRL_SHARPNESS 6 > { "sharpness", 0, V4L2_CID_SHARPNESS, 0, 0, > 0, 0, 0 }, -#define CTRL_LAST 7 > +#define CTRL_WHITE_BALANCE_TEMPERATURE 7 > + { "white_balance_temperature", I think we should replace '_' with ' ' just to make the output look more aligned when we run video -v. > + 0, V4L2_CID_WHITE_BALANCE_TEMPERATURE, > 0, 0, 0, 0, 0 }, +#define CTRL_LAST 8 > { NULL, 0, 0, 0, 0, 0, 0, 0 } > }; > > @@ -730,6 +733,14 @@ > if (vid->mode & M_IN_DEV) > dev_set_ctrl(vid, > CTRL_SATURATION, -1); break; > + case 'W': > + if (vid->mode & M_IN_DEV) > + dev_set_ctrl(vid, > CTRL_WHITE_BALANCE_TEMPERATURE, 10); > + break; > + case 'w': > + if (vid->mode & M_IN_DEV) > + dev_set_ctrl(vid, > CTRL_WHITE_BALANCE_TEMPERATURE, -10); > + break; style(9); If possible can you please try to break lines >80 chars. > default: > break; > } > @@ -1011,6 +1022,15 @@ > ctrls[ctrl].name, d->path); > return; > } > + /* The spec requires auto-white balance to be off before we > can set the > + * white balance temperature. */ style(9); Can you please format the comments according to "Multi-line" comment? > + if (ctrl == CTRL_WHITE_BALANCE_TEMPERATURE) { > + control.id = V4L2_CID_AUTO_WHITE_BALANCE; > + control.value = 0; > + if (ioctl(d->fd, VIDIOC_S_CTRL, &control) != 0) { > + warn("VIDIOC_S_CTRL(auto_white_balance)"); > + } > + } > val = ctrls[ctrl].cur + ctrls[ctrl].step * change; > if (val > ctrls[ctrl].max) > val = ctrls[ctrl].max; > @@ -1054,6 +1074,15 @@ > if (vid->verbose > 0) > fprintf(stderr, "%s now %d\n", ctrls[i].name, > ctrls[i].cur); > + /* After resetting the temperature, also turn > auto-white > + * balance back on. */ > + if (i == CTRL_WHITE_BALANCE_TEMPERATURE) { > + control.id = V4L2_CID_AUTO_WHITE_BALANCE; > + control.value = 1; > + if (ioctl(d->fd, VIDIOC_S_CTRL, &control) != > 0) { > + > warn("VIDIOC_S_CTRL(auto_white_balance)"); > + } > + } > } > } Here we would need to turn off the auto white balance as well before the reset because otherwise if you press reset without having altered the white balance temperature before, the auto white balance is still turned on resulting in the error you have already described: video: VIDIOC_S_CTRL(white_balance_temperature): Invalid argument Probably we should introduce a new function to toggle the auto white balance on and off instead of calling the same code three times. And the function should check whether the requested toggle is already in place before. Something like that maybe: +dev_set_ctrl_auto_white_balance(struct video *vid, int toggle) +{ + struct dev *d = &vid->dev; + struct v4l2_control control; + + control.id = V4L2_CID_AUTO_WHITE_BALANCE; + if (ioctl(d->fd, VIDIOC_G_CTRL, &control) != 0) + warn("VIDIOC_G_CTRL"); + if (control.value == toggle) + return; + + control.value = toggle; + if (ioctl(d->fd, VIDIOC_S_CTRL, &control) != 0) + warn("VIDIOC_S_CTRL"); +}
