[{"id":16823,"web_url":"https://patchwork.libcamera.org/comment/16823/","msgid":"<20210507090238.c3bbaibcvcejwwdi@uno.localdomain>","date":"2021-05-07T09:02:38","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: V4L2Control: remove\n\tV4L2Control classes","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\nOn Fri, May 07, 2021 at 11:17:27AM +0900, Hirokazu Honda wrote:\n> V4L2ControlId and V4L2ControlInfo are just convenience classes to\n> create ControlId and ControlInfo from v4l2_query_ext_control.\n> Therefore, there is no need of being a class. It is used only\n> from V4L2Device. This removes the classes and put the equivalent\n> functions of creating ControlId and ControlInfo in\n> v4l2_device.cc.\n\nIt's v4l2_device.cpp :)\n\nFor the ones who didn't follow the menu control support:\n- moving menu control support to V4L2Controls would have required\n passing the fd there to be able to call QUERYMENU\n- having menu controls in v4l2_device and the other contorls in\n v4l2_controls felt wrong\n- let's move everything to v4l2_device\n\n>\n> Signed-off-by: Hirokazu Honda \n> ---\n> include/libcamera/internal/meson.build | 1 -\n> include/libcamera/internal/v4l2_controls.h | 31 -----\n> include/libcamera/internal/v4l2_device.h | 4 +-\n> src/libcamera/meson.build | 1 -\n> src/libcamera/v4l2_controls.cpp | 151 ---------------------\n> src/libcamera/v4l2_device.cpp | 72 +++++++++-\n> 6 files changed, 71 insertions(+), 189 deletions(-)\n> delete mode 100644 include/libcamera/internal/v4l2_controls.h\n> delete mode 100644 src/libcamera/v4l2_controls.cpp\n>\n> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> index 1fe3918c..3fb0216d 100644\n> --- a/include/libcamera/internal/meson.build\n> +++ b/include/libcamera/internal/meson.build\n> @@ -42,7 +42,6 @@ libcamera_internal_headers = files([\n> 'thread.h',\n> 'timer.h',\n> 'utils.h',\n> - 'v4l2_controls.h',\n> 'v4l2_device.h',\n> 'v4l2_pixelformat.h',\n> 'v4l2_subdevice.h',\n> diff --git a/include/libcamera/internal/v4l2_controls.h b/include/libcamera/internal/v4l2_controls.h\n> deleted file mode 100644\n> index 0851b8dd..00000000\n> --- a/include/libcamera/internal/v4l2_controls.h\n> +++ /dev/null\n> @@ -1,31 +0,0 @@\n> -/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> -/*\n> - * Copyright (C) 2019, Google Inc.\n> - *\n> - * v4l2_controls.h - V4L2 Controls Support\n> - */\n> -\n> -#ifndef __LIBCAMERA_INTERNAL_V4L2_CONTROLS_H__\n> -#define __LIBCAMERA_INTERNAL_V4L2_CONTROLS_H__\n> -\n> -#include \n> -\n> -#include \n> -\n> -namespace libcamera {\n> -\n> -class V4L2ControlId : public ControlId\n> -{\n> -public:\n> -\tV4L2ControlId(const struct v4l2_query_ext_ctrl &ctrl);\n> -};\n> -\n> -class V4L2ControlInfo : public ControlInfo\n> -{\n> -public:\n> -\tV4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl);\n> -};\n> -\n> -} /* namespace libcamera */\n> -\n> -#endif /* __LIBCAMERA_INTERNAL_V4L2_CONTROLS_H__ */\n> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> index 087f07e7..116bbbaf 100644\n> --- a/include/libcamera/internal/v4l2_device.h\n> +++ b/include/libcamera/internal/v4l2_device.h\n> @@ -13,11 +13,11 @@\n>\n> #include \n>\n> +#include \n> #include \n> #include \n>\n> #include \"libcamera/internal/log.h\"\n> -#include \"libcamera/internal/v4l2_controls.h\"\n>\n> namespace libcamera {\n>\n> @@ -61,7 +61,7 @@ private:\n> \tvoid eventAvailable(EventNotifier *notifier);\n>\n> \tstd::map controlInfo_;\n> -\tstd::vector> controlIds_;\n> +\tstd::vector> controlIds_;\n> \tControlInfoMap controls_;\n> \tstd::string deviceNode_;\n> \tint fd_;\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index e0a48aa2..62251a64 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -51,7 +51,6 @@ libcamera_sources = files([\n> 'timer.cpp',\n> 'transform.cpp',\n> 'utils.cpp',\n> - 'v4l2_controls.cpp',\n> 'v4l2_device.cpp',\n> 'v4l2_pixelformat.cpp',\n> 'v4l2_subdevice.cpp',\n> diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp\n> deleted file mode 100644\n> index 3f8ec6ca..00000000\n> --- a/src/libcamera/v4l2_controls.cpp\n> +++ /dev/null\n> @@ -1,151 +0,0 @@\n> -/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> -/*\n> - * Copyright (C) 2019, Google Inc.\n> - *\n> - * v4l2_controls.cpp - V4L2 Controls Support\n> - */\n> -\n> -#include \"libcamera/internal/v4l2_controls.h\"\n> -\n> -#include \n> -\n> -/**\n> - * \\file v4l2_controls.h\n> - * \\brief Support for V4L2 Controls using the V4L2 Extended Controls APIs\n> - *\n> - * The V4L2 Control API allows application to inspect and modify sets of\n> - * configurable parameters on a video device or subdevice. The nature of the\n> - * parameters an application can modify using the control framework depends on\n> - * what the driver implements support for, and on the characteristics of the\n> - * underlying hardware platform. Generally controls are used to modify user\n> - * visible settings, such as the image brightness and exposure time, or\n> - * non-standard parameters which cannot be controlled through the V4L2 format\n> - * negotiation API.\n> - *\n> - * Controls are identified by a numerical ID, defined by the V4L2 kernel headers\n> - * and have an associated type. Each control has a value, which is the data that\n> - * can be modified with V4L2Device::setControls() or retrieved with\n> - * V4L2Device::getControls().\n> - *\n> - * The control's type along with the control's flags define the type of the\n> - * control's value content. Controls can transport a single data value stored in\n> - * variable inside the control, or they might as well deal with more complex\n> - * data types, such as arrays of matrices, stored in a contiguous memory\n> - * locations associated with the control and called 'the payload'. Such controls\n> - * are called 'compound controls' and are currently not supported by the\n> - * libcamera V4L2 control framework.\n> - *\n> - * libcamera implements support for controls using the V4L2 Extended Control\n> - * API, which allows future handling of controls with payloads of arbitrary\n> - * sizes.\n> - *\n> - * The libcamera V4L2 Controls framework operates on lists of controls, wrapped\n> - * by the ControlList class, to match the V4L2 extended controls API. The\n> - * interface to set and get control is implemented by the V4L2Device class, and\n> - * this file only provides the data type definitions.\n> - *\n> - * \\todo Add support for compound controls\n> - */\n> -\n> -namespace libcamera {\n> -\n> -namespace {\n> -\n> -std::string v4l2_ctrl_name(const struct v4l2_query_ext_ctrl &ctrl)\n> -{\n> -\tsize_t len = strnlen(ctrl.name, sizeof(ctrl.name));\n> -\treturn std::string(static_cast(ctrl.name), len);\n> -}\n> -\n> -ControlType v4l2_ctrl_type(const struct v4l2_query_ext_ctrl &ctrl)\n> -{\n> -\tswitch (ctrl.type) {\n> -\tcase V4L2_CTRL_TYPE_U8:\n> -\t\treturn ControlTypeByte;\n> -\n> -\tcase V4L2_CTRL_TYPE_BOOLEAN:\n> -\t\treturn ControlTypeBool;\n> -\n> -\tcase V4L2_CTRL_TYPE_INTEGER:\n> -\t\treturn ControlTypeInteger32;\n> -\n> -\tcase V4L2_CTRL_TYPE_INTEGER64:\n> -\t\treturn ControlTypeInteger64;\n> -\n> -\tcase V4L2_CTRL_TYPE_MENU:\n> -\tcase V4L2_CTRL_TYPE_BUTTON:\n> -\tcase V4L2_CTRL_TYPE_BITMASK:\n> -\tcase V4L2_CTRL_TYPE_INTEGER_MENU:\n> -\t\t/*\n> -\t\t * More precise types may be needed, for now use a 32-bit\n> -\t\t * integer type.\n> -\t\t */\n> -\t\treturn ControlTypeInteger32;\n> -\n> -\tdefault:\n> -\t\treturn ControlTypeNone;\n> -\t}\n> -}\n> -\n> -} /* namespace */\n> -\n> -/**\n> - * \\class V4L2ControlId\n> - * \\brief V4L2 control static metadata\n> - *\n> - * The V4L2ControlId class is a specialisation of the ControlId for V4L2\n> - * controls.\n> - */\n> -\n> -/**\n> - * \\brief Construct a V4L2ControlId from a struct v4l2_query_ext_ctrl\n> - * \\param[in] ctrl The struct v4l2_query_ext_ctrl as returned by the kernel\n> - */\n> -V4L2ControlId::V4L2ControlId(const struct v4l2_query_ext_ctrl &ctrl)\n> -\t: ControlId(ctrl.id, v4l2_ctrl_name(ctrl), v4l2_ctrl_type(ctrl))\n> -{\n> -}\n> -\n> -/**\n> - * \\class V4L2ControlInfo\n> - * \\brief Convenience specialisation of ControlInfo for V4L2 controls\n> - *\n> - * The V4L2ControlInfo class is a specialisation of the ControlInfo for V4L2\n> - * controls. It offers a convenience constructor from a struct\n> - * v4l2_query_ext_ctrl, and is otherwise equivalent to the ControlInfo class.\n> - */\n> -\n> -/**\n> - * \\brief Construct a V4L2ControlInfo from a struct v4l2_query_ext_ctrl\n> - * \\param[in] ctrl The struct v4l2_query_ext_ctrl as returned by the kernel\n> - */\n> -V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)\n> -{\n> -\tswitch (ctrl.type) {\n> -\tcase V4L2_CTRL_TYPE_U8:\n> -\t\tControlInfo::operator=(ControlInfo(static_cast(ctrl.minimum),\n> -\t\t\t\t\t\t static_cast(ctrl.maximum),\n> -\t\t\t\t\t\t static_cast(ctrl.default_value)));\n> -\t\tbreak;\n> -\n> -\tcase V4L2_CTRL_TYPE_BOOLEAN:\n> -\t\tControlInfo::operator=(ControlInfo(static_cast(ctrl.minimum),\n> -\t\t\t\t\t\t static_cast(ctrl.maximum),\n> -\t\t\t\t\t\t static_cast(ctrl.default_value)));\n> -\t\tbreak;\n> -\n> -\tcase V4L2_CTRL_TYPE_INTEGER64:\n> -\t\tControlInfo::operator=(ControlInfo(static_cast(ctrl.minimum),\n> -\t\t\t\t\t\t static_cast(ctrl.maximum),\n> -\t\t\t\t\t\t static_cast(ctrl.default_value)));\n> -\t\tbreak;\n> -\n> -\tdefault:\n> -\t\tControlInfo::operator=(ControlInfo(static_cast(ctrl.minimum),\n> -\t\t\t\t\t\t static_cast(ctrl.maximum),\n> -\t\t\t\t\t\t static_cast(ctrl.default_value)));\n> -\t\tbreak;\n> -\t}\n> -}\n> -\n> -} /* namespace libcamera */\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index 397029ac..41eb6454 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -20,7 +20,6 @@\n> #include \"libcamera/internal/log.h\"\n> #include \"libcamera/internal/sysfs.h\"\n> #include \"libcamera/internal/utils.h\"\n> -#include \"libcamera/internal/v4l2_controls.h\"\n>\n> /**\n> * \\file v4l2_device.h\n> @@ -31,6 +30,73 @@ namespace libcamera {\n>\n> LOG_DEFINE_CATEGORY(V4L2)\n>\n> +namespace {\n> +\n> +ControlType v4l2CtrlType(uint32_t ctrlType)\n> +{\n> +\tswitch (ctrlType) {\n> +\tcase V4L2_CTRL_TYPE_U8:\n> +\t\treturn ControlTypeByte;\n> +\n> +\tcase V4L2_CTRL_TYPE_BOOLEAN:\n> +\t\treturn ControlTypeBool;\n> +\n> +\tcase V4L2_CTRL_TYPE_INTEGER:\n> +\t\treturn ControlTypeInteger32;\n> +\n> +\tcase V4L2_CTRL_TYPE_INTEGER64:\n> +\t\treturn ControlTypeInteger64;\n> +\n> +\tcase V4L2_CTRL_TYPE_MENU:\n> +\tcase V4L2_CTRL_TYPE_BUTTON:\n> +\tcase V4L2_CTRL_TYPE_BITMASK:\n> +\tcase V4L2_CTRL_TYPE_INTEGER_MENU:\n> +\t\t/*\n> +\t\t * More precise types may be needed, for now use a 32-bit\n> +\t\t * integer type.\n> +\t\t */\n> +\t\treturn ControlTypeInteger32;\n> +\n> +\tdefault:\n> +\t\treturn ControlTypeNone;\n> +\t}\n> +}\n> +\n> +std::unique_ptr createControlId(const v4l2_query_ext_ctrl &ctrl)\n\nDo we gain anything in storing these as unique_ptr ? They will be\nstored in a vector, which at class deletion time will destroy them,\nright ? I know it was like this already though...\n\n> +{\n> +\tconst size_t len = strnlen(ctrl.name, sizeof(ctrl.name));\n> +\tconst std::string name(static_cast(ctrl.name), len);\n> +\tconst ControlType type = v4l2CtrlType(ctrl.type);\n> +\n> +\treturn std::make_unique(ctrl.id, name, type);\n> +}\n> +\n> +ControlInfo createControlInfo(const v4l2_query_ext_ctrl &ctrl)\n> +{\n> +\tswitch (ctrl.type) {\n> +\tcase V4L2_CTRL_TYPE_U8:\n> +\t\treturn ControlInfo(static_cast(ctrl.minimum),\n> +\t\t\t\t static_cast(ctrl.maximum),\n> +\t\t\t\t static_cast(ctrl.default_value));\n> +\n> +\tcase V4L2_CTRL_TYPE_BOOLEAN:\n> +\t\treturn ControlInfo(static_cast(ctrl.minimum),\n> +\t\t\t\t static_cast(ctrl.maximum),\n> +\t\t\t\t static_cast(ctrl.default_value));\n> +\n> +\tcase V4L2_CTRL_TYPE_INTEGER64:\n> +\t\treturn ControlInfo(static_cast(ctrl.minimum),\n> +\t\t\t\t static_cast(ctrl.maximum),\n> +\t\t\t\t static_cast(ctrl.default_value));\n> +\n> +\tdefault:\n> +\t\treturn ControlInfo(static_cast(ctrl.minimum),\n> +\t\t\t\t static_cast(ctrl.maximum),\n> +\t\t\t\t static_cast(ctrl.default_value));\n> +\t}\n> +}\n> +} /* namespace */\n\nI would question two minor things:\n\n1) static vs member functions\n2) function names, but that's really minor, I would have used\nv4l2ControlInfo and v4l2ControlId, but that's just me.\n\nThe patch itself is good and goes in the right direction imho\n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> +\n> /**\n> * \\class V4L2Device\n> * \\brief Base class for V4L2VideoDevice and V4L2Subdevice\n> @@ -502,10 +568,10 @@ void V4L2Device::listControls()\n> \t\t\tcontinue;\n> \t\t}\n>\n> -\t\tcontrolIds_.emplace_back(std::make_unique(ctrl));\n> +\t\tcontrolIds_.emplace_back(createControlId(ctrl));\n> \t\tcontrolInfo_.emplace(ctrl.id, ctrl);\n>\n> -\t\tctrls.emplace(controlIds_.back().get(), V4L2ControlInfo(ctrl));\n> +\t\tctrls.emplace(controlIds_.back().get(), createControlInfo(ctrl));\n> \t}\n>\n> \tcontrols_ = std::move(ctrls);\n> --\n> 2.31.1.607.g51e8a6a459-goog\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id E7CCFBF829\n\tfor ;\n\tFri, 7 May 2021 09:01:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B374F6890E;\n\tFri, 7 May 2021 11:01:56 +0200 (CEST)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3A51C688E4\n\tfor ;\n\tFri, 7 May 2021 11:01:55 +0200 (CEST)","from uno.localdomain (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id B5D02100005;\n\tFri, 7 May 2021 09:01:54 +0000 (UTC)"],"Date":"Fri, 7 May 2021 11:02:38 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210507090238.c3bbaibcvcejwwdi@uno.localdomain>","References":"<20210507021727.1848611-1-hiroh@chromium.org>\n\t<20210507021727.1848611-3-hiroh@chromium.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210507021727.1848611-3-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: V4L2Control: remove\n\tV4L2Control classes","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16849,"web_url":"https://patchwork.libcamera.org/comment/16849/","msgid":"","date":"2021-05-10T05:44:30","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: V4L2Control: remove\n\tV4L2Control classes","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo, thank you for reviewing.\n\nOn Fri, May 7, 2021 at 6:01 PM Jacopo Mondi wrote:\n\n> Hi Hiro,\n>\n> On Fri, May 07, 2021 at 11:17:27AM +0900, Hirokazu Honda wrote:\n> > V4L2ControlId and V4L2ControlInfo are just convenience classes to\n> > create ControlId and ControlInfo from v4l2_query_ext_control.\n> > Therefore, there is no need of being a class. It is used only\n> > from V4L2Device. This removes the classes and put the equivalent\n> > functions of creating ControlId and ControlInfo in\n> > v4l2_device.cc.\n>\n> It's v4l2_device.cpp :)\n>\n> For the ones who didn't follow the menu control support:\n> - moving menu control support to V4L2Controls would have required\n> passing the fd there to be able to call QUERYMENU\n> - having menu controls in v4l2_device and the other contorls in\n> v4l2_controls felt wrong\n> - let's move everything to v4l2_device\n>\n> >\n> > Signed-off-by: Hirokazu Honda \n> > ---\n> > include/libcamera/internal/meson.build | 1 -\n> > include/libcamera/internal/v4l2_controls.h | 31 -----\n> > include/libcamera/internal/v4l2_device.h | 4 +-\n> > src/libcamera/meson.build | 1 -\n> > src/libcamera/v4l2_controls.cpp | 151 ---------------------\n> > src/libcamera/v4l2_device.cpp | 72 +++++++++-\n> > 6 files changed, 71 insertions(+), 189 deletions(-)\n> > delete mode 100644 include/libcamera/internal/v4l2_controls.h\n> > delete mode 100644 src/libcamera/v4l2_controls.cpp\n> >\n> > diff --git a/include/libcamera/internal/meson.build\n> b/include/libcamera/internal/meson.build\n> > index 1fe3918c..3fb0216d 100644\n> > --- a/include/libcamera/internal/meson.build\n> > +++ b/include/libcamera/internal/meson.build\n> > @@ -42,7 +42,6 @@ libcamera_internal_headers = files([\n> > 'thread.h',\n> > 'timer.h',\n> > 'utils.h',\n> > - 'v4l2_controls.h',\n> > 'v4l2_device.h',\n> > 'v4l2_pixelformat.h',\n> > 'v4l2_subdevice.h',\n> > diff --git a/include/libcamera/internal/v4l2_controls.h\n> b/include/libcamera/internal/v4l2_controls.h\n> > deleted file mode 100644\n> > index 0851b8dd..00000000\n> > --- a/include/libcamera/internal/v4l2_controls.h\n> > +++ /dev/null\n> > @@ -1,31 +0,0 @@\n> > -/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > -/*\n> > - * Copyright (C) 2019, Google Inc.\n> > - *\n> > - * v4l2_controls.h - V4L2 Controls Support\n> > - */\n> > -\n> > -#ifndef __LIBCAMERA_INTERNAL_V4L2_CONTROLS_H__\n> > -#define __LIBCAMERA_INTERNAL_V4L2_CONTROLS_H__\n> > -\n> > -#include \n> > -\n> > -#include \n> > -\n> > -namespace libcamera {\n> > -\n> > -class V4L2ControlId : public ControlId\n> > -{\n> > -public:\n> > - V4L2ControlId(const struct v4l2_query_ext_ctrl &ctrl);\n> > -};\n> > -\n> > -class V4L2ControlInfo : public ControlInfo\n> > -{\n> > -public:\n> > - V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl);\n> > -};\n> > -\n> > -} /* namespace libcamera */\n> > -\n> > -#endif /* __LIBCAMERA_INTERNAL_V4L2_CONTROLS_H__ */\n> > diff --git a/include/libcamera/internal/v4l2_device.h\n> b/include/libcamera/internal/v4l2_device.h\n> > index 087f07e7..116bbbaf 100644\n> > --- a/include/libcamera/internal/v4l2_device.h\n> > +++ b/include/libcamera/internal/v4l2_device.h\n> > @@ -13,11 +13,11 @@\n> >\n> > #include \n> >\n> > +#include \n> > #include \n> > #include \n> >\n> > #include \"libcamera/internal/log.h\"\n> > -#include \"libcamera/internal/v4l2_controls.h\"\n> >\n> > namespace libcamera {\n> >\n> > @@ -61,7 +61,7 @@ private:\n> > void eventAvailable(EventNotifier *notifier);\n> >\n> > std::map controlInfo_;\n> > - std::vector> controlIds_;\n> > + std::vector> controlIds_;\n> > ControlInfoMap controls_;\n> > std::string deviceNode_;\n> > int fd_;\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index e0a48aa2..62251a64 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -51,7 +51,6 @@ libcamera_sources = files([\n> > 'timer.cpp',\n> > 'transform.cpp',\n> > 'utils.cpp',\n> > - 'v4l2_controls.cpp',\n> > 'v4l2_device.cpp',\n> > 'v4l2_pixelformat.cpp',\n> > 'v4l2_subdevice.cpp',\n> > diff --git a/src/libcamera/v4l2_controls.cpp\n> b/src/libcamera/v4l2_controls.cpp\n> > deleted file mode 100644\n> > index 3f8ec6ca..00000000\n> > --- a/src/libcamera/v4l2_controls.cpp\n> > +++ /dev/null\n> > @@ -1,151 +0,0 @@\n> > -/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > -/*\n> > - * Copyright (C) 2019, Google Inc.\n> > - *\n> > - * v4l2_controls.cpp - V4L2 Controls Support\n> > - */\n> > -\n> > -#include \"libcamera/internal/v4l2_controls.h\"\n> > -\n> > -#include \n> > -\n> > -/**\n> > - * \\file v4l2_controls.h\n> > - * \\brief Support for V4L2 Controls using the V4L2 Extended Controls\n> APIs\n> > - *\n> > - * The V4L2 Control API allows application to inspect and modify sets of\n> > - * configurable parameters on a video device or subdevice. The nature\n> of the\n> > - * parameters an application can modify using the control framework\n> depends on\n> > - * what the driver implements support for, and on the characteristics\n> of the\n> > - * underlying hardware platform. Generally controls are used to modify\n> user\n> > - * visible settings, such as the image brightness and exposure time, or\n> > - * non-standard parameters which cannot be controlled through the V4L2\n> format\n> > - * negotiation API.\n> > - *\n> > - * Controls are identified by a numerical ID, defined by the V4L2\n> kernel headers\n> > - * and have an associated type. Each control has a value, which is the\n> data that\n> > - * can be modified with V4L2Device::setControls() or retrieved with\n> > - * V4L2Device::getControls().\n> > - *\n> > - * The control's type along with the control's flags define the type of\n> the\n> > - * control's value content. Controls can transport a single data value\n> stored in\n> > - * variable inside the control, or they might as well deal with more\n> complex\n> > - * data types, such as arrays of matrices, stored in a contiguous memory\n> > - * locations associated with the control and called 'the payload'. Such\n> controls\n> > - * are called 'compound controls' and are currently not supported by the\n> > - * libcamera V4L2 control framework.\n> > - *\n> > - * libcamera implements support for controls using the V4L2 Extended\n> Control\n> > - * API, which allows future handling of controls with payloads of\n> arbitrary\n> > - * sizes.\n> > - *\n> > - * The libcamera V4L2 Controls framework operates on lists of controls,\n> wrapped\n> > - * by the ControlList class, to match the V4L2 extended controls API.\n> The\n> > - * interface to set and get control is implemented by the V4L2Device\n> class, and\n> > - * this file only provides the data type definitions.\n> > - *\n> > - * \\todo Add support for compound controls\n> > - */\n> > -\n> > -namespace libcamera {\n> > -\n> > -namespace {\n> > -\n> > -std::string v4l2_ctrl_name(const struct v4l2_query_ext_ctrl &ctrl)\n> > -{\n> > - size_t len = strnlen(ctrl.name, sizeof(ctrl.name));\n> > - return std::string(static_cast(ctrl.name), len);\n> > -}\n> > -\n> > -ControlType v4l2_ctrl_type(const struct v4l2_query_ext_ctrl &ctrl)\n> > -{\n> > - switch (ctrl.type) {\n> > - case V4L2_CTRL_TYPE_U8:\n> > - return ControlTypeByte;\n> > -\n> > - case V4L2_CTRL_TYPE_BOOLEAN:\n> > - return ControlTypeBool;\n> > -\n> > - case V4L2_CTRL_TYPE_INTEGER:\n> > - return ControlTypeInteger32;\n> > -\n> > - case V4L2_CTRL_TYPE_INTEGER64:\n> > - return ControlTypeInteger64;\n> > -\n> > - case V4L2_CTRL_TYPE_MENU:\n> > - case V4L2_CTRL_TYPE_BUTTON:\n> > - case V4L2_CTRL_TYPE_BITMASK:\n> > - case V4L2_CTRL_TYPE_INTEGER_MENU:\n> > - /*\n> > - * More precise types may be needed, for now use a 32-bit\n> > - * integer type.\n> > - */\n> > - return ControlTypeInteger32;\n> > -\n> > - default:\n> > - return ControlTypeNone;\n> > - }\n> > -}\n> > -\n> > -} /* namespace */\n> > -\n> > -/**\n> > - * \\class V4L2ControlId\n> > - * \\brief V4L2 control static metadata\n> > - *\n> > - * The V4L2ControlId class is a specialisation of the ControlId for V4L2\n> > - * controls.\n> > - */\n> > -\n> > -/**\n> > - * \\brief Construct a V4L2ControlId from a struct v4l2_query_ext_ctrl\n> > - * \\param[in] ctrl The struct v4l2_query_ext_ctrl as returned by the\n> kernel\n> > - */\n> > -V4L2ControlId::V4L2ControlId(const struct v4l2_query_ext_ctrl &ctrl)\n> > - : ControlId(ctrl.id, v4l2_ctrl_name(ctrl), v4l2_ctrl_type(ctrl))\n> > -{\n> > -}\n> > -\n> > -/**\n> > - * \\class V4L2ControlInfo\n> > - * \\brief Convenience specialisation of ControlInfo for V4L2 controls\n> > - *\n> > - * The V4L2ControlInfo class is a specialisation of the ControlInfo for\n> V4L2\n> > - * controls. It offers a convenience constructor from a struct\n> > - * v4l2_query_ext_ctrl, and is otherwise equivalent to the ControlInfo\n> class.\n> > - */\n> > -\n> > -/**\n> > - * \\brief Construct a V4L2ControlInfo from a struct v4l2_query_ext_ctrl\n> > - * \\param[in] ctrl The struct v4l2_query_ext_ctrl as returned by the\n> kernel\n> > - */\n> > -V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)\n> > -{\n> > - switch (ctrl.type) {\n> > - case V4L2_CTRL_TYPE_U8:\n> > -\n> ControlInfo::operator=(ControlInfo(static_cast(ctrl.minimum),\n> > -\n> static_cast(ctrl.maximum),\n> > -\n> static_cast(ctrl.default_value)));\n> > - break;\n> > -\n> > - case V4L2_CTRL_TYPE_BOOLEAN:\n> > -\n> ControlInfo::operator=(ControlInfo(static_cast(ctrl.minimum),\n> > -\n> static_cast(ctrl.maximum),\n> > -\n> static_cast(ctrl.default_value)));\n> > - break;\n> > -\n> > - case V4L2_CTRL_TYPE_INTEGER64:\n> > -\n> ControlInfo::operator=(ControlInfo(static_cast(ctrl.minimum),\n> > -\n> static_cast(ctrl.maximum),\n> > -\n> static_cast(ctrl.default_value)));\n> > - break;\n> > -\n> > - default:\n> > -\n> ControlInfo::operator=(ControlInfo(static_cast(ctrl.minimum),\n> > -\n> static_cast(ctrl.maximum),\n> > -\n> static_cast(ctrl.default_value)));\n> > - break;\n> > - }\n> > -}\n> > -\n> > -} /* namespace libcamera */\n> > diff --git a/src/libcamera/v4l2_device.cpp\n> b/src/libcamera/v4l2_device.cpp\n> > index 397029ac..41eb6454 100644\n> > --- a/src/libcamera/v4l2_device.cpp\n> > +++ b/src/libcamera/v4l2_device.cpp\n> > @@ -20,7 +20,6 @@\n> > #include \"libcamera/internal/log.h\"\n> > #include \"libcamera/internal/sysfs.h\"\n> > #include \"libcamera/internal/utils.h\"\n> > -#include \"libcamera/internal/v4l2_controls.h\"\n> >\n> > /**\n> > * \\file v4l2_device.h\n> > @@ -31,6 +30,73 @@ namespace libcamera {\n> >\n> > LOG_DEFINE_CATEGORY(V4L2)\n> >\n> > +namespace {\n> > +\n> > +ControlType v4l2CtrlType(uint32_t ctrlType)\n> > +{\n> > + switch (ctrlType) {\n> > + case V4L2_CTRL_TYPE_U8:\n> > + return ControlTypeByte;\n> > +\n> > + case V4L2_CTRL_TYPE_BOOLEAN:\n> > + return ControlTypeBool;\n> > +\n> > + case V4L2_CTRL_TYPE_INTEGER:\n> > + return ControlTypeInteger32;\n> > +\n> > + case V4L2_CTRL_TYPE_INTEGER64:\n> > + return ControlTypeInteger64;\n> > +\n> > + case V4L2_CTRL_TYPE_MENU:\n> > + case V4L2_CTRL_TYPE_BUTTON:\n> > + case V4L2_CTRL_TYPE_BITMASK:\n> > + case V4L2_CTRL_TYPE_INTEGER_MENU:\n> > + /*\n> > + * More precise types may be needed, for now use a 32-bit\n> > + * integer type.\n> > + */\n> > + return ControlTypeInteger32;\n> > +\n> > + default:\n> > + return ControlTypeNone;\n> > + }\n> > +}\n> > +\n> > +std::unique_ptr createControlId(const v4l2_query_ext_ctrl\n> &ctrl)\n>\n> Do we gain anything in storing these as unique_ptr ? They will be\n> stored in a vector, which at class deletion time will destroy them,\n> right ? I know it was like this already though...\n>\n>\nI tried in v2, but I couldn't because neither copy nor move is allowed for\nControlId.\nWe need to use unique_ptr here. :(\n\n-Hiro\n\n\n> > +{\n> > + const size_t len = strnlen(ctrl.name, sizeof(ctrl.name));\n> > + const std::string name(static_cast(ctrl.name), len);\n> > + const ControlType type = v4l2CtrlType(ctrl.type);\n> > +\n> > + return std::make_unique(ctrl.id, name, type);\n> > +}\n> > +\n> > +ControlInfo createControlInfo(const v4l2_query_ext_ctrl &ctrl)\n> > +{\n> > + switch (ctrl.type) {\n> > + case V4L2_CTRL_TYPE_U8:\n> > + return ControlInfo(static_cast(ctrl.minimum),\n> > + static_cast(ctrl.maximum),\n> > +\n> static_cast(ctrl.default_value));\n> > +\n> > + case V4L2_CTRL_TYPE_BOOLEAN:\n> > + return ControlInfo(static_cast(ctrl.minimum),\n> > + static_cast(ctrl.maximum),\n> > + static_cast(ctrl.default_value));\n> > +\n> > + case V4L2_CTRL_TYPE_INTEGER64:\n> > + return ControlInfo(static_cast(ctrl.minimum),\n> > + static_cast(ctrl.maximum),\n> > +\n> static_cast(ctrl.default_value));\n> > +\n> > + default:\n> > + return ControlInfo(static_cast(ctrl.minimum),\n> > + static_cast(ctrl.maximum),\n> > +\n> static_cast(ctrl.default_value));\n> > + }\n> > +}\n> > +} /* namespace */\n>\n> I would question two minor things:\n>\n> 1) static vs member functions\n> 2) function names, but that's really minor, I would have used\n> v4l2ControlInfo and v4l2ControlId, but that's just me.\n>\n> The patch itself is good and goes in the right direction imho\n>\n> Reviewed-by: Jacopo Mondi \n>\n> Thanks\n> j\n>\n> > +\n> > /**\n> > * \\class V4L2Device\n> > * \\brief Base class for V4L2VideoDevice and V4L2Subdevice\n> > @@ -502,10 +568,10 @@ void V4L2Device::listControls()\n> > continue;\n> > }\n> >\n> > -\n> controlIds_.emplace_back(std::make_unique(ctrl));\n> > + controlIds_.emplace_back(createControlId(ctrl));\n> > controlInfo_.emplace(ctrl.id, ctrl);\n> >\n> > - ctrls.emplace(controlIds_.back().get(),\n> V4L2ControlInfo(ctrl));\n> > + ctrls.emplace(controlIds_.back().get(),\n> createControlInfo(ctrl));\n> > }\n> >\n> > controls_ = std::move(ctrls);\n> > --\n> > 2.31.1.607.g51e8a6a459-goog\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 1F710BF831\n\tfor ;\n\tMon, 10 May 2021 05:44:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B5B3468918;\n\tMon, 10 May 2021 07:44:45 +0200 (CEST)","from mail-ed1-x52d.google.com (mail-ed1-x52d.google.com\n\t[IPv6:2a00:1450:4864:20::52d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2AF3E6153C\n\tfor ;\n\tMon, 10 May 2021 07:44:42 +0200 (CEST)","by mail-ed1-x52d.google.com with SMTP id l7so17260733edb.1\n\tfor ;\n\tSun, 09 May 2021 22:44:42 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"AUVvpR+i\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=W4BPCQr6yo+RLWPzeMMI757xVDJldSfcdiFdcmbRqCI=;\n\tb=AUVvpR+iWWyODgiUwHxyo3cv3hePT8jGRZ1KElXP/PQNV6+2SiDbjVK7Tbi50SVIPU\n\tnPblAvL8a9G1EIBbPgvaBMj8TTyOA25LHZNhE4ahE/6fwL8cH8KdxJJeXMUTq8sTl54a\n\tAFgqeO1Pc98E4z5xQty05nAksCU6/idu/ArLU=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=W4BPCQr6yo+RLWPzeMMI757xVDJldSfcdiFdcmbRqCI=;\n\tb=ZNcHdyQYE3qnNxN5gN8KA9Q1y2mTTuAx3pbacWLAs4Rl7gPBplaW61t6yOIlAYSCt+\n\tzSsGTdvNAwyEbtsel7EAIuBijOxOntJiPH9KLFaHLeW9Gblq1dhsbZiJhEb5q+Sb3Q5q\n\tCCEwxL57iRnvYnUHGmEXvVXsWAkgzhLQrJyq5KCV05lPQW0HewZmnQVGK+TopCY+JE/y\n\t8P/kZJTi9otJOW7yrpD/9uqxV+dJNKLRNBMuKaMdINjkbGTeW2b8j7bn/2VLeAXOJ6gD\n\ttmx9BLdJqbb1F8pkbX7hCTv5fBz2MZ0anSM7Ls5G+PI2FcsSFvYfL3tKDwjydI3doQcE\n\tJfHw==","X-Gm-Message-State":"AOAM532Uyv8laIQm2Rd3UbtJhNpvUGymSXkqGTsYsvLonc98v3Pqd1lR\n\t1tFJPKsrF6BsmI156W90/dw886cSviUSPjo7ADdPgNWrpTM=","X-Google-Smtp-Source":"ABdhPJxaas5H+mlDTpHY1VpWSnE32oREeZJmUC13X7mDgx5iRKp/ZJc4z5owDSgbDah7EZ1Y+lyGFm/zZMXbBb9j8+w=","X-Received":"by 2002:aa7:d40e:: with SMTP id\n\tz14mr22342949edq.73.1620625481741; \n\tSun, 09 May 2021 22:44:41 -0700 (PDT)","MIME-Version":"1.0","References":"<20210507021727.1848611-1-hiroh@chromium.org>\n\t<20210507021727.1848611-3-hiroh@chromium.org>\n\t<20210507090238.c3bbaibcvcejwwdi@uno.localdomain>","In-Reply-To":"<20210507090238.c3bbaibcvcejwwdi@uno.localdomain>","From":"Hirokazu Honda ","Date":"Mon, 10 May 2021 14:44:30 +0900","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: V4L2Control: remove\n\tV4L2Control classes","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"multipart/mixed;\n\tboundary=\"===============3456656006395657323==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]