[{"id":36214,"web_url":"https://patchwork.libcamera.org/comment/36214/","msgid":"","date":"2025-10-13T09:53:01","subject":"Re: [PATCH v4 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\n\n2025. 10. 11. 8:33 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> \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 | 6 +-\n> include/libcamera/ipa/ipa_controls.h | 12 +-\n> src/libcamera/control_serializer.cpp | 117 ++++++++++++------\n> src/libcamera/ipa_controls.cpp | 22 +++-\n> 4 files changed, 109 insertions(+), 48 deletions(-)\n> \n> diff --git a/include/libcamera/internal/control_serializer.h b/include/libcamera/internal/control_serializer.h\n> index 8a63ae44a13e..d1ecfbf99113 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 ControlType type,\n> \t\t\t\t bool isArray = false, unsigned int count = 1);\n\nThe default arguments don't seem to be used anywhere, so maybe they could be\nremoved to avoid potential issues there?\n\n\n> -\tControlInfo loadControlInfo(ByteStreamBuffer &buffer);\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..820908eea023 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,7 +34,7 @@ struct ipa_controls_header {\n> };\n> \n> struct ipa_control_value_entry {\n> -\tuint32_t id;\n> +\tuint32_t reserved;\n\nWhy not remove it completely or add to `padding` at the end?\n\n\n> \tuint8_t type;\n> \tuint8_t is_array;\n> \tuint16_t count;\n> @@ -42,10 +42,16 @@ struct ipa_control_value_entry {\n> \tuint32_t padding[1];\n> };\n> \n> +struct ipa_control_list_entry {\n> +\tuint32_t id;\n> +\tuint32_t reserved;\n\nIt's not clear to me why this `reserved` is added? If it wasn't there\nthen `ipa_control_list_entry` could pretty much be \"compatible\" with\nthe previous `ipa_control_value_entry` definition.\n\n\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> +\tuint32_t reserved;\n\nSame here, why not remove / add to padding at the end?\n\n\n> \tuint8_t direction;\n> \tuint8_t padding[3];\n> };\n> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> index 050f8512bd52..8d7a91680fd6 100644\n> --- a/src/libcamera/control_serializer.cpp\n> +++ b/src/libcamera/control_serializer.cpp\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> \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\nThis needs a corresponding adjustment in `ControlSerializer::binarySize(const ControlValue&)`.\n\n\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> \tsize_t valuesSize = 0;\n> \tfor (const auto &ctrl : infoMap)\n> \t\tvaluesSize += binarySize(ctrl.second);\n> @@ -280,11 +283,32 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,\n> \t\tstruct ipa_control_info_entry entry;\n> \t\tentry.id = id->id();\n> \t\tentry.type = id->type();\n> -\t\tentry.offset = values.offset();\n> \t\tentry.direction = static_cast(id->direction());\n> \t\tentries.write(&entry);\n> \n> -\t\tstore(info, values);\n> +\t\t/*\n> +\t\t * Write the metadata for the ControlValue entries as well,\n> +\t\t * since we need type, isArray, and numElements information for\n> +\t\t * min/max/def of the ControlInfo. Doing it this way is the\n> +\t\t * least intrusive in terms of changing the structs in\n> +\t\t * ipa_controls.h\n> +\t\t */\n> +\t\tstruct ipa_control_value_entry valueEntry;\n> +\n> +\t\tpopulateControlValueEntry(valueEntry, info.min(),\n> +\t\t\t\t\t values.offset());\n> +\t\tentries.write(&valueEntry);\n> +\t\tstore(info.min(), values);\n> +\n> +\t\tpopulateControlValueEntry(valueEntry, info.max(),\n> +\t\t\t\t\t values.offset());\n> +\t\tentries.write(&valueEntry);\n> +\t\tstore(info.max(), values);\n> +\n> +\t\tpopulateControlValueEntry(valueEntry, info.def(),\n> +\t\t\t\t\t values.offset());\n> +\t\tentries.write(&valueEntry);\n> +\t\tstore(info.def(), values);\n> \t}\n> \n> \tif (buffer.overflow())\n> @@ -341,7 +365,7 @@ int ControlSerializer::serialize(const ControlList &list,\n> \telse\n> \t\tidMapType = IPA_CONTROL_ID_MAP_V4L2;\n> \n> -\tsize_t entriesSize = list.size() * sizeof(struct ipa_control_value_entry);\n> +\tsize_t entriesSize = list.size() * sizeof(struct ipa_control_list_entry);\n> \tsize_t valuesSize = 0;\n> \tfor (const auto &ctrl : list)\n> \t\tvaluesSize += binarySize(ctrl.second);\n> @@ -365,12 +389,9 @@ int ControlSerializer::serialize(const ControlList &list,\n> \t\tunsigned int id = ctrl.first;\n> \t\tconst ControlValue &value = ctrl.second;\n> \n> -\t\tstruct ipa_control_value_entry entry;\n> +\t\tstruct ipa_control_list_entry entry;\n> \t\tentry.id = id;\n> -\t\tentry.type = value.type();\n> -\t\tentry.is_array = value.isArray();\n> -\t\tentry.count = value.numElements();\n> -\t\tentry.offset = values.offset();\n> +\t\tpopulateControlValueEntry(entry.value, value, values.offset());\n> \t\tentries.write(&entry);\n> \n> \t\tstore(value, values);\n> @@ -383,12 +404,10 @@ int ControlSerializer::serialize(const ControlList &list,\n> }\n> \n> ControlValue ControlSerializer::loadControlValue(ByteStreamBuffer &buffer,\n> +\t\t\t\t\t\t ControlType type,\n> \t\t\t\t\t\t bool isArray,\n> \t\t\t\t\t\t unsigned int count)\n> {\n> -\tControlType type;\n> -\tbuffer.read(&type);\n> -\n> \tControlValue value;\n> \n> \tvalue.reserve(type, isArray, count);\n> @@ -397,15 +416,6 @@ ControlValue ControlSerializer::loadControlValue(ByteStreamBuffer &buffer,\n> \treturn value;\n> }\n> \n> -ControlInfo ControlSerializer::loadControlInfo(ByteStreamBuffer &b)\n> -{\n> -\tControlValue min = loadControlValue(b);\n> -\tControlValue max = loadControlValue(b);\n> -\tControlValue def = loadControlValue(b);\n> -\n> -\treturn ControlInfo(min, max, def);\n> -}\n> -\n> /**\n> * \\fn template T ControlSerializer::deserialize(ByteStreamBuffer &buffer)\n> * \\brief Deserialize an object from a binary buffer\n> @@ -483,9 +493,11 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> \n> \tControlInfoMap::Map ctrls;\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> -\t\tif (!entry) {\n> +\t\tconst auto *entry = entries.read();\n> +\t\tconst auto *min_entry = entries.read();\n> +\t\tconst auto *max_entry = entries.read();\n> +\t\tconst auto *def_entry = entries.read();\n> +\t\tif (!entry || !min_entry || !max_entry || !def_entry) {\n> \t\t\tLOG(Serializer, Error) << \"Out of data\";\n> \t\t\treturn {};\n> \t\t}\n> @@ -511,15 +523,39 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> \t\tconst ControlId *controlId = idMap->at(entry->id);\n> \t\tASSERT(controlId);\n> \n> -\t\tif (entry->offset != values.offset()) {\n> +\t\tif (min_entry->offset != values.offset()) {\n> \t\t\tLOG(Serializer, Error)\n> -\t\t\t\t<< \"Bad data, entry offset mismatch (entry \"\n> +\t\t\t\t<< \"Bad data, entry offset mismatch (min entry \"\n> +\t\t\t\t<< i << \")\";\n> +\t\t\treturn {};\n> +\t\t}\n> +\t\tControlValue min =\n> +\t\t\tloadControlValue(values, static_cast(min_entry->type),\n> +\t\t\t\t\t min_entry->is_array, min_entry->count);\n> +\n> +\t\tif (max_entry->offset != values.offset()) {\n> +\t\t\tLOG(Serializer, Error)\n> +\t\t\t\t<< \"Bad data, entry offset mismatch (max entry \"\n> \t\t\t\t<< i << \")\";\n> \t\t\treturn {};\n> \t\t}\n> +\t\tControlValue max =\n> +\t\t\tloadControlValue(values, static_cast(max_entry->type),\n> +\t\t\t\t\t max_entry->is_array, max_entry->count);\n> +\n> +\t\tif (def_entry->offset != values.offset()) {\n> +\t\t\tLOG(Serializer, Error)\n> +\t\t\t\t<< \"Bad data, entry offset mismatch (def entry \"\n> +\t\t\t\t<< i << \")\";\n> +\t\t\treturn {};\n> +\t\t}\n> +\t\tControlValue def =\n> +\t\t\tloadControlValue(values, static_cast(def_entry->type),\n> +\t\t\t\t\t def_entry->is_array, def_entry->count);\n> +\n> \n> \t\t/* Create and store the ControlInfo. */\n> -\t\tctrls.emplace(controlId, loadControlInfo(values));\n> +\t\tctrls.emplace(controlId, ControlInfo(min, max, def));\n> \t}\n> \n> \t/*\n> @@ -618,12 +654,12 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> \tControlList ctrls(*idMap);\n> \n> \tfor (unsigned int i = 0; i < hdr->entries; ++i) {\n> -\t\tconst struct ipa_control_value_entry *entry =\n> -\t\t\tentries.read();\n> -\t\tif (!entry) {\n> +\t\tauto *list_entry = entries.read();\n> +\t\tif (!list_entry) {\n> \t\t\tLOG(Serializer, Error) << \"Out of data\";\n> \t\t\treturn {};\n> \t\t}\n> +\t\tconst ipa_control_value_entry *entry = &list_entry->value;\n> \n> \t\tif (entry->offset != values.offset()) {\n> \t\t\tLOG(Serializer, Error)\n> @@ -632,8 +668,9 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> \t\t\treturn {};\n> \t\t}\n> \n> -\t\tctrls.set(entry->id,\n> -\t\t\t loadControlValue(values, entry->is_array, entry->count));\n> +\t\tctrls.set(list_entry->id,\n> +\t\t\t loadControlValue(values, static_cast(entry->type),\n> +\t\t\t\t\t entry->is_array, entry->count));\n> \t}\n> \n> \treturn ctrls;\n> diff --git a/src/libcamera/ipa_controls.cpp b/src/libcamera/ipa_controls.cpp\n> index 12d92ebe894d..9133aec54b27 100644\n> --- a/src/libcamera/ipa_controls.cpp\n> +++ b/src/libcamera/ipa_controls.cpp\n> @@ -192,8 +192,8 @@ static_assert(sizeof(ipa_controls_header) == 32,\n> /**\n> * \\struct ipa_control_value_entry\n> * \\brief Description of a serialized ControlValue entry\n> - * \\var ipa_control_value_entry::id\n> - * The numerical ID of the control\n> + * \\var ipa_control_value_entry::reserved\n> + * Reserved for future extensions\n> * \\var ipa_control_value_entry::type\n> * The type of the control (defined by enum ControlType)\n> * \\var ipa_control_value_entry::is_array\n> @@ -210,6 +210,20 @@ static_assert(sizeof(ipa_controls_header) == 32,\n> static_assert(sizeof(ipa_control_value_entry) == 16,\n> \t \"Invalid ABI size change for struct ipa_control_value_entry\");\n> \n> +/**\n> + * \\struct ipa_control_list_entry\n> + * \\brief Description of a serialized ControlList entry\n> + * \\var ipa_control_list_entry::id\n> + * The numerical ID of the control\n> + * \\var ipa_control_list_entry::reserved\n> + * Reserved for future extensions\n> + * \\var ipa_control_list_entry::value\n> + * The description of the serialized ControlValue\n> + */\n> +\n> +static_assert(sizeof(ipa_control_list_entry) == 24,\n> +\t \"Invalid ABI size change for struct ipa_control_list_entry\");\n\nWe might want to check `alignof()` as well.\n\nTested-by: Barnabás Pőcze # rkisp1\n\n\nRegards,\nBarnabás Pőcze\n\n\n> +\n> /**\n> * \\struct ipa_control_info_entry\n> * \\brief Description of a serialized ControlInfo entry\n> @@ -217,8 +231,8 @@ static_assert(sizeof(ipa_control_value_entry) == 16,\n> * The numerical ID of the control\n> * \\var ipa_control_info_entry::type\n> * The type of the control (defined by enum ControlType)\n> - * \\var ipa_control_info_entry::offset\n> - * The offset in bytes from the beginning of the data section to the control\n> + * \\var ipa_control_info_entry::reserved\n> + * Reserved for future extensions\n> * info data (shall be a multiple of 8 bytes)\n> * \\var ipa_control_info_entry::direction\n> * The directions in which the control is allowed to be sent. This is a flags\n> --\n> 2.47.2\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 05180BF415\n\tfor ;\n\tMon, 13 Oct 2025 09:53:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CC74E60462;\n\tMon, 13 Oct 2025 11:53:06 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E83C860317\n\tfor ;\n\tMon, 13 Oct 2025 11:53:04 +0200 (CEST)","from [192.168.33.36] (185.182.214.105.nat.pool.zt.hu\n\t[185.182.214.105])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1F5F1FE;\n\tMon, 13 Oct 2025 11:51:27 +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=\"Z9d3wZbA\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1760349087;\n\tbh=KetQnRy3bpzwk0/2nj6mSaZzev6Du12sdUftcj+St9o=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=Z9d3wZbAr2d/J3OczakJSzLnIII1dIaT4/PiW7Fe9Nj79IbvbzCO3GNwXiM7FJyxb\n\tvicrdTOb0Pg1wjH8Q1TZ9dpQjJqze2XAU9REpFkuUJsheMpUVX8VO/5/AmfWDbpLbe\n\tql77CRmmK3mwwfj8YbL1eWNYA4Kzz31iZNHxcZtA=","Message-ID":"","Date":"Mon, 13 Oct 2025 11:53:01 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v4 1/2] libcamera: control_serializer: Add array info to\n\tserialized ControlValue","To":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","Cc":"barnabas.pocze@ideasonboard.com","References":"<20251011063333.2169364-1-paul.elder@ideasonboard.com>\n\t<-5tB4aZuFLg8fvkyl8HfSG7_qm0sHQFsE9xDsPnm0VNF7VOrUhe-OLpIPifKfnt1asMP5xukxCY4jM31N10VhA==@protonmail.internalid>\n\t<20251011063333.2169364-2-paul.elder@ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20251011063333.2169364-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\" "}}]