[{"id":18415,"web_url":"https://patchwork.libcamera.org/comment/18415/","msgid":"<20210729082553.GI2167@pyrite.rasen.tech>","date":"2021-07-29T08:25:53","subject":"Re: [libcamera-devel] [PATCH v2 2/5] libcamera: controls: Use\n\tControlIdMap in deserialization","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Wed, Jul 28, 2021 at 06:11:13PM +0200, Jacopo Mondi wrote:\n> Introduce a new field in the controls serialization protocol to\n> allow discerning which ControlIdMap a ControlInfoMap refers to.\n> \n> The newly introduced IdMapType enumeration describes the possible\n> info maps:\n> - Either the globally available controls::controls and\n> properties::properties maps, which are valid across IPC boundaries\n> - A ControlIdMap created locally by the V4L2 device, which is not valid\n> across the IPC boundaries\n> \n> At de-serialization time the idMapType filed is inspected and\n> - If the idmap is a globally available one, there's no need to create\n> new ControlId instances when populating the de-serialized\n> ControlInfoMap. Use the globally available map to retrieve the\n> ControlId reference and use it\n> - If the idmap is a map only available locally, create a new ControlId\n> as it used to happen before this patch.\n> \n> As a direct consequence, this change allows to perform lookup by\n\ns/to/us to/\n\n> ControlId reference on de-serialized ControlIdMap that refers to the\n> libcamera defined controls::controls and properties::properties.\n> \n> Signed-off-by: Jacopo Mondi \n\nI think it's good.\n\nReviewed-by: Paul Elder \n\n> ---\n> include/libcamera/ipa/ipa_controls.h | 9 +++-\n> src/libcamera/control_serializer.cpp | 73 +++++++++++++++++++++++-----\n> src/libcamera/ipa_controls.cpp | 31 ++++++++++++\n> 3 files changed, 101 insertions(+), 12 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/ipa_controls.h b/include/libcamera/ipa/ipa_controls.h\n> index 6d3bf279c22d..5b1690066314 100644\n> --- a/include/libcamera/ipa/ipa_controls.h\n> +++ b/include/libcamera/ipa/ipa_controls.h\n> @@ -15,13 +15,20 @@ extern \"C\" {\n> \n> #define IPA_CONTROLS_FORMAT_VERSION\t1\n> \n> +enum class IdMapType {\n> +\tLIBCAMERA_CONTROLS,\n> +\tLIBCAMERA_PROPERTIES,\n> +\tV4L2_CONTROLS,\n> +};\n> +\n> struct ipa_controls_header {\n> \tuint32_t version;\n> \tuint32_t handle;\n> \tuint32_t entries;\n> \tuint32_t size;\n> \tuint32_t data_offset;\n> -\tuint32_t reserved[3];\n> +\tIdMapType idMapType;\n> +\tuint32_t reserved[2];\n> };\n> \n> struct ipa_control_value_entry {\n> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> index df6ed93f477e..255f9d30fb85 100644\n> --- a/src/libcamera/control_serializer.cpp\n> +++ b/src/libcamera/control_serializer.cpp\n> @@ -17,6 +17,7 @@\n> #include \n> #include \n> #include \n> +#include \n> \n> #include \"libcamera/internal/byte_stream_buffer.h\"\n> \n> @@ -188,6 +189,16 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,\n> \tfor (const auto &ctrl : infoMap)\n> \t\tvaluesSize += binarySize(ctrl.second);\n> \n> +\t/* Serialize the id map type to be used when de-serializing. */\n> +\tconst ControlIdMap *idmap = &infoMap.idmap();\n> +\tenum IdMapType idMapType;\n> +\tif (idmap == &controls::controls)\n> +\t\tidMapType = IdMapType::LIBCAMERA_CONTROLS;\n> +\telse if (idmap == &properties::properties)\n> +\t\tidMapType = IdMapType::LIBCAMERA_PROPERTIES;\n> +\telse\n> +\t\tidMapType = IdMapType::V4L2_CONTROLS;\n> +\n> \t/* Prepare the packet header, assign a handle to the ControlInfoMap. */\n> \tstruct ipa_controls_header hdr;\n> \thdr.version = IPA_CONTROLS_FORMAT_VERSION;\n> @@ -195,6 +206,7 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,\n> \thdr.entries = infoMap.size();\n> \thdr.size = sizeof(hdr) + entriesSize + valuesSize;\n> \thdr.data_offset = sizeof(hdr) + entriesSize;\n> +\thdr.idMapType = idMapType;\n> \n> \tbuffer.write(&hdr);\n> \n> @@ -368,6 +380,34 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> \t\treturn {};\n> \t}\n> \n> +\t/*\n> +\t * Use the id map type to retrieve the ControlId instances associated\n> +\t * to the serialized numerical identifier.\n> +\t *\n> +\t * If a globally available id map such as controls::controls or\n> +\t * properties::properties is used, the ControlId instances can be\n> +\t * retrieved from there.\n> +\t *\n> +\t * If, otherwise, the idmap is valid only on one side of the IPC\n> +\t * boundary (as in case it has been created by the V4L2 devices)\n> +\t * the deserializer shall create new ControlId and store them in an\n> +\t * locally created id map.\n> +\t */\n> +\tconst ControlIdMap *idMap = nullptr;\n> +\tControlIdMap *localIdMap;\n> +\tswitch (hdr->idMapType) {\n> +\tcase IdMapType::LIBCAMERA_CONTROLS:\n> +\t\tidMap = &controls::controls;\n> +\t\tbreak;\n> +\tcase IdMapType::LIBCAMERA_PROPERTIES:\n> +\t\tidMap = &properties::properties;\n> +\t\tbreak;\n> +\tcase IdMapType::V4L2_CONTROLS:\n> +\t\tcontrolIdMaps_.emplace_back(std::make_unique());\n> +\t\tlocalIdMap = controlIdMaps_.back().get();\n> +\t\tbreak;\n> +\t}\n> +\n> \tByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr));\n> \tByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset);\n> \n> @@ -377,9 +417,6 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> \t}\n> \n> \tControlInfoMap::Map ctrls;\n> -\tcontrolIdMaps_.emplace_back(std::make_unique());\n> -\tControlIdMap *localIdMap = controlIdMaps_.back().get();\n> -\n> \tfor (unsigned int i = 0; i < hdr->entries; ++i) {\n> \t\tconst struct ipa_control_info_entry *entry =\n> \t\t\tentries.read();\n> @@ -388,15 +425,27 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> \t\t\treturn {};\n> \t\t}\n> \n> -\t\t/* Create and cache the individual ControlId. */\n> \t\tControlType type = static_cast(entry->type);\n> -\t\t/**\n> -\t\t * \\todo Find a way to preserve the control name for debugging\n> -\t\t * purpose.\n> +\n> +\t\t/*\n> +\t\t * If we have a valid id map, reuse the control id there stored,\n> +\t\t * Otherwise populate the local idmap with a new ControlId\n> +\t\t * reference.\n> \t\t */\n> -\t\tcontrolIds_.emplace_back(std::make_unique(entry->id, \"\", type));\n> -\t\tControlId *controlId = controlIds_.back().get();\n> -\t\t(*localIdMap)[entry->id] = controlId;\n> +\t\tconst ControlId *controlId;\n> +\t\tif (idMap) {\n> +\t\t\tcontrolId = idMap->at(entry->id);\n> +\t\t\tASSERT(controlId);\n> +\t\t} else {\n> +\t\t\t/**\n> +\t\t\t * \\todo Find a way to preserve the control name for\n> +\t\t\t * debugging purpose.\n> +\t\t\t */\n> +\t\t\tcontrolIds_.emplace_back(std::make_unique(entry->id,\n> +\t\t\t\t\t\t\t\t\t \"\", type));\n> +\t\t\tcontrolId = controlIds_.back().get();\n> +\t\t\t(*localIdMap)[entry->id] = controlId;\n> +\t\t}\n> \n> \t\tif (entry->offset != values.offset()) {\n> \t\t\tLOG(Serializer, Error)\n> @@ -405,10 +454,12 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> \t\t\treturn {};\n> \t\t}\n> \n> -\t\t/* Create and store the ControlInfo. */\n> \t\tctrls.emplace(controlId, loadControlInfo(type, values));\n> \t}\n> \n> +\tif (!idMap)\n> +\t\tidMap = localIdMap;\n> +\n> \t/*\n> \t * Create the ControlInfoMap in the cache, and store the map to handle\n> \t * association.\n> diff --git a/src/libcamera/ipa_controls.cpp b/src/libcamera/ipa_controls.cpp\n> index 8fd726513182..1323fa31a28f 100644\n> --- a/src/libcamera/ipa_controls.cpp\n> +++ b/src/libcamera/ipa_controls.cpp\n> @@ -134,6 +134,35 @@\n> * \\brief The current control serialization format version\n> */\n> \n> +/**\n> + * \\enum IdMapType\n> + * \\brief Enumerates the different control id map types\n> + *\n> + * Each ControlInfoMap and ControlList refers to a control id map that\n> + * associates the ControlId references to a numerical identifier.\n> + * During the serialization procedure the raw pointers to the ControlId\n> + * instances cannot be transported on the wire, hence their numerical id is\n> + * used to identify them in the serialized data buffer. At deserialization time\n> + * it is required to associate back to the numerical id the ControlId instance\n> + * it represents. This enumeration describes which ControlIdMap should be\n> + * used to perform such operation.\n> + *\n> + * \\var IdMapType::LIBCAMERA_CONTROLS\n> + * \\brief The numerical control identifier are resolved to a ControlId * using\n> + * the global controls::controls id map\n> + *\n> + * \\var IdMapType::LIBCAMERA_PROPERTIES\n> + * \\brief The numerical control identifier are resolved to a ControlId * using\n> + * the global properties::properties id map\n> + *\n> + * \\var IdMapType::V4L2_CONTROLS\n> + * \\brief ControlId for V4L2 defined controls are created by the video device\n> + * that enumerates them, and are not available across the IPC boundaries. The\n> + * deserializer shall create new ControlId instances for them as well as store\n> + * them in a dedicated ControlIdMap. Only lookup by numerical id can be\n> + * performed on de-serialized ControlInfoMap that represents V4L2 controls.\n> + */\n> +\n> /**\n> * \\struct ipa_controls_header\n> * \\brief Serialized control packet header\n> @@ -149,6 +178,8 @@\n> * The total packet size in bytes\n> * \\var ipa_controls_header::data_offset\n> * Offset in bytes from the beginning of the packet of the data section start\n> + * \\var ipa_controls_header::idMapType\n> + * The id map type as defined by the IdMapType enumeration\n> * \\var ipa_controls_header::reserved\n> * Reserved for future extensions\n> */\n> -- \n> 2.32.0\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 2E9FDC322E\n\tfor ;\n\tThu, 29 Jul 2021 08:26:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9B931687C2;\n\tThu, 29 Jul 2021 10:26:03 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 10A2068536\n\tfor ;\n\tThu, 29 Jul 2021 10:26:02 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 84EEEDD;\n\tThu, 29 Jul 2021 10:26:00 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"h0FEmz9M\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627547161;\n\tbh=j/eKo7viLJjb3+inI51LV1rakFkMPGnaSW+KYSvn8y0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=h0FEmz9MxLMewFWhdHnqSonF+QpYS/qVC61ZQBz0Sw45NHuvCKQ42F83NbVz/KbSz\n\tdECRM3kPcuZkNsj957ODz001qkf1maiTNvlkqK+WQU01JmLxobHfwLqmrxfWZimxKg\n\tlcKHtRzQy/xJB4zGbog8jF0bPXsbvrT0QNHfcJBE=","Date":"Thu, 29 Jul 2021 17:25:53 +0900","From":"paul.elder@ideasonboard.com","To":"Jacopo Mondi ","Message-ID":"<20210729082553.GI2167@pyrite.rasen.tech>","References":"<20210728161116.64489-1-jacopo@jmondi.org>\n\t<20210728161116.64489-3-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20210728161116.64489-3-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v2 2/5] libcamera: controls: Use\n\tControlIdMap in deserialization","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18523,"web_url":"https://patchwork.libcamera.org/comment/18523/","msgid":"","date":"2021-08-03T16:21:20","subject":"Re: [libcamera-devel] [PATCH v2 2/5] libcamera: controls: Use\n\tControlIdMap in deserialization","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Wed, Jul 28, 2021 at 06:11:13PM +0200, Jacopo Mondi wrote:\n> Introduce a new field in the controls serialization protocol to\n> allow discerning which ControlIdMap a ControlInfoMap refers to.\n> \n> The newly introduced IdMapType enumeration describes the possible\n> info maps:\n> - Either the globally available controls::controls and\n> properties::properties maps, which are valid across IPC boundaries\n> - A ControlIdMap created locally by the V4L2 device, which is not valid\n> across the IPC boundaries\n> \n> At de-serialization time the idMapType filed is inspected and\n> - If the idmap is a globally available one, there's no need to create\n> new ControlId instances when populating the de-serialized\n> ControlInfoMap. Use the globally available map to retrieve the\n> ControlId reference and use it\n> - If the idmap is a map only available locally, create a new ControlId\n> as it used to happen before this patch.\n> \n> As a direct consequence, this change allows to perform lookup by\n> ControlId reference on de-serialized ControlIdMap that refers to the\n> libcamera defined controls::controls and properties::properties.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/ipa/ipa_controls.h | 9 +++-\n> src/libcamera/control_serializer.cpp | 73 +++++++++++++++++++++++-----\n> src/libcamera/ipa_controls.cpp | 31 ++++++++++++\n> 3 files changed, 101 insertions(+), 12 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/ipa_controls.h b/include/libcamera/ipa/ipa_controls.h\n> index 6d3bf279c22d..5b1690066314 100644\n> --- a/include/libcamera/ipa/ipa_controls.h\n> +++ b/include/libcamera/ipa/ipa_controls.h\n> @@ -15,13 +15,20 @@ extern \"C\" {\n> \n> #define IPA_CONTROLS_FORMAT_VERSION\t1\n> \n> +enum class IdMapType {\n> +\tLIBCAMERA_CONTROLS,\n> +\tLIBCAMERA_PROPERTIES,\n> +\tV4L2_CONTROLS,\n> +};\n\nThis is in an extern \"C\" block, as the structures defined herein were\nmeant to be useable from C code. \"enum class\" is thus not valid. Let's\nname this enum ipa_control_id_map_type to match the rest of the file:\n\nenum ipa_control_id_map_type {\n\tIPA_CONTROL_ID_MAP_CONTROLS,\n\tIPA_CONTROL_ID_MAP_PROPERTIES,\n\tIPA_CONTROL_ID_MAP_V4L2,\n};\n\n> +\n> struct ipa_controls_header {\n> \tuint32_t version;\n> \tuint32_t handle;\n> \tuint32_t entries;\n> \tuint32_t size;\n> \tuint32_t data_offset;\n> -\tuint32_t reserved[3];\n> +\tIdMapType idMapType;\n\nAnd this field should then be named id_map_type.\n\n> +\tuint32_t reserved[2];\n> };\n> \n> struct ipa_control_value_entry {\n> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> index df6ed93f477e..255f9d30fb85 100644\n> --- a/src/libcamera/control_serializer.cpp\n> +++ b/src/libcamera/control_serializer.cpp\n> @@ -17,6 +17,7 @@\n> #include \n> #include \n> #include \n> +#include \n> \n> #include \"libcamera/internal/byte_stream_buffer.h\"\n> \n> @@ -188,6 +189,16 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,\n> \tfor (const auto &ctrl : infoMap)\n> \t\tvaluesSize += binarySize(ctrl.second);\n> \n> +\t/* Serialize the id map type to be used when de-serializing. */\n\nIt's kind of implied that the data we serialize is used for\ndeserialization :-)\n\n> +\tconst ControlIdMap *idmap = &infoMap.idmap();\n> +\tenum IdMapType idMapType;\n> +\tif (idmap == &controls::controls)\n> +\t\tidMapType = IdMapType::LIBCAMERA_CONTROLS;\n> +\telse if (idmap == &properties::properties)\n> +\t\tidMapType = IdMapType::LIBCAMERA_PROPERTIES;\n> +\telse\n> +\t\tidMapType = IdMapType::V4L2_CONTROLS;\n> +\n> \t/* Prepare the packet header, assign a handle to the ControlInfoMap. */\n> \tstruct ipa_controls_header hdr;\n> \thdr.version = IPA_CONTROLS_FORMAT_VERSION;\n> @@ -195,6 +206,7 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,\n> \thdr.entries = infoMap.size();\n> \thdr.size = sizeof(hdr) + entriesSize + valuesSize;\n> \thdr.data_offset = sizeof(hdr) + entriesSize;\n> +\thdr.idMapType = idMapType;\n> \n> \tbuffer.write(&hdr);\n> \n> @@ -368,6 +380,34 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> \t\treturn {};\n> \t}\n> \n> +\t/*\n> +\t * Use the id map type to retrieve the ControlId instances associated\n> +\t * to the serialized numerical identifier.\n\ns/to/with/\n\n> +\t *\n> +\t * If a globally available id map such as controls::controls or\n\ns/available/defined/\n\n> +\t * properties::properties is used, the ControlId instances can be\n> +\t * retrieved from there.\n> +\t *\n> +\t * If, otherwise, the idmap is valid only on one side of the IPC\n> +\t * boundary (as in case it has been created by the V4L2 devices)\n> +\t * the deserializer shall create new ControlId and store them in an\n> +\t * locally created id map.\n\nCould this be simplified to\n\n\t * Use the ControlIdMap corresponding to the id map type. If the type\n\t * references a globally defined id map (such as controls::controls\n\t * or properties::properties), Use it. Otherwise, create a local id map\n\t * that will be populated with dynamically created ControlId instances\n\t * when deserializing individual ControlInfoMap entries.\n\n> +\t */\n> +\tconst ControlIdMap *idMap = nullptr;\n> +\tControlIdMap *localIdMap;\n\nI'd initialize this to nullptr as well to ensure it will never be used\nuninitialized.\n\n> +\tswitch (hdr->idMapType) {\n> +\tcase IdMapType::LIBCAMERA_CONTROLS:\n> +\t\tidMap = &controls::controls;\n> +\t\tbreak;\n> +\tcase IdMapType::LIBCAMERA_PROPERTIES:\n> +\t\tidMap = &properties::properties;\n> +\t\tbreak;\n> +\tcase IdMapType::V4L2_CONTROLS:\n> +\t\tcontrolIdMaps_.emplace_back(std::make_unique());\n> +\t\tlocalIdMap = controlIdMaps_.back().get();\n\nI'd add\n\n\t\tidMap = localIdMap;\n\nhere (see below).\n\n> +\t\tbreak;\n\nNever trust a binary stream:\n\n\tdefault:\n\t\treturn {};\n\n> +\t}\n> +\n> \tByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr));\n> \tByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset);\n> \n> @@ -377,9 +417,6 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> \t}\n> \n> \tControlInfoMap::Map ctrls;\n> -\tcontrolIdMaps_.emplace_back(std::make_unique());\n> -\tControlIdMap *localIdMap = controlIdMaps_.back().get();\n> -\n> \tfor (unsigned int i = 0; i < hdr->entries; ++i) {\n> \t\tconst struct ipa_control_info_entry *entry =\n> \t\t\tentries.read();\n> @@ -388,15 +425,27 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> \t\t\treturn {};\n> \t\t}\n> \n> -\t\t/* Create and cache the individual ControlId. */\n> \t\tControlType type = static_cast(entry->type);\n> -\t\t/**\n> -\t\t * \\todo Find a way to preserve the control name for debugging\n> -\t\t * purpose.\n> +\n> +\t\t/*\n> +\t\t * If we have a valid id map, reuse the control id there stored,\n> +\t\t * Otherwise populate the local idmap with a new ControlId\n> +\t\t * reference.\n> \t\t */\n> -\t\tcontrolIds_.emplace_back(std::make_unique(entry->id, \"\", type));\n> -\t\tControlId *controlId = controlIds_.back().get();\n> -\t\t(*localIdMap)[entry->id] = controlId;\n> +\t\tconst ControlId *controlId;\n> +\t\tif (idMap) {\n> +\t\t\tcontrolId = idMap->at(entry->id);\n> +\t\t\tASSERT(controlId);\n> +\t\t} else {\n> +\t\t\t/**\n> +\t\t\t * \\todo Find a way to preserve the control name for\n> +\t\t\t * debugging purpose.\n> +\t\t\t */\n> +\t\t\tcontrolIds_.emplace_back(std::make_unique(entry->id,\n> +\t\t\t\t\t\t\t\t\t \"\", type));\n> +\t\t\tcontrolId = controlIds_.back().get();\n> +\t\t\t(*localIdMap)[entry->id] = controlId;\n> +\t\t}\n\nWould this be more readable (related to the \"see below\" comment above) ?\n\n\t\t/* If we're using a local id map, populate it. */\n\t\tif (localIdMap) {\n\t\t\t/**\n\t\t\t * \\todo Find a way to preserve the control name for\n\t\t\t * debugging purpose.\n\t\t\t */\n\t\t\tcontrolIds_.emplace_back(std::make_unique(entry->id,\n\t\t\t\t\t\t\t\t\t \"\", type));\n\t\t\t(*localIdMap)[entry->id] = controlIds_.back().get();\n\t\t}\n\n\t\tconst ControlId *controlId = idMap->at(entry->id);\n\t\tASSERT(controlId);\n\n> \n> \t\tif (entry->offset != values.offset()) {\n> \t\t\tLOG(Serializer, Error)\n> @@ -405,10 +454,12 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> \t\t\treturn {};\n> \t\t}\n> \n> -\t\t/* Create and store the ControlInfo. */\n> \t\tctrls.emplace(controlId, loadControlInfo(type, values));\n> \t}\n> \n> +\tif (!idMap)\n> +\t\tidMap = localIdMap;\n> +\n\nAnd this could be dropped.\n\n> \t/*\n> \t * Create the ControlInfoMap in the cache, and store the map to handle\n> \t * association.\n> diff --git a/src/libcamera/ipa_controls.cpp b/src/libcamera/ipa_controls.cpp\n> index 8fd726513182..1323fa31a28f 100644\n> --- a/src/libcamera/ipa_controls.cpp\n> +++ b/src/libcamera/ipa_controls.cpp\n> @@ -134,6 +134,35 @@\n> * \\brief The current control serialization format version\n> */\n> \n> +/**\n> + * \\enum IdMapType\n> + * \\brief Enumerates the different control id map types\n> + *\n> + * Each ControlInfoMap and ControlList refers to a control id map that\n> + * associates the ControlId references to a numerical identifier.\n> + * During the serialization procedure the raw pointers to the ControlId\n> + * instances cannot be transported on the wire, hence their numerical id is\n> + * used to identify them in the serialized data buffer. At deserialization time\n> + * it is required to associate back to the numerical id the ControlId instance\n> + * it represents. This enumeration describes which ControlIdMap should be\n> + * used to perform such operation.\n> + *\n> + * \\var IdMapType::LIBCAMERA_CONTROLS\n> + * \\brief The numerical control identifier are resolved to a ControlId * using\n> + * the global controls::controls id map\n> + *\n> + * \\var IdMapType::LIBCAMERA_PROPERTIES\n> + * \\brief The numerical control identifier are resolved to a ControlId * using\n> + * the global properties::properties id map\n> + *\n> + * \\var IdMapType::V4L2_CONTROLS\n> + * \\brief ControlId for V4L2 defined controls are created by the video device\n> + * that enumerates them, and are not available across the IPC boundaries. The\n> + * deserializer shall create new ControlId instances for them as well as store\n> + * them in a dedicated ControlIdMap. Only lookup by numerical id can be\n> + * performed on de-serialized ControlInfoMap that represents V4L2 controls.\n> + */\n> +\n> /**\n> * \\struct ipa_controls_header\n> * \\brief Serialized control packet header\n> @@ -149,6 +178,8 @@\n> * The total packet size in bytes\n> * \\var ipa_controls_header::data_offset\n> * Offset in bytes from the beginning of the packet of the data section start\n> + * \\var ipa_controls_header::idMapType\n> + * The id map type as defined by the IdMapType enumeration\n> * \\var ipa_controls_header::reserved\n> * Reserved for future extensions\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 1C7BBC3235\n\tfor ;\n\tTue, 3 Aug 2021 16:21:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8DD84687CC;\n\tTue, 3 Aug 2021 18:21:34 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DB57F6026A\n\tfor ;\n\tTue, 3 Aug 2021 18:21:32 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5634F3F0;\n\tTue, 3 Aug 2021 18:21:32 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"r/Bgui+E\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628007692;\n\tbh=jag0MZG+Ork8FkB66kN3RFhldS7ZArdZ9ui3mCAKLjE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=r/Bgui+EdEpFYLETV+j0SSk5e2bHhxrbmcakDCtxEI/I3Hhxp9Bl9R9PWUJ5USP9s\n\tTgnKveyfX+mgLoWIAoURVauD9Uqc5Dt3PJlGkXgP573VKnKyMhzvY1MPgY0tqvXeZa\n\t8x+M+PCaTiyqg0ke9SohqSXLHHZUXcw2XE24rpnw=","Date":"Tue, 3 Aug 2021 19:21:20 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210728161116.64489-1-jacopo@jmondi.org>\n\t<20210728161116.64489-3-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210728161116.64489-3-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v2 2/5] libcamera: controls: Use\n\tControlIdMap in deserialization","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18525,"web_url":"https://patchwork.libcamera.org/comment/18525/","msgid":"","date":"2021-08-03T16:41:45","subject":"Re: [libcamera-devel] [PATCH v2 2/5] libcamera: controls: Use\n\tControlIdMap in deserialization","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Aug 03, 2021 at 07:21:21PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n> \n> Thank you for the patch.\n> \n> On Wed, Jul 28, 2021 at 06:11:13PM +0200, Jacopo Mondi wrote:\n> > Introduce a new field in the controls serialization protocol to\n> > allow discerning which ControlIdMap a ControlInfoMap refers to.\n> > \n> > The newly introduced IdMapType enumeration describes the possible\n> > info maps:\n> > - Either the globally available controls::controls and\n> > properties::properties maps, which are valid across IPC boundaries\n> > - A ControlIdMap created locally by the V4L2 device, which is not valid\n> > across the IPC boundaries\n> > \n> > At de-serialization time the idMapType filed is inspected and\n> > - If the idmap is a globally available one, there's no need to create\n> > new ControlId instances when populating the de-serialized\n> > ControlInfoMap. Use the globally available map to retrieve the\n> > ControlId reference and use it\n> > - If the idmap is a map only available locally, create a new ControlId\n> > as it used to happen before this patch.\n> > \n> > As a direct consequence, this change allows to perform lookup by\n> > ControlId reference on de-serialized ControlIdMap that refers to the\n> > libcamera defined controls::controls and properties::properties.\n> > \n> > Signed-off-by: Jacopo Mondi \n\nThis is related to https://bugs.libcamera.org/show_bug.cgi?id=31, right\n? If so, should the series include a patch that addresses the todo\ncomment in ControlList::merge() ? Maybe it could be as simple as a\nrevert of 414babb60b54 (\"libcamera: controls: Remove merge assertion\").\n\n> > ---\n> > include/libcamera/ipa/ipa_controls.h | 9 +++-\n> > src/libcamera/control_serializer.cpp | 73 +++++++++++++++++++++++-----\n> > src/libcamera/ipa_controls.cpp | 31 ++++++++++++\n> > 3 files changed, 101 insertions(+), 12 deletions(-)\n> > \n> > diff --git a/include/libcamera/ipa/ipa_controls.h b/include/libcamera/ipa/ipa_controls.h\n> > index 6d3bf279c22d..5b1690066314 100644\n> > --- a/include/libcamera/ipa/ipa_controls.h\n> > +++ b/include/libcamera/ipa/ipa_controls.h\n> > @@ -15,13 +15,20 @@ extern \"C\" {\n> > \n> > #define IPA_CONTROLS_FORMAT_VERSION\t1\n> > \n> > +enum class IdMapType {\n> > +\tLIBCAMERA_CONTROLS,\n> > +\tLIBCAMERA_PROPERTIES,\n> > +\tV4L2_CONTROLS,\n> > +};\n> \n> This is in an extern \"C\" block, as the structures defined herein were\n> meant to be useable from C code. \"enum class\" is thus not valid. Let's\n> name this enum ipa_control_id_map_type to match the rest of the file:\n> \n> enum ipa_control_id_map_type {\n> \tIPA_CONTROL_ID_MAP_CONTROLS,\n> \tIPA_CONTROL_ID_MAP_PROPERTIES,\n> \tIPA_CONTROL_ID_MAP_V4L2,\n> };\n> \n> > +\n> > struct ipa_controls_header {\n> > \tuint32_t version;\n> > \tuint32_t handle;\n> > \tuint32_t entries;\n> > \tuint32_t size;\n> > \tuint32_t data_offset;\n> > -\tuint32_t reserved[3];\n> > +\tIdMapType idMapType;\n> \n> And this field should then be named id_map_type.\n> \n> > +\tuint32_t reserved[2];\n> > };\n> > \n> > struct ipa_control_value_entry {\n> > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> > index df6ed93f477e..255f9d30fb85 100644\n> > --- a/src/libcamera/control_serializer.cpp\n> > +++ b/src/libcamera/control_serializer.cpp\n> > @@ -17,6 +17,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > \n> > #include \"libcamera/internal/byte_stream_buffer.h\"\n> > \n> > @@ -188,6 +189,16 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,\n> > \tfor (const auto &ctrl : infoMap)\n> > \t\tvaluesSize += binarySize(ctrl.second);\n> > \n> > +\t/* Serialize the id map type to be used when de-serializing. */\n> \n> It's kind of implied that the data we serialize is used for\n> deserialization :-)\n> \n> > +\tconst ControlIdMap *idmap = &infoMap.idmap();\n> > +\tenum IdMapType idMapType;\n> > +\tif (idmap == &controls::controls)\n> > +\t\tidMapType = IdMapType::LIBCAMERA_CONTROLS;\n> > +\telse if (idmap == &properties::properties)\n> > +\t\tidMapType = IdMapType::LIBCAMERA_PROPERTIES;\n> > +\telse\n> > +\t\tidMapType = IdMapType::V4L2_CONTROLS;\n> > +\n> > \t/* Prepare the packet header, assign a handle to the ControlInfoMap. */\n> > \tstruct ipa_controls_header hdr;\n> > \thdr.version = IPA_CONTROLS_FORMAT_VERSION;\n> > @@ -195,6 +206,7 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,\n> > \thdr.entries = infoMap.size();\n> > \thdr.size = sizeof(hdr) + entriesSize + valuesSize;\n> > \thdr.data_offset = sizeof(hdr) + entriesSize;\n> > +\thdr.idMapType = idMapType;\n> > \n> > \tbuffer.write(&hdr);\n> > \n> > @@ -368,6 +380,34 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> > \t\treturn {};\n> > \t}\n> > \n> > +\t/*\n> > +\t * Use the id map type to retrieve the ControlId instances associated\n> > +\t * to the serialized numerical identifier.\n> \n> s/to/with/\n> \n> > +\t *\n> > +\t * If a globally available id map such as controls::controls or\n> \n> s/available/defined/\n> \n> > +\t * properties::properties is used, the ControlId instances can be\n> > +\t * retrieved from there.\n> > +\t *\n> > +\t * If, otherwise, the idmap is valid only on one side of the IPC\n> > +\t * boundary (as in case it has been created by the V4L2 devices)\n> > +\t * the deserializer shall create new ControlId and store them in an\n> > +\t * locally created id map.\n> \n> Could this be simplified to\n> \n> \t * Use the ControlIdMap corresponding to the id map type. If the type\n> \t * references a globally defined id map (such as controls::controls\n> \t * or properties::properties), Use it. Otherwise, create a local id map\n> \t * that will be populated with dynamically created ControlId instances\n> \t * when deserializing individual ControlInfoMap entries.\n> \n> > +\t */\n> > +\tconst ControlIdMap *idMap = nullptr;\n> > +\tControlIdMap *localIdMap;\n> \n> I'd initialize this to nullptr as well to ensure it will never be used\n> uninitialized.\n> \n> > +\tswitch (hdr->idMapType) {\n> > +\tcase IdMapType::LIBCAMERA_CONTROLS:\n> > +\t\tidMap = &controls::controls;\n> > +\t\tbreak;\n> > +\tcase IdMapType::LIBCAMERA_PROPERTIES:\n> > +\t\tidMap = &properties::properties;\n> > +\t\tbreak;\n> > +\tcase IdMapType::V4L2_CONTROLS:\n> > +\t\tcontrolIdMaps_.emplace_back(std::make_unique());\n> > +\t\tlocalIdMap = controlIdMaps_.back().get();\n> \n> I'd add\n> \n> \t\tidMap = localIdMap;\n> \n> here (see below).\n> \n> > +\t\tbreak;\n> \n> Never trust a binary stream:\n> \n> \tdefault:\n> \t\treturn {};\n> \n> > +\t}\n> > +\n> > \tByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr));\n> > \tByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset);\n> > \n> > @@ -377,9 +417,6 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> > \t}\n> > \n> > \tControlInfoMap::Map ctrls;\n> > -\tcontrolIdMaps_.emplace_back(std::make_unique());\n> > -\tControlIdMap *localIdMap = controlIdMaps_.back().get();\n> > -\n> > \tfor (unsigned int i = 0; i < hdr->entries; ++i) {\n> > \t\tconst struct ipa_control_info_entry *entry =\n> > \t\t\tentries.read();\n> > @@ -388,15 +425,27 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> > \t\t\treturn {};\n> > \t\t}\n> > \n> > -\t\t/* Create and cache the individual ControlId. */\n> > \t\tControlType type = static_cast(entry->type);\n> > -\t\t/**\n> > -\t\t * \\todo Find a way to preserve the control name for debugging\n> > -\t\t * purpose.\n> > +\n> > +\t\t/*\n> > +\t\t * If we have a valid id map, reuse the control id there stored,\n> > +\t\t * Otherwise populate the local idmap with a new ControlId\n> > +\t\t * reference.\n> > \t\t */\n> > -\t\tcontrolIds_.emplace_back(std::make_unique(entry->id, \"\", type));\n> > -\t\tControlId *controlId = controlIds_.back().get();\n> > -\t\t(*localIdMap)[entry->id] = controlId;\n> > +\t\tconst ControlId *controlId;\n> > +\t\tif (idMap) {\n> > +\t\t\tcontrolId = idMap->at(entry->id);\n> > +\t\t\tASSERT(controlId);\n> > +\t\t} else {\n> > +\t\t\t/**\n> > +\t\t\t * \\todo Find a way to preserve the control name for\n> > +\t\t\t * debugging purpose.\n> > +\t\t\t */\n> > +\t\t\tcontrolIds_.emplace_back(std::make_unique(entry->id,\n> > +\t\t\t\t\t\t\t\t\t \"\", type));\n> > +\t\t\tcontrolId = controlIds_.back().get();\n> > +\t\t\t(*localIdMap)[entry->id] = controlId;\n> > +\t\t}\n> \n> Would this be more readable (related to the \"see below\" comment above) ?\n> \n> \t\t/* If we're using a local id map, populate it. */\n> \t\tif (localIdMap) {\n> \t\t\t/**\n> \t\t\t * \\todo Find a way to preserve the control name for\n> \t\t\t * debugging purpose.\n> \t\t\t */\n> \t\t\tcontrolIds_.emplace_back(std::make_unique(entry->id,\n> \t\t\t\t\t\t\t\t\t \"\", type));\n> \t\t\t(*localIdMap)[entry->id] = controlIds_.back().get();\n> \t\t}\n> \n> \t\tconst ControlId *controlId = idMap->at(entry->id);\n> \t\tASSERT(controlId);\n> \n> > \n> > \t\tif (entry->offset != values.offset()) {\n> > \t\t\tLOG(Serializer, Error)\n> > @@ -405,10 +454,12 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> > \t\t\treturn {};\n> > \t\t}\n> > \n> > -\t\t/* Create and store the ControlInfo. */\n> > \t\tctrls.emplace(controlId, loadControlInfo(type, values));\n> > \t}\n> > \n> > +\tif (!idMap)\n> > +\t\tidMap = localIdMap;\n> > +\n> \n> And this could be dropped.\n> \n> > \t/*\n> > \t * Create the ControlInfoMap in the cache, and store the map to handle\n> > \t * association.\n> > diff --git a/src/libcamera/ipa_controls.cpp b/src/libcamera/ipa_controls.cpp\n> > index 8fd726513182..1323fa31a28f 100644\n> > --- a/src/libcamera/ipa_controls.cpp\n> > +++ b/src/libcamera/ipa_controls.cpp\n> > @@ -134,6 +134,35 @@\n> > * \\brief The current control serialization format version\n> > */\n> > \n> > +/**\n> > + * \\enum IdMapType\n> > + * \\brief Enumerates the different control id map types\n> > + *\n> > + * Each ControlInfoMap and ControlList refers to a control id map that\n> > + * associates the ControlId references to a numerical identifier.\n> > + * During the serialization procedure the raw pointers to the ControlId\n> > + * instances cannot be transported on the wire, hence their numerical id is\n> > + * used to identify them in the serialized data buffer. At deserialization time\n> > + * it is required to associate back to the numerical id the ControlId instance\n> > + * it represents. This enumeration describes which ControlIdMap should be\n> > + * used to perform such operation.\n> > + *\n> > + * \\var IdMapType::LIBCAMERA_CONTROLS\n> > + * \\brief The numerical control identifier are resolved to a ControlId * using\n> > + * the global controls::controls id map\n> > + *\n> > + * \\var IdMapType::LIBCAMERA_PROPERTIES\n> > + * \\brief The numerical control identifier are resolved to a ControlId * using\n> > + * the global properties::properties id map\n> > + *\n> > + * \\var IdMapType::V4L2_CONTROLS\n> > + * \\brief ControlId for V4L2 defined controls are created by the video device\n> > + * that enumerates them, and are not available across the IPC boundaries. The\n> > + * deserializer shall create new ControlId instances for them as well as store\n> > + * them in a dedicated ControlIdMap. Only lookup by numerical id can be\n> > + * performed on de-serialized ControlInfoMap that represents V4L2 controls.\n> > + */\n> > +\n> > /**\n> > * \\struct ipa_controls_header\n> > * \\brief Serialized control packet header\n> > @@ -149,6 +178,8 @@\n> > * The total packet size in bytes\n> > * \\var ipa_controls_header::data_offset\n> > * Offset in bytes from the beginning of the packet of the data section start\n> > + * \\var ipa_controls_header::idMapType\n> > + * The id map type as defined by the IdMapType enumeration\n> > * \\var ipa_controls_header::reserved\n> > * Reserved for future extensions\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 9AE31C3235\n\tfor ;\n\tTue, 3 Aug 2021 16:41:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1BAA0687CC;\n\tTue, 3 Aug 2021 18:41:59 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AE3926026A\n\tfor ;\n\tTue, 3 Aug 2021 18:41:57 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 254BA3F0;\n\tTue, 3 Aug 2021 18:41:57 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Q0S7MI8v\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628008917;\n\tbh=pumjzEJ7VzFFrTlfVwebcHTCa33XQ36OupK39RXdWgU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Q0S7MI8vzRdKQmHYyDKTigwuD7cH15zeCIOsSK/fYNwueYq7lkgM4/KZbNkvOKN1e\n\t/jlbQH6oTjtgZTjOnb2hXGsdr50wuFCB9MsBABtKCy69XcZbLoojIAfu9Z6hzqxPcg\n\t2ylOwYyGMxN8Yvfb7wjQhOA/T17is24ZIAFZxYjs=","Date":"Tue, 3 Aug 2021 19:41:45 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210728161116.64489-1-jacopo@jmondi.org>\n\t<20210728161116.64489-3-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 2/5] libcamera: controls: Use\n\tControlIdMap in deserialization","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18622,"web_url":"https://patchwork.libcamera.org/comment/18622/","msgid":"<20210809135838.rnmdq6ncvosbnov7@uno.localdomain>","date":"2021-08-09T13:58:38","subject":"Re: [libcamera-devel] [PATCH v2 2/5] libcamera: controls: Use\n\tControlIdMap in deserialization","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Tue, Aug 03, 2021 at 07:41:45PM +0300, Laurent Pinchart wrote:\n> On Tue, Aug 03, 2021 at 07:21:21PM +0300, Laurent Pinchart wrote:\n> > Hi Jacopo,\n> >\n> > Thank you for the patch.\n> >\n> > On Wed, Jul 28, 2021 at 06:11:13PM +0200, Jacopo Mondi wrote:\n> > > Introduce a new field in the controls serialization protocol to\n> > > allow discerning which ControlIdMap a ControlInfoMap refers to.\n> > >\n> > > The newly introduced IdMapType enumeration describes the possible\n> > > info maps:\n> > > - Either the globally available controls::controls and\n> > > properties::properties maps, which are valid across IPC boundaries\n> > > - A ControlIdMap created locally by the V4L2 device, which is not valid\n> > > across the IPC boundaries\n> > >\n> > > At de-serialization time the idMapType filed is inspected and\n> > > - If the idmap is a globally available one, there's no need to create\n> > > new ControlId instances when populating the de-serialized\n> > > ControlInfoMap. Use the globally available map to retrieve the\n> > > ControlId reference and use it\n> > > - If the idmap is a map only available locally, create a new ControlId\n> > > as it used to happen before this patch.\n> > >\n> > > As a direct consequence, this change allows to perform lookup by\n> > > ControlId reference on de-serialized ControlIdMap that refers to the\n> > > libcamera defined controls::controls and properties::properties.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n>\n> This is related to https://bugs.libcamera.org/show_bug.cgi?id=31, right\n> ? If so, should the series include a patch that addresses the todo\n> comment in ControlList::merge() ? Maybe it could be as simple as a\n> revert of 414babb60b54 (\"libcamera: controls: Remove merge assertion\").\n>\n\nI don't think this series solves the issue mentioned in the bug, not\nin full at least. If the idmap the ControlList refers to is one the\nglobally defined ones, then yes, they get re-used and the assertion\ncould be brought back in place. For ControlList that reference V4L2\ncontrols the serializer still creates its own local copy of the idmap,\nso the assertion would fail.\n\nwe can assert for controls::controls and properties::properties, but\nfor locally created idmaps there's no way around checking that all\nelemenents in one list are present in the other list as well.\n\n> > > ---\n> > > include/libcamera/ipa/ipa_controls.h | 9 +++-\n> > > src/libcamera/control_serializer.cpp | 73 +++++++++++++++++++++++-----\n> > > src/libcamera/ipa_controls.cpp | 31 ++++++++++++\n> > > 3 files changed, 101 insertions(+), 12 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/ipa/ipa_controls.h b/include/libcamera/ipa/ipa_controls.h\n> > > index 6d3bf279c22d..5b1690066314 100644\n> > > --- a/include/libcamera/ipa/ipa_controls.h\n> > > +++ b/include/libcamera/ipa/ipa_controls.h\n> > > @@ -15,13 +15,20 @@ extern \"C\" {\n> > >\n> > > #define IPA_CONTROLS_FORMAT_VERSION\t1\n> > >\n> > > +enum class IdMapType {\n> > > +\tLIBCAMERA_CONTROLS,\n> > > +\tLIBCAMERA_PROPERTIES,\n> > > +\tV4L2_CONTROLS,\n> > > +};\n> >\n> > This is in an extern \"C\" block, as the structures defined herein were\n> > meant to be useable from C code. \"enum class\" is thus not valid. Let's\n> > name this enum ipa_control_id_map_type to match the rest of the file:\n> >\n> > enum ipa_control_id_map_type {\n> > \tIPA_CONTROL_ID_MAP_CONTROLS,\n> > \tIPA_CONTROL_ID_MAP_PROPERTIES,\n> > \tIPA_CONTROL_ID_MAP_V4L2,\n> > };\n> >\n> > > +\n> > > struct ipa_controls_header {\n> > > \tuint32_t version;\n> > > \tuint32_t handle;\n> > > \tuint32_t entries;\n> > > \tuint32_t size;\n> > > \tuint32_t data_offset;\n> > > -\tuint32_t reserved[3];\n> > > +\tIdMapType idMapType;\n> >\n> > And this field should then be named id_map_type.\n> >\n> > > +\tuint32_t reserved[2];\n> > > };\n> > >\n> > > struct ipa_control_value_entry {\n> > > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> > > index df6ed93f477e..255f9d30fb85 100644\n> > > --- a/src/libcamera/control_serializer.cpp\n> > > +++ b/src/libcamera/control_serializer.cpp\n> > > @@ -17,6 +17,7 @@\n> > > #include \n> > > #include \n> > > #include \n> > > +#include \n> > >\n> > > #include \"libcamera/internal/byte_stream_buffer.h\"\n> > >\n> > > @@ -188,6 +189,16 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,\n> > > \tfor (const auto &ctrl : infoMap)\n> > > \t\tvaluesSize += binarySize(ctrl.second);\n> > >\n> > > +\t/* Serialize the id map type to be used when de-serializing. */\n> >\n> > It's kind of implied that the data we serialize is used for\n> > deserialization :-)\n> >\n> > > +\tconst ControlIdMap *idmap = &infoMap.idmap();\n> > > +\tenum IdMapType idMapType;\n> > > +\tif (idmap == &controls::controls)\n> > > +\t\tidMapType = IdMapType::LIBCAMERA_CONTROLS;\n> > > +\telse if (idmap == &properties::properties)\n> > > +\t\tidMapType = IdMapType::LIBCAMERA_PROPERTIES;\n> > > +\telse\n> > > +\t\tidMapType = IdMapType::V4L2_CONTROLS;\n> > > +\n> > > \t/* Prepare the packet header, assign a handle to the ControlInfoMap. */\n> > > \tstruct ipa_controls_header hdr;\n> > > \thdr.version = IPA_CONTROLS_FORMAT_VERSION;\n> > > @@ -195,6 +206,7 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,\n> > > \thdr.entries = infoMap.size();\n> > > \thdr.size = sizeof(hdr) + entriesSize + valuesSize;\n> > > \thdr.data_offset = sizeof(hdr) + entriesSize;\n> > > +\thdr.idMapType = idMapType;\n> > >\n> > > \tbuffer.write(&hdr);\n> > >\n> > > @@ -368,6 +380,34 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> > > \t\treturn {};\n> > > \t}\n> > >\n> > > +\t/*\n> > > +\t * Use the id map type to retrieve the ControlId instances associated\n> > > +\t * to the serialized numerical identifier.\n> >\n> > s/to/with/\n> >\n> > > +\t *\n> > > +\t * If a globally available id map such as controls::controls or\n> >\n> > s/available/defined/\n> >\n> > > +\t * properties::properties is used, the ControlId instances can be\n> > > +\t * retrieved from there.\n> > > +\t *\n> > > +\t * If, otherwise, the idmap is valid only on one side of the IPC\n> > > +\t * boundary (as in case it has been created by the V4L2 devices)\n> > > +\t * the deserializer shall create new ControlId and store them in an\n> > > +\t * locally created id map.\n> >\n> > Could this be simplified to\n> >\n> > \t * Use the ControlIdMap corresponding to the id map type. If the type\n> > \t * references a globally defined id map (such as controls::controls\n> > \t * or properties::properties), Use it. Otherwise, create a local id map\n> > \t * that will be populated with dynamically created ControlId instances\n> > \t * when deserializing individual ControlInfoMap entries.\n> >\n> > > +\t */\n> > > +\tconst ControlIdMap *idMap = nullptr;\n> > > +\tControlIdMap *localIdMap;\n> >\n> > I'd initialize this to nullptr as well to ensure it will never be used\n> > uninitialized.\n> >\n> > > +\tswitch (hdr->idMapType) {\n> > > +\tcase IdMapType::LIBCAMERA_CONTROLS:\n> > > +\t\tidMap = &controls::controls;\n> > > +\t\tbreak;\n> > > +\tcase IdMapType::LIBCAMERA_PROPERTIES:\n> > > +\t\tidMap = &properties::properties;\n> > > +\t\tbreak;\n> > > +\tcase IdMapType::V4L2_CONTROLS:\n> > > +\t\tcontrolIdMaps_.emplace_back(std::make_unique());\n> > > +\t\tlocalIdMap = controlIdMaps_.back().get();\n> >\n> > I'd add\n> >\n> > \t\tidMap = localIdMap;\n> >\n> > here (see below).\n> >\n> > > +\t\tbreak;\n> >\n> > Never trust a binary stream:\n> >\n> > \tdefault:\n> > \t\treturn {};\n> >\n> > > +\t}\n> > > +\n> > > \tByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr));\n> > > \tByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset);\n> > >\n> > > @@ -377,9 +417,6 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> > > \t}\n> > >\n> > > \tControlInfoMap::Map ctrls;\n> > > -\tcontrolIdMaps_.emplace_back(std::make_unique());\n> > > -\tControlIdMap *localIdMap = controlIdMaps_.back().get();\n> > > -\n> > > \tfor (unsigned int i = 0; i < hdr->entries; ++i) {\n> > > \t\tconst struct ipa_control_info_entry *entry =\n> > > \t\t\tentries.read();\n> > > @@ -388,15 +425,27 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> > > \t\t\treturn {};\n> > > \t\t}\n> > >\n> > > -\t\t/* Create and cache the individual ControlId. */\n> > > \t\tControlType type = static_cast(entry->type);\n> > > -\t\t/**\n> > > -\t\t * \\todo Find a way to preserve the control name for debugging\n> > > -\t\t * purpose.\n> > > +\n> > > +\t\t/*\n> > > +\t\t * If we have a valid id map, reuse the control id there stored,\n> > > +\t\t * Otherwise populate the local idmap with a new ControlId\n> > > +\t\t * reference.\n> > > \t\t */\n> > > -\t\tcontrolIds_.emplace_back(std::make_unique(entry->id, \"\", type));\n> > > -\t\tControlId *controlId = controlIds_.back().get();\n> > > -\t\t(*localIdMap)[entry->id] = controlId;\n> > > +\t\tconst ControlId *controlId;\n> > > +\t\tif (idMap) {\n> > > +\t\t\tcontrolId = idMap->at(entry->id);\n> > > +\t\t\tASSERT(controlId);\n> > > +\t\t} else {\n> > > +\t\t\t/**\n> > > +\t\t\t * \\todo Find a way to preserve the control name for\n> > > +\t\t\t * debugging purpose.\n> > > +\t\t\t */\n> > > +\t\t\tcontrolIds_.emplace_back(std::make_unique(entry->id,\n> > > +\t\t\t\t\t\t\t\t\t \"\", type));\n> > > +\t\t\tcontrolId = controlIds_.back().get();\n> > > +\t\t\t(*localIdMap)[entry->id] = controlId;\n> > > +\t\t}\n> >\n> > Would this be more readable (related to the \"see below\" comment above) ?\n> >\n> > \t\t/* If we're using a local id map, populate it. */\n> > \t\tif (localIdMap) {\n> > \t\t\t/**\n> > \t\t\t * \\todo Find a way to preserve the control name for\n> > \t\t\t * debugging purpose.\n> > \t\t\t */\n> > \t\t\tcontrolIds_.emplace_back(std::make_unique(entry->id,\n> > \t\t\t\t\t\t\t\t\t \"\", type));\n> > \t\t\t(*localIdMap)[entry->id] = controlIds_.back().get();\n> > \t\t}\n> >\n> > \t\tconst ControlId *controlId = idMap->at(entry->id);\n> > \t\tASSERT(controlId);\n> >\n> > >\n> > > \t\tif (entry->offset != values.offset()) {\n> > > \t\t\tLOG(Serializer, Error)\n> > > @@ -405,10 +454,12 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> > > \t\t\treturn {};\n> > > \t\t}\n> > >\n> > > -\t\t/* Create and store the ControlInfo. */\n> > > \t\tctrls.emplace(controlId, loadControlInfo(type, values));\n> > > \t}\n> > >\n> > > +\tif (!idMap)\n> > > +\t\tidMap = localIdMap;\n> > > +\n> >\n> > And this could be dropped.\n> >\n> > > \t/*\n> > > \t * Create the ControlInfoMap in the cache, and store the map to handle\n> > > \t * association.\n> > > diff --git a/src/libcamera/ipa_controls.cpp b/src/libcamera/ipa_controls.cpp\n> > > index 8fd726513182..1323fa31a28f 100644\n> > > --- a/src/libcamera/ipa_controls.cpp\n> > > +++ b/src/libcamera/ipa_controls.cpp\n> > > @@ -134,6 +134,35 @@\n> > > * \\brief The current control serialization format version\n> > > */\n> > >\n> > > +/**\n> > > + * \\enum IdMapType\n> > > + * \\brief Enumerates the different control id map types\n> > > + *\n> > > + * Each ControlInfoMap and ControlList refers to a control id map that\n> > > + * associates the ControlId references to a numerical identifier.\n> > > + * During the serialization procedure the raw pointers to the ControlId\n> > > + * instances cannot be transported on the wire, hence their numerical id is\n> > > + * used to identify them in the serialized data buffer. At deserialization time\n> > > + * it is required to associate back to the numerical id the ControlId instance\n> > > + * it represents. This enumeration describes which ControlIdMap should be\n> > > + * used to perform such operation.\n> > > + *\n> > > + * \\var IdMapType::LIBCAMERA_CONTROLS\n> > > + * \\brief The numerical control identifier are resolved to a ControlId * using\n> > > + * the global controls::controls id map\n> > > + *\n> > > + * \\var IdMapType::LIBCAMERA_PROPERTIES\n> > > + * \\brief The numerical control identifier are resolved to a ControlId * using\n> > > + * the global properties::properties id map\n> > > + *\n> > > + * \\var IdMapType::V4L2_CONTROLS\n> > > + * \\brief ControlId for V4L2 defined controls are created by the video device\n> > > + * that enumerates them, and are not available across the IPC boundaries. The\n> > > + * deserializer shall create new ControlId instances for them as well as store\n> > > + * them in a dedicated ControlIdMap. Only lookup by numerical id can be\n> > > + * performed on de-serialized ControlInfoMap that represents V4L2 controls.\n> > > + */\n> > > +\n> > > /**\n> > > * \\struct ipa_controls_header\n> > > * \\brief Serialized control packet header\n> > > @@ -149,6 +178,8 @@\n> > > * The total packet size in bytes\n> > > * \\var ipa_controls_header::data_offset\n> > > * Offset in bytes from the beginning of the packet of the data section start\n> > > + * \\var ipa_controls_header::idMapType\n> > > + * The id map type as defined by the IdMapType enumeration\n> > > * \\var ipa_controls_header::reserved\n> > > * Reserved for future extensions\n> > > */\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 5D694BD87D\n\tfor ;\n\tMon, 9 Aug 2021 13:57:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BE93B687EB;\n\tMon, 9 Aug 2021 15:57:52 +0200 (CEST)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 041D660269\n\tfor ;\n\tMon, 9 Aug 2021 15:57:51 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 7B3B540014;\n\tMon, 9 Aug 2021 13:57:50 +0000 (UTC)"],"Date":"Mon, 9 Aug 2021 15:58:38 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210809135838.rnmdq6ncvosbnov7@uno.localdomain>","References":"<20210728161116.64489-1-jacopo@jmondi.org>\n\t<20210728161116.64489-3-jacopo@jmondi.org>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 2/5] libcamera: controls: Use\n\tControlIdMap in deserialization","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]