[{"id":36187,"web_url":"https://patchwork.libcamera.org/comment/36187/","msgid":"<19ca4991-c3f2-4805-b6c7-c5b5b20a14b3@ideasonboard.com>","date":"2025-10-10T12:38:38","subject":"Re: [PATCH v3 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. 10. 13: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 adding the already-existing struct\n> ipa_control_value_entry to the serialized form of ControlInfoMap to\n> contain all the metadata associated with each ControlValue in\n> the min/max/def of each ControlInfo.\n> \n> The issue was not noticed before as the default value of the ControlInfo\n> of other array controls had been set to scalar values similar to\n> min/max, and ColourCorrectionMatrix was the first control to properly\n> define a non-scalar default value.\n> \n> Other array controls that define a scalar default value need to be\n> fixed to define a properly sized default ControlInfo value.\n> \n> Bug: https://bugs.libcamera.org/show_bug.cgi?id=285\n> Signed-off-by: Paul Elder \n> \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> src/libcamera/control_serializer.cpp | 76 +++++++++++++------\n> 2 files changed, 58 insertions(+), 24 deletions(-)\n> \n> diff --git a/include/libcamera/internal/control_serializer.h b/include/libcamera/internal/control_serializer.h\n> index 8a63ae44a13e..755ee68e1efe 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 unsigned int id,\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> \n> \tunsigned int serial_;\n> \tunsigned int serialSeed_;\n> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> index 050f8512bd52..f15050901412 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> @@ -197,14 +198,20 @@ void ControlSerializer::store(const ControlValue &value,\n> {\n> \tconst ControlType type = value.type();\n> \tbuffer.write(&type);\n\nWe could get rid of this serialized type as well and use the one in the entry header.\nI feel like the commit that introduced it (cbc2be34ed9e47f5b17d0955bf3496d735359795)\nwas trying to solve the same problem that this change solves, but it stopped at\njust handling different types, and did not consider different array-ness or size.\n\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 unsigned int id,\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.id = id;\n\nCould we remove the `id`? I believe it's not used for control values.\n\nI would do the following:\n\n struct ipa_control_value_entry { /* everything but the id */ };\n struct ipa_control_list_entry { uint32_t id; struct ipa_control_value_entry value; };\n\n\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 +239,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> @@ -284,7 +292,29 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,\n> \t\tentry.direction = static_cast(id->direction());\n> \t\tentries.write(&entry);\n> \n\nI'm wondering if `ipa_control_info_entry::offset` is of any use anymore?\n\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, id->id(), 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, id->id(), 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, id->id(), 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> @@ -366,11 +396,7 @@ int ControlSerializer::serialize(const ControlList &list,\n> \t\tconst ControlValue &value = ctrl.second;\n> \n> \t\tstruct ipa_control_value_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, id, value, values.offset());\n> \t\tentries.write(&entry);\n> \n> \t\tstore(value, values);\n> @@ -397,15 +423,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> @@ -485,7 +502,13 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\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 struct ipa_control_value_entry *min_entry =\n> +\t\t\tentries.read();\n\n const auto *x = entries.read();\n\nlooks a bit better to me.\n\n\n> +\t\tconst struct ipa_control_value_entry *max_entry =\n> +\t\t\tentries.read();\n> +\t\tconst struct ipa_control_value_entry *def_entry =\n> +\t\t\tentries.read();\n\nHave you tried adding these three to `ipa_control_info_entry` directly?\n\n\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> @@ -518,8 +541,15 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> \t\t\treturn {};\n> \t\t}\n> \n> +\t\tControlValue min = loadControlValue(values, min_entry->is_array,\n> +\t\t\t\t\t\t min_entry->count);\n> +\t\tControlValue max = loadControlValue(values, max_entry->is_array,\n> +\t\t\t\t\t\t max_entry->count);\n> +\t\tControlValue def = loadControlValue(values, def_entry->is_array,\n> +\t\t\t\t\t\t def_entry->count);\n> +\n\nShould `*_entry->offset` be checked here?\n\n\nRegards,\nBarnabás Pőcze\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/*","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 157A1BF415\n\tfor ;\n\tFri, 10 Oct 2025 12:38:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 477BC6B599;\n\tFri, 10 Oct 2025 14:38:45 +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 6DABC6B599\n\tfor ;\n\tFri, 10 Oct 2025 14:38:43 +0200 (CEST)","from [192.168.33.28] (185.182.214.121.nat.pool.zt.hu\n\t[185.182.214.121])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 455995B3;\n\tFri, 10 Oct 2025 14:37:07 +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=\"nBnmhJfn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1760099827;\n\tbh=3+rxayM/YToyxdIPAUa+fXwRic3ip/5wCaDYiN9SQPE=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=nBnmhJfn+LJ2km2ZLKSMAtPBLfbenMoB9Oeh8ErHelr0A1PtTkO/bWHS8Y9OznHlh\n\tL7e3ZEI6VLq1VzqEOyq9WKVhRbiGEx09XSQTkJCK8OXc0eDvhNMp2p6jGuhxCUxSsd\n\tsvkWxIbd15AjYpgf2wwtKQPs8QgY9yZTRzLqxbwg=","Message-ID":"<19ca4991-c3f2-4805-b6c7-c5b5b20a14b3@ideasonboard.com>","Date":"Fri, 10 Oct 2025 14:38:38 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v3 1/2] libcamera: control_serializer: Add array info to\n\tserialized ControlValue","To":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20251010113332.3030598-1-paul.elder@ideasonboard.com>\n\t<20251010113332.3030598-2-paul.elder@ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20251010113332.3030598-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":36203,"web_url":"https://patchwork.libcamera.org/comment/36203/","msgid":"<176015887792.2583768.13347460452247336766@neptunite.rasen.tech>","date":"2025-10-11T05:01:17","subject":"Re: [PATCH v3 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 Barnabás,\n\nThanks for the review.\n\nQuoting Barnabás Pőcze (2025-10-10 21:38:38)\n> Hi\n> \n> \n> 2025. 10. 10. 13: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 adding the already-existing struct\n> > ipa_control_value_entry to the serialized form of ControlInfoMap to\n> > contain all the metadata associated with each ControlValue in\n> > the min/max/def of each ControlInfo.\n> > \n> > The issue was not noticed before as the default value of the ControlInfo\n> > of other array controls had been set to scalar values similar to\n> > min/max, and ColourCorrectionMatrix was the first control to properly\n> > define a non-scalar default value.\n> > \n> > Other array controls that define a scalar default value need to be\n> > fixed to define a properly sized default ControlInfo value.\n> > \n> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=285\n> > Signed-off-by: Paul Elder \n> > \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> > src/libcamera/control_serializer.cpp | 76 +++++++++++++------\n> > 2 files changed, 58 insertions(+), 24 deletions(-)\n> > \n> > diff --git a/include/libcamera/internal/control_serializer.h b/include/libcamera/internal/control_serializer.h\n> > index 8a63ae44a13e..755ee68e1efe 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> > + unsigned int id,\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> > \n> > unsigned int serial_;\n> > unsigned int serialSeed_;\n> > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> > index 050f8512bd52..f15050901412 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> > 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> > @@ -197,14 +198,20 @@ void ControlSerializer::store(const ControlValue &value,\n> > {\n> > const ControlType type = value.type();\n> > buffer.write(&type);\n> \n> We could get rid of this serialized type as well and use the one in the entry header.\n> I feel like the commit that introduced it (cbc2be34ed9e47f5b17d0955bf3496d735359795)\n> was trying to solve the same problem that this change solves, but it stopped at\n> just handling different types, and did not consider different array-ness or size.\n\nOh good pint yeah we can do that.\n\n> \n> \n> > +\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> > + unsigned int id,\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.id = id;\n> \n> Could we remove the `id`? I believe it's not used for control values.\n\nI looked a bit more and now I see what you mean.\n\n> \n> I would do the following:\n> \n> struct ipa_control_value_entry { /* everything but the id */ };\n> struct ipa_control_list_entry { uint32_t id; struct ipa_control_value_entry value; };\n\nGood idea.\n\n> \n> \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 +239,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> > @@ -284,7 +292,29 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,\n> > entry.direction = static_cast(id->direction());\n> > entries.write(&entry);\n> > \n> \n> I'm wondering if `ipa_control_info_entry::offset` is of any use anymore?\n\nIt indeed does not look like it is.\n\n> \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> > + struct ipa_control_value_entry valueEntry;\n> > +\n> > + populateControlValueEntry(valueEntry, id->id(), info.min(),\n> > + values.offset());\n> > + entries.write(&valueEntry);\n> > + store(info.min(), values);\n> > +\n> > + populateControlValueEntry(valueEntry, id->id(), info.max(),\n> > + values.offset());\n> > + entries.write(&valueEntry);\n> > + store(info.max(), values);\n> > +\n> > + populateControlValueEntry(valueEntry, id->id(), info.def(),\n> > + values.offset());\n> > + entries.write(&valueEntry);\n> > + store(info.def(), values);\n> > }\n> > \n> > if (buffer.overflow())\n> > @@ -366,11 +396,7 @@ int ControlSerializer::serialize(const ControlList &list,\n> > const ControlValue &value = ctrl.second;\n> > \n> > struct ipa_control_value_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, id, value, values.offset());\n> > entries.write(&entry);\n> > \n> > store(value, values);\n> > @@ -397,15 +423,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> > @@ -485,7 +502,13 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\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 struct ipa_control_value_entry *min_entry =\n> > + entries.read();\n> \n> const auto *x = entries.read();\n> \n> looks a bit better to me.\n\nThat is indeed nicer.\n\n> \n> \n> > + const struct ipa_control_value_entry *max_entry =\n> > + entries.read();\n> > + const struct ipa_control_value_entry *def_entry =\n> > + entries.read();\n> \n> Have you tried adding these three to `ipa_control_info_entry` directly?\n\nOriginally I had wanted to avoid changing the ipa_controls.h header but ig\nsince we're aligning this with an ABI breaking release (and since nobody\nprobably depends on that interface anyway) it's probably fine to change it.\n\n> \n> \n> > + if (!entry || !min_entry || !max_entry || !def_entry) {\n> > LOG(Serializer, Error) << \"Out of data\";\n> > return {};\n> > }\n> > @@ -518,8 +541,15 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> > return {};\n> > }\n> > \n> > + ControlValue min = loadControlValue(values, min_entry->is_array,\n> > + min_entry->count);\n> > + ControlValue max = loadControlValue(values, max_entry->is_array,\n> > + max_entry->count);\n> > + ControlValue def = loadControlValue(values, def_entry->is_array,\n> > + def_entry->count);\n> > +\n> \n> Should `*_entry->offset` be checked here?\n\nYes it probably should.\n\n\nThanks,\n\nPaul\n\n> \n> \n> Regards,\n> Barnabás Pőcze\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>","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 AE527BE080\n\tfor ;\n\tSat, 11 Oct 2025 05:01:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E422D60313;\n\tSat, 11 Oct 2025 07:01:26 +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 2F3FC6030E\n\tfor ;\n\tSat, 11 Oct 2025 07:01:25 +0200 (CEST)","from neptunite.rasen.tech (unknown\n\t[IPv6:2404:7a81:160:2100:42b2:5255:36e:e46a])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 9753B56D;\n\tSat, 11 Oct 2025 06:59:48 +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=\"XiOskPaz\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1760158789;\n\tbh=v51OzIfXOSJD/vmbwduTzolCrysZKlKpVzi0Jx3GULs=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=XiOskPazFDoCE6cr8hx/7NBzh0kxAmVM+ShetnADBfFfoej6aeXktD0HS5RjJdyiW\n\tDbnKiosFiyl1x/bf+/7/PYNPMbuUd8lji86A6J9FP92EceH2zWi/7UndjB47k6t1V0\n\tM9c7hedOr/ZWXpIx/wHdNM4mDkEABOZZQNLKWAR8=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<19ca4991-c3f2-4805-b6c7-c5b5b20a14b3@ideasonboard.com>","References":"<20251010113332.3030598-1-paul.elder@ideasonboard.com>\n\t<20251010113332.3030598-2-paul.elder@ideasonboard.com>\n\t<19ca4991-c3f2-4805-b6c7-c5b5b20a14b3@ideasonboard.com>","Subject":"Re: [PATCH v3 1/2] libcamera: control_serializer: Add array info to\n\tserialized ControlValue","From":"Paul Elder ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Sat, 11 Oct 2025 14:01:17 +0900","Message-ID":"<176015887792.2583768.13347460452247336766@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":36205,"web_url":"https://patchwork.libcamera.org/comment/36205/","msgid":"<176016147182.1891906.10195489546031172447@neptunite.rasen.tech>","date":"2025-10-11T05:44:31","subject":"Re: [PATCH v3 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":"Quoting Paul Elder (2025-10-11 14:01:17)\n> Hi Barnabás,\n> \n> Thanks for the review.\n> \n> Quoting Barnabás Pőcze (2025-10-10 21:38:38)\n> > Hi\n> > \n> > \n> > 2025. 10. 10. 13: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 adding the already-existing struct\n> > > ipa_control_value_entry to the serialized form of ControlInfoMap to\n> > > contain all the metadata associated with each ControlValue in\n> > > the min/max/def of each ControlInfo.\n> > > \n> > > The issue was not noticed before as the default value of the ControlInfo\n> > > of other array controls had been set to scalar values similar to\n> > > min/max, and ColourCorrectionMatrix was the first control to properly\n> > > define a non-scalar default value.\n> > > \n> > > Other array controls that define a scalar default value need to be\n> > > fixed to define a properly sized default ControlInfo value.\n> > > \n> > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=285\n> > > Signed-off-by: Paul Elder \n> > > \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> > > src/libcamera/control_serializer.cpp | 76 +++++++++++++------\n> > > 2 files changed, 58 insertions(+), 24 deletions(-)\n> > > \n> > > diff --git a/include/libcamera/internal/control_serializer.h b/include/libcamera/internal/control_serializer.h\n> > > index 8a63ae44a13e..755ee68e1efe 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> > > + unsigned int id,\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> > > \n> > > unsigned int serial_;\n> > > unsigned int serialSeed_;\n> > > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> > > index 050f8512bd52..f15050901412 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> > > 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> > > @@ -197,14 +198,20 @@ void ControlSerializer::store(const ControlValue &value,\n> > > {\n> > > const ControlType type = value.type();\n> > > buffer.write(&type);\n> > \n> > We could get rid of this serialized type as well and use the one in the entry header.\n> > I feel like the commit that introduced it (cbc2be34ed9e47f5b17d0955bf3496d735359795)\n> > was trying to solve the same problem that this change solves, but it stopped at\n> > just handling different types, and did not consider different array-ness or size.\n> \n> Oh good pint yeah we can do that.\n> \n> > \n> > \n> > > +\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> > > + unsigned int id,\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.id = id;\n> > \n> > Could we remove the `id`? I believe it's not used for control values.\n> \n> I looked a bit more and now I see what you mean.\n> \n> > \n> > I would do the following:\n> > \n> > struct ipa_control_value_entry { /* everything but the id */ };\n> > struct ipa_control_list_entry { uint32_t id; struct ipa_control_value_entry value; };\n> \n> Good idea.\n> \n> > \n> > \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 +239,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> > > @@ -284,7 +292,29 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,\n> > > entry.direction = static_cast(id->direction());\n> > > entries.write(&entry);\n> > > \n> > \n> > I'm wondering if `ipa_control_info_entry::offset` is of any use anymore?\n> \n> It indeed does not look like it is.\n> \n> > \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> > > + struct ipa_control_value_entry valueEntry;\n> > > +\n> > > + populateControlValueEntry(valueEntry, id->id(), info.min(),\n> > > + values.offset());\n> > > + entries.write(&valueEntry);\n> > > + store(info.min(), values);\n> > > +\n> > > + populateControlValueEntry(valueEntry, id->id(), info.max(),\n> > > + values.offset());\n> > > + entries.write(&valueEntry);\n> > > + store(info.max(), values);\n> > > +\n> > > + populateControlValueEntry(valueEntry, id->id(), info.def(),\n> > > + values.offset());\n> > > + entries.write(&valueEntry);\n> > > + store(info.def(), values);\n> > > }\n> > > \n> > > if (buffer.overflow())\n> > > @@ -366,11 +396,7 @@ int ControlSerializer::serialize(const ControlList &list,\n> > > const ControlValue &value = ctrl.second;\n> > > \n> > > struct ipa_control_value_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, id, value, values.offset());\n> > > entries.write(&entry);\n> > > \n> > > store(value, values);\n> > > @@ -397,15 +423,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> > > @@ -485,7 +502,13 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\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 struct ipa_control_value_entry *min_entry =\n> > > + entries.read();\n> > \n> > const auto *x = entries.read();\n> > \n> > looks a bit better to me.\n> \n> That is indeed nicer.\n> \n> > \n> > \n> > > + const struct ipa_control_value_entry *max_entry =\n> > > + entries.read();\n> > > + const struct ipa_control_value_entry *def_entry =\n> > > + entries.read();\n> > \n> > Have you tried adding these three to `ipa_control_info_entry` directly?\n> \n> Originally I had wanted to avoid changing the ipa_controls.h header but ig\n> since we're aligning this with an ABI breaking release (and since nobody\n> probably depends on that interface anyway) it's probably fine to change it.\n\nOn second thought, this is going to get a bit hairy relating to reusing the\nserializing functions for ControlValue so I'm going to leave it as-is.\n\n\nPaul\n\n> \n> > \n> > \n> > > + if (!entry || !min_entry || !max_entry || !def_entry) {\n> > > LOG(Serializer, Error) << \"Out of data\";\n> > > return {};\n> > > }\n> > > @@ -518,8 +541,15 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> > > return {};\n> > > }\n> > > \n> > > + ControlValue min = loadControlValue(values, min_entry->is_array,\n> > > + min_entry->count);\n> > > + ControlValue max = loadControlValue(values, max_entry->is_array,\n> > > + max_entry->count);\n> > > + ControlValue def = loadControlValue(values, def_entry->is_array,\n> > > + def_entry->count);\n> > > +\n> > \n> > Should `*_entry->offset` be checked here?\n> \n> Yes it probably should.\n> \n> \n> Thanks,\n> \n> Paul\n> \n> > \n> > \n> > Regards,\n> > Barnabás Pőcze\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> >","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 EB706BF415\n\tfor ;\n\tSat, 11 Oct 2025 05:44:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0C26D60316;\n\tSat, 11 Oct 2025 07:44:40 +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 17DB66030E\n\tfor ;\n\tSat, 11 Oct 2025 07:44:39 +0200 (CEST)","from neptunite.rasen.tech (unknown\n\t[IPv6:2404:7a81:160:2100:42b2:5255:36e:e46a])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 19D6982A;\n\tSat, 11 Oct 2025 07:43:01 +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=\"sEb9arC6\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1760161382;\n\tbh=OdF1dclNViXkGzUYN5FrJk0/2hdejoWY1xwoQJkCbZQ=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=sEb9arC6zglN0ZMkDDT1eNcJl0ZSM3ZMsvFRbNPjNSPdsn9f5NxueYTHe1k+QhcnH\n\tUZ/pXDdIg8VnH2AYE4xUEgADWFLJ5Mbz6DZUevUHKsHJmFUyfcOIQvgiRiHmRbSOb0\n\tua7sY5kXBS2xjjx14YYH/96FvRaSlMd0fa6ifkKA=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<176015887792.2583768.13347460452247336766@neptunite.rasen.tech>","References":"<20251010113332.3030598-1-paul.elder@ideasonboard.com>\n\t<20251010113332.3030598-2-paul.elder@ideasonboard.com>\n\t<19ca4991-c3f2-4805-b6c7-c5b5b20a14b3@ideasonboard.com>\n\t<176015887792.2583768.13347460452247336766@neptunite.rasen.tech>","Subject":"Re: [PATCH v3 1/2] libcamera: control_serializer: Add array info to\n\tserialized ControlValue","From":"Paul Elder ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Sat, 11 Oct 2025 14:44:31 +0900","Message-ID":"<176016147182.1891906.10195489546031172447@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\" "}}]