[libcamera-devel] [PATCH v4 2/6] libcamera: V4L2Device: Support v4l2 menu control
Jacopo Mondi
jacopo at jmondi.org
Thu May 6 11:11:55 CEST 2021
Hi Hiro,
On Thu, May 06, 2021 at 04:54:45PM +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 | 65 +++++++++++++++++++++---
> 2 files changed, 61 insertions(+), 7 deletions(-)
>
> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h
> index 087f07e7..c7a2539c 100644
> --- a/include/libcamera/internal/v4l2_device.h
> +++ b/include/libcamera/internal/v4l2_device.h
> @@ -54,6 +54,9 @@ protected:
> int fd() const { return fd_; }
>
> private:
> + bool createControlInfoForMenu(const v4l2_query_ext_ctrl &ctrl,
> + ControlInfo &ctrlInfo);
> +
> 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 397029ac..7056c71a 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -452,6 +452,7 @@ int V4L2Device::ioctl(unsigned long request, void *argp)
> return 0;
> }
>
> +
Addional empty line
Doesn't checkstyle.py warn you ?
> /**
> * \fn V4L2Device::deviceNode()
> * \brief Retrieve the device node path
> @@ -459,10 +460,47 @@ int V4L2Device::ioctl(unsigned long request, void *argp)
> */
>
> /**
> - * \fn V4L2Device::fd()
> - * \brief Retrieve the V4L2 device file descriptor
> - * \return The V4L2 device file descriptor, -1 if the device node is not open
Where has the fd() documentation gone ?
> + * \brief Create ControlInfo for v4l2 menu ctrl.
> + * \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.
This part of the documentation doesn't apply anymore
> + *
> + * \return True on success or false otherwise
> */
> +bool V4L2Device::createControlInfoForMenu(const v4l2_query_ext_ctrl &ctrl,
> + ControlInfo &ctrlInfo)
Currently we construct V4L2ControlInfo passing in a
v4l2_query_ext_ctrl.
Can't we do the same and detect if we have to enumerate indices if the
control type is MENU, instead of creating a new function here ?
> +{
> + ASSERT(ctrl.type == V4L2_CTRL_TYPE_MENU ||
> + ctrl.type == V4L2_CTRL_TYPE_INTEGER_MENU);
> +
> + std::vector<ControlValue> indices;
> + v4l2_querymenu menu;
> + memset(&menu, 0, sizeof(menu));
> + menu.id = ctrl.id;
> +
> + for (menu.index = ctrl.minimum;
> + static_cast<int>(menu.index) <= ctrl.maximum; menu.index++) {
I was about to complain that you could loop until you don't get
EINVAL, but the V4L2 documentation supports what you have here
"It is possible for VIDIOC_QUERYMENU to return an EINVAL error code
for some indices between minimum and maximum. In that case that
particular menu item is not supported by this driver. Also note that
the minimum value is not necessarily 0."
> + if (ioctl(VIDIOC_QUERYMENU, &menu) != 0)
> + continue;
I would however be loud if the return code is !-EINVAL
> +
> + indices.emplace_back(static_cast<int32_t>(menu.index));
Has emplicing any benefit compared to push_back for POD like an
int32_t ?
> + }
> +
> + if (indices.empty()) {
> + LOG(V4L2, Error)
> + << "No applicable value: " << utils::hex(ctrl.id);
> +
> + return false;
> + }
> +
> + ctrlInfo = ControlInfo(indices);
Can't you return the ctrlInfo directly and exploit RVO ? or would it
become difficult to detect errors ? I would howerv consider try moving
this to the V4L2ControlInfo constructor first.
> +
> + return true;
> +}
>
> /*
> * \brief List and store information about all controls supported by the
> @@ -473,7 +511,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 +521,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 +546,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);
> @@ -535,6 +582,10 @@ void V4L2Device::updateControls(ControlList *ctrls,
> value.set<int64_t>(v4l2Ctrl->value64);
> break;
>
> + case ControlTypeInteger32:
> + value.set<int32_t>(v4l2Ctrl->value);
> + break;
> +
We have this here below:
default:
/*
* \todo To be changed when support for string controls
* will be added.
*/
value.set<int32_t>(v4l2Ctrl->value);
break;
wasn't it enough ?
Thanks
j
> case ControlTypeByte:
> /*
> * No action required, the VIDIOC_[GS]_EXT_CTRLS ioctl
> --
> 2.31.1.607.g51e8a6a459-goog
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list