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

[Laurent Pinchart]

Hi Hiro,

Thank you for the patch.

On Fri, May 28, 2021 at 12:05:27PM +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 |  2 +
>  src/libcamera/v4l2_device.cpp            | 47 ++++++++++++++++++++++--
>  2 files changed, 45 insertions(+), 4 deletions(-)
> 
> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h
> index b82e2a14..687be9b2 100644
> --- a/include/libcamera/internal/v4l2_device.h
> +++ b/include/libcamera/internal/v4l2_device.h
> @@ -56,6 +56,8 @@ protected:
>  	int fd() const { return fd_; }
>  
>  private:
> +	ControlInfo v4l2MenuControlInfo(const v4l2_query_ext_ctrl &ctrl);
> +
>  	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 caafbc2d..032252bb 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -530,6 +530,33 @@ 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

s/for v4l2 menu ctrl/for a V4L2 menu control/

> + * \param[in] ctrl The v4l2_query_ext_ctrl that represents a menu
> + * \param[out] ctrlInfo The created controlInfo

It's not an out parameter, it's the return value.

> + *
> + * The created ControlInfo contains indices acquired by VIDIOC_QUERYMENU.
> + *
> + * \return ControlInfo for v4l2 menu ctrl.
> + */
> +ControlInfo V4L2Device::v4l2MenuControlInfo(const v4l2_query_ext_ctrl &ctrl)
> +{
> +	std::vector<ControlValue> indices;
> +	v4l2_querymenu menu;

We usually prefix the V4L2 structures with struct. While not strictly
mandatory, it emphasizes that there are C structs from V4L2.

> +	memset(&menu, 0, sizeof(menu));

You could also write this

	struct v4l2_querymenu menu = { };

(I really wish designated initializers were available before C++20 :-().

> +	menu.id = ctrl.id;

As a sanity check, should we add

	if (ctrl.minimum <= 0)
		return ControlInfo();

?

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

	for (int32_t index = ctrl.minimum; index <= ctrl.maximum; ++index) }
		menu.index = index;

would allow dropping the cast. Up to you.

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

This could be

		indices.emplace_back(index);

or even

		indices.push_back(index);

as it won't make much of a difference.

> +	}
> +
> +	return ControlInfo(indices);

Would it be useful to also pass the default value to the ControlInfo
constructor ?

> +}
> +
>  /*
>   * \brief List and store information about all controls supported by the
>   * V4L2 device
> @@ -539,7 +566,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;
> @@ -550,16 +576,22 @@ void V4L2Device::listControls()
>  		    ctrl.flags & V4L2_CTRL_FLAG_DISABLED)
>  			continue;
>  
> +		ControlInfo ctrlInfo;

I'd add a blank line here.

>  		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:
> +			ctrlInfo = v4l2MenuControlInfo(ctrl);
>  			break;
> +
>  		/* \todo Support other control types. */
>  		default:
>  			LOG(V4L2, Debug)
> @@ -568,10 +600,13 @@ void V4L2Device::listControls()
>  			continue;
>  		}
>  
> +		LOG(V4L2, Debug) << "Control: " << ctrl.name
> +				 << " (" << utils::hex(ctrl.id) << ")";
> +
>  		controlIds_.emplace_back(v4l2ControlId(ctrl));
>  		controlInfo_.emplace(ctrl.id, ctrl);
>  
> -		ctrls.emplace(controlIds_.back().get(), v4l2ControlInfo(ctrl));
> +		ctrls.emplace(controlIds_.back().get(), std::move(ctrlInfo));
>  	}
>  
>  	controls_ = std::move(ctrls);
> @@ -638,6 +673,10 @@ void V4L2Device::updateControls(ControlList *ctrls,
>  			value.set<int64_t>(v4l2Ctrl->value64);
>  			break;
>  
> +		case ControlTypeInteger32:
> +			value.set<int32_t>(v4l2Ctrl->value);

This needs a rebase, it's now v4l2Ctrl.value.

With these minor issues addressed, the patch looks good to me. Thanks
for your continuous effort.

> +			break;
> +
>  		case ControlTypeByte:
>  			/*
>  			 * No action required, the VIDIOC_[GS]_EXT_CTRLS ioctl

-- 
Regards,

Laurent Pinchart