Oh, and lastly I forget, we would need a man page update as well listing the new knob :-)
On Wed, 15 Jul 2020 20:33:54 +0200 Marcus Glocker <[email protected]> wrote: > 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"); > +} >
