Add ability to set control values with video(1)

classic Classic list List threaded Threaded
23 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Add ability to set control values with video(1)

Laurence Tratt
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.


Laurie


Index: video.1
===================================================================
RCS file: /cvs/xenocara/app/video/video.1,v
retrieving revision 1.14
diff -u -r1.14 video.1
--- video.1 25 Feb 2019 12:34:35 -0000 1.14
+++ video.1 13 Jul 2020 18:33:51 -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.30
diff -u -r1.30 video.c
--- video.c 1 Jul 2020 06:45:24 -0000 1.30
+++ video.c 13 Jul 2020 18:33:51 -0000
@@ -189,7 +189,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,6 +215,7 @@
 void dev_set_ctrl(struct video *, int, 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 *);
@@ -237,9 +241,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
@@ -1172,6 +1176,38 @@
  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)
 {
@@ -1432,6 +1468,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;
@@ -1471,6 +1509,19 @@
  if (!parse_size(vid) || !choose_size(vid))
  return 0;
 
+ if (vid->mode & M_RESET) {
+ dev_reset_ctrls(vid);
+ return 1;
+ }
+
+ if (vid->mode & M_SET_CTRL) {
+ if (!parse_ctrl(vid, &ctrl_id, &ctrl_val)) {
+ return 0;
+ }
+ dev_set_ctrl(vid, ctrl_id, ctrl_val);
+ return 1;
+ }
+
  vid->bpf = vid->width * vid->height * encs[vid->enc].bpp / NBBY;
 
  if (vid->verbose > 0) {
@@ -1900,7 +1951,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);
@@ -1909,6 +1960,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) {
@@ -2006,6 +2068,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)

Reply | Threaded
Open this post in threaded view
|

Re: Add ability to set control values with video(1)

Laurence Tratt
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);
 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);
  }
@@ -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;
+ }
+
+ 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)

Reply | Threaded
Open this post in threaded view
|

Re: Add ability to set control values with video(1)

Marcus Glocker
Hi Laurie,

See my comments inline.

On Fri, 17 Jul 2020 22:51:30 +0100
Laurence Tratt <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Add ability to set control values with video(1)

Laurence Tratt
On Tue, Jul 21, 2020 at 09:01:26PM +0200, Marcus Glocker wrote:

Hello Marcus,

Thanks for the comments! Again, I agree with all of them with a couple of
comments:

> 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.

I think it needs renaming because I managed to misread "dev_set_ctrl" as
"set absolute value" at first (hence the subtly incorrect first patch I sent
out). Finding good names is hard, but you're right that "inc" is confusing
here. How about dev_set_ctrl_rel, which I think gets the intent across
clearly, and makes the relation to dev_set_ctrl_abs clear?

> 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 ...

On my C920 I only need to do this to turn auto white_balance_temperature
back on. I tested this with:

  $ video -c white_balance_temperature=6500
  $ video
  $ video -c reset
  $ video

If pressing "r" in the final "video" call doesn't visibly change anything,
then (assuming auto white balance doesn't set things to 6500K!) I conclude
that resetting has worked. I wonder if it's the same for your C930e and
auto white_balance_temperature or not?

Unfortunately when I try messing with mmap_init, it seems that to turn on
auto white_balance_temperature, the final ioctl ("start video stream") has
to be executed. For any other control, we don't need to call mmap_init.

I agree that this is less than ideal, but I don't know if we can do anything
about it (other than, perhaps, only calling mmap_init for "-c reset", but
that feels rather fragile).


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 21 Jul 2020 20:15:01 -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 21 Jul 2020 20:15:01 -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_set_ctrl_abs(struct video *vid, int, int);
+void dev_set_ctrl_rel(struct video *, int, int);
 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_set_ctrl_rel(vid, CTRL_SHARPNESS, 1);
  break;
  case 'a':
  if (vid->mode & M_IN_DEV)
- dev_set_ctrl(vid, CTRL_SHARPNESS, -1);
+ dev_set_ctrl_rel(vid, CTRL_SHARPNESS, -1);
  break;
  case 'B':
  if (vid->mode & M_IN_DEV)
- dev_set_ctrl(vid, CTRL_BRIGHTNESS, 1);
+ dev_set_ctrl_rel(vid, CTRL_BRIGHTNESS, 1);
  break;
  case 'b':
  if (vid->mode & M_IN_DEV)
- dev_set_ctrl(vid, CTRL_BRIGHTNESS, -1);
+ dev_set_ctrl_rel(vid, CTRL_BRIGHTNESS, -1);
  break;
  case 'C':
  if (vid->mode & M_IN_DEV)
- dev_set_ctrl(vid, CTRL_CONTRAST, 1);
+ dev_set_ctrl_rel(vid, CTRL_CONTRAST, 1);
  break;
  case 'c':
  if (vid->mode & M_IN_DEV)
- dev_set_ctrl(vid, CTRL_CONTRAST, -1);
+ dev_set_ctrl_rel(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_set_ctrl_rel(vid, CTRL_GAIN, 1);
  break;
  case 'g':
  if (vid->mode & M_IN_DEV)
- dev_set_ctrl(vid, CTRL_GAIN, -1);
+ dev_set_ctrl_rel(vid, CTRL_GAIN, -1);
  break;
  case 'H':
  if (vid->mode & M_IN_DEV)
- dev_set_ctrl(vid, CTRL_HUE, 1);
+ dev_set_ctrl_rel(vid, CTRL_HUE, 1);
  break;
  case 'h':
  if (vid->mode & M_IN_DEV)
- dev_set_ctrl(vid, CTRL_HUE, -1);
+ dev_set_ctrl_rel(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_set_ctrl_rel(vid, CTRL_GAMMA, 1);
  break;
  case 'm':
  if (vid->mode & M_IN_DEV)
- dev_set_ctrl(vid, CTRL_GAMMA, -1);
+ dev_set_ctrl_rel(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_set_ctrl_rel(vid, CTRL_SATURATION, 1);
  break;
  case 's':
  if (vid->mode & M_IN_DEV)
- dev_set_ctrl(vid, CTRL_SATURATION, -1);
+ dev_set_ctrl_rel(vid, CTRL_SATURATION, -1);
  break;
  case 'W':
  if (vid->mode & M_IN_DEV)
- dev_set_ctrl(vid,
+ dev_set_ctrl_rel(vid,
     CTRL_WHITE_BALANCE_TEMPERATURE, 10);
  break;
  case 'w':
  if (vid->mode & M_IN_DEV)
- dev_set_ctrl(vid,
+ dev_set_ctrl_rel(vid,
     CTRL_WHITE_BALANCE_TEMPERATURE, -10);
  break;
  default:
@@ -1010,11 +1015,10 @@
 }
 
 void
-dev_set_ctrl(struct video *vid, int ctrl, int change)
+dev_set_ctrl_abs(struct video *vid, int ctrl, int val)
 {
  struct dev *d = &vid->dev;
  struct v4l2_control control;
- int val;
 
  if (ctrl < 0 || ctrl >= CTRL_LAST) {
  warnx("invalid control");
@@ -1032,7 +1036,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)
@@ -1055,6 +1058,25 @@
 }
 
 void
+dev_set_ctrl_rel(struct video *vid, int ctrl, int change)
+{
+ struct dev *d = &vid->dev;
+ int val;
+
+ 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;
+ }
+ val = ctrls[ctrl].cur + ctrls[ctrl].step * change;
+ dev_set_ctrl_abs(vid, ctrl, val);
+}
+
+void
 dev_set_ctrl_auto_white_balance(struct video *vid, int toggle)
 {
  struct dev *d = &vid->dev;
@@ -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);
  }
@@ -1221,6 +1225,36 @@
  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 +1515,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;
@@ -1553,17 +1589,29 @@
  if ((vid->mode & M_IN_DEV) && !dev_init(vid))
  return 0;
 
+ if (vid->mmap_on) {
+ if (!mmap_init(vid))
+ return 0;
+ }
+
+ 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 +1997,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 +2006,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 +2114,9 @@
 
  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)

Reply | Threaded
Open this post in threaded view
|

Re: Add ability to set control values with video(1)

Marcus Glocker
Hello Laurie,

On Tue, 21 Jul 2020 21:18:15 +0100
Laurence Tratt <[hidden email]> wrote:

> On Tue, Jul 21, 2020 at 09:01:26PM +0200, Marcus Glocker wrote:
>
> Hello Marcus,
>
> Thanks for the comments! Again, I agree with all of them with a
> couple of comments:
>
> > 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.  
>
> I think it needs renaming because I managed to misread "dev_set_ctrl"
> as "set absolute value" at first (hence the subtly incorrect first
> patch I sent out). Finding good names is hard, but you're right that
> "inc" is confusing here. How about dev_set_ctrl_rel, which I think
> gets the intent across clearly, and makes the relation to
> dev_set_ctrl_abs clear?

That makes much more sense to me.

> > 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 ...  
>
> On my C920 I only need to do this to turn auto
> white_balance_temperature back on. I tested this with:
>
>   $ video -c white_balance_temperature=6500
>   $ video
>   $ video -c reset
>   $ video
>
> If pressing "r" in the final "video" call doesn't visibly change
> anything, then (assuming auto white balance doesn't set things to
> 6500K!) I conclude that resetting has worked. I wonder if it's the
> same for your C930e and auto white_balance_temperature or not?
>
> Unfortunately when I try messing with mmap_init, it seems that to
> turn on auto white_balance_temperature, the final ioctl ("start video
> stream") has to be executed. For any other control, we don't need to
> call mmap_init.
>
> I agree that this is less than ideal, but I don't know if we can do
> anything about it (other than, perhaps, only calling mmap_init for
> "-c reset", but that feels rather fragile).

Ah right, I got you wrong then, sorry.  I thought you require this for
each control setting.

I've tested this here as well in the meantime by leaving mmap_init() on
its original location (doesn't get involved for '-c reset') with three
different cams:

* Logitech C930e: I see the same problem like you do with your C920
  finally.
* Logitech QuickCam Pro 9000: reset works fine.
* SunplusIT Inc Integrated Camera: reset works fine.

This seems to be a problem only with some cams when turning the
auto white balance temperature back on while the stream is off, the
setting doesn't get recognized by the cam afterwards.

I'm basically OK with your last diff, except the mmap_init() location
change:  I don't like to turn the cam stream on only for setting this
parameter because some cams can't handle the obvious.

I tried out some things with the C930e to get the auto white balance
temperature back on without having the stream on, but no luck so far.

I would aim to get your diff in without the mmap_init() change.  Maybe
we'll find a solution/workaround for this partial problem later?

One more inline comment regarding the man page.

>
> Laurie

Thanks,
Marcus

>
> 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 21 Jul 2020 20:15:01 -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 .

'-c reset -v' will not only display the default values, but also do
reset the cam to them.  Shouldn't the sentence be more something like
the following since this sounds like '-c reset -v' can only display the
default values?

"The available controls can be found with -q and the default values
are displayed during a reset with -c reset -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 21 Jul 2020 20:15:01 -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_set_ctrl_abs(struct video *vid, int, int);
> +void dev_set_ctrl_rel(struct video *, int, int);
>  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_set_ctrl_rel(vid,
> CTRL_SHARPNESS, 1); break;
>   case 'a':
>   if (vid->mode & M_IN_DEV)
> - dev_set_ctrl(vid,
> CTRL_SHARPNESS, -1);
> + dev_set_ctrl_rel(vid,
> CTRL_SHARPNESS, -1); break;
>   case 'B':
>   if (vid->mode & M_IN_DEV)
> - dev_set_ctrl(vid,
> CTRL_BRIGHTNESS, 1);
> + dev_set_ctrl_rel(vid,
> CTRL_BRIGHTNESS, 1); break;
>   case 'b':
>   if (vid->mode & M_IN_DEV)
> - dev_set_ctrl(vid,
> CTRL_BRIGHTNESS, -1);
> + dev_set_ctrl_rel(vid,
> CTRL_BRIGHTNESS, -1); break;
>   case 'C':
>   if (vid->mode & M_IN_DEV)
> - dev_set_ctrl(vid,
> CTRL_CONTRAST, 1);
> + dev_set_ctrl_rel(vid,
> CTRL_CONTRAST, 1); break;
>   case 'c':
>   if (vid->mode & M_IN_DEV)
> - dev_set_ctrl(vid,
> CTRL_CONTRAST, -1);
> + dev_set_ctrl_rel(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_set_ctrl_rel(vid,
> CTRL_GAIN, 1); break;
>   case 'g':
>   if (vid->mode & M_IN_DEV)
> - dev_set_ctrl(vid, CTRL_GAIN,
> -1);
> + dev_set_ctrl_rel(vid,
> CTRL_GAIN, -1); break;
>   case 'H':
>   if (vid->mode & M_IN_DEV)
> - dev_set_ctrl(vid, CTRL_HUE,
> 1);
> + dev_set_ctrl_rel(vid,
> CTRL_HUE, 1); break;
>   case 'h':
>   if (vid->mode & M_IN_DEV)
> - dev_set_ctrl(vid, CTRL_HUE,
> -1);
> + dev_set_ctrl_rel(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_set_ctrl_rel(vid,
> CTRL_GAMMA, 1); break;
>   case 'm':
>   if (vid->mode & M_IN_DEV)
> - dev_set_ctrl(vid,
> CTRL_GAMMA, -1);
> + dev_set_ctrl_rel(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_set_ctrl_rel(vid,
> CTRL_SATURATION, 1); break;
>   case 's':
>   if (vid->mode & M_IN_DEV)
> - dev_set_ctrl(vid,
> CTRL_SATURATION, -1);
> + dev_set_ctrl_rel(vid,
> CTRL_SATURATION, -1); break;
>   case 'W':
>   if (vid->mode & M_IN_DEV)
> - dev_set_ctrl(vid,
> + dev_set_ctrl_rel(vid,
>      CTRL_WHITE_BALANCE_TEMPERATURE,
> 10); break;
>   case 'w':
>   if (vid->mode & M_IN_DEV)
> - dev_set_ctrl(vid,
> + dev_set_ctrl_rel(vid,
>      CTRL_WHITE_BALANCE_TEMPERATURE,
> -10); break;
>   default:
> @@ -1010,11 +1015,10 @@
>  }
>  
>  void
> -dev_set_ctrl(struct video *vid, int ctrl, int change)
> +dev_set_ctrl_abs(struct video *vid, int ctrl, int val)
>  {
>   struct dev *d = &vid->dev;
>   struct v4l2_control control;
> - int val;
>  
>   if (ctrl < 0 || ctrl >= CTRL_LAST) {
>   warnx("invalid control");
> @@ -1032,7 +1036,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)
> @@ -1055,6 +1058,25 @@
>  }
>  
>  void
> +dev_set_ctrl_rel(struct video *vid, int ctrl, int change)
> +{
> + struct dev *d = &vid->dev;
> + int val;
> +
> + 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;
> + }
> + val = ctrls[ctrl].cur + ctrls[ctrl].step * change;
> + dev_set_ctrl_abs(vid, ctrl, val);
> +}
> +
> +void
>  dev_set_ctrl_auto_white_balance(struct video *vid, int toggle)
>  {
>   struct dev *d = &vid->dev;
> @@ -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);
>   }
> @@ -1221,6 +1225,36 @@
>   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 +1515,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;
> @@ -1553,17 +1589,29 @@
>   if ((vid->mode & M_IN_DEV) && !dev_init(vid))
>   return 0;
>  
> + if (vid->mmap_on) {
> + if (!mmap_init(vid))
> + return 0;
> + }
> +
> + 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 +1997,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 +2006,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 +2114,9 @@
>  
>   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)
>

Reply | Threaded
Open this post in threaded view
|

Re: Add ability to set control values with video(1)

Laurence Tratt
On Wed, Jul 22, 2020 at 10:23:19PM +0200, Marcus Glocker wrote:

Hello Marcus,

> I've tested this here as well in the meantime by leaving mmap_init() on
> its original location (doesn't get involved for '-c reset') with three
> different cams:
>
> * Logitech C930e: I see the same problem like you do with your C920
>   finally.
> * Logitech QuickCam Pro 9000: reset works fine.
> * SunplusIT Inc Integrated Camera: reset works fine.

Hmph, that's slightly annoying of Logitech :/

> This seems to be a problem only with some cams when turning the
> auto white balance temperature back on while the stream is off, the
> setting doesn't get recognized by the cam afterwards.
>
> I'm basically OK with your last diff, except the mmap_init() location
> change:  I don't like to turn the cam stream on only for setting this
> parameter because some cams can't handle the obvious.
>
> I tried out some things with the C930e to get the auto white balance
> temperature back on without having the stream on, but no luck so far.
>
> I would aim to get your diff in without the mmap_init() change.  Maybe
> we'll find a solution/workaround for this partial problem later?

If we can find a change that allows this, I'd be happy! When I briefly
tested things on Windows, the Logitech app there activated the stream before
changing settings, so it's quite possible that they never tested changing
some settings with the stream off. v4l2-ctl might have some clues in it, but
their model is subtly different than ours and forces the user to understand
a lot more about their camera (e.g. they force the user to turn off auto
white balance before they allow them to set manual white balance, a
differentiation which IMHO only makes sense if you've read the UVC spec).

That makes me think that if/when uvideo is extended to deal with terminal
control requests (e.g. zoom, exposure, focus) we'll find that other settings
with "auto" options have similar problems. Honestly, if the price of
controlling exposure and focus is having to turn the camera stream on, I
think I will consider that an acceptable trade-off, given how useful those
settings are :)

>> +.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 .
> '-c reset -v' will not only display the default values, but also do
> reset the cam to them.  Shouldn't the sentence be more something like
> the following since this sounds like '-c reset -v' can only display the
> default values?
>
> "The available controls can be found with -q and the default values
> are displayed during a reset with -c reset -v."

Works for me!


Laurie

Reply | Threaded
Open this post in threaded view
|

Re: Add ability to set control values with video(1)

Marcus Glocker
On Wed, 22 Jul 2020 21:52:27 +0100
Laurence Tratt <[hidden email]> wrote:

> On Wed, Jul 22, 2020 at 10:23:19PM +0200, Marcus Glocker wrote:
>
> Hello Marcus,
>
> > I've tested this here as well in the meantime by leaving
> > mmap_init() on its original location (doesn't get involved for '-c
> > reset') with three different cams:
> >
> > * Logitech C930e: I see the same problem like you do with your C920
> >   finally.
> > * Logitech QuickCam Pro 9000: reset works fine.
> > * SunplusIT Inc Integrated Camera: reset works fine.  
>
> Hmph, that's slightly annoying of Logitech :/
>
> > This seems to be a problem only with some cams when turning the
> > auto white balance temperature back on while the stream is off, the
> > setting doesn't get recognized by the cam afterwards.
> >
> > I'm basically OK with your last diff, except the mmap_init()
> > location change:  I don't like to turn the cam stream on only for
> > setting this parameter because some cams can't handle the obvious.
> >
> > I tried out some things with the C930e to get the auto white balance
> > temperature back on without having the stream on, but no luck so
> > far.
> >
> > I would aim to get your diff in without the mmap_init() change.
> > Maybe we'll find a solution/workaround for this partial problem
> > later?  
>
> If we can find a change that allows this, I'd be happy! When I briefly
> tested things on Windows, the Logitech app there activated the stream
> before changing settings, so it's quite possible that they never
> tested changing some settings with the stream off. v4l2-ctl might
> have some clues in it, but their model is subtly different than ours
> and forces the user to understand a lot more about their camera (e.g.
> they force the user to turn off auto white balance before they allow
> them to set manual white balance, a differentiation which IMHO only
> makes sense if you've read the UVC spec).
>
> That makes me think that if/when uvideo is extended to deal with
> terminal control requests (e.g. zoom, exposure, focus) we'll find
> that other settings with "auto" options have similar problems.
> Honestly, if the price of controlling exposure and focus is having to
> turn the camera stream on, I think I will consider that an acceptable
> trade-off, given how useful those settings are :)

How's that?

We read the current value of the white balance temperature auto control
(since this gets set correctly during the reset), and just reset it
again on the next cam start up after the video stream has been started?
By that we avoid the video stream on/off dance when using '-c'.

I've quickly tested this here and it seems to work for me ...


Index: video.1
===================================================================
RCS file: /cvs/xenocara/app/video/video.1,v
retrieving revision 1.15
diff -u -p -u -p -r1.15 video.1
--- video.1 17 Jul 2020 07:51:23 -0000 1.15
+++ video.1 23 Jul 2020 19:44:21 -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 @@ Index of
 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 are displayed during a reset 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 -p -u -p -r1.31 video.c
--- video.c 17 Jul 2020 07:51:23 -0000 1.31
+++ video.c 23 Jul 2020 19:44:22 -0000
@@ -192,7 +192,10 @@ struct video {
 #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 @@ int dev_get_ctrls(struct video *);
 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_set_ctrl_auto_white_balance(struct video *, int);
+void dev_set_ctrl_abs(struct video *vid, int, int);
+void dev_set_ctrl_rel(struct video *, int, int);
+void dev_set_ctrl_auto_white_balance(struct video *, int, 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 @@ void
 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 @@ display_event(struct video *vid)
  switch (str) {
  case 'A':
  if (vid->mode & M_IN_DEV)
- dev_set_ctrl(vid, CTRL_SHARPNESS, 1);
+ dev_set_ctrl_rel(vid, CTRL_SHARPNESS, 1);
  break;
  case 'a':
  if (vid->mode & M_IN_DEV)
- dev_set_ctrl(vid, CTRL_SHARPNESS, -1);
+ dev_set_ctrl_rel(vid, CTRL_SHARPNESS, -1);
  break;
  case 'B':
  if (vid->mode & M_IN_DEV)
- dev_set_ctrl(vid, CTRL_BRIGHTNESS, 1);
+ dev_set_ctrl_rel(vid, CTRL_BRIGHTNESS, 1);
  break;
  case 'b':
  if (vid->mode & M_IN_DEV)
- dev_set_ctrl(vid, CTRL_BRIGHTNESS, -1);
+ dev_set_ctrl_rel(vid, CTRL_BRIGHTNESS, -1);
  break;
  case 'C':
  if (vid->mode & M_IN_DEV)
- dev_set_ctrl(vid, CTRL_CONTRAST, 1);
+ dev_set_ctrl_rel(vid, CTRL_CONTRAST, 1);
  break;
  case 'c':
  if (vid->mode & M_IN_DEV)
- dev_set_ctrl(vid, CTRL_CONTRAST, -1);
+ dev_set_ctrl_rel(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_set_ctrl_rel(vid, CTRL_GAIN, 1);
  break;
  case 'g':
  if (vid->mode & M_IN_DEV)
- dev_set_ctrl(vid, CTRL_GAIN, -1);
+ dev_set_ctrl_rel(vid, CTRL_GAIN, -1);
  break;
  case 'H':
  if (vid->mode & M_IN_DEV)
- dev_set_ctrl(vid, CTRL_HUE, 1);
+ dev_set_ctrl_rel(vid, CTRL_HUE, 1);
  break;
  case 'h':
  if (vid->mode & M_IN_DEV)
- dev_set_ctrl(vid, CTRL_HUE, -1);
+ dev_set_ctrl_rel(vid, CTRL_HUE, -1);
  break;
  case 'O':
  if (!wout && vid->verbose > 0)
@@ -710,11 +715,11 @@ display_event(struct video *vid)
  break;
  case 'M':
  if (vid->mode & M_IN_DEV)
- dev_set_ctrl(vid, CTRL_GAMMA, 1);
+ dev_set_ctrl_rel(vid, CTRL_GAMMA, 1);
  break;
  case 'm':
  if (vid->mode & M_IN_DEV)
- dev_set_ctrl(vid, CTRL_GAMMA, -1);
+ dev_set_ctrl_rel(vid, CTRL_GAMMA, -1);
  break;
  case 'p':
  hold = !hold;
@@ -728,20 +733,20 @@ display_event(struct video *vid)
  break;
  case 'S':
  if (vid->mode & M_IN_DEV)
- dev_set_ctrl(vid, CTRL_SATURATION, 1);
+ dev_set_ctrl_rel(vid, CTRL_SATURATION, 1);
  break;
  case 's':
  if (vid->mode & M_IN_DEV)
- dev_set_ctrl(vid, CTRL_SATURATION, -1);
+ dev_set_ctrl_rel(vid, CTRL_SATURATION, -1);
  break;
  case 'W':
  if (vid->mode & M_IN_DEV)
- dev_set_ctrl(vid,
+ dev_set_ctrl_rel(vid,
     CTRL_WHITE_BALANCE_TEMPERATURE, 10);
  break;
  case 'w':
  if (vid->mode & M_IN_DEV)
- dev_set_ctrl(vid,
+ dev_set_ctrl_rel(vid,
     CTRL_WHITE_BALANCE_TEMPERATURE, -10);
  break;
  default:
@@ -1010,11 +1015,10 @@ dev_get_ctrls(struct video *vid)
 }
 
 void
-dev_set_ctrl(struct video *vid, int ctrl, int change)
+dev_set_ctrl_abs(struct video *vid, int ctrl, int val)
 {
  struct dev *d = &vid->dev;
  struct v4l2_control control;
- int val;
 
  if (ctrl < 0 || ctrl >= CTRL_LAST) {
  warnx("invalid control");
@@ -1030,9 +1034,8 @@ dev_set_ctrl(struct video *vid, int ctrl
  * The spec requires auto-white balance to be off before
  * we can set the white balance temperature.
  */
- dev_set_ctrl_auto_white_balance(vid, 0);
+ dev_set_ctrl_auto_white_balance(vid, 0, 0);
  }
- val = ctrls[ctrl].cur + ctrls[ctrl].step * change;
  if (val > ctrls[ctrl].max)
  val = ctrls[ctrl].max;
  else if (val < ctrls[ctrl].min)
@@ -1055,20 +1058,46 @@ dev_set_ctrl(struct video *vid, int ctrl
 }
 
 void
-dev_set_ctrl_auto_white_balance(struct video *vid, int toggle)
+dev_set_ctrl_rel(struct video *vid, int ctrl, int change)
+{
+ struct dev *d = &vid->dev;
+ int val;
+
+ 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;
+ }
+ val = ctrls[ctrl].cur + ctrls[ctrl].step * change;
+ dev_set_ctrl_abs(vid, ctrl, val);
+}
+
+void
+dev_set_ctrl_auto_white_balance(struct video *vid, int value, int reset)
 {
  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)
+ 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");
+ if (reset) {
+ if (ioctl(d->fd, VIDIOC_S_CTRL, &control) != 0)
+ warn("VIDIOC_S_CTRL");
+ } else {
+ if (control.value == value)
+ return;
+ control.value = value;
+ if (ioctl(d->fd, VIDIOC_S_CTRL, &control) != 0)
+ warn("VIDIOC_S_CTRL");
+ }
 }
 
 void
@@ -1081,27 +1110,9 @@ dev_reset_ctrls(struct video *vid)
  for (i = 0; i < CTRL_LAST; i++) {
  if (!ctrls[i].supported)
  continue;
+ dev_set_ctrl_abs(vid, i, ctrls[i].def);
  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);
- if (i == CTRL_WHITE_BALANCE_TEMPERATURE) {
- dev_set_ctrl_auto_white_balance(vid, 1);
+ dev_set_ctrl_auto_white_balance(vid, 1, 0);
  }
  }
 }
@@ -1221,6 +1232,36 @@ dev_init(struct video *vid)
  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 +1522,8 @@ mmap_stop(struct video *vid)
 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;
@@ -1553,6 +1596,18 @@ setup(struct video *vid)
  if ((vid->mode & M_IN_DEV) && !dev_init(vid))
  return 0;
 
+ 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;
 
@@ -1564,6 +1619,13 @@ setup(struct video *vid)
  return 0;
  }
 
+ /*
+ * Reset the current White Balance Temperature Auto Control value
+ * after the video stream has been started since some cams only
+ * process this control while the video stream is on.
+ */
+ dev_set_ctrl_auto_white_balance(vid, 0, 1);
+
  if (vid->mode & M_OUT_XV)
  net_wm_supported(vid);
 
@@ -1949,7 +2011,7 @@ main(int argc, char *argv[])
  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 +2020,17 @@ main(int argc, char *argv[])
  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 +2128,9 @@ main(int argc, char *argv[])
 
  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)

Reply | Threaded
Open this post in threaded view
|

Re: Add ability to set control values with video(1)

Laurence Tratt
On Thu, Jul 23, 2020 at 09:56:39PM +0200, Marcus Glocker wrote:

Hello Marcus,

> We read the current value of the white balance temperature auto control
> (since this gets set correctly during the reset), and just reset it again
> on the next cam start up after the video stream has been started?

Aha, very clever -- I hadn't thought of doing that!

I've tried your version and it works correctly in all the scenarios I can
think of. I think this is now ready to go in!


Laurie

Reply | Threaded
Open this post in threaded view
|

Re: Add ability to set control values with video(1)

Marcus Glocker
Hi Laurie,

On Thu, 23 Jul 2020 21:07:26 +0100
Laurence Tratt <[hidden email]> wrote:

> On Thu, Jul 23, 2020 at 09:56:39PM +0200, Marcus Glocker wrote:
>
> Hello Marcus,
>
> > We read the current value of the white balance temperature auto
> > control (since this gets set correctly during the reset), and just
> > reset it again on the next cam start up after the video stream has
> > been started?  
>
> Aha, very clever -- I hadn't thought of doing that!
>
> I've tried your version and it works correctly in all the scenarios I
> can think of. I think this is now ready to go in!

Ok, cool - I try to collect an OK therefore :-)

>
> Laurie
>

Thanks,
Marcus

Reply | Threaded
Open this post in threaded view
|

Re: Add ability to set control values with video(1)

Laurence Tratt
In reply to this post by Marcus Glocker
On Thu, Jul 23, 2020 at 09:56:39PM +0200, Marcus Glocker wrote:

Hello Marcus,

> We read the current value of the white balance temperature auto control
> (since this gets set correctly during the reset), and just reset it again
> on the next cam start up after the video stream has been started?
> By that we avoid the video stream on/off dance when using '-c'.

I missed an obvious scenario :/ "video -c reset" then "use the webcam in a
browser" (or ffplay or whatever). Depending on your webcam, this won't appear
to have done a full reset in such a scenario?


Laurie

Reply | Threaded
Open this post in threaded view
|

Re: Add ability to set control values with video(1)

Marcus Glocker
On Fri, 24 Jul 2020 15:13:19 +0100
Laurence Tratt <[hidden email]> wrote:

> On Thu, Jul 23, 2020 at 09:56:39PM +0200, Marcus Glocker wrote:
>
> Hello Marcus,
>
> > We read the current value of the white balance temperature auto
> > control (since this gets set correctly during the reset), and just
> > reset it again on the next cam start up after the video stream has
> > been started? By that we avoid the video stream on/off dance when
> > using '-c'.  
>
> I missed an obvious scenario :/ "video -c reset" then "use the webcam
> in a browser" (or ffplay or whatever). Depending on your webcam, this
> won't appear to have done a full reset in such a scenario?

That's true and basically out of our control I would say ...

Anyway, matthieu@ came up with an idea yesterday which I like and also
think would be the better approach finally:

Instead of introducing the CLI parameter controls in video(1), we could
introduce videoctl(1) in base, same as we have with audioctl(1).  This
also gives us the possibility to only display the current video
parameters.

For things like the auto white balance temperature control we could
implement tweaks easier there, like only turning the stream on before we
set back auto white balance temperature to 1.

I also want to sound this idea here to check if somebody has objections
with that approach?

>
> Laurie
>

Marcus

Reply | Threaded
Open this post in threaded view
|

Re: Add ability to set control values with video(1)

Theo de Raadt-2
Marcus Glocker <[hidden email]> wrote:

> Instead of introducing the CLI parameter controls in video(1), we could
> introduce videoctl(1) in base, same as we have with audioctl(1).  This
> also gives us the possibility to only display the current video
> parameters.

I must have missed something.  Why does it need to be a seperate comment.
Won't this produce confusion?  Why can video do this?

Is it really correct for the video hardware to have persistant settings?
Would it not be better if the required mode was always commanded when
a video is being recorded?

Reply | Threaded
Open this post in threaded view
|

Re: Add ability to set control values with video(1)

Theo de Raadt-2
In reply to this post by Marcus Glocker
  $ video -c white_balance_temperature=6500
  $ video
  $ video -c reset
  $ video

BTW, no other subsystem of ours has this "reset" type thing.

A few subsystems have persistance, but persistant modes are usually only
in the software layer, not in the hardware layer.

The normal idiom is when last-close happens in a driver, all modal-state
is lost and restored to default, and when you use the driver again --
the new open gets you a raw configuration which is then changed via
ioctl, before futher use.

Reply | Threaded
Open this post in threaded view
|

Re: Add ability to set control values with video(1)

Matthieu Herrb-3
In reply to this post by Theo de Raadt-2
On Sat, Jul 25, 2020 at 09:17:24AM -0600, Theo de Raadt wrote:

> Marcus Glocker <[hidden email]> wrote:
>
> > Instead of introducing the CLI parameter controls in video(1), we could
> > introduce videoctl(1) in base, same as we have with audioctl(1).  This
> > also gives us the possibility to only display the current video
> > parameters.
>
> I must have missed something.  Why does it need to be a seperate comment.
> Won't this produce confusion?  Why can video do this?
>
> Is it really correct for the video hardware to have persistant settings?
> Would it not be better if the required mode was always commanded when
> a video is being recorded?

I've not followed UVC hardware too closely; it seems that some of the
cameras are always fully automatically adjusting their parameters,
while others allow for manual setting that remains between device
open().

And some of the controls are not available when video(4) is used from
a browser (for conferencing).

On a 2nd thought having that integrated with video(1) allows to
control the values with visual feedback so it makes sense to keep only
one program. But ihmo it would be more user friendly if the command
line syntax was more regular with audio or wscons control programs.

--
Matthieu Herrb

Reply | Threaded
Open this post in threaded view
|

Re: Add ability to set control values with video(1)

Theo de Raadt-2
Matthieu Herrb <[hidden email]> wrote:

> On Sat, Jul 25, 2020 at 09:17:24AM -0600, Theo de Raadt wrote:
> > Marcus Glocker <[hidden email]> wrote:
> >
> > > Instead of introducing the CLI parameter controls in video(1), we could
> > > introduce videoctl(1) in base, same as we have with audioctl(1).  This
> > > also gives us the possibility to only display the current video
> > > parameters.
> >
> > I must have missed something.  Why does it need to be a seperate comment.
> > Won't this produce confusion?  Why can video do this?
> >
> > Is it really correct for the video hardware to have persistant settings?
> > Would it not be better if the required mode was always commanded when
> > a video is being recorded?
>
> I've not followed UVC hardware too closely; it seems that some of the
> cameras are always fully automatically adjusting their parameters,
> while others allow for manual setting that remains between device
> open().
>
> And some of the controls are not available when video(4) is used from
> a browser (for conferencing).
>
> On a 2nd thought having that integrated with video(1) allows to
> control the values with visual feedback so it makes sense to keep only
> one program. But ihmo it would be more user friendly if the command
> line syntax was more regular with audio or wscons control programs.

My primary concern is about a user changing settings which then persist
past close.

Upon re-open, how do they know what mode they are in?

I understand the default mode for a camera might not be nice.  But at
least it is a default.  If the previous use of the camera put it into
a nasty mode, does this mean all new users must first force default?

Now you don't know what mode it is in.  As a result, you must *always*
demand change into a specific mode.  Rather than making things simpler,
doesn't use of a camera become potentially more complicated?

Reply | Threaded
Open this post in threaded view
|

Re: Add ability to set control values with video(1)

Stuart Henderson
In reply to this post by Theo de Raadt-2
On 2020/07/25 09:20, Theo de Raadt wrote:
> The normal idiom is when last-close happens in a driver, all modal-state
> is lost and restored to default, and when you use the driver again --
> the new open gets you a raw configuration which is then changed via
> ioctl, before futher use.

Isn't this a bit like volume controls for audio which do exist outside
of a particular application's opening of the device?

Reply | Threaded
Open this post in threaded view
|

Re: Add ability to set control values with video(1)

Theo de Raadt-2
Stuart Henderson <[hidden email]> wrote:

> On 2020/07/25 09:20, Theo de Raadt wrote:
> > The normal idiom is when last-close happens in a driver, all modal-state
> > is lost and restored to default, and when you use the driver again --
> > the new open gets you a raw configuration which is then changed via
> > ioctl, before futher use.
>
> Isn't this a bit like volume controls for audio which do exist outside
> of a particular application's opening of the device?

That is soft state, and operate in a global experience context.

Please also consider more than volume controls.  With audio, I think
it is the only major knob, but what's being proposed is a big collection
of knobs.

I'm not sure I see the set of camera ops the same way as volume control.

Also see my message relating to 'reset', back to the default state.
No such thing exists in audio.

I think this is the difference between subsystem (soft state), versus
driver state (much more hard state)



Reply | Threaded
Open this post in threaded view
|

Re: Add ability to set control values with video(1)

Marcus Glocker
In reply to this post by Theo de Raadt-2
On Sat, Jul 25, 2020 at 10:27:15AM -0600, Theo de Raadt wrote:

> Matthieu Herrb <[hidden email]> wrote:
>
> > On Sat, Jul 25, 2020 at 09:17:24AM -0600, Theo de Raadt wrote:
> > > Marcus Glocker <[hidden email]> wrote:
> > >
> > > > Instead of introducing the CLI parameter controls in video(1), we could
> > > > introduce videoctl(1) in base, same as we have with audioctl(1).  This
> > > > also gives us the possibility to only display the current video
> > > > parameters.
> > >
> > > I must have missed something.  Why does it need to be a seperate comment.
> > > Won't this produce confusion?  Why can video do this?
> > >
> > > Is it really correct for the video hardware to have persistant settings?
> > > Would it not be better if the required mode was always commanded when
> > > a video is being recorded?
> >
> > I've not followed UVC hardware too closely; it seems that some of the
> > cameras are always fully automatically adjusting their parameters,
> > while others allow for manual setting that remains between device
> > open().
> >
> > And some of the controls are not available when video(4) is used from
> > a browser (for conferencing).
> >
> > On a 2nd thought having that integrated with video(1) allows to
> > control the values with visual feedback so it makes sense to keep only
> > one program. But ihmo it would be more user friendly if the command
> > line syntax was more regular with audio or wscons control programs.
>
> My primary concern is about a user changing settings which then persist
> past close.
>
> Upon re-open, how do they know what mode they are in?
>
> I understand the default mode for a camera might not be nice.  But at
> least it is a default.  If the previous use of the camera put it into
> a nasty mode, does this mean all new users must first force default?
>
> Now you don't know what mode it is in.  As a result, you must *always*
> demand change into a specific mode.  Rather than making things simpler,
> doesn't use of a camera become potentially more complicated?

OK - Lets put aside the audioctl(1) discussion.  Maybe the idea isn't
that good than I initially thought and would indeed over-complicate
things.

Regarding the current behavior of video(1) for the default controls;
When you change the controls today through the video(1) keys, they are
already sticky, since the reset control function only can be called
through the 'r' key-press.  If we want to go for a default mode on close,
of course that function could be easily copied to the video(1) closing
routine.

The idea of the patch on the other side was to enable somebody to
easily set his preferred controls, e.g. through a script.  At the
moment if you want to set your preferred controls on cam start-up,
that's only possible by the key-presses which is odd obviously if you
need to change a couple of controls every time by pressing keys.  How
many people would finally find that an useful feature is another
question I guess.

I have no strong opinion about the line syntax, but I agree keeping it
similar to other commands would be good.  E.g. adapting video(1) to
the audioctl(1) syntax and doing something like

        $ video brightness=200 contrast=150

could be an option - If we want to follow this feature at all.

Reply | Threaded
Open this post in threaded view
|

Re: Add ability to set control values with video(1)

Laurence Tratt
In reply to this post by Theo de Raadt-2
On Sat, Jul 25, 2020 at 10:27:15AM -0600, Theo de Raadt wrote:

Hello Theo,

> My primary concern is about a user changing settings which then persist
> past close.
>
> Upon re-open, how do they know what mode they are in?
>
> I understand the default mode for a camera might not be nice.  But at least
> it is a default.  If the previous use of the camera put it into a nasty
> mode, does this mean all new users must first force default?
>
> Now you don't know what mode it is in.  As a result, you must *always*
> demand change into a specific mode.  Rather than making things simpler,
> doesn't use of a camera become potentially more complicated?

From what I can tell, there are two ways to control a uvideo device:

  1) The "semi-persistent" state changes that video(1) can make that affects
  subsequent apps which access the device. My patch simply makes those state
  changes possible from the command-line instead of forcing the user to open
  a video and hold down keys until they reach the desired state. In other
  words, it doesn't change how you control the device: "-c reset" is
  equivalent to running video(1) and pressing "r", for example.

  2) Control via a loopback device. For example, on Windows, Logitech allow
  you to change controls in their app where you can see video; they then
  expose a second internal device which other apps can use; I think controls
  are reset when the Logitech app is closed.

On Linux I believe v4l2-ctl works as video(1) does (semi-persistent state
changes) but Linux also has video loopback devices. Presumably they could do
something similar to the Logitech Windows app, but I don't know if they do so
or not.

Unless we develop a loopback facility (or, perhaps, some sort of uvideo
daemon roughly equivalent to sndiod), I don't think we have much choice but
to continue with the semi-persistent state changes that video(1) has always
been capable of. It is a bit icky, but it's the only way, for example, to
change a webcam's brightness before taking a video call in a web browser.


Laurie

Reply | Threaded
Open this post in threaded view
|

Re: Add ability to set control values with video(1)

Marcus Glocker
On Sat, 25 Jul 2020 18:27:45 +0100
Laurence Tratt <[hidden email]> wrote:

> On Sat, Jul 25, 2020 at 10:27:15AM -0600, Theo de Raadt wrote:
>
> Hello Theo,
>
> > My primary concern is about a user changing settings which then
> > persist past close.
> >
> > Upon re-open, how do they know what mode they are in?
> >
> > I understand the default mode for a camera might not be nice.  But
> > at least it is a default.  If the previous use of the camera put it
> > into a nasty mode, does this mean all new users must first force
> > default?
> >
> > Now you don't know what mode it is in.  As a result, you must
> > *always* demand change into a specific mode.  Rather than making
> > things simpler, doesn't use of a camera become potentially more
> > complicated?  
>
> From what I can tell, there are two ways to control a uvideo device:
>
>   1) The "semi-persistent" state changes that video(1) can make that
> affects subsequent apps which access the device. My patch simply
> makes those state changes possible from the command-line instead of
> forcing the user to open a video and hold down keys until they reach
> the desired state. In otReset all supported controls to their default
> value and quit.her words, it doesn't change how you control the
> device: "-c reset" is equivalent to running video(1) and pressing
> "r", for example.
>
>   2) Control via a loopback device. For example, on Windows, Logitech
> allow you to change controls in their app where you can see video;
> they then expose a second internal device which other apps can use; I
> think controls are reset when the Logitech app is closed.
>
> On Linux I believe v4l2-ctl works as video(1) does (semi-persistent
> state changes) but Linux also has video loopback devices. Presumably
> they could do something similar to the Logitech Windows app, but I
> don't know if they do so or not.
>
> Unless we develop a loopback facility (or, perhaps, some sort of
> uvideo daemon roughly equivalent to sndiod), I don't think we have
> much choice but to continue with the semi-persistent state changes
> that video(1) has always been capable of. It is a bit icky, but it's
> the only way, for example, to change a webcam's brightness before
> taking a video call in a web browser.

OK - Let me try to pick this thread up once more :-)

Lets assume we leave the current behaviour of video(1) as is, which
doesn't reset back the default control values on device close.

This adapted patch adds two new options and the ability to set multiple
control values on the CLI with a similar interface as sysctl(8) has:

* c: List all current supported control values and quit
  (-q is already occupied).

* d: Reset all supported controls to their default value and quit
  (-r is already occupied).
  -> This does the same thing as when pressing 'r' in the GUI.

Some examples:

        $ doas video -c
        brightness=128
        contrast=32
        saturation=64
        hue=0
        gamma=120
        sharpness=2
        white_balance_temperature=4000
        $

        $ doas video brightness=200 sharpness=4
        brightness: 128 -> 200
        sharpness: 2 -> 4
        $

        $ doas video brightness
        brightness: 200
        $

        $ doas video -c
        brightness=200
        contrast=32
        saturation=64
        hue=0
        gamma=120
        sharpness=4
        white_balance_temperature=4000
        $

        $ doas video -d
        $

        $ doas video -c
        brightness=128
        contrast=32
        saturation=64
        hue=0
        gamma=120
        sharpness=2
        white_balance_temperature=4000
        $

        $ doas video -f /dev/video1 gain=1
        gain: 0 -> 1
        $

        $ doas video -f /dev/video1 -c    
        brightness=128
        contrast=128
        saturation=128
        gain=1
        sharpness=128
        white_balance_temperature=4000
        $

What we could think about optionally is to reset to the default control
values on device close when we did use the GUI mode, but leave the
controls sticky when we set them through the CLI, and explicitly
document that in the video(1) man page for the [control[=value]]
arguments.

But entirely removing the sticky controls I think is odd since as
already mentioned before, it will remove the ability to preset any
controls before we enter an application where we can't change them,
like in an browser.  And web conference calls are pretty common
nowadays ...


Index: video.1
===================================================================
RCS file: /cvs/xenocara/app/video/video.1,v
retrieving revision 1.15
diff -u -p -u -p -r1.15 video.1
--- video.1 17 Jul 2020 07:51:23 -0000 1.15
+++ video.1 29 Jul 2020 05:02:51 -0000
@@ -25,7 +25,7 @@
 .Sh SYNOPSIS
 .Nm
 .Bk -words
-.Op Fl \&gqRv
+.Op Fl \&cdgqRv
 .Op Fl a Ar adaptor
 .Op Fl e Ar encoding
 .Op Fl f Ar file
@@ -34,6 +34,7 @@
 .Op Fl o Ar output
 .Op Fl r Ar rate
 .Op Fl s Ar size
+.Op Ar control Ns Op = Ns Ar value
 .Ek
 .Sh DESCRIPTION
 .Nm
@@ -81,6 +82,10 @@ Index of
 adaptor to use.
 The default is 0, the first adaptor reported by
 .Xr X 7 .
+.It Fl c
+List all current supported control values and quit.
+.It Fl d
+Reset all supported controls to their default value and quit.
 .It Fl e Ar encoding
 Lowercase FOURCC name of video encoding to use.
 Valid arguments are
@@ -219,6 +224,14 @@ Verbose mode.
 Multiple instances of this option are allowed.
 Each instance increases the level of informational output printed to
 .Ar stderr .
+.It Ar control Ns Op = Ns Ar value
+Retrieve the specified
+.Ar control ,
+or attempt to set it to
+.Ar value .
+Multiple
+.Ar control Ns Op = Ns Ar value
+arguments may be given.
 .El
 .Pp
 .Nm
Index: video.c
===================================================================
RCS file: /cvs/xenocara/app/video/video.c,v
retrieving revision 1.31
diff -u -p -u -p -r1.31 video.c
--- video.c 17 Jul 2020 07:51:23 -0000 1.31
+++ video.c 29 Jul 2020 05:02:52 -0000
@@ -192,6 +192,8 @@ struct video {
 #define M_IN_FILE 0x4
 #define M_OUT_FILE 0x8
 #define M_QUERY 0x10
+#define M_QUERY_CTRLS 0x20
+#define M_RESET 0x40
  int mode;
  int verbose;
 };
@@ -211,11 +213,14 @@ int dev_get_rates(struct video *);
 int dev_get_ctrls(struct video *);
 void dev_dump_info(struct video *);
 void dev_dump_query(struct video *);
+void dev_dump_query_ctrls(struct video *);
 int dev_init(struct video *);
-void dev_set_ctrl(struct video *, int, int);
-void dev_set_ctrl_auto_white_balance(struct video *, int);
+int dev_set_ctrl_abs(struct video *vid, int, int);
+void dev_set_ctrl_rel(struct video *, int, int);
+void dev_set_ctrl_auto_white_balance(struct video *, int, int);
 void dev_reset_ctrls(struct video *);
 
+int parse_ctrl(struct video *, int, char **);
 int parse_size(struct video *);
 int choose_size(struct video *);
 int choose_enc(struct video *);
@@ -240,10 +245,10 @@ extern char *__progname;
 void
 usage(void)
 {
- fprintf(stderr, "usage: %s [-gqRv] "
+ fprintf(stderr, "usage: %s [-cdgqRv] "
     "[-a adaptor] [-e encoding] [-f file] [-i input] [-O
output]\n"
-    "       %*s [-o output] [-r rate] [-s size]\n", __progname,
-    (int)strlen(__progname), "");
+    "       %*s [-o output] [-r rate] [-s size]
[control[=value]]\n",
+    __progname, (int)strlen(__progname), "");
 }
 
 int
@@ -657,46 +662,46 @@ display_event(struct video *vid)
  switch (str) {
  case 'A':
  if (vid->mode & M_IN_DEV)
- dev_set_ctrl(vid,
CTRL_SHARPNESS, 1);
+ dev_set_ctrl_rel(vid,
CTRL_SHARPNESS, 1); break;
  case 'a':
  if (vid->mode & M_IN_DEV)
- dev_set_ctrl(vid,
CTRL_SHARPNESS, -1);
+ dev_set_ctrl_rel(vid,
CTRL_SHARPNESS, -1); break;
  case 'B':
  if (vid->mode & M_IN_DEV)
- dev_set_ctrl(vid,
CTRL_BRIGHTNESS, 1);
+ dev_set_ctrl_rel(vid,
CTRL_BRIGHTNESS, 1); break;
  case 'b':
  if (vid->mode & M_IN_DEV)
- dev_set_ctrl(vid,
CTRL_BRIGHTNESS, -1);
+ dev_set_ctrl_rel(vid,
CTRL_BRIGHTNESS, -1); break;
  case 'C':
  if (vid->mode & M_IN_DEV)
- dev_set_ctrl(vid,
CTRL_CONTRAST, 1);
+ dev_set_ctrl_rel(vid,
CTRL_CONTRAST, 1); break;
  case 'c':
  if (vid->mode & M_IN_DEV)
- dev_set_ctrl(vid,
CTRL_CONTRAST, -1);
+ dev_set_ctrl_rel(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_set_ctrl_rel(vid,
CTRL_GAIN, 1); break;
  case 'g':
  if (vid->mode & M_IN_DEV)
- dev_set_ctrl(vid, CTRL_GAIN,
-1);
+ dev_set_ctrl_rel(vid,
CTRL_GAIN, -1); break;
  case 'H':
  if (vid->mode & M_IN_DEV)
- dev_set_ctrl(vid, CTRL_HUE, 1);
+ dev_set_ctrl_rel(vid,
CTRL_HUE, 1); break;
  case 'h':
  if (vid->mode & M_IN_DEV)
- dev_set_ctrl(vid, CTRL_HUE,
-1);
+ dev_set_ctrl_rel(vid,
CTRL_HUE, -1); break;
  case 'O':
  if (!wout && vid->verbose > 0)
@@ -710,11 +715,11 @@ display_event(struct video *vid)
  break;
  case 'M':
  if (vid->mode & M_IN_DEV)
- dev_set_ctrl(vid, CTRL_GAMMA,
1);
+ dev_set_ctrl_rel(vid,
CTRL_GAMMA, 1); break;
  case 'm':
  if (vid->mode & M_IN_DEV)
- dev_set_ctrl(vid, CTRL_GAMMA,
-1);
+ dev_set_ctrl_rel(vid,
CTRL_GAMMA, -1); break;
  case 'p':
  hold = !hold;
@@ -728,20 +733,20 @@ display_event(struct video *vid)
  break;
  case 'S':
  if (vid->mode & M_IN_DEV)
- dev_set_ctrl(vid,
CTRL_SATURATION, 1);
+ dev_set_ctrl_rel(vid,
CTRL_SATURATION, 1); break;
  case 's':
  if (vid->mode & M_IN_DEV)
- dev_set_ctrl(vid,
CTRL_SATURATION, -1);
+ dev_set_ctrl_rel(vid,
CTRL_SATURATION, -1); break;
  case 'W':
  if (vid->mode & M_IN_DEV)
- dev_set_ctrl(vid,
+ dev_set_ctrl_rel(vid,
     CTRL_WHITE_BALANCE_TEMPERATURE,
10); break;
  case 'w':
  if (vid->mode & M_IN_DEV)
- dev_set_ctrl(vid,
+ dev_set_ctrl_rel(vid,
     CTRL_WHITE_BALANCE_TEMPERATURE,
-10); break;
  default:
@@ -1009,30 +1014,28 @@ dev_get_ctrls(struct video *vid)
  return 1;
 }
 
-void
-dev_set_ctrl(struct video *vid, int ctrl, int change)
+int
+dev_set_ctrl_abs(struct video *vid, int ctrl, int val)
 {
  struct dev *d = &vid->dev;
  struct v4l2_control control;
- int val;
 
  if (ctrl < 0 || ctrl >= CTRL_LAST) {
  warnx("invalid control");
- return;
+ return -1;
  }
  if (!ctrls[ctrl].supported) {
  warnx("control %s not supported by %s",
     ctrls[ctrl].name, d->path);
- return;
+ return -1;
  }
  if (ctrl == CTRL_WHITE_BALANCE_TEMPERATURE) {
  /*
  * The spec requires auto-white balance to be off
before
  * we can set the white balance temperature.
  */
- dev_set_ctrl_auto_white_balance(vid, 0);
+ dev_set_ctrl_auto_white_balance(vid, 0, 0);
  }
- val = ctrls[ctrl].cur + ctrls[ctrl].step * change;
  if (val > ctrls[ctrl].max)
  val = ctrls[ctrl].max;
  else if (val < ctrls[ctrl].min)
@@ -1041,34 +1044,62 @@ dev_set_ctrl(struct video *vid, int ctrl
  control.value = val;
  if (ioctl(d->fd, VIDIOC_S_CTRL, &control) != 0) {
  warn("VIDIOC_S_CTRL");
- return;
+ return -1;
  }
  control.id = ctrls[ctrl].id;
  if (ioctl(d->fd, VIDIOC_G_CTRL, &control) != 0) {
  warn("VIDIOC_G_CTRL");
- return;
+ return -1;
  }
  ctrls[ctrl].cur = control.value;
  if (vid->verbose > 0)
  fprintf(stderr, "%s now %d\n", ctrls[ctrl].name,
     ctrls[ctrl].cur);
+
+ return 0;
 }
 
 void
-dev_set_ctrl_auto_white_balance(struct video *vid, int toggle)
+dev_set_ctrl_rel(struct video *vid, int ctrl, int change)
+{
+ struct dev *d = &vid->dev;
+ int val;
+
+ 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;
+ }
+ val = ctrls[ctrl].cur + ctrls[ctrl].step * change;
+ dev_set_ctrl_abs(vid, ctrl, val);
+}
+
+void
+dev_set_ctrl_auto_white_balance(struct video *vid, int value, int
reset) {
  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)
+ 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");
+ if (reset) {
+ if (ioctl(d->fd, VIDIOC_S_CTRL, &control) != 0)
+ warn("VIDIOC_S_CTRL");
+ } else {
+ if (control.value == value)
+ return;
+ control.value = value;
+ if (ioctl(d->fd, VIDIOC_S_CTRL, &control) != 0)
+ warn("VIDIOC_S_CTRL");
+ }
 }
 
 void
@@ -1081,27 +1112,9 @@ dev_reset_ctrls(struct video *vid)
  for (i = 0; i < CTRL_LAST; i++) {
  if (!ctrls[i].supported)
  continue;
+ dev_set_ctrl_abs(vid, i, ctrls[i].def);
  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);
- if (i == CTRL_WHITE_BALANCE_TEMPERATURE) {
- dev_set_ctrl_auto_white_balance(vid, 1);
+ dev_set_ctrl_auto_white_balance(vid, 1, 0);
  }
  }
 }
@@ -1171,6 +1184,22 @@ dev_dump_query(struct video *vid)
  dev_dump_info(vid);
 }
 
+void
+dev_dump_query_ctrls(struct video *vid)
+{
+ int i;
+
+ if (!dev_check_caps(vid))
+ return;
+ if (!dev_get_ctrls(vid))
+ return;
+
+ for (i = 0; i < CTRL_LAST; i++) {
+ if (ctrls[i].supported)
+ fprintf(stderr, "%s=%d\n", ctrls[i].name,
ctrls[i].cur);
+ }
+}
+
 int
 dev_init(struct video *vid)
 {
@@ -1222,6 +1251,64 @@ dev_init(struct video *vid)
 }
 
 int
+parse_ctrl(struct video *vid, int argc, char **argv)
+{
+ int i, val_old, val_new;
+ char *p;
+ const char *errstr;
+
+ if (*argv == NULL)
+ return 1; /* No control arguments found. */
+
+ if (!dev_check_caps(vid))
+ return 0;
+ if (!dev_get_ctrls(vid))
+ return 0;
+
+ for (; argc > 0; argc--, argv++) {
+ p = strchr(*argv, '=');
+
+ /* Display control value. */
+ if (p == NULL) {
+ for (i = 0; i < CTRL_LAST; i++) {
+ if (!strcmp(*argv, ctrls[i].name)) {
+ fprintf(stderr, "%s: %d\n",
+    ctrls[i].name,
ctrls[i].cur);
+ break;
+ }
+ }
+ if (i == CTRL_LAST)
+ warnx("%s: unknown control",
*argv);
+ continue;
+ }
+
+ /* Set control value. */
+ for (i = 0, *p++ = '\0'; i < CTRL_LAST; i++) {
+ if (strcmp(*argv, ctrls[i].name) != 0)
+ continue;
+ if (*p == '\0') {
+ warnx("%s: no value", *argv);
+ break;
+ }
+ val_new = strtonum(p, 0, 65535, &errstr);
+ if (errstr != NULL) {
+ warnx("%s: %s", *argv, errstr);
+ return 0;
+ }
+ val_old = ctrls[i].cur;
+ if (dev_set_ctrl_abs(vid, i, val_new) == 0)
+ fprintf(stderr, "%s: %d -> %d\n",
+    ctrls[i].name, val_old,
ctrls[i].cur);
+ break;
+ }
+ if (i == CTRL_LAST)
+ warnx("%s: unknown control", *argv);
+ }
+
+ return 0;
+}
+
+int
 parse_size(struct video *vid)
 {
  struct xdsp *x = &vid->xdsp;
@@ -1564,6 +1651,13 @@ setup(struct video *vid)
  return 0;
  }
 
+ /*
+ * Reset the current White Balance Temperature Auto Control
value
+ * after the video stream has been started since some cams only
+ * process this control while the video stream is on.
+ */
+ dev_set_ctrl_auto_white_balance(vid, 0, 1);
+
  if (vid->mode & M_OUT_XV)
  net_wm_supported(vid);
 
@@ -1949,7 +2043,7 @@ main(int argc, char *argv[])
  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, "cdgqRva:e:f:i:O:o:r:s:")) !=
-1) { switch (ch) {
  case 'a':
  x->cur_adap = strtonum(optarg, 0, 4, &errstr);
@@ -1958,6 +2052,14 @@ main(int argc, char *argv[])
  errs++;
  }
  break;
+ case 'c':
+ vid.mode |= M_QUERY_CTRLS;
+ vid.mode &= ~M_OUT_XV;
+ break;
+ case 'd':
+ vid.mode |= M_RESET;
+ vid.mode &= ~M_OUT_XV;
+ break;
  case 'e':
  vid.enc = find_enc(optarg);
  if (vid.enc >= ENC_LAST) {
@@ -2043,6 +2145,14 @@ main(int argc, char *argv[])
  argc -= optind;
  argv += optind;
 
+ if (!parse_ctrl(&vid, argc, argv))
+ cleanup(&vid, 0);
+
+ if (vid.mode & M_QUERY_CTRLS) {
+ dev_dump_query_ctrls(&vid);
+ cleanup(&vid, 0);
+ }
+
  if (vid.mode & M_QUERY) {
  if (pledge("stdio rpath wpath video", NULL) == -1)
  err(1, "pledge");
@@ -2055,6 +2165,11 @@ main(int argc, char *argv[])
 
  if (!setup(&vid))
  cleanup(&vid, 1);
+
+ if (vid.mode & M_RESET) {
+ dev_reset_ctrls(&vid);
+ cleanup(&vid, 0);
+ }
 
  if (vid.mode & M_IN_FILE) {
  if (pledge("stdio rpath", NULL) == -1)

12