[{"id":36848,"web_url":"https://patchwork.libcamera.org/comment/36848/","msgid":"","date":"2025-11-17T10:25:35","subject":"Re: [PATCH v6 1/2] libcamera: control_serializer: Add array info to\n\tserialized ControlValue","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n2025. 11. 17. 9:08 keltezéssel, Paul Elder írta:\n> Array controls (eg. ColourCorrectionMatrix, FrameDurationLimits,\n> ColourGains) are serialized properly by the ControlSerializer, but are\n> not deserialized properly. This is because their arrayness and size are\n> not considered during deserialization.\n> \n> Fix this by adding arrayness and size to the serialized form of all\n> ControlValues. This is achieved by fully serializing the min/max/def\n> ControlValue's metadata associated with each ControlInfo entry in the\n> ControlInfoMap.\n> \n> While at it, clean up the serialization format of ControlValues and\n> ControlLists:\n> - ControlValue's id is only used by ControlList, so add a new struct for\n> ControlList entries to contain it, and remove id from ControlValue\n> - Remove offset from ControlInfo's entry, as it is no longer needed,\n> since the serialized data of a ControlInfo has now been converted to\n> simply three serialized ControlValues\n> - Remove the type from the serialized data of ControlValue, as it is\n> already in the metadata entry\n> \n> The issue regarding array controls was not noticed before because the\n> default value of the ControlInfo of other array controls had been set to\n> scalar values similar to how min/max are set, and ColourCorrectionMatrix\n> was the first control to properly define a non-scalar default value.\n> \n> Bug: https://bugs.libcamera.org/show_bug.cgi?id=285\n> Signed-off-by: Paul Elder \n> Tested-by: Barnabás Pőcze # rkisp1\n> Reviewed-by: Daniel Scally \n> \n> ---\n> Changes in v6:\n> - move ipa_control_value_entry into ipa_control_info_entry\n> - update documentation of the serialized format\n> \n> Changes in v5:\n> - re/move reserved fields in ipa_controls.h\n> - remote default parameters from loadControlValue\n> - fix binarySize of ControlValue\n> \n> Changes in v4:\n> - remove id from ipa_control_value_entry, as it is only used by\n> ControlList\n> - move id into a new struct ipa_control_list_entry\n> - remove offset from ipa_control_info_entry, as it is no longer used\n> - since ControlInfo is no longer serialized, and instead it is three\n> ControlValues serialized individually\n> - remove type from the serialized data portion of ControlValue, as the\n> information is in the metadata entries\n> - error-check the offsets when deserializing ControlInfoMap\n> \n> Changes in v3:\n> - instead of adding an extra header to store isArray and numElements\n> like in v2, just reuse struct ipa_control_value_entry, and add these\n> entries to the serialized form of ControlInfoMap to store information\n> for each of the three ControlValues that make of min and max and def\n> in ControlInfoMap\n> \n> Changes in v2:\n> - make it so that the *serialized form* of ControlValue includes\n> arrayness and size instead\n> - Compared to v1, this ties the arrayness and size information to\n> ControlValue instead of ControlId, which as a side effect allows us\n> to support scalar and array min/max values, not just def values.\n> This also gives us support for variable-length arrays\n> ---\n> .../libcamera/internal/control_serializer.h | 8 +-\n> include/libcamera/ipa/ipa_controls.h | 16 +-\n> src/libcamera/control_serializer.cpp | 106 +++++++----\n> src/libcamera/ipa_controls.cpp | 178 ++++++++++--------\n> 4 files changed, 182 insertions(+), 126 deletions(-)\n> \n> diff --git a/include/libcamera/internal/control_serializer.h b/include/libcamera/internal/control_serializer.h\n> index 8a63ae44a13e..307ecba572fc 100644\n> --- a/include/libcamera/internal/control_serializer.h\n> +++ b/include/libcamera/internal/control_serializer.h\n> @@ -47,9 +47,13 @@ private:\n> \tstatic void store(const ControlValue &value, ByteStreamBuffer &buffer);\n> \tstatic void store(const ControlInfo &info, ByteStreamBuffer &buffer);\n> \n> +\tvoid populateControlValueEntry(struct ipa_control_value_entry &entry,\n> +\t\t\t\t const ControlValue &value,\n> +\t\t\t\t uint32_t offset);\n> +\n> \tControlValue loadControlValue(ByteStreamBuffer &buffer,\n> -\t\t\t\t bool isArray = false, unsigned int count = 1);\n> -\tControlInfo loadControlInfo(ByteStreamBuffer &buffer);\n> +\t\t\t\t ControlType type,\n> +\t\t\t\t bool isArray, unsigned int count);\n> \n> \tunsigned int serial_;\n> \tunsigned int serialSeed_;\n> diff --git a/include/libcamera/ipa/ipa_controls.h b/include/libcamera/ipa/ipa_controls.h\n> index 980668c86bcc..6af962ff325e 100644\n> --- a/include/libcamera/ipa/ipa_controls.h\n> +++ b/include/libcamera/ipa/ipa_controls.h\n> @@ -15,7 +15,7 @@ namespace libcamera {\n> extern \"C\" {\n> #endif\n> \n> -#define IPA_CONTROLS_FORMAT_VERSION\t1\n> +#define IPA_CONTROLS_FORMAT_VERSION\t2\n> \n> enum ipa_controls_id_map_type {\n> \tIPA_CONTROL_ID_MAP_CONTROLS,\n> @@ -34,20 +34,26 @@ struct ipa_controls_header {\n> };\n> \n> struct ipa_control_value_entry {\n> -\tuint32_t id;\n> \tuint8_t type;\n> \tuint8_t is_array;\n> \tuint16_t count;\n> \tuint32_t offset;\n> -\tuint32_t padding[1];\n> +\tuint32_t reserved[2];\n> +};\n> +\n> +struct ipa_control_list_entry {\n> +\tuint32_t id;\n> +\tstruct ipa_control_value_entry value;\n> };\n> \n> struct ipa_control_info_entry {\n> \tuint32_t id;\n> \tuint32_t type;\n> -\tuint32_t offset;\n> \tuint8_t direction;\n> -\tuint8_t padding[3];\n> +\tuint8_t padding[7];\n> +\tstruct ipa_control_value_entry min;\n> +\tstruct ipa_control_value_entry max;\n> +\tstruct ipa_control_value_entry def;\n> };\n> \n> #ifdef __cplusplus\n> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> index 050f8512bd52..843e2772f848 100644\n> --- a/src/libcamera/control_serializer.cpp\n> +++ b/src/libcamera/control_serializer.cpp\n> @@ -144,7 +144,7 @@ void ControlSerializer::reset()\n> \n> size_t ControlSerializer::binarySize(const ControlValue &value)\n> {\n> -\treturn sizeof(ControlType) + value.data().size_bytes();\n> +\treturn value.data().size_bytes();\n> }\n> \n> size_t ControlSerializer::binarySize(const ControlInfo &info)\n> @@ -164,7 +164,8 @@ size_t ControlSerializer::binarySize(const ControlInfo &info)\n> size_t ControlSerializer::binarySize(const ControlInfoMap &infoMap)\n> {\n> \tsize_t size = sizeof(struct ipa_controls_header)\n> -\t\t + infoMap.size() * sizeof(struct ipa_control_info_entry);\n> +\t\t + infoMap.size() * (sizeof(struct ipa_control_info_entry) +\n> +\t\t\t\t\t3 * sizeof(struct ipa_control_value_entry));\n\nIs the `3 * ...` part still needed? I think it should already be accounted for since\nthey are now part of `ipa_control_info_entry`?\n\n\n> \n> \tfor (const auto &ctrl : infoMap)\n> \t\tsize += binarySize(ctrl.second);\n> @@ -184,7 +185,7 @@ size_t ControlSerializer::binarySize(const ControlInfoMap &infoMap)\n> size_t ControlSerializer::binarySize(const ControlList &list)\n> {\n> \tsize_t size = sizeof(struct ipa_controls_header)\n> -\t\t + list.size() * sizeof(struct ipa_control_value_entry);\n> +\t\t + list.size() * sizeof(struct ipa_control_list_entry);\n> \n> \tfor (const auto &ctrl : list)\n> \t\tsize += binarySize(ctrl.second);\n> @@ -195,16 +196,17 @@ size_t ControlSerializer::binarySize(const ControlList &list)\n> void ControlSerializer::store(const ControlValue &value,\n> \t\t\t ByteStreamBuffer &buffer)\n> {\n> -\tconst ControlType type = value.type();\n> -\tbuffer.write(&type);\n> \tbuffer.write(value.data());\n> }\n> \n> -void ControlSerializer::store(const ControlInfo &info, ByteStreamBuffer &buffer)\n> +void ControlSerializer::populateControlValueEntry(struct ipa_control_value_entry &entry,\n> +\t\t\t\t\t\t const ControlValue &value,\n> +\t\t\t\t\t\t uint32_t offset)\n> {\n> -\tstore(info.min(), buffer);\n> -\tstore(info.max(), buffer);\n> -\tstore(info.def(), buffer);\n> +\tentry.type = value.type();\n> +\tentry.is_array = value.isArray();\n> +\tentry.count = value.numElements();\n> +\tentry.offset = offset;\n> }\n> \n> /**\n> @@ -232,7 +234,8 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,\n> \n> \t/* Compute entries and data required sizes. */\n> \tsize_t entriesSize = infoMap.size()\n> -\t\t\t * sizeof(struct ipa_control_info_entry);\n> +\t\t\t * (sizeof(struct ipa_control_info_entry) +\n> +\t\t\t 3 * sizeof(struct ipa_control_value_entry));\n\nSame here.\n\n\n> [...]\n\nRegards,\nBarnabás Pőcze","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 99F97BD80A\n\tfor ;\n\tMon, 17 Nov 2025 10:25:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C5F1E606A0;\n\tMon, 17 Nov 2025 11:25:45 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 63B3C606A0\n\tfor ;\n\tMon, 17 Nov 2025 11:25:43 +0100 (CET)","from [192.168.33.31] (185.221.143.100.nat.pool.zt.hu\n\t[185.221.143.100])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C6D33142;\n\tMon, 17 Nov 2025 11:23:39 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"lnw0lncP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1763375020;\n\tbh=qbo08WD8DQmImgVX0ypWxscp4G/LxGNcBiITqb+rWFs=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=lnw0lncP8/QjbwTMh4AEXBngS69EFwyU3pMLW2lgzrvXDQu2RS1Y5XRhiR3y6L8/3\n\tJQWr9i4Y6svoxPyzfjF7QYKl4z4wylU8C2eYSuDF0tOuka6qShPiudzstf9XKuE547\n\ty5VZgy/5HhoxXOw8/D8gfrMdNjEFXuobedeIy6iM=","Message-ID":"","Date":"Mon, 17 Nov 2025 11:25:35 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v6 1/2] libcamera: control_serializer: Add array info to\n\tserialized ControlValue","To":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","Cc":"Daniel Scally ","References":"<20251117080818.3009835-1-paul.elder@ideasonboard.com>\n\t<20251117080818.3009835-2-paul.elder@ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20251117080818.3009835-2-paul.elder@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]