Patch Detail
Show a patch.
GET /api/1.1/patches/1998/?format=api
{ "id": 1998, "url": "https://patchwork.libcamera.org/api/1.1/patches/1998/?format=api", "web_url": "https://patchwork.libcamera.org/patch/1998/", "project": { "id": 1, "url": "https://patchwork.libcamera.org/api/1.1/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": "<20190924171440.29758-6-jacopo@jmondi.org>", "date": "2019-09-24T17:14:40", "name": "[libcamera-devel,v2,5/5] libcamera: controls: Control framework refresh", "commit_ref": null, "pull_url": null, "state": "superseded", "archived": false, "hash": "0296fe1c562617ce893f8c6002de79de5ded3876", "submitter": { "id": 3, "url": "https://patchwork.libcamera.org/api/1.1/people/3/?format=api", "name": "Jacopo Mondi", "email": "jacopo@jmondi.org" }, "delegate": null, "mbox": "https://patchwork.libcamera.org/patch/1998/mbox/", "series": [ { "id": 505, "url": "https://patchwork.libcamera.org/api/1.1/series/505/?format=api", "web_url": "https://patchwork.libcamera.org/project/libcamera/list/?series=505", "date": "2019-09-24T17:14:35", "name": "libcamera: Control framework backend rewor", "version": 2, "mbox": "https://patchwork.libcamera.org/series/505/mbox/" } ], "comments": "https://patchwork.libcamera.org/api/patches/1998/comments/", "check": "pending", "checks": "https://patchwork.libcamera.org/api/patches/1998/checks/", "tags": {}, "headers": { "Return-Path": "<jacopo@jmondi.org>", "Received": [ "from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[217.70.183.200])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 13C4260BCF\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 24 Sep 2019 19:13:10 +0200 (CEST)", "from uno.homenet.telecomitalia.it\n\t(host89-248-dynamic.45-213-r.retail.telecomitalia.it [213.45.248.89])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay7-d.mail.gandi.net (Postfix) with ESMTPSA id F41F22000B\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 24 Sep 2019 17:13:08 +0000 (UTC)" ], "X-Originating-IP": "213.45.248.89", "From": "Jacopo Mondi <jacopo@jmondi.org>", "To": "libcamera-devel@lists.libcamera.org", "Date": "Tue, 24 Sep 2019 19:14:40 +0200", "Message-Id": "<20190924171440.29758-6-jacopo@jmondi.org>", "X-Mailer": "git-send-email 2.23.0", "In-Reply-To": "<20190924171440.29758-1-jacopo@jmondi.org>", "References": "<20190924171440.29758-1-jacopo@jmondi.org>", "MIME-Version": "1.0", "Content-Transfer-Encoding": "8bit", "Subject": "[libcamera-devel] [PATCH v2 5/5] libcamera: controls: Control\n\tframework refresh", "X-BeenThere": "libcamera-devel@lists.libcamera.org", "X-Mailman-Version": "2.1.29", "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": "Tue, 24 Sep 2019 17:13:10 -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 | 26 +++--\n src/libcamera/controls.cpp | 165 +++++++++++++++++++---------\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 | 5 +-\n 7 files changed, 135 insertions(+), 73 deletions(-)", "diff": "diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\nindex d3065c0f3f16..c299ed94d9e2 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@@ -30,28 +30,30 @@ class ControlInfo : public DataInfo\n public:\n \texplicit ControlInfo(ControlId id, const DataValue &min = 0,\n \t\t\t const DataValue &max = 0);\n-\tControlId id() const { return ident_->id; }\n-\tconst char *name() const { return ident_->name; }\n-\tDataType type() const { return ident_->type; }\n+\tControlId id() const { return meta_->id; }\n+\tconst char *name() const { return meta_->name; }\n+\tDataType type() const { return meta_->type; }\n \n \tstd::string toString() const;\n \n private:\n-\tconst struct ControlIdentifier *ident_;\n+\tconst struct ControlMetadata *meta_;\n };\n \n using ControlInfoMap = std::unordered_map<ControlId, ControlInfo>;\n \n+using ControlValue = DataValue;\n+\n class ControlList\n {\n private:\n-\tusing ControlListMap = std::unordered_map<const ControlInfo *, DataValue>;\n+\tusing ControlMap = std::unordered_map<const ControlInfo *, ControlValue>;\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 +66,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+\tControlValue &operator[](ControlId id);\n+\tControlValue &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 3c2a8dc50a16..65001259a3df 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,52 @@ 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- * 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+ * The control information is 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- * \\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+ * \\todo Expand the control meta-data definition to support more complex\n+ * control types and describe their characteristics (ie. Number of data elements\n+ * for compound control types, versioning etc).\n+ */\n+\n+/*\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,28 +129,28 @@ 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-\tident_ = &iter->second;\n+\tmeta_ = &iter->second;\n }\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 data type\n+ * \\brief Retrieve the control data type from the constant metadata\n * \\return The control type\n */\n \n@@ -161,15 +172,36 @@ std::string ControlInfo::toString() const\n * \\brief A map of ControlId to ControlInfo\n */\n \n+/**\n+ * \\typedef ControlValue\n+ * \\brief A control value stored in the ControlList class\n+ *\n+ * The ControlValue type provides the value associated with a control, stored\n+ * in a ControlList and there associated with a ControlInfo that provides\n+ * the information for the control id, type and data size.\n+ */\n+\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 ControlValue instances and provide\n+ * additional validation against the control information reported by a Camera.\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+ * 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+ * the control value is validated against the static information provided by the\n+ * ControlInfo instance associated with the control.\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+ * A list is only valid 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 */\n \n@@ -179,8 +211,9 @@ std::string ControlInfo::toString() const\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+ * provided by the Camera the list of controls refers to. It is used to make\n+ * sure only controls supported by the camera can be added to the list and it is\n+ * only valid as long as the camera that provided it stays valid.\n */\n ControlList::ControlList(const ControlInfoMap &infoMap)\n \t: infoMap_(infoMap)\n@@ -199,14 +232,14 @@ ControlList::ControlList(const ControlInfoMap &infoMap)\n \n /**\n * \\fn iterator ControlList::begin()\n- * \\brief Retrieve an iterator to the first Control in the list\n- * \\return An iterator to the first Control in the list\n+ * \\brief Retrieve an iterator to the first ControlValue in the list\n+ * \\return An iterator to the first ControlValue in the list\n */\n \n /**\n * \\fn const_iterator ControlList::begin() const\n- * \\brief Retrieve a const_iterator to the first Control in the list\n- * \\return A const_iterator to the first Control in the list\n+ * \\brief Retrieve a const_iterator to the first ControlValue in the list\n+ * \\return A const_iterator to the first ControlValue in the list\n */\n \n /**\n@@ -227,8 +260,8 @@ ControlList::ControlList(const ControlInfoMap &infoMap)\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+ * The behaviour is undefined if the camera the list refers to is not valid\n+ * anymore.\n *\n * \\return True if the list contains a matching control, false otherwise\n */\n@@ -247,6 +280,10 @@ bool ControlList::contains(ControlId id) const\n /**\n * \\brief Check if the list contains a control with the specified \\a info\n * \\param[in] info The control info\n+ *\n+ * The behaviour is undefined if the camera the list refers to is not valid\n+ * anymore.\n+ *\n * \\return True if the list contains a matching control, false otherwise\n */\n bool ControlList::contains(const ControlInfo *info) const\n@@ -263,7 +300,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@@ -278,18 +315,23 @@ bool ControlList::contains(const ControlInfo *info) const\n * This method returns a reference to the control identified by \\a id, inserting\n * it in the list if the ID is not already present.\n *\n- * The behaviour is undefined if the control \\a id is not supported by the\n- * camera that the ControlList refers to.\n+ * If the control \\a id is not supported by the camera the ControlList\n+ * refers to, an empty control is returned and no control value is added to the\n+ * list.\n+ *\n+ * The behaviour is undefined if the camera the list refers to is not valid\n+ * anymore.\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+ControlValue &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 ControlValue empty;\n \t\treturn empty;\n \t}\n \n@@ -304,22 +346,35 @@ 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+ * If the control \\a info is not supported by the camera the ControlList\n+ * refers to, an empty control is returned and no control value is added to the\n+ * list.\n+ *\n+ * The behaviour is undefined if the camera the list refers to is not valid\n+ * anymore.\n+ *\n+ * \\return A reference to the value of the control identified by \\a info if the\n+ * control is supported, an empty 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+ * The behaviour is undefined if the camera the list refers to is not valid\n+ * anymore.\n+ *\n+ * \\return True if control list have been merged, false otherwise\n */\n-void ControlList::update(const ControlList &other)\n+bool ControlList::merge(const ControlList &other)\n {\n \t/*\n \t * Make sure all controls in the other list are supported by this\n@@ -331,14 +386,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..8965210550d2 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\tControlValue &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..f26a91f86ec1 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\tControlValue &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..f04f441c018a 100644\n--- a/test/controls/control_list.cpp\n+++ b/test/controls/control_list.cpp\n@@ -159,7 +159,10 @@ protected:\n \n \t\tnewList[Brightness] = 128;\n \t\tnewList[Saturation] = 255;\n-\t\tnewList.update(list);\n+\t\tif (!newList.merge(list)) {\n+\t\t\tcout << \"Failed to merge control lists\" << endl;\n+\t\t\treturn TestFail;\n+\t\t}\n \n \t\tlist.clear();\n \n", "prefixes": [ "libcamera-devel", "v2", "5/5" ] }