<div dir="ltr"><div dir="ltr">Hi Jacopo, thank you for reviewing.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, May 7, 2021 at 6:01 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 Fri, May 07, 2021 at 11:17:27AM +0900, Hirokazu Honda wrote:<br>
> V4L2ControlId and V4L2ControlInfo are just convenience classes to<br>
> create ControlId and ControlInfo from v4l2_query_ext_control.<br>
> Therefore, there is no need of being a class. It is used only<br>
> from V4L2Device. This removes the classes and put the equivalent<br>
> functions of creating ControlId and ControlInfo in<br>
> v4l2_device.cc.<br>
<br>
It's v4l2_device.cpp :)<br>
<br>
For the ones who didn't follow the menu control support:<br>
- moving menu control support to V4L2Controls would have required<br>
passing the fd there to be able to call QUERYMENU<br>
- having menu controls in v4l2_device and the other contorls in<br>
v4l2_controls felt wrong<br>
- let's move everything to v4l2_device<br>
<br>
><br>
> Signed-off-by: Hirokazu Honda <<a href="mailto:hiroh@chromium.org" target="_blank">hiroh@chromium.org</a>><br>
> ---<br>
> include/libcamera/internal/meson.build | 1 -<br>
> include/libcamera/internal/v4l2_controls.h | 31 -----<br>
> include/libcamera/internal/v4l2_device.h | 4 +-<br>
> src/libcamera/meson.build | 1 -<br>
> src/libcamera/v4l2_controls.cpp | 151 ---------------------<br>
> src/libcamera/v4l2_device.cpp | 72 +++++++++-<br>
> 6 files changed, 71 insertions(+), 189 deletions(-)<br>
> delete mode 100644 include/libcamera/internal/v4l2_controls.h<br>
> delete mode 100644 src/libcamera/v4l2_controls.cpp<br>
><br>
> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build<br>
> index 1fe3918c..3fb0216d 100644<br>
> --- a/include/libcamera/internal/meson.build<br>
> +++ b/include/libcamera/internal/meson.build<br>
> @@ -42,7 +42,6 @@ libcamera_internal_headers = files([<br>
> 'thread.h',<br>
> 'timer.h',<br>
> 'utils.h',<br>
> - 'v4l2_controls.h',<br>
> 'v4l2_device.h',<br>
> 'v4l2_pixelformat.h',<br>
> 'v4l2_subdevice.h',<br>
> diff --git a/include/libcamera/internal/v4l2_controls.h b/include/libcamera/internal/v4l2_controls.h<br>
> deleted file mode 100644<br>
> index 0851b8dd..00000000<br>
> --- a/include/libcamera/internal/v4l2_controls.h<br>
> +++ /dev/null<br>
> @@ -1,31 +0,0 @@<br>
> -/* SPDX-License-Identifier: LGPL-2.1-or-later */<br>
> -/*<br>
> - * Copyright (C) 2019, Google Inc.<br>
> - *<br>
> - * v4l2_controls.h - V4L2 Controls Support<br>
> - */<br>
> -<br>
> -#ifndef __LIBCAMERA_INTERNAL_V4L2_CONTROLS_H__<br>
> -#define __LIBCAMERA_INTERNAL_V4L2_CONTROLS_H__<br>
> -<br>
> -#include <linux/videodev2.h><br>
> -<br>
> -#include <libcamera/controls.h><br>
> -<br>
> -namespace libcamera {<br>
> -<br>
> -class V4L2ControlId : public ControlId<br>
> -{<br>
> -public:<br>
> - V4L2ControlId(const struct v4l2_query_ext_ctrl &ctrl);<br>
> -};<br>
> -<br>
> -class V4L2ControlInfo : public ControlInfo<br>
> -{<br>
> -public:<br>
> - V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl);<br>
> -};<br>
> -<br>
> -} /* namespace libcamera */<br>
> -<br>
> -#endif /* __LIBCAMERA_INTERNAL_V4L2_CONTROLS_H__ */<br>
> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h<br>
> index 087f07e7..116bbbaf 100644<br>
> --- a/include/libcamera/internal/v4l2_device.h<br>
> +++ b/include/libcamera/internal/v4l2_device.h<br>
> @@ -13,11 +13,11 @@<br>
><br>
> #include <linux/videodev2.h><br>
><br>
> +#include <libcamera/controls.h><br>
> #include <libcamera/signal.h><br>
> #include <libcamera/span.h><br>
><br>
> #include "libcamera/internal/log.h"<br>
> -#include "libcamera/internal/v4l2_controls.h"<br>
><br>
> namespace libcamera {<br>
><br>
> @@ -61,7 +61,7 @@ private:<br>
> void eventAvailable(EventNotifier *notifier);<br>
><br>
> std::map<unsigned int, struct v4l2_query_ext_ctrl> controlInfo_;<br>
> - std::vector<std::unique_ptr<V4L2ControlId>> controlIds_;<br>
> + std::vector<std::unique_ptr<ControlId>> controlIds_;<br>
> ControlInfoMap controls_;<br>
> std::string deviceNode_;<br>
> int fd_;<br>
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build<br>
> index e0a48aa2..62251a64 100644<br>
> --- a/src/libcamera/meson.build<br>
> +++ b/src/libcamera/meson.build<br>
> @@ -51,7 +51,6 @@ libcamera_sources = files([<br>
> 'timer.cpp',<br>
> 'transform.cpp',<br>
> 'utils.cpp',<br>
> - 'v4l2_controls.cpp',<br>
> 'v4l2_device.cpp',<br>
> 'v4l2_pixelformat.cpp',<br>
> 'v4l2_subdevice.cpp',<br>
> diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp<br>
> deleted file mode 100644<br>
> index 3f8ec6ca..00000000<br>
> --- a/src/libcamera/v4l2_controls.cpp<br>
> +++ /dev/null<br>
> @@ -1,151 +0,0 @@<br>
> -/* SPDX-License-Identifier: LGPL-2.1-or-later */<br>
> -/*<br>
> - * Copyright (C) 2019, Google Inc.<br>
> - *<br>
> - * v4l2_controls.cpp - V4L2 Controls Support<br>
> - */<br>
> -<br>
> -#include "libcamera/internal/v4l2_controls.h"<br>
> -<br>
> -#include <string.h><br>
> -<br>
> -/**<br>
> - * \file v4l2_controls.h<br>
> - * \brief Support for V4L2 Controls using the V4L2 Extended Controls APIs<br>
> - *<br>
> - * The V4L2 Control API allows application to inspect and modify sets of<br>
> - * configurable parameters on a video device or subdevice. The nature of the<br>
> - * parameters an application can modify using the control framework depends on<br>
> - * what the driver implements support for, and on the characteristics of the<br>
> - * underlying hardware platform. Generally controls are used to modify user<br>
> - * visible settings, such as the image brightness and exposure time, or<br>
> - * non-standard parameters which cannot be controlled through the V4L2 format<br>
> - * negotiation API.<br>
> - *<br>
> - * Controls are identified by a numerical ID, defined by the V4L2 kernel headers<br>
> - * and have an associated type. Each control has a value, which is the data that<br>
> - * can be modified with V4L2Device::setControls() or retrieved with<br>
> - * V4L2Device::getControls().<br>
> - *<br>
> - * The control's type along with the control's flags define the type of the<br>
> - * control's value content. Controls can transport a single data value stored in<br>
> - * variable inside the control, or they might as well deal with more complex<br>
> - * data types, such as arrays of matrices, stored in a contiguous memory<br>
> - * locations associated with the control and called 'the payload'. Such controls<br>
> - * are called 'compound controls' and are currently not supported by the<br>
> - * libcamera V4L2 control framework.<br>
> - *<br>
> - * libcamera implements support for controls using the V4L2 Extended Control<br>
> - * API, which allows future handling of controls with payloads of arbitrary<br>
> - * sizes.<br>
> - *<br>
> - * The libcamera V4L2 Controls framework operates on lists of controls, wrapped<br>
> - * by the ControlList class, to match the V4L2 extended controls API. The<br>
> - * interface to set and get control is implemented by the V4L2Device class, and<br>
> - * this file only provides the data type definitions.<br>
> - *<br>
> - * \todo Add support for compound controls<br>
> - */<br>
> -<br>
> -namespace libcamera {<br>
> -<br>
> -namespace {<br>
> -<br>
> -std::string v4l2_ctrl_name(const struct v4l2_query_ext_ctrl &ctrl)<br>
> -{<br>
> - 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>
> - return std::string(static_cast<const char *>(<a href="http://ctrl.name" rel="noreferrer" target="_blank">ctrl.name</a>), len);<br>
> -}<br>
> -<br>
> -ControlType v4l2_ctrl_type(const struct v4l2_query_ext_ctrl &ctrl)<br>
> -{<br>
> - switch (ctrl.type) {<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>
> -} /* namespace */<br>
> -<br>
> -/**<br>
> - * \class V4L2ControlId<br>
> - * \brief V4L2 control static metadata<br>
> - *<br>
> - * The V4L2ControlId class is a specialisation of the ControlId for V4L2<br>
> - * controls.<br>
> - */<br>
> -<br>
> -/**<br>
> - * \brief Construct a V4L2ControlId from a struct v4l2_query_ext_ctrl<br>
> - * \param[in] ctrl The struct v4l2_query_ext_ctrl as returned by the kernel<br>
> - */<br>
> -V4L2ControlId::V4L2ControlId(const struct v4l2_query_ext_ctrl &ctrl)<br>
> - : ControlId(<a href="http://ctrl.id" rel="noreferrer" target="_blank">ctrl.id</a>, v4l2_ctrl_name(ctrl), v4l2_ctrl_type(ctrl))<br>
> -{<br>
> -}<br>
> -<br>
> -/**<br>
> - * \class V4L2ControlInfo<br>
> - * \brief Convenience specialisation of ControlInfo for V4L2 controls<br>
> - *<br>
> - * The V4L2ControlInfo class is a specialisation of the ControlInfo for V4L2<br>
> - * controls. It offers a convenience constructor from a struct<br>
> - * v4l2_query_ext_ctrl, and is otherwise equivalent to the ControlInfo class.<br>
> - */<br>
> -<br>
> -/**<br>
> - * \brief Construct a V4L2ControlInfo from a struct v4l2_query_ext_ctrl<br>
> - * \param[in] ctrl The struct v4l2_query_ext_ctrl as returned by the kernel<br>
> - */<br>
> -V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)<br>
> -{<br>
> - switch (ctrl.type) {<br>
> - case V4L2_CTRL_TYPE_U8:<br>
> - ControlInfo::operator=(ControlInfo(static_cast<uint8_t>(ctrl.minimum),<br>
> - static_cast<uint8_t>(ctrl.maximum),<br>
> - static_cast<uint8_t>(ctrl.default_value)));<br>
> - break;<br>
> -<br>
> - case V4L2_CTRL_TYPE_BOOLEAN:<br>
> - ControlInfo::operator=(ControlInfo(static_cast<bool>(ctrl.minimum),<br>
> - static_cast<bool>(ctrl.maximum),<br>
> - static_cast<bool>(ctrl.default_value)));<br>
> - break;<br>
> -<br>
> - case V4L2_CTRL_TYPE_INTEGER64:<br>
> - ControlInfo::operator=(ControlInfo(static_cast<int64_t>(ctrl.minimum),<br>
> - static_cast<int64_t>(ctrl.maximum),<br>
> - static_cast<int64_t>(ctrl.default_value)));<br>
> - break;<br>
> -<br>
> - default:<br>
> - ControlInfo::operator=(ControlInfo(static_cast<int32_t>(ctrl.minimum),<br>
> - static_cast<int32_t>(ctrl.maximum),<br>
> - static_cast<int32_t>(ctrl.default_value)));<br>
> - break;<br>
> - }<br>
> -}<br>
> -<br>
> -} /* namespace libcamera */<br>
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp<br>
> index 397029ac..41eb6454 100644<br>
> --- a/src/libcamera/v4l2_device.cpp<br>
> +++ b/src/libcamera/v4l2_device.cpp<br>
> @@ -20,7 +20,6 @@<br>
> #include "libcamera/internal/log.h"<br>
> #include "libcamera/internal/sysfs.h"<br>
> #include "libcamera/internal/utils.h"<br>
> -#include "libcamera/internal/v4l2_controls.h"<br>
><br>
> /**<br>
> * \file v4l2_device.h<br>
> @@ -31,6 +30,73 @@ namespace libcamera {<br>
><br>
> 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> createControlId(const v4l2_query_ext_ctrl &ctrl)<br>
<br>
Do we gain anything in storing these as unique_ptr ? They will be<br>
stored in a vector, which at class deletion time will destroy them,<br>
right ? I know it was like this already though...<br>
<br></blockquote><div><br></div><div>I tried in v2, but I couldn't because neither copy nor move is allowed for ControlId.</div><div>We need to use unique_ptr here. :(</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>
> + 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 createControlInfo(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>
I would question two minor things:<br>
<br>
1) static vs member functions<br>
2) function names, but that's really minor, I would have used<br>
v4l2ControlInfo and v4l2ControlId, but that's just me.<br>
<br>
The patch itself is good and goes in the right direction imho<br>
<br>
Reviewed-by: Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>><br>
<br>
Thanks<br>
j<br>
<br>
> +<br>
> /**<br>
> * \class V4L2Device<br>
> * \brief Base class for V4L2VideoDevice and V4L2Subdevice<br>
> @@ -502,10 +568,10 @@ void V4L2Device::listControls()<br>
> continue;<br>
> }<br>
><br>
> - controlIds_.emplace_back(std::make_unique<V4L2ControlId>(ctrl));<br>
> + controlIds_.emplace_back(createControlId(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(), createControlInfo(ctrl));<br>
> }<br>
><br>
> controls_ = std::move(ctrls);<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>