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