<div dir="ltr"><div dir="ltr">Hi Laurent,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, May 24, 2021 at 1:20 PM Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</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 Mon, May 24, 2021 at 06:57:03AM +0300, Laurent Pinchart wrote:<br>
> On Mon, May 10, 2021 at 02:42:42PM +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.cpp.<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>
> > ---<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>
> Nice diffstat :-)<br>
> <br>
> Reviewed-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>><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..f9414043 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> 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>
> > /**<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(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>
<br>
There's another instance of V4L2ControlInfo in<br>
V4L2Device::updateControlInfo(). I think it can just be replace with<br>
v4l2ControlInfo(). If that's fine with you, I can handle this when<br>
applying.<br>
<br></blockquote><div><br></div><div>Indeed. Thanks in advance. I have no idea why I haven't caught it when creating this pacth. :(</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">
> > + ctrls.emplace(controlIds_.back().get(), v4l2ControlInfo(ctrl));<br>
> > }<br>
> > <br>
> > controls_ = std::move(ctrls);<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>