<div dir="ltr"><div dir="ltr">Hi Jacopo,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, May 6, 2021 at 6:11 PM Jacopo Mondi <<a href="mailto:jacopo@jmondi.org">jacopo@jmondi.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Hiro,<br>
<br>
On Thu, May 06, 2021 at 04:54:45PM +0900, Hirokazu Honda wrote:<br>
> This adds a support of v4l2 menu. The control info for v4l2 menu<br>
> contains indices without names and 64bit values of querymenu.<br>
><br>
> Signed-off-by: Hirokazu Honda <<a href="mailto:hiroh@chromium.org" target="_blank">hiroh@chromium.org</a>><br>
> ---<br>
> include/libcamera/internal/v4l2_device.h | 3 ++<br>
> src/libcamera/v4l2_device.cpp | 65 +++++++++++++++++++++---<br>
> 2 files changed, 61 insertions(+), 7 deletions(-)<br>
><br>
> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h<br>
> index 087f07e7..c7a2539c 100644<br>
> --- a/include/libcamera/internal/v4l2_device.h<br>
> +++ b/include/libcamera/internal/v4l2_device.h<br>
> @@ -54,6 +54,9 @@ protected:<br>
> int fd() const { return fd_; }<br>
><br>
> private:<br>
> + bool createControlInfoForMenu(const v4l2_query_ext_ctrl &ctrl,<br>
> + ControlInfo &ctrlInfo);<br>
> +<br>
> void listControls();<br>
> void updateControls(ControlList *ctrls,<br>
> Span<const v4l2_ext_control> v4l2Ctrls);<br>
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp<br>
> index 397029ac..7056c71a 100644<br>
> --- a/src/libcamera/v4l2_device.cpp<br>
> +++ b/src/libcamera/v4l2_device.cpp<br>
> @@ -452,6 +452,7 @@ int V4L2Device::ioctl(unsigned long request, void *argp)<br>
> return 0;<br>
> }<br>
><br>
> +<br>
<br>
Addional empty line<br>
<br>
Doesn't checkstyle.py warn you ?<br>
<br>
> /**<br>
> * \fn V4L2Device::deviceNode()<br>
> * \brief Retrieve the device node path<br>
> @@ -459,10 +460,47 @@ int V4L2Device::ioctl(unsigned long request, void *argp)<br>
> */<br>
><br>
> /**<br>
> - * \fn V4L2Device::fd()<br>
> - * \brief Retrieve the V4L2 device file descriptor<br>
> - * \return The V4L2 device file descriptor, -1 if the device node is not open<br>
<br>
Where has the fd() documentation gone ?<br>
<br>
> + * \brief Create ControlInfo for v4l2 menu ctrl.<br>
> + * \param[in] ctrl The v4l2_query_ext_ctrl that represents a menu<br>
> + * \param[out] ctrlInfo The created controlInfo<br>
> + *<br>
> + * The created ControlInfo contains not only values and also extra values, which<br>
> + * are indices and name/value acquired by VIDIOC_QUERYMENU, respectively. The<br>
> + * extra values contains std::string if the type of \a ctrl is<br>
> + * V4L2_CTRL_TYPE_MENU or int64_t otherwise.<br>
<br>
This part of the documentation doesn't apply anymore<br>
<br>
> + *<br>
> + * \return True on success or false otherwise<br>
> */<br>
> +bool V4L2Device::createControlInfoForMenu(const v4l2_query_ext_ctrl &ctrl,<br>
> + ControlInfo &ctrlInfo)<br>
<br>
Currently we construct V4L2ControlInfo passing in a<br>
v4l2_query_ext_ctrl.<br>
<br>
Can't we do the same and detect if we have to enumerate indices if the<br>
control type is MENU, instead of creating a new function here ?<br>
<br></blockquote><div><br></div><div>To do that, we need to pass fd to call QUERYMENU.</div><div>I think the direction is opposite. I would remove V4L2CtrlInfo because it just calls a few functions to construct ControlInfo.</div><div>What do you think?</div><div> </div><div>-Hiro</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +{<br>
> + ASSERT(ctrl.type == V4L2_CTRL_TYPE_MENU ||<br>
> + ctrl.type == V4L2_CTRL_TYPE_INTEGER_MENU);<br>
> +<br>
> + std::vector<ControlValue> indices;<br>
> + v4l2_querymenu menu;<br>
> + memset(&menu, 0, sizeof(menu));<br>
> + <a href="http://menu.id" rel="noreferrer" target="_blank">menu.id</a> = <a href="http://ctrl.id" rel="noreferrer" target="_blank">ctrl.id</a>;<br>
> +<br>
> + for (menu.index = ctrl.minimum;<br>
> + static_cast<int>(menu.index) <= ctrl.maximum; menu.index++) {<br>
<br>
I was about to complain that you could loop until you don't get<br>
EINVAL, but the V4L2 documentation supports what you have here<br>
<br>
"It is possible for VIDIOC_QUERYMENU to return an EINVAL error code<br>
for some indices between minimum and maximum. In that case that<br>
particular menu item is not supported by this driver. Also note that<br>
the minimum value is not necessarily 0."<br>
<br>
> + if (ioctl(VIDIOC_QUERYMENU, &menu) != 0)<br>
> + continue;<br>
<br>
I would however be loud if the return code is !-EINVAL<br>
<br>
> +<br>
> + indices.emplace_back(static_cast<int32_t>(menu.index));<br>
<br>
Has emplicing any benefit compared to push_back for POD like an<br>
int32_t ?<br>
<br>
> + }<br>
> +<br>
> + if (indices.empty()) {<br>
> + LOG(V4L2, Error)<br>
> + << "No applicable value: " << utils::hex(<a href="http://ctrl.id" rel="noreferrer" target="_blank">ctrl.id</a>);<br>
> +<br>
> + return false;<br>
> + }<br>
> +<br>
> + ctrlInfo = ControlInfo(indices);<br>
<br>
Can't you return the ctrlInfo directly and exploit RVO ? or would it<br>
become difficult to detect errors ? I would howerv consider try moving<br>
this to the V4L2ControlInfo constructor first.<br>
<br>
> +<br>
> + return true;<br>
> +}<br>
><br>
> /*<br>
> * \brief List and store information about all controls supported by the<br>
> @@ -473,7 +511,6 @@ void V4L2Device::listControls()<br>
> ControlInfoMap::Map ctrls;<br>
> struct v4l2_query_ext_ctrl ctrl = {};<br>
><br>
> - /* \todo Add support for menu controls. */<br>
> while (1) {<br>
> <a href="http://ctrl.id" rel="noreferrer" target="_blank">ctrl.id</a> |= V4L2_CTRL_FLAG_NEXT_CTRL |<br>
> V4L2_CTRL_FLAG_NEXT_COMPOUND;<br>
> @@ -484,15 +521,22 @@ void V4L2Device::listControls()<br>
> ctrl.flags & V4L2_CTRL_FLAG_DISABLED)<br>
> continue;<br>
><br>
> + ControlInfo ctrlInfo;<br>
> switch (ctrl.type) {<br>
> case V4L2_CTRL_TYPE_INTEGER:<br>
> case V4L2_CTRL_TYPE_BOOLEAN:<br>
> - case V4L2_CTRL_TYPE_MENU:<br>
> case V4L2_CTRL_TYPE_BUTTON:<br>
> case V4L2_CTRL_TYPE_INTEGER64:<br>
> case V4L2_CTRL_TYPE_BITMASK:<br>
> - case V4L2_CTRL_TYPE_INTEGER_MENU:<br>
> case V4L2_CTRL_TYPE_U8:<br>
> + ctrlInfo = V4L2ControlInfo(ctrl);<br>
> + break;<br>
> +<br>
> + case V4L2_CTRL_TYPE_INTEGER_MENU:<br>
> + case V4L2_CTRL_TYPE_MENU:<br>
> + if (!createControlInfoForMenu(ctrl, ctrlInfo))<br>
> + continue;<br>
> +<br>
> break;<br>
> /* \todo Support other control types. */<br>
> default:<br>
> @@ -502,10 +546,13 @@ void V4L2Device::listControls()<br>
> continue;<br>
> }<br>
><br>
> + LOG(V4L2, Debug) << "Control: " << <a href="http://ctrl.name" rel="noreferrer" target="_blank">ctrl.name</a><br>
> + << " (" << utils::hex(<a href="http://ctrl.id" rel="noreferrer" target="_blank">ctrl.id</a>) << ")";<br>
> +<br>
> controlIds_.emplace_back(std::make_unique<V4L2ControlId>(ctrl));<br>
> controlInfo_.emplace(<a href="http://ctrl.id" rel="noreferrer" target="_blank">ctrl.id</a>, ctrl);<br>
><br>
> - ctrls.emplace(controlIds_.back().get(), V4L2ControlInfo(ctrl));<br>
> + ctrls.emplace(controlIds_.back().get(), ctrlInfo);<br>
> }<br>
><br>
> controls_ = std::move(ctrls);<br>
> @@ -535,6 +582,10 @@ void V4L2Device::updateControls(ControlList *ctrls,<br>
> value.set<int64_t>(v4l2Ctrl->value64);<br>
> break;<br>
><br>
> + case ControlTypeInteger32:<br>
> + value.set<int32_t>(v4l2Ctrl->value);<br>
> + break;<br>
> +<br>
<br>
We have this here below:<br>
<br>
default:<br>
/*<br>
* \todo To be changed when support for string controls<br>
* will be added.<br>
*/<br>
value.set<int32_t>(v4l2Ctrl->value);<br>
break;<br>
<br>
wasn't it enough ?<br>
<br>
Thanks<br>
j<br>
<br>
> case ControlTypeByte:<br>
> /*<br>
> * No action required, the VIDIOC_[GS]_EXT_CTRLS ioctl<br>
> --<br>
> 2.31.1.607.g51e8a6a459-goog<br>
><br>
> _______________________________________________<br>
> libcamera-devel mailing list<br>
> <a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a><br>
> <a href="https://lists.libcamera.org/listinfo/libcamera-devel" rel="noreferrer" target="_blank">https://lists.libcamera.org/listinfo/libcamera-devel</a><br>
</blockquote></div></div>