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)
