Hi Laurie, See my comments inline.
On Fri, 17 Jul 2020 22:51:30 +0100 Laurence Tratt <[email protected]> wrote: > On Mon, Jul 13, 2020 at 07:39:41PM +0100, Laurence Tratt wrote: > > > video(1) allows users to adjust controls such as brightness, > > saturation (etc.) depending on the input device in question. These > > values persist even after video(1) has quit, allowing you to e.g. > > increase the brightness of a webcam before connecting to a video > > call. However, the only way to adjust values is to hold down keys > > in the GUI, which is slow, error prone, and can't easily be > > scripted. > > > > This patch adds a "-c" option to video(1) which either takes the > > special value "reset" or a "control=value" pair. For example: > > > > $ video -c reset > > > > resets all the controls to their default values. Assuming the input > > device in question supports brightness one can set that as follows: > > > > $ video -c brightness=200 > > > > Note that the available controls, and their min/max values, will > > vary from device to device. > > > > To keep the patch simple, only one "-c" option can be passed to > > video(1) at a time. Note that passing this option causes video(1) > > to quit before displaying video (in identical fashion to "-q") > > which makes it useful for scripting purposes. > > The attached patch reworks things a bit. First, it now works with > white_balance_temperature, which (at least on my C920) requires > mmap_init to be called first. Second, the previous patch sometimes > set controls to surprising values because it called what is (in > effect) a "change control by X" function. This patch now renames the > "old" function to "dev_inc_ctrl" and introduces a new > "dev_set_ctrl_abs". This then provides an obvious opportunity to > simplify the reset function. > > With this patch I can do things like: > > $ video -c white_balance_temperature=6500 > $ video -c brightness=200 > $ video && video -c reset && video > > and see changes being made as appropriate. > > > Laurie > > > > Index: video.1 > =================================================================== > RCS file: /cvs/xenocara/app/video/video.1,v > retrieving revision 1.15 > diff -u -r1.15 video.1 > --- video.1 17 Jul 2020 07:51:23 -0000 1.15 > +++ video.1 17 Jul 2020 21:44:49 -0000 > @@ -27,6 +27,7 @@ > .Bk -words > .Op Fl \&gqRv > .Op Fl a Ar adaptor > +.Op Fl c Ar reset | control=value > .Op Fl e Ar encoding > .Op Fl f Ar file > .Op Fl i Ar input > @@ -81,6 +82,15 @@ > adaptor to use. > The default is 0, the first adaptor reported by > .Xr X 7 . > +.It Fl c Ar reset | control=value > +Set control value (e.g. brightness) and exit. The special name > +.Ql reset > +resets all values to their default. The available controls can be > found +with > +.Fl q > +and the default values with > +.Fl c Ar reset > +.Fl v . > .It Fl e Ar encoding > Lowercase FOURCC name of video encoding to use. > Valid arguments are > Index: video.c > =================================================================== > RCS file: /cvs/xenocara/app/video/video.c,v > retrieving revision 1.31 > diff -u -r1.31 video.c > --- video.c 17 Jul 2020 07:51:23 -0000 1.31 > +++ video.c 17 Jul 2020 21:44:49 -0000 > @@ -192,7 +192,10 @@ > #define M_IN_FILE 0x4 > #define M_OUT_FILE 0x8 > #define M_QUERY 0x10 > +#define M_RESET 0x20 > +#define M_SET_CTRL 0x40 > int mode; > + char *set_ctrl_str; > int verbose; > }; > > @@ -212,10 +215,12 @@ > void dev_dump_info(struct video *); > void dev_dump_query(struct video *); > int dev_init(struct video *); > -void dev_set_ctrl(struct video *, int, int); > +void dev_inc_ctrl(struct video *, int, int); > +void dev_set_ctrl_abs(struct video *vid, int, int); I'm a bit confused by the dev_set_ctrl() function renaming. Does 'inc' stand for increment? And if yes, it makes not a lot sense to me since we use this function to increase and decrease values. Leaving the dev_set_ctrl() as is and introduce dev_set_ctrl_abs() would make more sense to me then. Or rename the dev_inc_ctrl() to dev_chg_ctrl() instead. > void dev_set_ctrl_auto_white_balance(struct video *, int); > void dev_reset_ctrls(struct video *); > > +int parse_ctrl(struct video *, int *, int *); > int parse_size(struct video *); > int choose_size(struct video *); > int choose_enc(struct video *); > @@ -241,9 +246,9 @@ > usage(void) > { > fprintf(stderr, "usage: %s [-gqRv] " > - "[-a adaptor] [-e encoding] [-f file] [-i input] [-O > output]\n" > - " %*s [-o output] [-r rate] [-s size]\n", > __progname, > - (int)strlen(__progname), ""); > + "[-a adaptor] [-c reset|control=value] [-e encoding] [-f > file]\n" > + " %*s [-i input] [-O output] [-o output] [-r rate] > [-s size]\n", > + __progname, (int)strlen(__progname), ""); > } > > int > @@ -657,46 +662,46 @@ > switch (str) { > case 'A': > if (vid->mode & M_IN_DEV) > - dev_set_ctrl(vid, > CTRL_SHARPNESS, 1); > + dev_inc_ctrl(vid, > CTRL_SHARPNESS, 1); break; > case 'a': > if (vid->mode & M_IN_DEV) > - dev_set_ctrl(vid, > CTRL_SHARPNESS, -1); > + dev_inc_ctrl(vid, > CTRL_SHARPNESS, -1); break; > case 'B': > if (vid->mode & M_IN_DEV) > - dev_set_ctrl(vid, > CTRL_BRIGHTNESS, 1); > + dev_inc_ctrl(vid, > CTRL_BRIGHTNESS, 1); break; > case 'b': > if (vid->mode & M_IN_DEV) > - dev_set_ctrl(vid, > CTRL_BRIGHTNESS, -1); > + dev_inc_ctrl(vid, > CTRL_BRIGHTNESS, -1); break; > case 'C': > if (vid->mode & M_IN_DEV) > - dev_set_ctrl(vid, > CTRL_CONTRAST, 1); > + dev_inc_ctrl(vid, > CTRL_CONTRAST, 1); break; > case 'c': > if (vid->mode & M_IN_DEV) > - dev_set_ctrl(vid, > CTRL_CONTRAST, -1); > + dev_inc_ctrl(vid, > CTRL_CONTRAST, -1); break; > case 'f': > resize_window(vid, 1); > break; > case 'G': > if (vid->mode & M_IN_DEV) > - dev_set_ctrl(vid, CTRL_GAIN, > 1); > + dev_inc_ctrl(vid, CTRL_GAIN, > 1); break; > case 'g': > if (vid->mode & M_IN_DEV) > - dev_set_ctrl(vid, CTRL_GAIN, > -1); > + dev_inc_ctrl(vid, CTRL_GAIN, > -1); break; > case 'H': > if (vid->mode & M_IN_DEV) > - dev_set_ctrl(vid, CTRL_HUE, > 1); > + dev_inc_ctrl(vid, CTRL_HUE, > 1); break; > case 'h': > if (vid->mode & M_IN_DEV) > - dev_set_ctrl(vid, CTRL_HUE, > -1); > + dev_inc_ctrl(vid, CTRL_HUE, > -1); break; > case 'O': > if (!wout && vid->verbose > 0) > @@ -710,11 +715,11 @@ > break; > case 'M': > if (vid->mode & M_IN_DEV) > - dev_set_ctrl(vid, > CTRL_GAMMA, 1); > + dev_inc_ctrl(vid, > CTRL_GAMMA, 1); break; > case 'm': > if (vid->mode & M_IN_DEV) > - dev_set_ctrl(vid, > CTRL_GAMMA, -1); > + dev_inc_ctrl(vid, > CTRL_GAMMA, -1); break; > case 'p': > hold = !hold; > @@ -728,20 +733,20 @@ > break; > case 'S': > if (vid->mode & M_IN_DEV) > - dev_set_ctrl(vid, > CTRL_SATURATION, 1); > + dev_inc_ctrl(vid, > CTRL_SATURATION, 1); break; > case 's': > if (vid->mode & M_IN_DEV) > - dev_set_ctrl(vid, > CTRL_SATURATION, -1); > + dev_inc_ctrl(vid, > CTRL_SATURATION, -1); break; > case 'W': > if (vid->mode & M_IN_DEV) > - dev_set_ctrl(vid, > + dev_inc_ctrl(vid, > CTRL_WHITE_BALANCE_TEMPERATURE, > 10); break; > case 'w': > if (vid->mode & M_IN_DEV) > - dev_set_ctrl(vid, > + dev_inc_ctrl(vid, > CTRL_WHITE_BALANCE_TEMPERATURE, > -10); break; > default: > @@ -1010,10 +1015,9 @@ > } > > void > -dev_set_ctrl(struct video *vid, int ctrl, int change) > +dev_inc_ctrl(struct video *vid, int ctrl, int change) > { > struct dev *d = &vid->dev; > - struct v4l2_control control; > int val; > > if (ctrl < 0 || ctrl >= CTRL_LAST) { > @@ -1025,6 +1029,25 @@ > ctrls[ctrl].name, d->path); > return; > } > + val = ctrls[ctrl].cur + ctrls[ctrl].step * change; > + dev_set_ctrl_abs(vid, ctrl, val); > +} > + > +void > +dev_set_ctrl_abs(struct video *vid, int ctrl, int val) > +{ > + struct dev *d = &vid->dev; > + struct v4l2_control control; > + > + if (ctrl < 0 || ctrl >= CTRL_LAST) { > + warnx("invalid control"); > + return; > + } > + if (!ctrls[ctrl].supported) { > + warnx("control %s not supported by %s", > + ctrls[ctrl].name, d->path); > + return; > + } > if (ctrl == CTRL_WHITE_BALANCE_TEMPERATURE) { > /* > * The spec requires auto-white balance to be off > before @@ -1032,7 +1055,6 @@ > */ > dev_set_ctrl_auto_white_balance(vid, 0); > } > - val = ctrls[ctrl].cur + ctrls[ctrl].step * change; > if (val > ctrls[ctrl].max) > val = ctrls[ctrl].max; > else if (val < ctrls[ctrl].min) > @@ -1081,25 +1103,7 @@ > for (i = 0; i < CTRL_LAST; i++) { > if (!ctrls[i].supported) > continue; > - if (i == CTRL_WHITE_BALANCE_TEMPERATURE) { > - /* > - * We might be asked to reset before the > white balance > - * temperature has been adjusted, so we need > to make > - * sure that auto-white balance really is > off. > - */ > - dev_set_ctrl_auto_white_balance(vid, 0); > - } > - control.id = ctrls[i].id; > - control.value = ctrls[i].def; > - if (ioctl(d->fd, VIDIOC_S_CTRL, &control) != 0) > - warn("VIDIOC_S_CTRL(%s)", ctrls[i].name); > - control.id = ctrls[i].id; > - if (ioctl(d->fd, VIDIOC_G_CTRL, &control) != 0) > - warn("VIDIOC_G_CTRL(%s)", ctrls[i].name); > - ctrls[i].cur = control.value; > - if (vid->verbose > 0) > - fprintf(stderr, "%s now %d\n", ctrls[i].name, > - ctrls[i].cur); > + dev_set_ctrl_abs(vid, i, ctrls[i].def); > if (i == CTRL_WHITE_BALANCE_TEMPERATURE) { > dev_set_ctrl_auto_white_balance(vid, 1); > } In general (there are some of this cases in your diff), can you please remove the braces after a control statement if there is only a single, unwrapped line? > @@ -1221,6 +1225,39 @@ > return 1; > } > > + > +int > +parse_ctrl(struct video *vid, int *ctrl_id, int *ctrl_val) > +{ > + char *valp; > + const char *errstr; > + > + if (!vid->set_ctrl_str) { > + return 0; > + } > + > + valp = strsep(&vid->set_ctrl_str, "="); > + if (*valp == '\0' || vid->set_ctrl_str == '\0') { > + return 0; > + } > + for (*ctrl_id = 0; *ctrl_id < CTRL_LAST; (*ctrl_id)++) { > + if (strcmp(valp, ctrls[*ctrl_id].name) == 0) { > + break; > + } > + } > + if (*ctrl_id == CTRL_LAST) { > + warnx("Unknown control '%s'", valp); > + return 0; > + } > + *ctrl_val = strtonum(vid->set_ctrl_str, ctrls[*ctrl_id].min, > + ctrls[*ctrl_id].max, &errstr); > + if (errstr != NULL) { > + warnx("control value '%s' is %s", valp, errstr); > + return 0; > + } > + return 1; > +} > + > int > parse_size(struct video *vid) > { > @@ -1481,6 +1518,8 @@ > int > setup(struct video *vid) > { > + int ctrl_id, ctrl_val; > + > if (vid->mode & M_IN_FILE) { > if (!strcmp(vid->iofile, "-")) > vid->iofile_fd = STDIN_FILENO; > @@ -1520,6 +1559,7 @@ > if (!parse_size(vid) || !choose_size(vid)) > return 0; > > + > vid->bpf = vid->width * vid->height * encs[vid->enc].bpp / > NBBY; > if (vid->verbose > 0) { > @@ -1553,17 +1593,30 @@ > if ((vid->mode & M_IN_DEV) && !dev_init(vid)) > return 0; > > + if (vid->mmap_on) { > + if (!mmap_init(vid)) > + return 0; > + } Ouch! It really shouldn't be required to initialize the mmap buffers and turn on the cams stream thereby each time you set a control parameter. Think of a larger script which sets multiple control parameters, turning on and off the stream each time, and there are cams which are slow in turning on their streams. With my C930e setting the controls works without turning on the stream first, and that's what I would expect. Can you figure out why this is required for your C920? There must be another solution to set controls without turning on the stream first ... > + > + if (vid->mode & M_SET_CTRL) { > + if (!parse_ctrl(vid, &ctrl_id, &ctrl_val)) { > + return 0; > + } > + dev_set_ctrl_abs(vid, ctrl_id, ctrl_val); > + return 1; > + } > + > + if (vid->mode & M_RESET) { > + dev_reset_ctrls(vid); > + return 1; > + } > + > if ((vid->mode & M_OUT_XV) && !xv_init(vid)) > return 0; > > if (vid->sz_str && !strcmp(vid->sz_str, "full")) > resize_window(vid, 1); > > - if (vid->mmap_on) { > - if (!mmap_init(vid)) > - return 0; > - } > - > if (vid->mode & M_OUT_XV) > net_wm_supported(vid); > > @@ -1949,7 +2002,7 @@ > vid.mmap_on = 1; /* mmap method is default */ > wout = 1; > > - while ((ch = getopt(argc, argv, "gqRva:e:f:i:O:o:r:s:")) != > -1) { > + while ((ch = getopt(argc, argv, "gqRva:c:e:f:i:O:o:r:s:")) > != -1) { switch (ch) { > case 'a': > x->cur_adap = strtonum(optarg, 0, 4, > &errstr); @@ -1958,6 +2011,17 @@ > errs++; > } > break; > + case 'c': > + if ((vid.mode & M_RESET) || (vid.mode & > M_SET_CTRL)) { > + warnx("Only one '-c' option > allowed."); > + errs++; > + } else if (strcmp(optarg, "reset") == 0) { > + vid.mode |= M_RESET; > + } else { > + vid.mode |= M_SET_CTRL; > + vid.set_ctrl_str = strdup(optarg); > + } > + break; > case 'e': > vid.enc = find_enc(optarg); > if (vid.enc >= ENC_LAST) { > @@ -2055,6 +2119,10 @@ > > if (!setup(&vid)) > cleanup(&vid, 1); > + > + if ((vid.mode & M_RESET) || (vid.mode & M_SET_CTRL)) { > + cleanup(&vid, 0); > + } > > if (vid.mode & M_IN_FILE) { > if (pledge("stdio rpath", NULL) == -1) > Thanks, Marcus
