Hello Laurie,
On Tue, 21 Jul 2020 21:18:15 +0100
Laurence Tratt <[email protected]> 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)
>
