Patch Detail
Show a patch.
GET /api/patches/1989/?format=api
{ "id": 1989, "url": "https://patchwork.libcamera.org/api/patches/1989/?format=api", "web_url": "https://patchwork.libcamera.org/patch/1989/", "project": { "id": 1, "url": "https://patchwork.libcamera.org/api/projects/1/?format=api", "name": "libcamera", "link_name": "libcamera", "list_id": "libcamera_core", "list_email": "libcamera-devel@lists.libcamera.org", "web_url": "", "scm_url": "", "webscm_url": "" }, "msgid": "<20190918103424.14536-6-jacopo@jmondi.org>", "date": "2019-09-18T10:34:24", "name": "[libcamera-devel,5/5] libcamera: controls: Control framework refresh", "commit_ref": null, "pull_url": null, "state": "superseded", "archived": false, "hash": "a3a44ebcacff60a5e9547ae9f48fb41e7c73c9b2", "submitter": { "id": 3, "url": "https://patchwork.libcamera.org/api/people/3/?format=api", "name": "Jacopo Mondi", "email": "jacopo@jmondi.org" }, "delegate": { "id": 15, "url": "https://patchwork.libcamera.org/api/users/15/?format=api", "username": "jmondi", "first_name": "Jacopo", "last_name": "Mondi", "email": "jacopo@jmondi.org" }, "mbox": "https://patchwork.libcamera.org/patch/1989/mbox/", "series": [ { "id": 501, "url": "https://patchwork.libcamera.org/api/series/501/?format=api", "web_url": "https://patchwork.libcamera.org/project/libcamera/list/?series=501", "date": "2019-09-18T10:34:19", "name": "libcamera: Control framework backend rework", "version": 1, "mbox": "https://patchwork.libcamera.org/series/501/mbox/" } ], "comments": "https://patchwork.libcamera.org/api/patches/1989/comments/", "check": "pending", "checks": "https://patchwork.libcamera.org/api/patches/1989/checks/", "tags": {}, "headers": { "Return-Path": "<jacopo@jmondi.org>", "Received": [ "from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ECC6E61918\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 18 Sep 2019 12:32:52 +0200 (CEST)", "from uno.lan (2-224-242-101.ip172.fastwebnet.it [2.224.242.101])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id A1987E0003\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 18 Sep 2019 10:32:52 +0000 (UTC)" ], "X-Originating-IP": "2.224.242.101", "From": "Jacopo Mondi <jacopo@jmondi.org>", "To": "libcamera-devel@lists.libcamera.org", "Date": "Wed, 18 Sep 2019 12:34:24 +0200", "Message-Id": "<20190918103424.14536-6-jacopo@jmondi.org>", "X-Mailer": "git-send-email 2.23.0", "In-Reply-To": "<20190918103424.14536-1-jacopo@jmondi.org>", "References": "<20190918103424.14536-1-jacopo@jmondi.org>", "MIME-Version": "1.0", "Content-Transfer-Encoding": "8bit", "Subject": "[libcamera-devel] [PATCH 5/5] libcamera: controls: Control\n\tframework refresh", "X-BeenThere": "libcamera-devel@lists.libcamera.org", "X-Mailman-Version": "2.1.23", "Precedence": "list", "List-Id": "<libcamera-devel.lists.libcamera.org>", "List-Unsubscribe": "<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>", "List-Archive": "<https://lists.libcamera.org/pipermail/libcamera-devel/>", "List-Post": "<mailto:libcamera-devel@lists.libcamera.org>", "List-Help": "<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>", "List-Subscribe": "<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>", "X-List-Received-Date": "Wed, 18 Sep 2019 10:32:53 -0000" }, "content": "Now that the control data value and info storage have been unified with\nV4L2 control a refresh of naming and minor bits had to be performed.\nThe control framework implementation uses the word 'control' in types\nand definition to refer to different things, making it harder to follow.\n\nPerform a rename of elements defined by the control framework to enforce\nthe following concepts:\n- control metadata:\n static control metadata information such as id, type and name, defined by\n libcamera\n- control information:\n constant information associated with a control used for validation of\n control values, provided by pipeline handlers and defined by the\n capabilities of the device the control applies to\n- control:\n a control identifier with an associated value, provided by\n applications when setting a control to a specific value\n\nUpdate the framework documentation trying to use those concepts\nconsistently.\n\nSigned-off-by: Jacopo Mondi <jacopo@jmondi.org>\n---\n include/libcamera/controls.h | 19 ++--\n src/libcamera/controls.cpp | 129 +++++++++++++++++-----------\n src/libcamera/gen-controls.awk | 2 +-\n src/libcamera/meson.build | 6 +-\n src/libcamera/pipeline/uvcvideo.cpp | 2 +-\n src/libcamera/pipeline/vimc.cpp | 2 +-\n test/controls/control_list.cpp | 2 +-\n 7 files changed, 97 insertions(+), 65 deletions(-)\n\n--\n2.23.0", "diff": "diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\nindex d3065c0f3f16..174bc92732aa 100644\n--- a/include/libcamera/controls.h\n+++ b/include/libcamera/controls.h\n@@ -19,7 +19,7 @@ namespace libcamera {\n\n class Camera;\n\n-struct ControlIdentifier {\n+struct ControlMetadata {\n \tControlId id;\n \tconst char *name;\n \tDataType type;\n@@ -37,21 +37,22 @@ public:\n \tstd::string toString() const;\n\n private:\n-\tconst struct ControlIdentifier *ident_;\n+\tconst struct ControlMetadata *ident_;\n };\n\n using ControlInfoMap = std::unordered_map<ControlId, ControlInfo>;\n\n+using Control = DataValue;\n class ControlList\n {\n private:\n-\tusing ControlListMap = std::unordered_map<const ControlInfo *, DataValue>;\n+\tusing ControlMap = std::unordered_map<const ControlInfo *, Control>;\n\n public:\n \tControlList(const ControlInfoMap &infoMap);\n\n-\tusing iterator = ControlListMap::iterator;\n-\tusing const_iterator = ControlListMap::const_iterator;\n+\tusing iterator = ControlMap::iterator;\n+\tusing const_iterator = ControlMap::const_iterator;\n\n \titerator begin() { return controls_.begin(); }\n \titerator end() { return controls_.end(); }\n@@ -64,14 +65,14 @@ public:\n \tstd::size_t size() const { return controls_.size(); }\n \tvoid clear() { controls_.clear(); }\n\n-\tDataValue &operator[](ControlId id);\n-\tDataValue &operator[](const ControlInfo *info) { return controls_[info]; }\n+\tControl &operator[](ControlId id);\n+\tControl &operator[](const ControlInfo *info) { return controls_[info]; }\n\n-\tvoid update(const ControlList &list);\n+\tbool merge(const ControlList &list);\n\n private:\n \tconst ControlInfoMap &infoMap_;\n-\tControlListMap controls_;\n+\tControlMap controls_;\n };\n\n } /* namespace libcamera */\ndiff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\nindex 184d544c05bc..028ffc1e80b9 100644\n--- a/src/libcamera/controls.cpp\n+++ b/src/libcamera/controls.cpp\n@@ -7,9 +7,6 @@\n\n #include <libcamera/controls.h>\n\n-#include <sstream>\n-#include <string>\n-\n #include <libcamera/camera.h>\n\n #include \"log.h\"\n@@ -17,7 +14,7 @@\n\n /**\n * \\file controls.h\n- * \\brief Describes control framework and controls supported by a camera\n+ * \\brief Control framework implementation\n */\n\n namespace libcamera {\n@@ -72,38 +69,50 @@ LOG_DEFINE_CATEGORY(Controls)\n */\n\n /**\n- * \\struct ControlIdentifier\n- * \\brief Describe a ControlId with control specific constant meta-data\n+ * \\struct ControlMetadata\n+ * \\brief Describe the control static meta-data\n+ *\n+ * Defines the Control static meta-data information, auto-generated from the\n+ * ControlId documentation.\n+ *\n+ * The control information are defined in the control_id.h file, parsed by\n+ * the gen-controls.awk script to produce a control_metadata.cpp file that\n+ * provides a static list of control id with an associated type and name. The\n+ * file is not for public use, and so no suitable header exists as\n+ * its sole usage is for the ControlMetadata definition.\n *\n- * Defines a Control with a unique ID, a name, and a type.\n- * This structure is used as static part of the auto-generated control\n- * definitions, which are generated from the ControlId documentation.\n+ * \\todo Expand the control meta-data definition to support more complex\n+ * control types and describe their characteristics (ie. Number of deta elements\n+ * for compound control types, versioning etc).\n *\n- * \\var ControlIdentifier::id\n+ * \\var ControlMetadata::id\n * The unique ID for a control\n- * \\var ControlIdentifier::name\n+ * \\var ControlMetadata::name\n * The string representation of the control\n- * \\var ControlIdentifier::type\n+ * \\var ControlMetadata::type\n * The ValueType required to represent the control value\n */\n\n /*\n- * The controlTypes are automatically generated to produce a control_types.cpp\n- * output. This file is not for public use, and so no suitable header exists\n- * for this sole usage of the controlTypes reference. As such the extern is\n- * only defined here for use during the ControlInfo constructor and should not\n+ * The ControlTypes map is defined by the generated control_metadata.cpp file\n+ * and only used here during the ControlInfo construction and should not\n * be referenced directly elsewhere.\n */\n-extern const std::unordered_map<ControlId, ControlIdentifier> controlTypes;\n+extern const std::unordered_map<ControlId, ControlMetadata> controlTypes;\n\n /**\n * \\class ControlInfo\n * \\brief Describe the information and capabilities of a Control\n *\n- * The ControlInfo represents control specific meta-data which is constant on a\n- * per camera basis. ControlInfo classes are constructed by pipeline handlers\n- * to expose the controls they support and the metadata needed to utilise those\n- * controls.\n+ * The ControlInfo class associates the control's constant metadata defined by\n+ * libcamera and collected in the ControlMetadata class, with its static\n+ * information, such the range of supported values and the control size,\n+ * described by the DataInfo base class and defined by the capabilities of\n+ * the Camera device that implements the control.\n+ *\n+ * ControlInfo instances are constructed by pipeline handlers and associated\n+ * with the Camera devices they register to expose the list of controls they\n+ * support and the static information required to use those controls.\n */\n\n /**\n@@ -118,7 +127,7 @@ ControlInfo::ControlInfo(ControlId id, const DataValue &min,\n {\n \tauto iter = controlTypes.find(id);\n \tif (iter == controlTypes.end()) {\n-\t\tLOG(Controls, Fatal) << \"Attempt to create invalid ControlInfo\";\n+\t\tLOG(Controls, Fatal) << \"Control \" << id << \" not defined\";\n \t\treturn;\n \t}\n\n@@ -127,19 +136,19 @@ ControlInfo::ControlInfo(ControlId id, const DataValue &min,\n\n /**\n * \\fn ControlInfo::id()\n- * \\brief Retrieve the control ID\n+ * \\brief Retrieve the control ID from the control constant metadata\n * \\return The control ID\n */\n\n /**\n * \\fn ControlInfo::name()\n- * \\brief Retrieve the control name string\n+ * \\brief Retrieve the control name string from the control constant metadata\n * \\return The control name string\n */\n\n /**\n * \\fn ControlInfo::type()\n- * \\brief Retrieve the control designated type\n+ * \\brief Retrieve the control designated type from the control constant metadata\n * \\return The control type\n */\n\n@@ -161,21 +170,37 @@ std::string ControlInfo::toString() const\n * \\brief A map of ControlId to ControlInfo\n */\n\n+/**\n+ * \\typedef Control\n+ * \\brief Use 'Control' in place of DataValue in the ControlList class\n+ *\n+ * \\todo If more data and operations than the ones defined by DataValue\n+ * will need to be implemented for controls, make this typedef a class that\n+ * inherits from DataValue.\n+ */\n+\n /**\n * \\class ControlList\n- * \\brief Associate a list of ControlId with their values for a camera\n+ * \\brief List of controls values\n *\n- * A ControlList wraps a map of ControlId to DataValue and provide\n- * additional validation against the control information exposed by a Camera.\n+ * A ControlList wraps a map of ControlId to Control instances and provide\n+ * additional validation against the control information reported by a Camera.\n *\n- * A list is only valid for as long as the camera it refers to is valid. After\n- * that calling any method of the ControlList class other than its destructor\n- * will cause undefined behaviour.\n+ * ControlList instances are initially created empty and should be populated by\n+ * assigning a value to each control added to the list. The added control is\n+ * validated to verify it is supported by the Camera the list refers to, and\n+ * validate the provided value against the static information provided by the\n+ * ControlInfo instance associated with the control.\n */\n\n /**\n- * \\brief Construct a ControlList with a reference to the Camera it applies on\n+ * \\brief Construct a ControlList with a map of control information\n * \\param[in] infoMap The ControlInfoMap of the camera the control list refers to\n+ *\n+ * The provided \\a infoMap collects the control id and the associated static\n+ * information for each control that can be set on the list. \\a infoMap is\n+ * provided by the Camera the list of controls refers to and it is used to make\n+ * sure only controls supported by the camera can be added to the list.\n */\n ControlList::ControlList(const ControlInfoMap &infoMap)\n \t: infoMap_(infoMap)\n@@ -221,10 +246,6 @@ ControlList::ControlList(const ControlInfoMap &infoMap)\n /**\n * \\brief Check if the list contains a control with the specified \\a id\n * \\param[in] id The control ID\n- *\n- * The behaviour is undefined if the control \\a id is not supported by the\n- * camera that the ControlList refers to.\n- *\n * \\return True if the list contains a matching control, false otherwise\n */\n bool ControlList::contains(ControlId id) const\n@@ -258,7 +279,7 @@ bool ControlList::contains(const ControlInfo *info) const\n /**\n * \\fn ControlList::size()\n * \\brief Retrieve the number of controls in the list\n- * \\return The number of DataValue entries stored in the list\n+ * \\return The number of Control entries stored in the list\n */\n\n /**\n@@ -276,15 +297,16 @@ bool ControlList::contains(const ControlInfo *info) const\n * The behaviour is undefined if the control \\a id is not supported by the\n * camera that the ControlList refers to.\n *\n- * \\return A reference to the value of the control identified by \\a id\n+ * \\return A reference to the value of the control identified by \\a id if the\n+ * control is supported, an empty control otherwise\n */\n-DataValue &ControlList::operator[](ControlId id)\n+Control &ControlList::operator[](ControlId id)\n {\n \tconst auto info = infoMap_.find(id);\n \tif (info == infoMap_.end()) {\n \t\tLOG(Controls, Error) << \"Control \" << id << \" not supported\";\n\n-\t\tstatic DataValue empty;\n+\t\tstatic Control empty;\n \t\treturn empty;\n \t}\n\n@@ -299,22 +321,29 @@ DataValue &ControlList::operator[](ControlId id)\n * This method returns a reference to the control identified by \\a info,\n * inserting it in the list if the info is not already present.\n *\n- * \\return A reference to the value of the control identified by \\a info\n+ * The behaviour is undefined if the control \\a info is not supported by the\n+ * camera that the ControlList refers to.\n+ *\n+ * \\return A reference to the value of the control identified by \\a info if the\n+ * control is supported, an mpty control otherwise\n */\n\n /**\n- * \\brief Update the list with a union of itself and \\a other\n- * \\param other The other list\n+ * \\brief Merge the control in the list with controls from \\a other\n+ * \\param other The control list to merge with\n *\n * Update the control list to include all values from the \\a other list.\n- * Elements in the list whose control IDs are contained in \\a other are updated\n+ * Elements in the list whose control IDs are contained in \\a other are merged\n * with the value from \\a other. Elements in the \\a other list that have no\n * corresponding element in the list are added to the list with their value.\n *\n- * The behaviour is undefined if the two lists refer to different Camera\n- * instances.\n+ * All controls in \\a other should be supported by the list, otherwise no\n+ * control is updated.\n+ *\n+ * \\return True if control list have been merged, false otherwise\n+ *\n */\n-void ControlList::update(const ControlList &other)\n+bool ControlList::merge(const ControlList &other)\n {\n \t/*\n \t * Make sure if all controls in other's list are supported by this\n@@ -326,14 +355,16 @@ void ControlList::update(const ControlList &other)\n\n \t\tif (infoMap_.find(id) == infoMap_.end()) {\n \t\t\tLOG(Controls, Error)\n-\t\t\t\t<< \"Failed to update control list with control: \"\n+\t\t\t\t<< \"Failed to merge control list with control: \"\n \t\t\t\t<< id;\n-\t\t\treturn;\n+\t\t\treturn false;\n \t\t}\n \t}\n\n \tfor (auto &control : other)\n \t\tcontrols_[control.first] = control.second;\n+\n+\treturn true;\n }\n\n } /* namespace libcamera */\ndiff --git a/src/libcamera/gen-controls.awk b/src/libcamera/gen-controls.awk\nindex abeb0218546b..b7f6532dfd43 100755\n--- a/src/libcamera/gen-controls.awk\n+++ b/src/libcamera/gen-controls.awk\n@@ -89,7 +89,7 @@ function GenerateTable(file) {\n\n \tEnterNameSpace(file)\n\n-\tprint \"extern const std::unordered_map<ControlId, ControlIdentifier>\" > file\n+\tprint \"extern const std::unordered_map<ControlId, ControlMetadata>\" > file\n \tprint \"controlTypes {\" > file\n \tfor (i=1; i <= id; ++i) {\n \t\tprintf \"\\t{ %s, { %s, \\\"%s\\\", DataType%s } },\\n\", names[i], names[i], names[i], types[i] > file\ndiff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\nindex c3100a1709e0..c4fcd0569bd7 100644\n--- a/src/libcamera/meson.build\n+++ b/src/libcamera/meson.build\n@@ -60,13 +60,13 @@ endif\n\n gen_controls = files('gen-controls.awk')\n\n-control_types_cpp = custom_target('control_types_cpp',\n+control_metadata_cpp = custom_target('control_metadata_cpp',\n input : files('controls.cpp'),\n- output : 'control_types.cpp',\n+ output : 'control_metadata.cpp',\n depend_files : gen_controls,\n command : [gen_controls, '@INPUT@', '--code', '@OUTPUT@'])\n\n-libcamera_sources += control_types_cpp\n+libcamera_sources += control_metadata_cpp\n\n gen_version = join_paths(meson.source_root(), 'utils', 'gen-version.sh')\n\ndiff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\nindex dd253f94ca3d..d7f7fb0d6322 100644\n--- a/src/libcamera/pipeline/uvcvideo.cpp\n+++ b/src/libcamera/pipeline/uvcvideo.cpp\n@@ -231,7 +231,7 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)\n\n \tfor (auto it : request->controls()) {\n \t\tconst ControlInfo *ci = it.first;\n-\t\tDataValue &value = it.second;\n+\t\tControl &value = it.second;\n\n \t\tswitch (ci->id()) {\n \t\tcase Brightness:\ndiff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\nindex 3df239bdb277..1d36ec54bc3b 100644\n--- a/src/libcamera/pipeline/vimc.cpp\n+++ b/src/libcamera/pipeline/vimc.cpp\n@@ -284,7 +284,7 @@ int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request)\n\n \tfor (auto it : request->controls()) {\n \t\tconst ControlInfo *ci = it.first;\n-\t\tDataValue &value = it.second;\n+\t\tControl &value = it.second;\n\n \t\tswitch (ci->id()) {\n \t\tcase Brightness:\ndiff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp\nindex 3c6eb40c091b..2a90cffe5106 100644\n--- a/test/controls/control_list.cpp\n+++ b/test/controls/control_list.cpp\n@@ -159,7 +159,7 @@ protected:\n\n \t\tnewList[Brightness] = 128;\n \t\tnewList[Saturation] = 255;\n-\t\tnewList.update(list);\n+\t\tnewList.merge(list);\n\n \t\tlist.clear();\n\n", "prefixes": [ "libcamera-devel", "5/5" ] }