[{"id":36579,"web_url":"https://patchwork.libcamera.org/comment/36579/","msgid":"","date":"2025-10-31T15:20:58","subject":"Re: [PATCH v5 1/2] libcamera: control_serializer: Add array info to\n\tserialized ControlValue","submitter":{"id":156,"url":"https://patchwork.libcamera.org/api/people/156/","name":"Dan Scally","email":"dan.scally@ideasonboard.com"},"content":"Hi Paul, thanks for the set.\n\nOn 14/10/2025 14:34, Paul Elder wrote:\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\nIt's referenced a couple of times in the documentation for ipa_controls.h:\n\nhttps://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/ipa_controls.cpp#n92\nhttps://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/ipa_controls.cpp#n109\nhttps://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/ipa_controls.cpp#n122\n\nThose probably need amending too in that case\n\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> \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 | 11 +-\n> src/libcamera/control_serializer.cpp | 119 ++++++++++++------\n> src/libcamera/ipa_controls.cpp | 20 ++-\n> 4 files changed, 108 insertions(+), 50 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..ecdf6051ffd9 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,25 @@ 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;\n\nIs this just to satisfy the static assert in the .cpp:\n\nstatic_assert(sizeof(ipa_control_value_entry) == 16,\n\t \"Invalid ABI size change for struct ipa_control_value_entry\");\n\n? Do we need those checks? Can't we just rely on checking the version field in the header to \nguarantee compatibility? Not something that should block this set of course.\n\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> +\tuint32_t reserved;\n> };\n> \n> #ifdef __cplusplus\n> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> index 050f8512bd52..f576a868204f 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\nWas that a bug? I can't see why the sizeof(ControlType) would be included in the first place.\n\nLater edit: Oh, you mentioned it in the commit message...I should read more carefully.\n\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> \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> \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\nI think the sentiment of the comment more properly belongs in the commit message - I would just drop \nit here. Either way though, all the functional changes look good to me:\n\nReviewed-by: Daniel Scally \n\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..8f73c8fda811 100644\n> --- a/src/libcamera/ipa_controls.cpp\n> +++ b/src/libcamera/ipa_controls.cpp\n> @@ -192,8 +192,6 @@ 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::type\n> * The type of the control (defined by enum ControlType)\n> * \\var ipa_control_value_entry::is_array\n> @@ -205,11 +203,25 @@ static_assert(sizeof(ipa_controls_header) == 32,\n> * value data (shall be a multiple of 8 bytes).\n> * \\var ipa_control_value_entry::padding\n> * Padding bytes (shall be set to 0)\n> + * \\var ipa_control_value_entry::reserved\n> + * Reserved for future extensions\n> */\n> \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::value\n> + * The description of the serialized ControlValue\n> + */\n> +\n> +static_assert(sizeof(ipa_control_list_entry) == 20,\n> +\t \"Invalid ABI size change for struct ipa_control_list_entry\");\n> +\n> /**\n> * \\struct ipa_control_info_entry\n> * \\brief Description of a serialized ControlInfo entry\n> @@ -217,8 +229,6 @@ 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\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> @@ -226,6 +236,8 @@ static_assert(sizeof(ipa_control_value_entry) == 16,\n> * metadata). \\sa ControlId::Direction\n> * \\var ipa_control_info_entry::padding\n> * Padding bytes (shall be set to 0)\n> + * \\var ipa_control_info_entry::reserved\n> + * Reserved for future extensions\n> */\n> \n> static_assert(sizeof(ipa_control_info_entry) == 16,","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 6EDFBC3259\n\tfor ;\n\tFri, 31 Oct 2025 15:21:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1CC68609B7;\n\tFri, 31 Oct 2025 16:21:03 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 57F7760947\n\tfor ;\n\tFri, 31 Oct 2025 16:21:01 +0100 (CET)","from [192.168.0.43]\n\t(cpc141996-chfd3-2-0-cust928.12-3.cable.virginm.net [86.13.91.161])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 38B6699F;\n\tFri, 31 Oct 2025 16:19:10 +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=\"CigH3Ota\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1761923950;\n\tbh=7EbgMAT8cErVzzJu68ZodATaMfPsF96TwrwecKX1O2I=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=CigH3OtakAY28R8xzfEoqMHNozoibqHdWttfWCmPkh5w96zzw+rKdo+R7F6H0UnHU\n\tC6IGEU8k1XcPnutxZP4PFkaQnYe/ztf10tT0zj8R5Lswc0pb39pV8vGIvYkaVoIpda\n\tqRjZzlNqbRNXb20S54er9QdTRAHMe4s5iwhWSyj0=","Message-ID":"","Date":"Fri, 31 Oct 2025 15:20:58 +0000","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v5 1/2] libcamera: control_serializer: Add array info to\n\tserialized ControlValue","To":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","Cc":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","References":"<20251014133404.3194952-1-paul.elder@ideasonboard.com>\n\t<20251014133404.3194952-2-paul.elder@ideasonboard.com>","Content-Language":"en-US","From":"Dan Scally ","In-Reply-To":"<20251014133404.3194952-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\" "}},{"id":36816,"web_url":"https://patchwork.libcamera.org/comment/36816/","msgid":"<176312686538.1993330.6756953672559867163@neptunite.rasen.tech>","date":"2025-11-14T13:27:45","subject":"Re: [PATCH v5 1/2] libcamera: control_serializer: Add array info to\n\tserialized ControlValue","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Dan,\n\nThanks for the review.\n\nQuoting Dan Scally (2025-11-01 00:20:58)\n> Hi Paul, thanks for the set.\n> \n> On 14/10/2025 14:34, Paul Elder wrote:\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> \n> It's referenced a couple of times in the documentation for ipa_controls.h:\n> \n> https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/ipa_controls.cpp#n92\n> https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/ipa_controls.cpp#n109\n> https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/ipa_controls.cpp#n122\n> \n> Those probably need amending too in that case\n\nIndeed they are, I missed that.\n\n> \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> > \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 | 11 +-\n> > src/libcamera/control_serializer.cpp | 119 ++++++++++++------\n> > src/libcamera/ipa_controls.cpp | 20 ++-\n> > 4 files changed, 108 insertions(+), 50 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> > static void store(const ControlValue &value, ByteStreamBuffer &buffer);\n> > static void store(const ControlInfo &info, ByteStreamBuffer &buffer);\n> > \n> > + void populateControlValueEntry(struct ipa_control_value_entry &entry,\n> > + const ControlValue &value,\n> > + uint32_t offset);\n> > +\n> > ControlValue loadControlValue(ByteStreamBuffer &buffer,\n> > - bool isArray = false, unsigned int count = 1);\n> > - ControlInfo loadControlInfo(ByteStreamBuffer &buffer);\n> > + ControlType type,\n> > + bool isArray, unsigned int count);\n> > \n> > unsigned int serial_;\n> > unsigned int serialSeed_;\n> > diff --git a/include/libcamera/ipa/ipa_controls.h b/include/libcamera/ipa/ipa_controls.h\n> > index 980668c86bcc..ecdf6051ffd9 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 1\n> > +#define IPA_CONTROLS_FORMAT_VERSION 2\n> > \n> > enum ipa_controls_id_map_type {\n> > IPA_CONTROL_ID_MAP_CONTROLS,\n> > @@ -34,20 +34,25 @@ struct ipa_controls_header {\n> > };\n> > \n> > struct ipa_control_value_entry {\n> > - uint32_t id;\n> > uint8_t type;\n> > uint8_t is_array;\n> > uint16_t count;\n> > uint32_t offset;\n> > uint32_t padding[1];\n> > + uint32_t reserved;\n> \n> Is this just to satisfy the static assert in the .cpp:\n\nafaiu it's for alignment, though then I suppose we could instead make\npadding[2] instead of adding reserved.\n\nSince the fields are moved around we don't have ABI compatibility anyway. I\noriginally renamed the newly-unused fields to reserved to maintain a semblance\nof ABI compatibility but I guess the fields aren't going to be used and\nBarnabás shot them down in v4. We're going to merge this in an ABI-breaking\nrelease anyway so I went with it.\n\n> \n> static_assert(sizeof(ipa_control_value_entry) == 16,\n> \"Invalid ABI size change for struct ipa_control_value_entry\");\n> \n> ? Do we need those checks? Can't we just rely on checking the version field in the header to \n> guarantee compatibility? Not something that should block this set of course.\n\nI think it's better to keep the checks.\n\n> \n> > +};\n> > +\n> > +struct ipa_control_list_entry {\n> > + uint32_t id;\n> > + struct ipa_control_value_entry value;\n> > };\n> > \n> > struct ipa_control_info_entry {\n> > uint32_t id;\n> > uint32_t type;\n> > - uint32_t offset;\n> > uint8_t direction;\n> > uint8_t padding[3];\n> > + uint32_t reserved;\n> > };\n> > \n> > #ifdef __cplusplus\n> > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> > index 050f8512bd52..f576a868204f 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> > - return sizeof(ControlType) + value.data().size_bytes();\n> > + return value.data().size_bytes();\n> \n> Was that a bug? I can't see why the sizeof(ControlType) would be included in the first place.\n> \n> Later edit: Oh, you mentioned it in the commit message...I should read more carefully.\n> \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> > size_t size = sizeof(struct ipa_controls_header)\n> > - + infoMap.size() * sizeof(struct ipa_control_info_entry);\n> > + + infoMap.size() * (sizeof(struct ipa_control_info_entry) +\n> > + 3 * sizeof(struct ipa_control_value_entry));\n> > \n> > for (const auto &ctrl : infoMap)\n> > size += 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> > size_t size = sizeof(struct ipa_controls_header)\n> > - + list.size() * sizeof(struct ipa_control_value_entry);\n> > + + list.size() * sizeof(struct ipa_control_list_entry);\n> > \n> > for (const auto &ctrl : list)\n> > size += binarySize(ctrl.second);\n> > @@ -195,16 +196,17 @@ size_t ControlSerializer::binarySize(const ControlList &list)\n> > void ControlSerializer::store(const ControlValue &value,\n> > ByteStreamBuffer &buffer)\n> > {\n> > - const ControlType type = value.type();\n> > - buffer.write(&type);\n> > buffer.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> > + const ControlValue &value,\n> > + uint32_t offset)\n> > {\n> > - store(info.min(), buffer);\n> > - store(info.max(), buffer);\n> > - store(info.def(), buffer);\n> > + entry.type = value.type();\n> > + entry.is_array = value.isArray();\n> > + entry.count = value.numElements();\n> > + entry.offset = offset;\n> > }\n> > \n> > /**\n> > @@ -232,7 +234,8 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,\n> > \n> > /* Compute entries and data required sizes. */\n> > size_t entriesSize = infoMap.size()\n> > - * sizeof(struct ipa_control_info_entry);\n> > + * (sizeof(struct ipa_control_info_entry) +\n> > + 3 * sizeof(struct ipa_control_value_entry));\n> > size_t valuesSize = 0;\n> > for (const auto &ctrl : infoMap)\n> > valuesSize += binarySize(ctrl.second);\n> > @@ -280,11 +283,32 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,\n> > struct ipa_control_info_entry entry;\n> > entry.id = id->id();\n> > entry.type = id->type();\n> > - entry.offset = values.offset();\n> > entry.direction = static_cast(id->direction());\n> > entries.write(&entry);\n> > \n> > - store(info, values);\n> > + /*\n> > + * Write the metadata for the ControlValue entries as well,\n> > + * since we need type, isArray, and numElements information for\n> > + * min/max/def of the ControlInfo. Doing it this way is the\n> > + * least intrusive in terms of changing the structs in\n> > + * ipa_controls.h\n> > + */\n> \n> I think the sentiment of the comment more properly belongs in the commit message - I would just drop \n> it here. Either way though, all the functional changes look good to me:\n\nHm, good point.\n\n> \n> Reviewed-by: Daniel Scally \n\nThanks,\n\nPaul\n\n> \n> > + struct ipa_control_value_entry valueEntry;\n> > +\n> > + populateControlValueEntry(valueEntry, info.min(),\n> > + values.offset());\n> > + entries.write(&valueEntry);\n> > + store(info.min(), values);\n> > +\n> > + populateControlValueEntry(valueEntry, info.max(),\n> > + values.offset());\n> > + entries.write(&valueEntry);\n> > + store(info.max(), values);\n> > +\n> > + populateControlValueEntry(valueEntry, info.def(),\n> > + values.offset());\n> > + entries.write(&valueEntry);\n> > + store(info.def(), values);\n> > }\n> > \n> > if (buffer.overflow())\n> > @@ -341,7 +365,7 @@ int ControlSerializer::serialize(const ControlList &list,\n> > else\n> > idMapType = IPA_CONTROL_ID_MAP_V4L2;\n> > \n> > - size_t entriesSize = list.size() * sizeof(struct ipa_control_value_entry);\n> > + size_t entriesSize = list.size() * sizeof(struct ipa_control_list_entry);\n> > size_t valuesSize = 0;\n> > for (const auto &ctrl : list)\n> > valuesSize += binarySize(ctrl.second);\n> > @@ -365,12 +389,9 @@ int ControlSerializer::serialize(const ControlList &list,\n> > unsigned int id = ctrl.first;\n> > const ControlValue &value = ctrl.second;\n> > \n> > - struct ipa_control_value_entry entry;\n> > + struct ipa_control_list_entry entry;\n> > entry.id = id;\n> > - entry.type = value.type();\n> > - entry.is_array = value.isArray();\n> > - entry.count = value.numElements();\n> > - entry.offset = values.offset();\n> > + populateControlValueEntry(entry.value, value, values.offset());\n> > entries.write(&entry);\n> > \n> > store(value, values);\n> > @@ -383,12 +404,10 @@ int ControlSerializer::serialize(const ControlList &list,\n> > }\n> > \n> > ControlValue ControlSerializer::loadControlValue(ByteStreamBuffer &buffer,\n> > + ControlType type,\n> > bool isArray,\n> > unsigned int count)\n> > {\n> > - ControlType type;\n> > - buffer.read(&type);\n> > -\n> > ControlValue value;\n> > \n> > value.reserve(type, isArray, count);\n> > @@ -397,15 +416,6 @@ ControlValue ControlSerializer::loadControlValue(ByteStreamBuffer &buffer,\n> > return value;\n> > }\n> > \n> > -ControlInfo ControlSerializer::loadControlInfo(ByteStreamBuffer &b)\n> > -{\n> > - ControlValue min = loadControlValue(b);\n> > - ControlValue max = loadControlValue(b);\n> > - ControlValue def = loadControlValue(b);\n> > -\n> > - return 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> > ControlInfoMap::Map ctrls;\n> > for (unsigned int i = 0; i < hdr->entries; ++i) {\n> > - const struct ipa_control_info_entry *entry =\n> > - entries.read();\n> > - if (!entry) {\n> > + const auto *entry = entries.read();\n> > + const auto *min_entry = entries.read();\n> > + const auto *max_entry = entries.read();\n> > + const auto *def_entry = entries.read();\n> > + if (!entry || !min_entry || !max_entry || !def_entry) {\n> > LOG(Serializer, Error) << \"Out of data\";\n> > return {};\n> > }\n> > @@ -511,15 +523,39 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> > const ControlId *controlId = idMap->at(entry->id);\n> > ASSERT(controlId);\n> > \n> > - if (entry->offset != values.offset()) {\n> > + if (min_entry->offset != values.offset()) {\n> > LOG(Serializer, Error)\n> > - << \"Bad data, entry offset mismatch (entry \"\n> > + << \"Bad data, entry offset mismatch (min entry \"\n> > + << i << \")\";\n> > + return {};\n> > + }\n> > + ControlValue min =\n> > + loadControlValue(values, static_cast(min_entry->type),\n> > + min_entry->is_array, min_entry->count);\n> > +\n> > + if (max_entry->offset != values.offset()) {\n> > + LOG(Serializer, Error)\n> > + << \"Bad data, entry offset mismatch (max entry \"\n> > << i << \")\";\n> > return {};\n> > }\n> > + ControlValue max =\n> > + loadControlValue(values, static_cast(max_entry->type),\n> > + max_entry->is_array, max_entry->count);\n> > +\n> > + if (def_entry->offset != values.offset()) {\n> > + LOG(Serializer, Error)\n> > + << \"Bad data, entry offset mismatch (def entry \"\n> > + << i << \")\";\n> > + return {};\n> > + }\n> > + ControlValue def =\n> > + loadControlValue(values, static_cast(def_entry->type),\n> > + def_entry->is_array, def_entry->count);\n> > +\n> > \n> > /* Create and store the ControlInfo. */\n> > - ctrls.emplace(controlId, loadControlInfo(values));\n> > + ctrls.emplace(controlId, ControlInfo(min, max, def));\n> > }\n> > \n> > /*\n> > @@ -618,12 +654,12 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> > ControlList ctrls(*idMap);\n> > \n> > for (unsigned int i = 0; i < hdr->entries; ++i) {\n> > - const struct ipa_control_value_entry *entry =\n> > - entries.read();\n> > - if (!entry) {\n> > + auto *list_entry = entries.read();\n> > + if (!list_entry) {\n> > LOG(Serializer, Error) << \"Out of data\";\n> > return {};\n> > }\n> > + const ipa_control_value_entry *entry = &list_entry->value;\n> > \n> > if (entry->offset != values.offset()) {\n> > LOG(Serializer, Error)\n> > @@ -632,8 +668,9 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> > return {};\n> > }\n> > \n> > - ctrls.set(entry->id,\n> > - loadControlValue(values, entry->is_array, entry->count));\n> > + ctrls.set(list_entry->id,\n> > + loadControlValue(values, static_cast(entry->type),\n> > + entry->is_array, entry->count));\n> > }\n> > \n> > return ctrls;\n> > diff --git a/src/libcamera/ipa_controls.cpp b/src/libcamera/ipa_controls.cpp\n> > index 12d92ebe894d..8f73c8fda811 100644\n> > --- a/src/libcamera/ipa_controls.cpp\n> > +++ b/src/libcamera/ipa_controls.cpp\n> > @@ -192,8 +192,6 @@ 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::type\n> > * The type of the control (defined by enum ControlType)\n> > * \\var ipa_control_value_entry::is_array\n> > @@ -205,11 +203,25 @@ static_assert(sizeof(ipa_controls_header) == 32,\n> > * value data (shall be a multiple of 8 bytes).\n> > * \\var ipa_control_value_entry::padding\n> > * Padding bytes (shall be set to 0)\n> > + * \\var ipa_control_value_entry::reserved\n> > + * Reserved for future extensions\n> > */\n> > \n> > static_assert(sizeof(ipa_control_value_entry) == 16,\n> > \"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::value\n> > + * The description of the serialized ControlValue\n> > + */\n> > +\n> > +static_assert(sizeof(ipa_control_list_entry) == 20,\n> > + \"Invalid ABI size change for struct ipa_control_list_entry\");\n> > +\n> > /**\n> > * \\struct ipa_control_info_entry\n> > * \\brief Description of a serialized ControlInfo entry\n> > @@ -217,8 +229,6 @@ 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> \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> > @@ -226,6 +236,8 @@ static_assert(sizeof(ipa_control_value_entry) == 16,\n> > * metadata). \\sa ControlId::Direction\n> > * \\var ipa_control_info_entry::padding\n> > * Padding bytes (shall be set to 0)\n> > + * \\var ipa_control_info_entry::reserved\n> > + * Reserved for future extensions\n> > */\n> > \n> > static_assert(sizeof(ipa_control_info_entry) == 16,\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 BA806C3241\n\tfor ;\n\tFri, 14 Nov 2025 13:27:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B86BB60A80;\n\tFri, 14 Nov 2025 14:27:53 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 962BA606E6\n\tfor ;\n\tFri, 14 Nov 2025 14:27:52 +0100 (CET)","from neptunite.rasen.tech (unknown\n\t[IPv6:2404:7a81:160:2100:7517:c3d8:1e81:2996])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 2976A161;\n\tFri, 14 Nov 2025 14:25:50 +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=\"SIZsg/uk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1763126751;\n\tbh=uvz500hd7uJB1nQazX7dm09ZTauxKKxcC437B+OWGhg=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=SIZsg/ukbfR6NBPyObjJWE0V5t7hWe89gRnOaq0a0ejx+Ib0egfYodT93y/+YeMYB\n\tyDbZFaVJLgPhuAfllbG85A6ESIGdT4fAeIUj583WTbTcQn94kQuMocoD1pwnVpdsii\n\tRZTlsJ7sBJxV5G46/TzX6CO5gVGsDVakEhnMZJWQ=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20251014133404.3194952-1-paul.elder@ideasonboard.com>\n\t<20251014133404.3194952-2-paul.elder@ideasonboard.com>\n\t","Subject":"Re: [PATCH v5 1/2] libcamera: control_serializer: Add array info to\n\tserialized ControlValue","From":"Paul Elder ","Cc":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","To":"Dan Scally ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 14 Nov 2025 22:27:45 +0900","Message-ID":"<176312686538.1993330.6756953672559867163@neptunite.rasen.tech>","User-Agent":"alot/0.0.0","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\" "}},{"id":36817,"web_url":"https://patchwork.libcamera.org/comment/36817/","msgid":"<77b66626-8cda-4e11-b8a8-4b163da523d0@ideasonboard.com>","date":"2025-11-14T14:53:47","subject":"Re: [PATCH v5 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. 14. 14:27 keltezéssel, Paul Elder írta:\n> Hi Dan,\n> \n> Thanks for the review.\n> \n> Quoting Dan Scally (2025-11-01 00:20:58)\n>> Hi Paul, thanks for the set.\n>>\n>> On 14/10/2025 14:34, Paul Elder wrote:\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>>\n>> It's referenced a couple of times in the documentation for ipa_controls.h:\n>>\n>> https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/ipa_controls.cpp#n92\n>> https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/ipa_controls.cpp#n109\n>> https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/ipa_controls.cpp#n122\n>>\n>> Those probably need amending too in that case\n> \n> Indeed they are, I missed that.\n> \n>>\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>>>\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 | 11 +-\n>>> src/libcamera/control_serializer.cpp | 119 ++++++++++++------\n>>> src/libcamera/ipa_controls.cpp | 20 ++-\n>>> 4 files changed, 108 insertions(+), 50 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> [...]\n>>> struct ipa_control_value_entry {\n>>> - uint32_t id;\n>>> uint8_t type;\n>>> uint8_t is_array;\n>>> uint16_t count;\n>>> uint32_t offset;\n>>> uint32_t padding[1];\n>>> + uint32_t reserved;\n>>\n>> Is this just to satisfy the static assert in the .cpp:\n> \n> afaiu it's for alignment, though then I suppose we could instead make\n> padding[2] instead of adding reserved.\n\nI believe it would be nice to merge them either as `reserved[2]` or `padding[2]`.\n\n\n> \n> Since the fields are moved around we don't have ABI compatibility anyway. I\n> originally renamed the newly-unused fields to reserved to maintain a semblance\n> of ABI compatibility but I guess the fields aren't going to be used and\n> Barnabás shot them down in v4. We're going to merge this in an ABI-breaking\n> release anyway so I went with it.\n\nMy thinking was (and is) that since there is no need to stay compatible, it's simpler to just\nmake the necessary modifications in the most convenient way (e.g. adding/moving/removing fields,\nchanging types, etc.).\n\n\n> \n>>\n>> static_assert(sizeof(ipa_control_value_entry) == 16,\n>> \"Invalid ABI size change for struct ipa_control_value_entry\");\n>>\n>> ? Do we need those checks? Can't we just rely on checking the version field in the header to\n>> guarantee compatibility? Not something that should block this set of course.\n> \n> I think it's better to keep the checks.\n\nI think so too, to ensure that the struct has the same size on every platform.\n\n\nRegards,\nBarnabás Pőcze\n\n> \n>>\n>>> +};\n>>> +\n>>> +struct ipa_control_list_entry {\n>>> + uint32_t id;\n>>> + struct ipa_control_value_entry value;\n>>> };\n>>> \n>>> struct ipa_control_info_entry {\n>>> uint32_t id;\n>>> uint32_t type;\n>>> - uint32_t offset;\n>>> uint8_t direction;\n>>> uint8_t padding[3];\n>>> + uint32_t reserved;\n>>> };\n>>> \n>>> #ifdef __cplusplus\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 9CB75C3263\n\tfor ;\n\tFri, 14 Nov 2025 14:53:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D9BD6609D8;\n\tFri, 14 Nov 2025 15:53:53 +0100 (CET)","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 D55BF606E6\n\tfor ;\n\tFri, 14 Nov 2025 15:53:52 +0100 (CET)","from [192.168.33.35] (185.221.143.100.nat.pool.zt.hu\n\t[185.221.143.100])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BCF30664;\n\tFri, 14 Nov 2025 15:51:50 +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=\"f6sfmgob\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1763131911;\n\tbh=Rwp/tJhm+xuh8xzmTQp2dLbDk8Pz+VPD+9bPsa6mxKA=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=f6sfmgobVNweegk8VyU1qFts70DFF+Xbk750SYOver33Nwc+wMqQS7i8jnaXClnUx\n\tMzeIVSqMY2+SWr4K/jYU9pol/tBJ4WMNX05iBbqRgpTRfiDbWUo7uXpTlgpeJuoNly\n\tOr7rXXIjdTEkOwDm3Z9BnopOynax9k3HfKgvp068=","Message-ID":"<77b66626-8cda-4e11-b8a8-4b163da523d0@ideasonboard.com>","Date":"Fri, 14 Nov 2025 15:53:47 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v5 1/2] libcamera: control_serializer: Add array info to\n\tserialized ControlValue","To":"Paul Elder ,\n\tDan Scally ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20251014133404.3194952-1-paul.elder@ideasonboard.com>\n\t<20251014133404.3194952-2-paul.elder@ideasonboard.com>\n\t\n\t<176312686538.1993330.6756953672559867163@neptunite.rasen.tech>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<176312686538.1993330.6756953672559867163@neptunite.rasen.tech>","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\" "}}]