[libcamera-devel] [PATCH v5 2/6] libcamera: V4L2Device: Support v4l2 menu control

[Jacopo Mondi]

Hi Hiro,

On Wed, May 19, 2021 at 04:59:37PM +0900, Hirokazu Honda wrote:
> This adds a support of v4l2 menu. The control info for v4l2 menu
> contains indices without names and 64bit values of querymenu.
>
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
>  include/libcamera/internal/v4l2_device.h |  3 ++
>  src/libcamera/v4l2_device.cpp            | 64 ++++++++++++++++++++++--
>  2 files changed, 63 insertions(+), 4 deletions(-)
>
> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h
> index 5ba9b23b..34080b07 100644
> --- a/include/libcamera/internal/v4l2_device.h
> +++ b/include/libcamera/internal/v4l2_device.h
> @@ -56,6 +56,9 @@ protected:
>  	int fd() const { return fd_; }
>
>  private:
> +	bool createControlInfoForMenu(const v4l2_query_ext_ctrl &ctrl,
> +				      ControlInfo &ctrlInfo);
> +

We create ControlInfo from a v4l2_query_ext_ctrl with
V4L2Device::v4l2ControlInfo()

I would name this V4L2Device::v4l2MenuControlInfo() or maybe just
expand V4L2Device::v4l2ControlInfo() with support for menu controls

>  	void listControls();
>  	void updateControls(ControlList *ctrls,
>  			    Span<const v4l2_ext_control> v4l2Ctrls);
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index 515cbdfe..9f7873f1 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -464,6 +464,49 @@ int V4L2Device::ioctl(unsigned long request, void *argp)
>   * \return The V4L2 device file descriptor, -1 if the device node is not open
>   */
>
> +/**
> + * \brief Create ControlInfo for v4l2 menu ctrl.

No '.' at the end of briefs

> + * \param[in] ctrl The v4l2_query_ext_ctrl that represents a menu
> + * \param[out] ctrlInfo The created controlInfo
> + *
> + * The created ControlInfo contains not only values and also extra values, which
> + * are indices and name/value acquired by VIDIOC_QUERYMENU, respectively. The
> + * extra values contains std::string if the type of \a ctrl is
> + * V4L2_CTRL_TYPE_MENU or int64_t otherwise.

I don't think this applies in full anymore

> + *
> + * \return True on success or false otherwise
> + */
> +bool V4L2Device::createControlInfoForMenu(const v4l2_query_ext_ctrl &ctrl,
> +					  ControlInfo &ctrlInfo)

ctrlInfo is an output parameter, a * is better suited

> +{
> +	ASSERT(ctrl.type == V4L2_CTRL_TYPE_MENU ||
> +	       ctrl.type == V4L2_CTRL_TYPE_INTEGER_MENU);

This is internal code, the assertion makes only sure this patch does
what you intend, does it bring any value ?

> +
> +	std::vector<ControlValue> indices;
> +	v4l2_querymenu menu;
> +	memset(&menu, 0, sizeof(menu));

I don't recall if we already had a discussion about using menu{};

> +	menu.id = ctrl.id;
> +
> +	for (menu.index = ctrl.minimum;
> +	     static_cast<int>(menu.index) <= ctrl.maximum; menu.index++) {

Do you need the cast ? Here it compiles fine without (clang 11.1.0)


> +		if (ioctl(VIDIOC_QUERYMENU, &menu) != 0)
> +			continue;
> +
> +		indices.emplace_back(static_cast<int32_t>(menu.index));
> +	}
> +
> +	if (indices.empty()) {

Can this happen ?
More in general, can this function fail at all ?

I don't like we have
        ctrlInfo = v4l2ControlInfo(ctrl)
and a few lines below
        if (!createControlInfoForMenu(ctrl, ctrlInfo);

which are basically doing the same thing (create a ControlInfo from a
v4l2_query_ext_ctrl)

I would suggest either
        ctrlInfo = v4l2ControlInfo(ctrl)
        ....
        ctrlInfo = v4l2MenuControlInfo(ctrl)

or just support menu in v4l2ControlInfo()


> +		LOG(V4L2, Error)
> +			<< "No applicable value: " << utils::hex(ctrl.id);
> +
> +		return false;
> +	}
> +
> +	ctrlInfo = ControlInfo(indices);
> +
> +	return true;
> +}
> +
>  /*
>   * \brief List and store information about all controls supported by the
>   * V4L2 device
> @@ -473,7 +516,6 @@ void V4L2Device::listControls()
>  	ControlInfoMap::Map ctrls;
>  	struct v4l2_query_ext_ctrl ctrl = {};
>
> -	/* \todo Add support for menu controls. */
>  	while (1) {
>  		ctrl.id |= V4L2_CTRL_FLAG_NEXT_CTRL |
>  			   V4L2_CTRL_FLAG_NEXT_COMPOUND;
> @@ -484,15 +526,22 @@ void V4L2Device::listControls()
>  		    ctrl.flags & V4L2_CTRL_FLAG_DISABLED)
>  			continue;
>
> +		ControlInfo ctrlInfo;
>  		switch (ctrl.type) {
>  		case V4L2_CTRL_TYPE_INTEGER:
>  		case V4L2_CTRL_TYPE_BOOLEAN:
> -		case V4L2_CTRL_TYPE_MENU:
>  		case V4L2_CTRL_TYPE_BUTTON:
>  		case V4L2_CTRL_TYPE_INTEGER64:
>  		case V4L2_CTRL_TYPE_BITMASK:
> -		case V4L2_CTRL_TYPE_INTEGER_MENU:
>  		case V4L2_CTRL_TYPE_U8:
> +			ctrlInfo = V4L2ControlInfo(ctrl);
> +			break;
> +
> +		case V4L2_CTRL_TYPE_INTEGER_MENU:
> +		case V4L2_CTRL_TYPE_MENU:
> +			if (!createControlInfoForMenu(ctrl, ctrlInfo))
> +				continue;
> +
>  			break;
>  		/* \todo Support other control types. */
>  		default:
> @@ -502,10 +551,13 @@ void V4L2Device::listControls()
>  			continue;
>  		}
>
> +		LOG(V4L2, Debug) << "Control: " << ctrl.name
> +				 << " (" << utils::hex(ctrl.id) << ")";
> +
>  		controlIds_.emplace_back(std::make_unique<V4L2ControlId>(ctrl));
>  		controlInfo_.emplace(ctrl.id, ctrl);
>
> -		ctrls.emplace(controlIds_.back().get(), V4L2ControlInfo(ctrl));
> +		ctrls.emplace(controlIds_.back().get(), ctrlInfo);
>  	}
>
>  	controls_ = std::move(ctrls);
> @@ -572,6 +624,10 @@ void V4L2Device::updateControls(ControlList *ctrls,
>  			value.set<int64_t>(v4l2Ctrl->value64);
>  			break;
>
> +		case ControlTypeInteger32:
> +			value.set<int32_t>(v4l2Ctrl->value);
> +			break;
> +
>  		case ControlTypeByte:
>  			/*
>  			 * No action required, the VIDIOC_[GS]_EXT_CTRLS ioctl
> --
> 2.31.1.751.gd2f1c929bd-goog
>