Patch Detail
Show a patch.
GET /api/1.1/patches/24579/?format=api
{ "id": 24579, "url": "https://patchwork.libcamera.org/api/1.1/patches/24579/?format=api", "web_url": "https://patchwork.libcamera.org/patch/24579/", "project": { "id": 1, "url": "https://patchwork.libcamera.org/api/1.1/projects/1/?format=api", "name": "libcamera", "link_name": "libcamera", "list_id": "libcamera_core", "list_email": "libcamera-devel@lists.libcamera.org", "web_url": "", "scm_url": "", "webscm_url": "" }, "msgid": "<20251010113332.3030598-2-paul.elder@ideasonboard.com>", "date": "2025-10-10T11:33:29", "name": "[v3,1/2] libcamera: control_serializer: Add array info to serialized ControlValue", "commit_ref": null, "pull_url": null, "state": "superseded", "archived": false, "hash": "b7b8bf6f65b1ced8d300fe673243d75c082814c5", "submitter": { "id": 17, "url": "https://patchwork.libcamera.org/api/1.1/people/17/?format=api", "name": "Paul Elder", "email": "paul.elder@ideasonboard.com" }, "delegate": null, "mbox": "https://patchwork.libcamera.org/patch/24579/mbox/", "series": [ { "id": 5483, "url": "https://patchwork.libcamera.org/api/1.1/series/5483/?format=api", "web_url": "https://patchwork.libcamera.org/project/libcamera/list/?series=5483", "date": "2025-10-10T11:33:28", "name": "Fix ControlSerializer deserializing array controls", "version": 3, "mbox": "https://patchwork.libcamera.org/series/5483/mbox/" } ], "comments": "https://patchwork.libcamera.org/api/patches/24579/comments/", "check": "pending", "checks": "https://patchwork.libcamera.org/api/patches/24579/checks/", "tags": {}, "headers": { "Return-Path": "<libcamera-devel-bounces@lists.libcamera.org>", "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 9915CBE080\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 10 Oct 2025 11:33:52 +0000 (UTC)", "from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C77EB6B60E;\n\tFri, 10 Oct 2025 13:33:50 +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 877E66B599\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 10 Oct 2025 13:33:49 +0200 (CEST)", "from neptunite.hamster-moth.ts.net (unknown\n\t[IPv6:2404:7a81:160:2100:fa2a:8f34:95fc:6e6b])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CE8C9EFE;\n\tFri, 10 Oct 2025 13:32:12 +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=\"eD3PojqB\"; dkim-atps=neutral", "DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1760095934;\n\tbh=sehZUsouTiEImz9m/s8rG0Jl2JMH2HMmnQHTPis7UHQ=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=eD3PojqBp38YVIsni4nyWcKm2Zdipv4lz6Eoqc05LaD58Q3ICalEUdTr+BP9rTbiR\n\trWOlC1bX8lZ58PCOOgJWL6wS04etmB6N49fS0nLUTtmZvFdIcxE2iJdPGqMg3pCG6C\n\tqoK7O9unXYzhKLGvECHQKhePxbzJ5petCGx4pTuE=", "From": "Paul Elder <paul.elder@ideasonboard.com>", "To": "libcamera-devel@lists.libcamera.org", "Cc": "Paul Elder <paul.elder@ideasonboard.com>, barnabas.pocze@ideasonboard.com", "Subject": "[PATCH v3 1/2] libcamera: control_serializer: Add array info to\n\tserialized ControlValue", "Date": "Fri, 10 Oct 2025 20:33:29 +0900", "Message-ID": "<20251010113332.3030598-2-paul.elder@ideasonboard.com>", "X-Mailer": "git-send-email 2.47.2", "In-Reply-To": "<20251010113332.3030598-1-paul.elder@ideasonboard.com>", "References": "<20251010113332.3030598-1-paul.elder@ideasonboard.com>", "MIME-Version": "1.0", "Content-Transfer-Encoding": "8bit", "X-BeenThere": "libcamera-devel@lists.libcamera.org", "X-Mailman-Version": "2.1.29", "Precedence": "list", "List-Id": "<libcamera-devel.lists.libcamera.org>", "List-Unsubscribe": "<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>", "List-Archive": "<https://lists.libcamera.org/pipermail/libcamera-devel/>", "List-Post": "<mailto:libcamera-devel@lists.libcamera.org>", "List-Help": "<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>", "List-Subscribe": "<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>", "Errors-To": "libcamera-devel-bounces@lists.libcamera.org", "Sender": "\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>" }, "content": "Array controls (eg. ColourCorrectionMatrix, FrameDurationLimits,\nColourGains) are serialized properly by the ControlSerializer, but are\nnot deserialized properly. This is because their arrayness and size are\nnot considered during deserialization.\n\nFix this by adding arrayness and size to the serialized form of all\nControlValues. This is achieved by adding the already-existing struct\nipa_control_value_entry to the serialized form of ControlInfoMap to\ncontain all the metadata associated with each ControlValue in\nthe min/max/def of each ControlInfo.\n\nThe issue was not noticed before as the default value of the ControlInfo\nof other array controls had been set to scalar values similar to\nmin/max, and ColourCorrectionMatrix was the first control to properly\ndefine a non-scalar default value.\n\nOther array controls that define a scalar default value need to be\nfixed to define a properly sized default ControlInfo value.\n\nBug: https://bugs.libcamera.org/show_bug.cgi?id=285\nSigned-off-by: Paul Elder <paul.elder@ideasonboard.com>\n\n---\nChanges 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\nChanges 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(-)", "diff": "diff --git a/include/libcamera/internal/control_serializer.h b/include/libcamera/internal/control_serializer.h\nindex 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_;\ndiff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\nindex 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+\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+\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<ControlId::DirectionFlags::Type>(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, 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<typename T> T ControlSerializer::deserialize(ByteStreamBuffer &buffer)\n * \\brief Deserialize an object from a binary buffer\n@@ -485,7 +502,13 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(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<decltype(*entry)>();\n-\t\tif (!entry) {\n+\t\tconst struct ipa_control_value_entry *min_entry =\n+\t\t\tentries.read<decltype(*min_entry)>();\n+\t\tconst struct ipa_control_value_entry *max_entry =\n+\t\t\tentries.read<decltype(*max_entry)>();\n+\t\tconst struct ipa_control_value_entry *def_entry =\n+\t\t\tentries.read<decltype(*def_entry)>();\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<ControlInfoMap>(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 \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", "prefixes": [ "v3", "1/2" ] }