<div dir="ltr"><div dir="ltr">Hi Jacopo,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Jun 11, 2021 at 1:04 AM 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, Jun 10, 2021 at 05:25:35PM +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>
> Reviewed-by: Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>><br>
> Reviewed-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>><br>
> ---<br>
> include/libcamera/internal/v4l2_device.h | 5 +<br>
> src/libcamera/v4l2_device.cpp | 186 +++++++++++++++--------<br>
> 2 files changed, 127 insertions(+), 64 deletions(-)<br>
><br>
> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h<br>
> index b82e2a14..c318e976 100644<br>
> --- a/include/libcamera/internal/v4l2_device.h<br>
> +++ b/include/libcamera/internal/v4l2_device.h<br>
> @@ -56,6 +56,11 @@ protected:<br>
> int fd() const { return fd_; }<br>
><br>
> private:<br>
> + static ControlType v4l2CtrlType(uint32_t ctrlType);<br>
> + static std::unique_ptr<ControlId> v4l2ControlId(const v4l2_query_ext_ctrl &ctrl);<br>
> + ControlInfo v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl);<br>
> + ControlInfo v4l2MenuControlInfo(const v4l2_query_ext_ctrl &ctrl);<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 b39c6266..cda3769a 100644<br>
> --- a/src/libcamera/v4l2_device.cpp<br>
> +++ b/src/libcamera/v4l2_device.cpp<br>
> @@ -32,69 +32,6 @@ LOG_DEFINE_CATEGORY(V4L2)<br>
><br>
> namespace {<br>
><br>
> -ControlType v4l2CtrlType(uint32_t ctrlType)<br>
> -{<br>
> - switch (ctrlType) {<br>
> - case V4L2_CTRL_TYPE_U8:<br>
> - return ControlTypeByte;<br>
> -<br>
> - case V4L2_CTRL_TYPE_BOOLEAN:<br>
> - return ControlTypeBool;<br>
> -<br>
> - case V4L2_CTRL_TYPE_INTEGER:<br>
> - return ControlTypeInteger32;<br>
> -<br>
> - case V4L2_CTRL_TYPE_INTEGER64:<br>
> - return ControlTypeInteger64;<br>
> -<br>
> - case V4L2_CTRL_TYPE_MENU:<br>
> - case V4L2_CTRL_TYPE_BUTTON:<br>
> - case V4L2_CTRL_TYPE_BITMASK:<br>
> - case V4L2_CTRL_TYPE_INTEGER_MENU:<br>
> - /*<br>
> - * More precise types may be needed, for now use a 32-bit<br>
> - * integer type.<br>
> - */<br>
> - return ControlTypeInteger32;<br>
> -<br>
> - default:<br>
> - return ControlTypeNone;<br>
> - }<br>
> -}<br>
> -<br>
> -std::unique_ptr<ControlId> v4l2ControlId(const v4l2_query_ext_ctrl &ctrl)<br>
> -{<br>
> - const size_t len = strnlen(<a href="http://ctrl.name" rel="noreferrer" target="_blank">ctrl.name</a>, sizeof(<a href="http://ctrl.name" rel="noreferrer" target="_blank">ctrl.name</a>));<br>
> - const std::string name(static_cast<const char *>(<a href="http://ctrl.name" rel="noreferrer" target="_blank">ctrl.name</a>), len);<br>
> - const ControlType type = v4l2CtrlType(ctrl.type);<br>
> -<br>
> - return std::make_unique<ControlId>(<a href="http://ctrl.id" rel="noreferrer" target="_blank">ctrl.id</a>, name, type);<br>
> -}<br>
> -<br>
> -ControlInfo v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl)<br>
> -{<br>
> - switch (ctrl.type) {<br>
> - case V4L2_CTRL_TYPE_U8:<br>
> - return ControlInfo(static_cast<uint8_t>(ctrl.minimum),<br>
> - static_cast<uint8_t>(ctrl.maximum),<br>
> - static_cast<uint8_t>(ctrl.default_value));<br>
> -<br>
> - case V4L2_CTRL_TYPE_BOOLEAN:<br>
> - return ControlInfo(static_cast<bool>(ctrl.minimum),<br>
> - static_cast<bool>(ctrl.maximum),<br>
> - static_cast<bool>(ctrl.default_value));<br>
> -<br>
> - case V4L2_CTRL_TYPE_INTEGER64:<br>
> - return ControlInfo(static_cast<int64_t>(ctrl.minimum),<br>
> - static_cast<int64_t>(ctrl.maximum),<br>
> - static_cast<int64_t>(ctrl.default_value));<br>
> -<br>
> - default:<br>
> - return ControlInfo(static_cast<int32_t>(ctrl.minimum),<br>
> - static_cast<int32_t>(ctrl.maximum),<br>
> - static_cast<int32_t>(ctrl.default_value));<br>
> - }<br>
> -}<br>
> } /* namespace */<br>
<br>
Thanks for addressing my request, but now we have an empty namespace<br>
which could be dropped.<br>
<br>
I have a few more picky comments on the newly added documentation.<br>
With your ack they can be applied when merging.<br>
<br>
><br>
> /**<br>
> @@ -524,6 +461,121 @@ int V4L2Device::ioctl(unsigned long request, void *argp)<br>
> * \return The V4L2 device file descriptor, -1 if the device node is not open<br>
> */<br>
><br>
> +/**<br>
> + * \brief Retrieve ControlType for V4L2 control type<br>
<br>
Retrieve the libcamera control type associated with the V4L2 control type<br>
<br>
> + * \param[in] ctrlType V4L2 control type<br>
> + *<br>
> + * \return ControlType for \a ctrlType<br>
<br>
The ControlType corresponding to \a ctrlType<br>
<br>
> + */<br>
> +ControlType V4L2Device::v4l2CtrlType(uint32_t ctrlType)<br>
> +{<br>
> + switch (ctrlType) {<br>
> + case V4L2_CTRL_TYPE_U8:<br>
> + return ControlTypeByte;<br>
> +<br>
> + case V4L2_CTRL_TYPE_BOOLEAN:<br>
> + return ControlTypeBool;<br>
> +<br>
> + case V4L2_CTRL_TYPE_INTEGER:<br>
> + return ControlTypeInteger32;<br>
> +<br>
> + case V4L2_CTRL_TYPE_INTEGER64:<br>
> + return ControlTypeInteger64;<br>
> +<br>
> + case V4L2_CTRL_TYPE_MENU:<br>
> + case V4L2_CTRL_TYPE_BUTTON:<br>
> + case V4L2_CTRL_TYPE_BITMASK:<br>
> + case V4L2_CTRL_TYPE_INTEGER_MENU:<br>
> + /*<br>
> + * More precise types may be needed, for now use a 32-bit<br>
> + * integer type.<br>
> + */<br>
> + return ControlTypeInteger32;<br>
> +<br>
> + default:<br>
> + return ControlTypeNone;<br>
> + }<br>
> +}<br>
> +<br>
> +/**<br>
> + * \brief Create ControlId for v4l2_query_ext_ctrl<br>
<br>
\brief Create a ControlId for a V4L2 control<br>
<br>
> + * \param[in] ctrl v4l2_query_ext_ctrl<br>
<br>
Your below description sound better:<br>
<br>
\param[in] ctrl The v4l2_query_ext_ctrl that represents a V4L2 control<br>
<br>
> + *<br>
> + * \return ControlId for \a ctrl<br>
> + */<br>
> +std::unique_ptr<ControlId> V4L2Device::v4l2ControlId(const v4l2_query_ext_ctrl &ctrl)<br>
> +{<br>
> + const size_t len = strnlen(<a href="http://ctrl.name" rel="noreferrer" target="_blank">ctrl.name</a>, sizeof(<a href="http://ctrl.name" rel="noreferrer" target="_blank">ctrl.name</a>));<br>
> + const std::string name(static_cast<const char *>(<a href="http://ctrl.name" rel="noreferrer" target="_blank">ctrl.name</a>), len);<br>
> + const ControlType type = v4l2CtrlType(ctrl.type);<br>
> +<br>
> + return std::make_unique<ControlId>(<a href="http://ctrl.id" rel="noreferrer" target="_blank">ctrl.id</a>, name, type);<br>
> +}<br>
> +<br>
> +/**<br>
> + * \brief Create ControlInfo from v4l2_query_ext_ctrl<br>
> + * \param[in] ctrl v4l2_query_ext_ctrl<br>
<br>
Your below description sound better:<br>
<br>
\param[in] ctrl The v4l2_query_ext_ctrl that represents a V4L2 control<br>
<br>
<br>
> + *<br>
> + * \return ControlInfo from \a v4l2_query_ext_ctrl<br>
<br>
A ControlInfo created from \a ctrl<br>
<br>
> + */<br>
> +ControlInfo V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl)<br>
> +{<br>
> + switch (ctrl.type) {<br>
> + case V4L2_CTRL_TYPE_U8:<br>
> + return ControlInfo(static_cast<uint8_t>(ctrl.minimum),<br>
> + static_cast<uint8_t>(ctrl.maximum),<br>
> + static_cast<uint8_t>(ctrl.default_value));<br>
> +<br>
> + case V4L2_CTRL_TYPE_BOOLEAN:<br>
> + return ControlInfo(static_cast<bool>(ctrl.minimum),<br>
> + static_cast<bool>(ctrl.maximum),<br>
> + static_cast<bool>(ctrl.default_value));<br>
> +<br>
> + case V4L2_CTRL_TYPE_INTEGER64:<br>
> + return ControlInfo(static_cast<int64_t>(ctrl.minimum),<br>
> + static_cast<int64_t>(ctrl.maximum),<br>
> + static_cast<int64_t>(ctrl.default_value));<br>
> +<br>
> + case V4L2_CTRL_TYPE_INTEGER_MENU:<br>
> + case V4L2_CTRL_TYPE_MENU:<br>
> + return v4l2MenuControlInfo(ctrl);<br>
> +<br>
> + default:<br>
> + return ControlInfo(static_cast<int32_t>(ctrl.minimum),<br>
> + static_cast<int32_t>(ctrl.maximum),<br>
> + static_cast<int32_t>(ctrl.default_value));<br>
> + }<br>
> +}<br>
> +<br>
> +/**<br>
> + * \brief Create ControlInfo for a V4L2 menu control<br>
> + * \param[in] ctrl The v4l2_query_ext_ctrl that represents a menu<br>
<br>
.. a V4L2 menu control<br>
<br>
> + *<br>
> + * The created ControlInfo contains indices acquired by VIDIOC_QUERYMENU.<br>
> + *<br>
> + * \return ControlInfo for a V4L2 menu control<br>
<br>
I would apply the same suggestion given above<br>
<br></blockquote><div><br></div><div>All of them sound good to me.</div><div>Thanks.</div><div><br></div><div>-Hiro</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> + */<br>
> +ControlInfo V4L2Device::v4l2MenuControlInfo(const struct v4l2_query_ext_ctrl &ctrl)<br>
> +{<br>
> + std::vector<ControlValue> indices;<br>
> + struct v4l2_querymenu 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>
> + if (ctrl.minimum < 0)<br>
> + return ControlInfo();<br>
> +<br>
> + for (int32_t index = ctrl.minimum; index <= ctrl.maximum; ++index) {<br>
> + menu.index = index;<br>
> + if (ioctl(VIDIOC_QUERYMENU, &menu) != 0)<br>
> + continue;<br>
> +<br>
> + indices.push_back(index);<br>
> + }<br>
> +<br>
> + return ControlInfo(indices,<br>
> + ControlValue(static_cast<int32_t>(ctrl.default_value)));<br>
> +}<br>
> +<br>
> /*<br>
> * \brief List and store information about all controls supported by the<br>
> * V4L2 device<br>
> @@ -533,7 +585,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>
> @@ -562,6 +613,9 @@ 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(v4l2ControlId(ctrl));<br>
> controlInfo_.emplace(<a href="http://ctrl.id" rel="noreferrer" target="_blank">ctrl.id</a>, ctrl);<br>
><br>
> @@ -630,6 +684,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>
> case ControlTypeByte:<br>
> /*<br>
> * No action required, the VIDIOC_[GS]_EXT_CTRLS ioctl<br>
> --<br>
> 2.32.0.rc1.229.g3e70b5a671-goog<br>
><br>
</blockquote></div></div>