Re: [RFC PATCH 5/5] v4l2-ctl: add an option to list controls in a machine-readable format

[Hans Verkuil <hverkuil@xs4all.nl>]

On 01/03/2019 07:01 PM, Antonio Ospite wrote:
> Add a new option --list-ctrls-values to list the values of controls in
> a format which can be passed again to --set-ctrl.
> 
> This can be useful to save and restore device settings:
> 
>   $ v4l2-ctl --list-ctrls-values >settings.txt 2>/dev/null
>   $ v4l2-ctl --set-ctrl "$(cat settings.txt)"
> 
> The new option has been tested with the vivid driver and it works well
> enough to be useful with a real driver as well.
> 
> String controls are not supported for now, as they may not be parsed
> correctly by --set-ctrl if they contain a comma or a single quote.
> 
> Signed-off-by: Antonio Ospite <ao2@ao2.it>
> ---
>  utils/v4l2-ctl/v4l2-ctl-common.cpp | 72 ++++++++++++++++++++++++++----
>  utils/v4l2-ctl/v4l2-ctl.1.in       |  4 ++
>  utils/v4l2-ctl/v4l2-ctl.cpp        |  1 +
>  utils/v4l2-ctl/v4l2-ctl.h          |  1 +
>  4 files changed, 69 insertions(+), 9 deletions(-)
> 
> diff --git a/utils/v4l2-ctl/v4l2-ctl-common.cpp b/utils/v4l2-ctl/v4l2-ctl-common.cpp
> index 7777b45c..b4124608 100644
> --- a/utils/v4l2-ctl/v4l2-ctl-common.cpp
> +++ b/utils/v4l2-ctl/v4l2-ctl-common.cpp
> @@ -93,6 +93,9 @@ void common_usage(void)
>  	       "  -l, --list-ctrls   display all controls and their values [VIDIOC_QUERYCTRL]\n"
>  	       "  -L, --list-ctrls-menus\n"
>  	       "		     display all controls and their menus [VIDIOC_QUERYMENU]\n"
> +	       "  -m, --list-ctrls-values\n"
> +	       "		     display all controls and their values in a format compatible with\n"
> +	       "		     --set-ctrls (the 'm' stands for \"machine readable output\")\n"
>  	       "  -r, --subset <ctrl>[,<offset>,<size>]+\n"
>  	       "                     the subset of the N-dimensional array to get/set for control <ctrl>,\n"
>  	       "                     for every dimension an (<offset>, <size>) tuple is given.\n"
> @@ -409,6 +412,46 @@ static void print_qctrl(int fd, struct v4l2_query_ext_ctrl *queryctrl,
>  	}
>  }
>  
> +static void print_qctrl_values(int fd, struct v4l2_query_ext_ctrl *queryctrl,
> +		struct v4l2_ext_control *ctrl, int show_menus)
> +{
> +	std::string s = name2var(queryctrl->name);
> +
> +	if (queryctrl->nr_of_dims == 0) {
> +		switch (queryctrl->type) {
> +		case V4L2_CTRL_TYPE_INTEGER:
> +		case V4L2_CTRL_TYPE_BOOLEAN:
> +		case V4L2_CTRL_TYPE_MENU:
> +		case V4L2_CTRL_TYPE_INTEGER_MENU:
> +			printf("%s=%d,", s.c_str(), ctrl->value);
> +			break;
> +		case V4L2_CTRL_TYPE_BITMASK:
> +			printf("%s=0x%08x,", s.c_str(), ctrl->value);
> +			break;
> +		case V4L2_CTRL_TYPE_INTEGER64:
> +			printf("%s=%lld,", s.c_str(), ctrl->value64);
> +			break;
> +		case V4L2_CTRL_TYPE_STRING:
> +			fprintf(stderr, "%s: string controls unsupported for now\n", queryctrl->name);
> +			break;
> +		default:
> +			fprintf(stderr, "%s: unsupported payload type\n", queryctrl->name);
> +			break;
> +		}
> +	}
> +
> +	if (queryctrl->nr_of_dims)
> +		fprintf(stderr, "%s: unsupported payload type (multi-dimensional)\n", queryctrl->name);
> +
> +	if (queryctrl->flags)
> +		fprintf(stderr, "%s: ignoring flags\n", queryctrl->name);
> +
> +	if ((queryctrl->type == V4L2_CTRL_TYPE_MENU ||
> +	     queryctrl->type == V4L2_CTRL_TYPE_INTEGER_MENU) && show_menus) {
> +		fprintf(stderr, "%s: ignoring menus\n", queryctrl->name);
> +	}
> +}
> +
>  static void print_class_name(const char *name)
>  {
>  	printf("\n%s\n\n", name);
> @@ -426,7 +469,8 @@ static int print_control(int fd, struct v4l2_query_ext_ctrl &qctrl, struct print
>  	if (qctrl.flags & V4L2_CTRL_FLAG_DISABLED)
>  		return 1;
>  	if (qctrl.type == V4L2_CTRL_TYPE_CTRL_CLASS) {
> -		format->print_class_name(qctrl.name);
> +		if (format->print_class_name)
> +			format->print_class_name(qctrl.name);
>  		return 1;
>  	}
>  	ext_ctrl.id = qctrl.id;
> @@ -1102,13 +1146,23 @@ void common_get(cv4l_fd &_fd)
>  
>  void common_list(cv4l_fd &fd)
>  {
> -	if (options[OptListCtrls] || options[OptListCtrlsMenus]) {
> -		struct print_format classic_format = {
> -			.print_class_name = print_class_name,
> -			.print_qctrl = print_qctrl,
> -			.show_menus = options[OptListCtrlsMenus],
> -		};
> -
> -		list_controls(fd.g_fd(), &classic_format);
> +	if (options[OptListCtrls] || options[OptListCtrlsMenus] || options[OptListCtrlsValues]) {
> +		if (options[OptListCtrlsValues]) {
> +			struct print_format machine_format = {
> +				.print_class_name = NULL,
> +				.print_qctrl = print_qctrl_values,
> +				.show_menus = 0,
> +			};
> +
> +			list_controls(fd.g_fd(), &machine_format);
> +		} else {
> +			struct print_format classic_format = {
> +				.print_class_name = print_class_name,
> +				.print_qctrl = print_qctrl,
> +				.show_menus = options[OptListCtrlsMenus],
> +			};
> +
> +			list_controls(fd.g_fd(), &classic_format);
> +		}

I don't like this struct print_format.

I would prefer something like this:

Rename print_qctrl to print_qctrl_readable() and create a new print_qctrl:

static void print_qctrl(int fd, struct v4l2_query_ext_ctrl *queryctrl,
                struct v4l2_ext_control *ctrl, int show_menus)
{
	if (options[OptListCtrlsValues])
		print_qctrl_values(fd, queryctrl, ctrl, show_menus);
	else
		print_qctrl_readable(fd, queryctrl, ctrl, show_menus);
}

And in print_control you can just skip printing the class name if
options[OptListCtrlsValues] is set.

I would like to see string controls being supported. I would recommend
to just write the string as a hexdump. It avoids having to escape characters.

The same can be done for compound/array controls. In fact, you could write
all controls that way. It would simplify things a lot.

Also, when options[OptListCtrlsValues] is set you should skip all WRITE_ONLY
controls, all BUTTON controls, and all volatile controls. They are not
relevant if you are just interested in controls that can be set.

Regards,

	Hans

>  	}
>  }
> diff --git a/utils/v4l2-ctl/v4l2-ctl.1.in b/utils/v4l2-ctl/v4l2-ctl.1.in
> index e60c2d49..98cc7b72 100644
> --- a/utils/v4l2-ctl/v4l2-ctl.1.in
> +++ b/utils/v4l2-ctl/v4l2-ctl.1.in
> @@ -98,6 +98,10 @@ Display all controls and their values [VIDIOC_QUERYCTRL].
>  \fB-L\fR, \fB--list-ctrls-menus\fR
>  Display all controls and their menus [VIDIOC_QUERYMENU].
>  .TP
> +\fB-m\fR, \fB--list-ctrls-values\fR
> +display all controls and their values in a format compatible with
> +--set-ctrls (the 'm' stands for "machine readable output")
> +.TP
>  \fB-r\fR, \fB--subset\fR \fI<ctrl>\fR[,\fI<offset>\fR,\fI<size>\fR]+
>  The subset of the N-dimensional array to get/set for control \fI<ctrl>\fR,
>  for every dimension an (\fI<offset>\fR, \fI<size>\fR) tuple is given.
> diff --git a/utils/v4l2-ctl/v4l2-ctl.cpp b/utils/v4l2-ctl/v4l2-ctl.cpp
> index a65262f6..647e1778 100644
> --- a/utils/v4l2-ctl/v4l2-ctl.cpp
> +++ b/utils/v4l2-ctl/v4l2-ctl.cpp
> @@ -142,6 +142,7 @@ static struct option long_options[] = {
>  	{"info", no_argument, 0, OptGetDriverInfo},
>  	{"list-ctrls", no_argument, 0, OptListCtrls},
>  	{"list-ctrls-menus", no_argument, 0, OptListCtrlsMenus},
> +	{"list-ctrls-values", no_argument, 0, OptListCtrlsValues},
>  	{"set-ctrl", required_argument, 0, OptSetCtrl},
>  	{"get-ctrl", required_argument, 0, OptGetCtrl},
>  	{"get-tuner", no_argument, 0, OptGetTuner},
> diff --git a/utils/v4l2-ctl/v4l2-ctl.h b/utils/v4l2-ctl/v4l2-ctl.h
> index 5a52a0a4..e60a1ea1 100644
> --- a/utils/v4l2-ctl/v4l2-ctl.h
> +++ b/utils/v4l2-ctl/v4l2-ctl.h
> @@ -65,6 +65,7 @@ enum Option {
>  	OptConcise = 'k',
>  	OptListCtrls = 'l',
>  	OptListCtrlsMenus = 'L',
> +	OptListCtrlsValues = 'm',
>  	OptListOutputs = 'N',
>  	OptListInputs = 'n',
>  	OptGetOutput = 'O',
>