[{"id":35336,"web_url":"https://patchwork.libcamera.org/comment/35336/","msgid":"<175492155549.1641235.14089828007550134576@ping.linuxembedded.co.uk>","date":"2025-08-11T14:12:35","subject":"Re: [PATCH v1] libcamera: control_serializer: Do not alias byte\n\tbuffer","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Barnabás Pőcze (2025-08-11 14:11:59)\n> Aliasing an arbitrary buffer with some `T *` is unlikely to yield good\n> results, primarily due to potential alignment issues. So don't do it,\n> instead create the objects on the stack and read into them, this\n> removes any and all strict aliasing or alignment concerns.\n> \n> Bug: https://bugs.libcamera.org/show_bug.cgi?id=221\n> Signed-off-by: Barnabás Pőcze \n> ---\n> src/libcamera/control_serializer.cpp | 75 +++++++++++++---------------\n> 1 file changed, 36 insertions(+), 39 deletions(-)\n> \n> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> index 050f8512b..722ff231c 100644\n> --- a/src/libcamera/control_serializer.cpp\n> +++ b/src/libcamera/control_serializer.cpp\n> @@ -427,22 +427,22 @@ ControlInfo ControlSerializer::loadControlInfo(ByteStreamBuffer &b)\n> template<>\n> ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &buffer)\n> {\n> - const struct ipa_controls_header *hdr = buffer.read();\n> - if (!hdr) {\n> + struct ipa_controls_header hdr;\n> + if (buffer.read(&hdr) < 0) {\n\nAha, but the key difference is that before it was 'zero copy' just\npointing at the data, while now we're making a copy ?\n\nI wonder if we can detect the alignment and only do the copy if\nunaligned? Or maybe that's just not worth it and we should just accept\nthe copy - this shouldn't be a high data volume for the most parts...\n\nBut maybe its something we want to be able to profile/monitor in the\nfuture.\n\n\nOr perhaps can we find a way to align data when it goes into the\nByteStreamBuffer with some alignment padding sections somehow?\n\n\nIf fixing/specifying the alignment in the buffer isn't possible easy -\nthen I could certainly imagine accepting this as a current possible solution ...\n\n> LOG(Serializer, Error) << \"Out of data\";\n> return {};\n> }\n> \n> - auto iter = infoMaps_.find(hdr->handle);\n> + auto iter = infoMaps_.find(hdr.handle);\n> if (iter != infoMaps_.end()) {\n> LOG(Serializer, Debug) << \"Use cached ControlInfoMap\";\n> return iter->second;\n> }\n> \n> - if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) {\n> + if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) {\n> LOG(Serializer, Error)\n> << \"Unsupported controls format version \"\n> - << hdr->version;\n> + << hdr.version;\n> return {};\n> }\n> \n> @@ -455,7 +455,7 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> */\n> const ControlIdMap *idMap = nullptr;\n> ControlIdMap *localIdMap = nullptr;\n> - switch (hdr->id_map_type) {\n> + switch (hdr.id_map_type) {\n> case IPA_CONTROL_ID_MAP_CONTROLS:\n> idMap = &controls::controls;\n> break;\n> @@ -469,12 +469,12 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> break;\n> default:\n> LOG(Serializer, Error)\n> - << \"Unknown id map type: \" << hdr->id_map_type;\n> + << \"Unknown id map type: \" << hdr.id_map_type;\n> return {};\n> }\n> \n> - ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr));\n> - ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset);\n> + ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr));\n> + ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset);\n> \n> if (buffer.overflow()) {\n> LOG(Serializer, Error) << \"Out of data\";\n> @@ -482,36 +482,35 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> }\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> + for (unsigned int i = 0; i < hdr.entries; ++i) {\n> + struct ipa_control_info_entry entry;\n> + if (entries.read(&entry) < 0) {\n> LOG(Serializer, Error) << \"Out of data\";\n> return {};\n> }\n> \n> - ControlType type = static_cast(entry->type);\n> + ControlType type = static_cast(entry.type);\n> \n> /* If we're using a local id map, populate it. */\n> if (localIdMap) {\n> ControlId::DirectionFlags flags{\n> - static_cast(entry->direction)\n> + static_cast(entry.direction)\n> };\n> \n> /**\n> * \\todo Find a way to preserve the control name for\n> * debugging purpose.\n> */\n> - controlIds_.emplace_back(std::make_unique(entry->id,\n> + controlIds_.emplace_back(std::make_unique(entry.id,\n> \"\", \"local\", type,\n> flags));\n> - (*localIdMap)[entry->id] = controlIds_.back().get();\n> + (*localIdMap)[entry.id] = controlIds_.back().get();\n> }\n> \n> - const ControlId *controlId = idMap->at(entry->id);\n> + const ControlId *controlId = idMap->at(entry.id);\n> ASSERT(controlId);\n> \n> - if (entry->offset != values.offset()) {\n> + if (entry.offset != values.offset()) {\n> LOG(Serializer, Error)\n> << \"Bad data, entry offset mismatch (entry \"\n> << i << \")\";\n> @@ -526,9 +525,9 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> * Create the ControlInfoMap in the cache, and store the map to handle\n> * association.\n> */\n> - infoMaps_[hdr->handle] = ControlInfoMap(std::move(ctrls), *idMap);\n> - ControlInfoMap &map = infoMaps_[hdr->handle];\n> - infoMapHandles_[&map] = hdr->handle;\n> + infoMaps_[hdr.handle] = ControlInfoMap(std::move(ctrls), *idMap);\n> + ControlInfoMap &map = infoMaps_[hdr.handle];\n> + infoMapHandles_[&map] = hdr.handle;\n> \n> return map;\n> }\n> @@ -545,21 +544,21 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> template<>\n> ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer)\n> {\n> - const struct ipa_controls_header *hdr = buffer.read();\n> - if (!hdr) {\n> + struct ipa_controls_header hdr;\n> + if (buffer.read(&hdr) < 0) {\n> LOG(Serializer, Error) << \"Out of data\";\n> return {};\n> }\n> \n> - if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) {\n> + if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) {\n> LOG(Serializer, Error)\n> << \"Unsupported controls format version \"\n> - << hdr->version;\n> + << hdr.version;\n> return {};\n> }\n> \n> - ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr));\n> - ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset);\n> + ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr));\n> + ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset);\n> \n> if (buffer.overflow()) {\n> LOG(Serializer, Error) << \"Out of data\";\n> @@ -576,10 +575,10 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> * ControlInfoMap is available.\n> */\n> const ControlIdMap *idMap;\n> - if (hdr->handle) {\n> + if (hdr.handle) {\n> auto iter = std::find_if(infoMapHandles_.begin(), infoMapHandles_.end(),\n> [&](decltype(infoMapHandles_)::value_type &entry) {\n> - return entry.second == hdr->handle;\n> + return entry.second == hdr.handle;\n> });\n> if (iter == infoMapHandles_.end()) {\n> LOG(Serializer, Error)\n> @@ -590,7 +589,7 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> const ControlInfoMap *infoMap = iter->first;\n> idMap = &infoMap->idmap();\n> } else {\n> - switch (hdr->id_map_type) {\n> + switch (hdr.id_map_type) {\n> case IPA_CONTROL_ID_MAP_CONTROLS:\n> idMap = &controls::controls;\n> break;\n> @@ -601,7 +600,7 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> \n> case IPA_CONTROL_ID_MAP_V4L2:\n> default:\n> - if (hdr->entries > 0)\n> + if (hdr.entries > 0)\n> LOG(Serializer, Fatal)\n> << \"A list of V4L2 controls requires a ControlInfoMap\";\n> \n> @@ -617,23 +616,21 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> */\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> + for (unsigned int i = 0; i < hdr.entries; ++i) {\n> + struct ipa_control_value_entry entry;\n> + if (entries.read(&entry) < 0) {\n> LOG(Serializer, Error) << \"Out of data\";\n> return {};\n> }\n> \n> - if (entry->offset != values.offset()) {\n> + if (entry.offset != values.offset()) {\n> LOG(Serializer, Error)\n> << \"Bad data, entry offset mismatch (entry \"\n> << i << \")\";\n> return {};\n> }\n> \n> - ctrls.set(entry->id,\n> - loadControlValue(values, entry->is_array, entry->count));\n> + ctrls.set(entry.id, loadControlValue(values, entry.is_array, entry.count));\n> }\n> \n> return ctrls;\n> -- \n> 2.50.1\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 DB2E8BEFBE\n\tfor ;\n\tMon, 11 Aug 2025 14:12:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EB69F6923C;\n\tMon, 11 Aug 2025 16:12:38 +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 3821269233\n\tfor ;\n\tMon, 11 Aug 2025 16:12:38 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E88D082A;\n\tMon, 11 Aug 2025 16:11:45 +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=\"OoYughus\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1754921506;\n\tbh=PtCO9BHJnwO2gznBlyzqAlWd8tHOUS22F2TOtcZPJZE=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=OoYughusuiRlTcplcWLJPURnALOcLWn+LPsrDc98n0/WUcXD2qNc7WKsduCC4g/l6\n\tKps71sr9vWvSvr4qHbiGfvNiG/02l5x4VB4VsuuoDCKGj7DnrfvBsqqFyX+ek2O/Og\n\tTJHMDrivvwkl4D5JM6Ou480Jgmqs12wnh2dMy9Fo=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250811131159.1342981-1-barnabas.pocze@ideasonboard.com>","References":"<20250811131159.1342981-1-barnabas.pocze@ideasonboard.com>","Subject":"Re: [PATCH v1] libcamera: control_serializer: Do not alias byte\n\tbuffer","From":"Kieran Bingham ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 11 Aug 2025 15:12:35 +0100","Message-ID":"<175492155549.1641235.14089828007550134576@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","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":35337,"web_url":"https://patchwork.libcamera.org/comment/35337/","msgid":"<333a28b4-f758-4e57-a13e-bf7f9ee42755@ideasonboard.com>","date":"2025-08-11T14:18:57","subject":"Re: [PATCH v1] libcamera: control_serializer: Do not alias byte\n\tbuffer","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. 08. 11. 16:12 keltezéssel, Kieran Bingham írta:\n> Quoting Barnabás Pőcze (2025-08-11 14:11:59)\n>> Aliasing an arbitrary buffer with some `T *` is unlikely to yield good\n>> results, primarily due to potential alignment issues. So don't do it,\n>> instead create the objects on the stack and read into them, this\n>> removes any and all strict aliasing or alignment concerns.\n>>\n>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=221\n>> Signed-off-by: Barnabás Pőcze \n>> ---\n>> src/libcamera/control_serializer.cpp | 75 +++++++++++++---------------\n>> 1 file changed, 36 insertions(+), 39 deletions(-)\n>>\n>> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n>> index 050f8512b..722ff231c 100644\n>> --- a/src/libcamera/control_serializer.cpp\n>> +++ b/src/libcamera/control_serializer.cpp\n>> @@ -427,22 +427,22 @@ ControlInfo ControlSerializer::loadControlInfo(ByteStreamBuffer &b)\n>> template<>\n>> ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &buffer)\n>> {\n>> - const struct ipa_controls_header *hdr = buffer.read();\n>> - if (!hdr) {\n>> + struct ipa_controls_header hdr;\n>> + if (buffer.read(&hdr) < 0) {\n> \n> Aha, but the key difference is that before it was 'zero copy' just\n> pointing at the data, while now we're making a copy ?\n\nYes.\n\n\n> \n> I wonder if we can detect the alignment and only do the copy if\n> unaligned? Or maybe that's just not worth it and we should just accept\n> the copy - this shouldn't be a high data volume for the most parts...\n\nI feel like the control id/info map/list/value allocation easily dominates the\ncost of these small copies. These are all <=32 bytes.\n\n\n> \n> But maybe its something we want to be able to profile/monitor in the\n> future.\n> \n> \n> Or perhaps can we find a way to align data when it goes into the\n> ByteStreamBuffer with some alignment padding sections somehow?\n\nI could be missing something, but given how ipc serialization is done\nat the moment, it does not seem entirely trivial.\n\n\n> \n> \n> If fixing/specifying the alignment in the buffer isn't possible easy -\n> then I could certainly imagine accepting this as a current possible solution ...\n> \n>> LOG(Serializer, Error) << \"Out of data\";\n>> return {};\n>> }\n>> \n>> - auto iter = infoMaps_.find(hdr->handle);\n>> + auto iter = infoMaps_.find(hdr.handle);\n>> if (iter != infoMaps_.end()) {\n>> LOG(Serializer, Debug) << \"Use cached ControlInfoMap\";\n>> return iter->second;\n>> }\n>> \n>> - if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) {\n>> + if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) {\n>> LOG(Serializer, Error)\n>> << \"Unsupported controls format version \"\n>> - << hdr->version;\n>> + << hdr.version;\n>> return {};\n>> }\n>> \n>> @@ -455,7 +455,7 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n>> */\n>> const ControlIdMap *idMap = nullptr;\n>> ControlIdMap *localIdMap = nullptr;\n>> - switch (hdr->id_map_type) {\n>> + switch (hdr.id_map_type) {\n>> case IPA_CONTROL_ID_MAP_CONTROLS:\n>> idMap = &controls::controls;\n>> break;\n>> @@ -469,12 +469,12 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n>> break;\n>> default:\n>> LOG(Serializer, Error)\n>> - << \"Unknown id map type: \" << hdr->id_map_type;\n>> + << \"Unknown id map type: \" << hdr.id_map_type;\n>> return {};\n>> }\n>> \n>> - ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr));\n>> - ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset);\n>> + ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr));\n>> + ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset);\n>> \n>> if (buffer.overflow()) {\n>> LOG(Serializer, Error) << \"Out of data\";\n>> @@ -482,36 +482,35 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n>> }\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>> + for (unsigned int i = 0; i < hdr.entries; ++i) {\n>> + struct ipa_control_info_entry entry;\n>> + if (entries.read(&entry) < 0) {\n>> LOG(Serializer, Error) << \"Out of data\";\n>> return {};\n>> }\n>> \n>> - ControlType type = static_cast(entry->type);\n>> + ControlType type = static_cast(entry.type);\n>> \n>> /* If we're using a local id map, populate it. */\n>> if (localIdMap) {\n>> ControlId::DirectionFlags flags{\n>> - static_cast(entry->direction)\n>> + static_cast(entry.direction)\n>> };\n>> \n>> /**\n>> * \\todo Find a way to preserve the control name for\n>> * debugging purpose.\n>> */\n>> - controlIds_.emplace_back(std::make_unique(entry->id,\n>> + controlIds_.emplace_back(std::make_unique(entry.id,\n>> \"\", \"local\", type,\n>> flags));\n>> - (*localIdMap)[entry->id] = controlIds_.back().get();\n>> + (*localIdMap)[entry.id] = controlIds_.back().get();\n>> }\n>> \n>> - const ControlId *controlId = idMap->at(entry->id);\n>> + const ControlId *controlId = idMap->at(entry.id);\n>> ASSERT(controlId);\n>> \n>> - if (entry->offset != values.offset()) {\n>> + if (entry.offset != values.offset()) {\n>> LOG(Serializer, Error)\n>> << \"Bad data, entry offset mismatch (entry \"\n>> << i << \")\";\n>> @@ -526,9 +525,9 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n>> * Create the ControlInfoMap in the cache, and store the map to handle\n>> * association.\n>> */\n>> - infoMaps_[hdr->handle] = ControlInfoMap(std::move(ctrls), *idMap);\n>> - ControlInfoMap &map = infoMaps_[hdr->handle];\n>> - infoMapHandles_[&map] = hdr->handle;\n>> + infoMaps_[hdr.handle] = ControlInfoMap(std::move(ctrls), *idMap);\n>> + ControlInfoMap &map = infoMaps_[hdr.handle];\n>> + infoMapHandles_[&map] = hdr.handle;\n>> \n>> return map;\n>> }\n>> @@ -545,21 +544,21 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n>> template<>\n>> ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer)\n>> {\n>> - const struct ipa_controls_header *hdr = buffer.read();\n>> - if (!hdr) {\n>> + struct ipa_controls_header hdr;\n>> + if (buffer.read(&hdr) < 0) {\n>> LOG(Serializer, Error) << \"Out of data\";\n>> return {};\n>> }\n>> \n>> - if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) {\n>> + if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) {\n>> LOG(Serializer, Error)\n>> << \"Unsupported controls format version \"\n>> - << hdr->version;\n>> + << hdr.version;\n>> return {};\n>> }\n>> \n>> - ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr));\n>> - ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset);\n>> + ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr));\n>> + ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset);\n>> \n>> if (buffer.overflow()) {\n>> LOG(Serializer, Error) << \"Out of data\";\n>> @@ -576,10 +575,10 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n>> * ControlInfoMap is available.\n>> */\n>> const ControlIdMap *idMap;\n>> - if (hdr->handle) {\n>> + if (hdr.handle) {\n>> auto iter = std::find_if(infoMapHandles_.begin(), infoMapHandles_.end(),\n>> [&](decltype(infoMapHandles_)::value_type &entry) {\n>> - return entry.second == hdr->handle;\n>> + return entry.second == hdr.handle;\n>> });\n>> if (iter == infoMapHandles_.end()) {\n>> LOG(Serializer, Error)\n>> @@ -590,7 +589,7 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n>> const ControlInfoMap *infoMap = iter->first;\n>> idMap = &infoMap->idmap();\n>> } else {\n>> - switch (hdr->id_map_type) {\n>> + switch (hdr.id_map_type) {\n>> case IPA_CONTROL_ID_MAP_CONTROLS:\n>> idMap = &controls::controls;\n>> break;\n>> @@ -601,7 +600,7 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n>> \n>> case IPA_CONTROL_ID_MAP_V4L2:\n>> default:\n>> - if (hdr->entries > 0)\n>> + if (hdr.entries > 0)\n>> LOG(Serializer, Fatal)\n>> << \"A list of V4L2 controls requires a ControlInfoMap\";\n>> \n>> @@ -617,23 +616,21 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n>> */\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>> + for (unsigned int i = 0; i < hdr.entries; ++i) {\n>> + struct ipa_control_value_entry entry;\n>> + if (entries.read(&entry) < 0) {\n>> LOG(Serializer, Error) << \"Out of data\";\n>> return {};\n>> }\n>> \n>> - if (entry->offset != values.offset()) {\n>> + if (entry.offset != values.offset()) {\n>> LOG(Serializer, Error)\n>> << \"Bad data, entry offset mismatch (entry \"\n>> << i << \")\";\n>> return {};\n>> }\n>> \n>> - ctrls.set(entry->id,\n>> - loadControlValue(values, entry->is_array, entry->count));\n>> + ctrls.set(entry.id, loadControlValue(values, entry.is_array, entry.count));\n>> }\n>> \n>> return ctrls;\n>> -- \n>> 2.50.1\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 2541ABDCC1\n\tfor ;\n\tMon, 11 Aug 2025 14:19:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 27B086923C;\n\tMon, 11 Aug 2025 16:19:02 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 09DCC69233\n\tfor ;\n\tMon, 11 Aug 2025 16:19:01 +0200 (CEST)","from [192.168.33.20] (185.221.140.182.nat.pool.zt.hu\n\t[185.221.140.182])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B8D8182A;\n\tMon, 11 Aug 2025 16:18:08 +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=\"AzZZoV63\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1754921888;\n\tbh=PftDINJL9Zs7O+hBswO/djjmSU94uH31kB9m/8ARq64=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=AzZZoV63pSxA2hilDV42MhvvwOATE2cdMNIxx1Va8xpml0L8CInBlrtGr+AFAuv0P\n\th2fjjzHXhZ5GZP7UbRV0w+uX5mswqXPE+M4vuaNYBw/F9raWFwiuhEYb1mVl/sZ6sg\n\ttuxg4RG6eJ7Xkwg/SywqKL6Af7AYmPFHpi1ZdcIU=","Message-ID":"<333a28b4-f758-4e57-a13e-bf7f9ee42755@ideasonboard.com>","Date":"Mon, 11 Aug 2025 16:18:57 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v1] libcamera: control_serializer: Do not alias byte\n\tbuffer","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20250811131159.1342981-1-barnabas.pocze@ideasonboard.com>\n\t<175492155549.1641235.14089828007550134576@ping.linuxembedded.co.uk>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<175492155549.1641235.14089828007550134576@ping.linuxembedded.co.uk>","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":35338,"web_url":"https://patchwork.libcamera.org/comment/35338/","msgid":"<175492223974.1641235.4760089293007493036@ping.linuxembedded.co.uk>","date":"2025-08-11T14:23:59","subject":"Re: [PATCH v1] libcamera: control_serializer: Do not alias byte\n\tbuffer","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Barnabás Pőcze (2025-08-11 15:18:57)\n> Hi\n> \n> 2025. 08. 11. 16:12 keltezéssel, Kieran Bingham írta:\n> > Quoting Barnabás Pőcze (2025-08-11 14:11:59)\n> >> Aliasing an arbitrary buffer with some `T *` is unlikely to yield good\n> >> results, primarily due to potential alignment issues. So don't do it,\n> >> instead create the objects on the stack and read into them, this\n> >> removes any and all strict aliasing or alignment concerns.\n> >>\n> >> Bug: https://bugs.libcamera.org/show_bug.cgi?id=221\n> >> Signed-off-by: Barnabás Pőcze \n> >> ---\n> >> src/libcamera/control_serializer.cpp | 75 +++++++++++++---------------\n> >> 1 file changed, 36 insertions(+), 39 deletions(-)\n> >>\n> >> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> >> index 050f8512b..722ff231c 100644\n> >> --- a/src/libcamera/control_serializer.cpp\n> >> +++ b/src/libcamera/control_serializer.cpp\n> >> @@ -427,22 +427,22 @@ ControlInfo ControlSerializer::loadControlInfo(ByteStreamBuffer &b)\n> >> template<>\n> >> ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &buffer)\n> >> {\n> >> - const struct ipa_controls_header *hdr = buffer.read();\n> >> - if (!hdr) {\n> >> + struct ipa_controls_header hdr;\n> >> + if (buffer.read(&hdr) < 0) {\n> > \n> > Aha, but the key difference is that before it was 'zero copy' just\n> > pointing at the data, while now we're making a copy ?\n> \n> Yes.\n> \n> \n> > \n> > I wonder if we can detect the alignment and only do the copy if\n> > unaligned? Or maybe that's just not worth it and we should just accept\n> > the copy - this shouldn't be a high data volume for the most parts...\n> \n> I feel like the control id/info map/list/value allocation easily dominates the\n> cost of these small copies. These are all <=32 bytes.\n\nHrm.. indeed - we don't normally expect ControlLists to contain large\ndata.\n\nI thnik CCMs with a 3x3 matrix is 'hopefully' the biggest we expect for\nthe most part and things like lens shading tables shouldn't be\npopultated through a control list.\n\nSo ... yes a copy seems quick and trivial...\n\nI'll put this in already:\n\n\nReviewed-by: Kieran Bingham \n\nBut probably want this also checked by either or both of Laurent and\nPaul who know the IPC mechanisms well.\n\n--\nKieran\n\n\n> \n> \n> > \n> > But maybe its something we want to be able to profile/monitor in the\n> > future.\n> > \n> > \n> > Or perhaps can we find a way to align data when it goes into the\n> > ByteStreamBuffer with some alignment padding sections somehow?\n> \n> I could be missing something, but given how ipc serialization is done\n> at the moment, it does not seem entirely trivial.\n> \n> \n> > \n> > \n> > If fixing/specifying the alignment in the buffer isn't possible easy -\n> > then I could certainly imagine accepting this as a current possible solution ...\n> > \n> >> LOG(Serializer, Error) << \"Out of data\";\n> >> return {};\n> >> }\n> >> \n> >> - auto iter = infoMaps_.find(hdr->handle);\n> >> + auto iter = infoMaps_.find(hdr.handle);\n> >> if (iter != infoMaps_.end()) {\n> >> LOG(Serializer, Debug) << \"Use cached ControlInfoMap\";\n> >> return iter->second;\n> >> }\n> >> \n> >> - if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) {\n> >> + if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) {\n> >> LOG(Serializer, Error)\n> >> << \"Unsupported controls format version \"\n> >> - << hdr->version;\n> >> + << hdr.version;\n> >> return {};\n> >> }\n> >> \n> >> @@ -455,7 +455,7 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> >> */\n> >> const ControlIdMap *idMap = nullptr;\n> >> ControlIdMap *localIdMap = nullptr;\n> >> - switch (hdr->id_map_type) {\n> >> + switch (hdr.id_map_type) {\n> >> case IPA_CONTROL_ID_MAP_CONTROLS:\n> >> idMap = &controls::controls;\n> >> break;\n> >> @@ -469,12 +469,12 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> >> break;\n> >> default:\n> >> LOG(Serializer, Error)\n> >> - << \"Unknown id map type: \" << hdr->id_map_type;\n> >> + << \"Unknown id map type: \" << hdr.id_map_type;\n> >> return {};\n> >> }\n> >> \n> >> - ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr));\n> >> - ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset);\n> >> + ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr));\n> >> + ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset);\n> >> \n> >> if (buffer.overflow()) {\n> >> LOG(Serializer, Error) << \"Out of data\";\n> >> @@ -482,36 +482,35 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> >> }\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> >> + for (unsigned int i = 0; i < hdr.entries; ++i) {\n> >> + struct ipa_control_info_entry entry;\n> >> + if (entries.read(&entry) < 0) {\n> >> LOG(Serializer, Error) << \"Out of data\";\n> >> return {};\n> >> }\n> >> \n> >> - ControlType type = static_cast(entry->type);\n> >> + ControlType type = static_cast(entry.type);\n> >> \n> >> /* If we're using a local id map, populate it. */\n> >> if (localIdMap) {\n> >> ControlId::DirectionFlags flags{\n> >> - static_cast(entry->direction)\n> >> + static_cast(entry.direction)\n> >> };\n> >> \n> >> /**\n> >> * \\todo Find a way to preserve the control name for\n> >> * debugging purpose.\n> >> */\n> >> - controlIds_.emplace_back(std::make_unique(entry->id,\n> >> + controlIds_.emplace_back(std::make_unique(entry.id,\n> >> \"\", \"local\", type,\n> >> flags));\n> >> - (*localIdMap)[entry->id] = controlIds_.back().get();\n> >> + (*localIdMap)[entry.id] = controlIds_.back().get();\n> >> }\n> >> \n> >> - const ControlId *controlId = idMap->at(entry->id);\n> >> + const ControlId *controlId = idMap->at(entry.id);\n> >> ASSERT(controlId);\n> >> \n> >> - if (entry->offset != values.offset()) {\n> >> + if (entry.offset != values.offset()) {\n> >> LOG(Serializer, Error)\n> >> << \"Bad data, entry offset mismatch (entry \"\n> >> << i << \")\";\n> >> @@ -526,9 +525,9 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> >> * Create the ControlInfoMap in the cache, and store the map to handle\n> >> * association.\n> >> */\n> >> - infoMaps_[hdr->handle] = ControlInfoMap(std::move(ctrls), *idMap);\n> >> - ControlInfoMap &map = infoMaps_[hdr->handle];\n> >> - infoMapHandles_[&map] = hdr->handle;\n> >> + infoMaps_[hdr.handle] = ControlInfoMap(std::move(ctrls), *idMap);\n> >> + ControlInfoMap &map = infoMaps_[hdr.handle];\n> >> + infoMapHandles_[&map] = hdr.handle;\n> >> \n> >> return map;\n> >> }\n> >> @@ -545,21 +544,21 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> >> template<>\n> >> ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer)\n> >> {\n> >> - const struct ipa_controls_header *hdr = buffer.read();\n> >> - if (!hdr) {\n> >> + struct ipa_controls_header hdr;\n> >> + if (buffer.read(&hdr) < 0) {\n> >> LOG(Serializer, Error) << \"Out of data\";\n> >> return {};\n> >> }\n> >> \n> >> - if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) {\n> >> + if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) {\n> >> LOG(Serializer, Error)\n> >> << \"Unsupported controls format version \"\n> >> - << hdr->version;\n> >> + << hdr.version;\n> >> return {};\n> >> }\n> >> \n> >> - ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr));\n> >> - ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset);\n> >> + ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr));\n> >> + ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset);\n> >> \n> >> if (buffer.overflow()) {\n> >> LOG(Serializer, Error) << \"Out of data\";\n> >> @@ -576,10 +575,10 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> >> * ControlInfoMap is available.\n> >> */\n> >> const ControlIdMap *idMap;\n> >> - if (hdr->handle) {\n> >> + if (hdr.handle) {\n> >> auto iter = std::find_if(infoMapHandles_.begin(), infoMapHandles_.end(),\n> >> [&](decltype(infoMapHandles_)::value_type &entry) {\n> >> - return entry.second == hdr->handle;\n> >> + return entry.second == hdr.handle;\n> >> });\n> >> if (iter == infoMapHandles_.end()) {\n> >> LOG(Serializer, Error)\n> >> @@ -590,7 +589,7 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> >> const ControlInfoMap *infoMap = iter->first;\n> >> idMap = &infoMap->idmap();\n> >> } else {\n> >> - switch (hdr->id_map_type) {\n> >> + switch (hdr.id_map_type) {\n> >> case IPA_CONTROL_ID_MAP_CONTROLS:\n> >> idMap = &controls::controls;\n> >> break;\n> >> @@ -601,7 +600,7 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> >> \n> >> case IPA_CONTROL_ID_MAP_V4L2:\n> >> default:\n> >> - if (hdr->entries > 0)\n> >> + if (hdr.entries > 0)\n> >> LOG(Serializer, Fatal)\n> >> << \"A list of V4L2 controls requires a ControlInfoMap\";\n> >> \n> >> @@ -617,23 +616,21 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> >> */\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> >> + for (unsigned int i = 0; i < hdr.entries; ++i) {\n> >> + struct ipa_control_value_entry entry;\n> >> + if (entries.read(&entry) < 0) {\n> >> LOG(Serializer, Error) << \"Out of data\";\n> >> return {};\n> >> }\n> >> \n> >> - if (entry->offset != values.offset()) {\n> >> + if (entry.offset != values.offset()) {\n> >> LOG(Serializer, Error)\n> >> << \"Bad data, entry offset mismatch (entry \"\n> >> << i << \")\";\n> >> return {};\n> >> }\n> >> \n> >> - ctrls.set(entry->id,\n> >> - loadControlValue(values, entry->is_array, entry->count));\n> >> + ctrls.set(entry.id, loadControlValue(values, entry.is_array, entry.count));\n> >> }\n> >> \n> >> return ctrls;\n> >> -- \n> >> 2.50.1\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 4EDA3BEFBE\n\tfor ;\n\tMon, 11 Aug 2025 14:24:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2B67C69233;\n\tMon, 11 Aug 2025 16:24:04 +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 743C369233\n\tfor ;\n\tMon, 11 Aug 2025 16:24:02 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5F52D82A;\n\tMon, 11 Aug 2025 16:23:10 +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=\"qd4mvr9q\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1754922190;\n\tbh=pqloaqP2VeGcpnOqOplt2bJYAhc1k+Jgp0veigkxS70=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=qd4mvr9qP6gBadWFZ6aeGD803MO0M9cV6JfNzea+h75evhtGhPqpStk5x7u/7/DP6\n\txpAsUxCn90H43pT5V/L2c6Ynm/rvypnCOFy16ZGEopCruiOee8zfB9cWkv5VeXoR8y\n\trXW6ocZ8rtKWAqMVIv+O1ZJf12lXhlgem0/4mzuw=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<333a28b4-f758-4e57-a13e-bf7f9ee42755@ideasonboard.com>","References":"<20250811131159.1342981-1-barnabas.pocze@ideasonboard.com>\n\t<175492155549.1641235.14089828007550134576@ping.linuxembedded.co.uk>\n\t<333a28b4-f758-4e57-a13e-bf7f9ee42755@ideasonboard.com>","Subject":"Re: [PATCH v1] libcamera: control_serializer: Do not alias byte\n\tbuffer","From":"Kieran Bingham ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 11 Aug 2025 15:23:59 +0100","Message-ID":"<175492223974.1641235.4760089293007493036@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","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":35339,"web_url":"https://patchwork.libcamera.org/comment/35339/","msgid":"<3c6c6b31-5af8-4201-923c-d749c4ad6e51@ideasonboard.com>","date":"2025-08-11T14:28:10","subject":"Re: [PATCH v1] libcamera: control_serializer: Do not alias byte\n\tbuffer","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"2025. 08. 11. 16:23 keltezéssel, Kieran Bingham írta:\n> Quoting Barnabás Pőcze (2025-08-11 15:18:57)\n>> Hi\n>>\n>> 2025. 08. 11. 16:12 keltezéssel, Kieran Bingham írta:\n>>> Quoting Barnabás Pőcze (2025-08-11 14:11:59)\n>>>> Aliasing an arbitrary buffer with some `T *` is unlikely to yield good\n>>>> results, primarily due to potential alignment issues. So don't do it,\n>>>> instead create the objects on the stack and read into them, this\n>>>> removes any and all strict aliasing or alignment concerns.\n>>>>\n>>>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=221\n>>>> Signed-off-by: Barnabás Pőcze \n>>>> ---\n>>>> src/libcamera/control_serializer.cpp | 75 +++++++++++++---------------\n>>>> 1 file changed, 36 insertions(+), 39 deletions(-)\n>>>>\n>>>> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n>>>> index 050f8512b..722ff231c 100644\n>>>> --- a/src/libcamera/control_serializer.cpp\n>>>> +++ b/src/libcamera/control_serializer.cpp\n>>>> @@ -427,22 +427,22 @@ ControlInfo ControlSerializer::loadControlInfo(ByteStreamBuffer &b)\n>>>> template<>\n>>>> ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &buffer)\n>>>> {\n>>>> - const struct ipa_controls_header *hdr = buffer.read();\n>>>> - if (!hdr) {\n>>>> + struct ipa_controls_header hdr;\n>>>> + if (buffer.read(&hdr) < 0) {\n>>>\n>>> Aha, but the key difference is that before it was 'zero copy' just\n>>> pointing at the data, while now we're making a copy ?\n>>\n>> Yes.\n>>\n>>\n>>>\n>>> I wonder if we can detect the alignment and only do the copy if\n>>> unaligned? Or maybe that's just not worth it and we should just accept\n>>> the copy - this shouldn't be a high data volume for the most parts...\n>>\n>> I feel like the control id/info map/list/value allocation easily dominates the\n>> cost of these small copies. These are all <=32 bytes.\n> \n> Hrm.. indeed - we don't normally expect ControlLists to contain large\n> data.\n> \n> I thnik CCMs with a 3x3 matrix is 'hopefully' the biggest we expect for\n> the most part and things like lens shading tables shouldn't be\n> popultated through a control list.\n\nThis change only affects the headers, e.g. the bytes of the control value\ndata is still read the same way, directly from the buffer.\n\n\n> \n> So ... yes a copy seems quick and trivial...\n> \n> I'll put this in already:\n> \n> \n> Reviewed-by: Kieran Bingham \n> \n> But probably want this also checked by either or both of Laurent and\n> Paul who know the IPC mechanisms well.\n> \n> --\n> Kieran\n> \n> \n>>\n>>\n>>>\n>>> But maybe its something we want to be able to profile/monitor in the\n>>> future.\n>>>\n>>>\n>>> Or perhaps can we find a way to align data when it goes into the\n>>> ByteStreamBuffer with some alignment padding sections somehow?\n>>\n>> I could be missing something, but given how ipc serialization is done\n>> at the moment, it does not seem entirely trivial.\n>>\n>>\n>>>\n>>>\n>>> If fixing/specifying the alignment in the buffer isn't possible easy -\n>>> then I could certainly imagine accepting this as a current possible solution ...\n>>>\n>>>> LOG(Serializer, Error) << \"Out of data\";\n>>>> return {};\n>>>> }\n>>>> \n>>>> - auto iter = infoMaps_.find(hdr->handle);\n>>>> + auto iter = infoMaps_.find(hdr.handle);\n>>>> if (iter != infoMaps_.end()) {\n>>>> LOG(Serializer, Debug) << \"Use cached ControlInfoMap\";\n>>>> return iter->second;\n>>>> }\n>>>> \n>>>> - if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) {\n>>>> + if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) {\n>>>> LOG(Serializer, Error)\n>>>> << \"Unsupported controls format version \"\n>>>> - << hdr->version;\n>>>> + << hdr.version;\n>>>> return {};\n>>>> }\n>>>> \n>>>> @@ -455,7 +455,7 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n>>>> */\n>>>> const ControlIdMap *idMap = nullptr;\n>>>> ControlIdMap *localIdMap = nullptr;\n>>>> - switch (hdr->id_map_type) {\n>>>> + switch (hdr.id_map_type) {\n>>>> case IPA_CONTROL_ID_MAP_CONTROLS:\n>>>> idMap = &controls::controls;\n>>>> break;\n>>>> @@ -469,12 +469,12 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n>>>> break;\n>>>> default:\n>>>> LOG(Serializer, Error)\n>>>> - << \"Unknown id map type: \" << hdr->id_map_type;\n>>>> + << \"Unknown id map type: \" << hdr.id_map_type;\n>>>> return {};\n>>>> }\n>>>> \n>>>> - ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr));\n>>>> - ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset);\n>>>> + ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr));\n>>>> + ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset);\n>>>> \n>>>> if (buffer.overflow()) {\n>>>> LOG(Serializer, Error) << \"Out of data\";\n>>>> @@ -482,36 +482,35 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n>>>> }\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>>>> + for (unsigned int i = 0; i < hdr.entries; ++i) {\n>>>> + struct ipa_control_info_entry entry;\n>>>> + if (entries.read(&entry) < 0) {\n>>>> LOG(Serializer, Error) << \"Out of data\";\n>>>> return {};\n>>>> }\n>>>> \n>>>> - ControlType type = static_cast(entry->type);\n>>>> + ControlType type = static_cast(entry.type);\n>>>> \n>>>> /* If we're using a local id map, populate it. */\n>>>> if (localIdMap) {\n>>>> ControlId::DirectionFlags flags{\n>>>> - static_cast(entry->direction)\n>>>> + static_cast(entry.direction)\n>>>> };\n>>>> \n>>>> /**\n>>>> * \\todo Find a way to preserve the control name for\n>>>> * debugging purpose.\n>>>> */\n>>>> - controlIds_.emplace_back(std::make_unique(entry->id,\n>>>> + controlIds_.emplace_back(std::make_unique(entry.id,\n>>>> \"\", \"local\", type,\n>>>> flags));\n>>>> - (*localIdMap)[entry->id] = controlIds_.back().get();\n>>>> + (*localIdMap)[entry.id] = controlIds_.back().get();\n>>>> }\n>>>> \n>>>> - const ControlId *controlId = idMap->at(entry->id);\n>>>> + const ControlId *controlId = idMap->at(entry.id);\n>>>> ASSERT(controlId);\n>>>> \n>>>> - if (entry->offset != values.offset()) {\n>>>> + if (entry.offset != values.offset()) {\n>>>> LOG(Serializer, Error)\n>>>> << \"Bad data, entry offset mismatch (entry \"\n>>>> << i << \")\";\n>>>> @@ -526,9 +525,9 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n>>>> * Create the ControlInfoMap in the cache, and store the map to handle\n>>>> * association.\n>>>> */\n>>>> - infoMaps_[hdr->handle] = ControlInfoMap(std::move(ctrls), *idMap);\n>>>> - ControlInfoMap &map = infoMaps_[hdr->handle];\n>>>> - infoMapHandles_[&map] = hdr->handle;\n>>>> + infoMaps_[hdr.handle] = ControlInfoMap(std::move(ctrls), *idMap);\n>>>> + ControlInfoMap &map = infoMaps_[hdr.handle];\n>>>> + infoMapHandles_[&map] = hdr.handle;\n>>>> \n>>>> return map;\n>>>> }\n>>>> @@ -545,21 +544,21 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n>>>> template<>\n>>>> ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer)\n>>>> {\n>>>> - const struct ipa_controls_header *hdr = buffer.read();\n>>>> - if (!hdr) {\n>>>> + struct ipa_controls_header hdr;\n>>>> + if (buffer.read(&hdr) < 0) {\n>>>> LOG(Serializer, Error) << \"Out of data\";\n>>>> return {};\n>>>> }\n>>>> \n>>>> - if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) {\n>>>> + if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) {\n>>>> LOG(Serializer, Error)\n>>>> << \"Unsupported controls format version \"\n>>>> - << hdr->version;\n>>>> + << hdr.version;\n>>>> return {};\n>>>> }\n>>>> \n>>>> - ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr));\n>>>> - ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset);\n>>>> + ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr));\n>>>> + ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset);\n>>>> \n>>>> if (buffer.overflow()) {\n>>>> LOG(Serializer, Error) << \"Out of data\";\n>>>> @@ -576,10 +575,10 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n>>>> * ControlInfoMap is available.\n>>>> */\n>>>> const ControlIdMap *idMap;\n>>>> - if (hdr->handle) {\n>>>> + if (hdr.handle) {\n>>>> auto iter = std::find_if(infoMapHandles_.begin(), infoMapHandles_.end(),\n>>>> [&](decltype(infoMapHandles_)::value_type &entry) {\n>>>> - return entry.second == hdr->handle;\n>>>> + return entry.second == hdr.handle;\n>>>> });\n>>>> if (iter == infoMapHandles_.end()) {\n>>>> LOG(Serializer, Error)\n>>>> @@ -590,7 +589,7 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n>>>> const ControlInfoMap *infoMap = iter->first;\n>>>> idMap = &infoMap->idmap();\n>>>> } else {\n>>>> - switch (hdr->id_map_type) {\n>>>> + switch (hdr.id_map_type) {\n>>>> case IPA_CONTROL_ID_MAP_CONTROLS:\n>>>> idMap = &controls::controls;\n>>>> break;\n>>>> @@ -601,7 +600,7 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n>>>> \n>>>> case IPA_CONTROL_ID_MAP_V4L2:\n>>>> default:\n>>>> - if (hdr->entries > 0)\n>>>> + if (hdr.entries > 0)\n>>>> LOG(Serializer, Fatal)\n>>>> << \"A list of V4L2 controls requires a ControlInfoMap\";\n>>>> \n>>>> @@ -617,23 +616,21 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n>>>> */\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>>>> + for (unsigned int i = 0; i < hdr.entries; ++i) {\n>>>> + struct ipa_control_value_entry entry;\n>>>> + if (entries.read(&entry) < 0) {\n>>>> LOG(Serializer, Error) << \"Out of data\";\n>>>> return {};\n>>>> }\n>>>> \n>>>> - if (entry->offset != values.offset()) {\n>>>> + if (entry.offset != values.offset()) {\n>>>> LOG(Serializer, Error)\n>>>> << \"Bad data, entry offset mismatch (entry \"\n>>>> << i << \")\";\n>>>> return {};\n>>>> }\n>>>> \n>>>> - ctrls.set(entry->id,\n>>>> - loadControlValue(values, entry->is_array, entry->count));\n>>>> + ctrls.set(entry.id, loadControlValue(values, entry.is_array, entry.count));\n>>>> }\n>>>> \n>>>> return ctrls;\n>>>> -- \n>>>> 2.50.1\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 0D709BDCC1\n\tfor ;\n\tMon, 11 Aug 2025 14:28:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 25D1969240;\n\tMon, 11 Aug 2025 16:28:15 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 05C5A69233\n\tfor ;\n\tMon, 11 Aug 2025 16:28:13 +0200 (CEST)","from [192.168.33.20] (185.221.140.182.nat.pool.zt.hu\n\t[185.221.140.182])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A4DA882A;\n\tMon, 11 Aug 2025 16:27:21 +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=\"gfPBWCzP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1754922441;\n\tbh=C5HsU+9aAiVGYYnptCvsKWcvmJ4XJgrm2X/R2T0eqBI=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=gfPBWCzPLkLhaRdvPpi0NZxnFYbDfLYFa34NatZrSm5XBbFSxUw55q83lmo7ynYUz\n\tU3tq9d6t11a9bmV4Xpc4UOyKCLnIk2nOzyt8bEiG11OE3mQ7HZ4dvkT0YPB44Ayn1i\n\tDwya5TRQ2BKIQvwwXLiKk0S7rQSj0PqeIO7TZlwo=","Message-ID":"<3c6c6b31-5af8-4201-923c-d749c4ad6e51@ideasonboard.com>","Date":"Mon, 11 Aug 2025 16:28:10 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v1] libcamera: control_serializer: Do not alias byte\n\tbuffer","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20250811131159.1342981-1-barnabas.pocze@ideasonboard.com>\n\t<175492155549.1641235.14089828007550134576@ping.linuxembedded.co.uk>\n\t<333a28b4-f758-4e57-a13e-bf7f9ee42755@ideasonboard.com>\n\t<175492223974.1641235.4760089293007493036@ping.linuxembedded.co.uk>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<175492223974.1641235.4760089293007493036@ping.linuxembedded.co.uk>","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":35342,"web_url":"https://patchwork.libcamera.org/comment/35342/","msgid":"<20250811143803.GI21313@pendragon.ideasonboard.com>","date":"2025-08-11T14:38:03","subject":"Re: [PATCH v1] libcamera: control_serializer: Do not alias byte\n\tbuffer","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Aug 11, 2025 at 04:28:10PM +0200, Barnabás Pőcze wrote:\n> 2025. 08. 11. 16:23 keltezéssel, Kieran Bingham írta:\n> > Quoting Barnabás Pőcze (2025-08-11 15:18:57)\n> >> 2025. 08. 11. 16:12 keltezéssel, Kieran Bingham írta:\n> >>> Quoting Barnabás Pőcze (2025-08-11 14:11:59)\n> >>>> Aliasing an arbitrary buffer with some `T *` is unlikely to yield good\n> >>>> results, primarily due to potential alignment issues. So don't do it,\n> >>>> instead create the objects on the stack and read into them, this\n> >>>> removes any and all strict aliasing or alignment concerns.\n> >>>>\n> >>>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=221\n> >>>> Signed-off-by: Barnabás Pőcze \n> >>>> ---\n> >>>> src/libcamera/control_serializer.cpp | 75 +++++++++++++---------------\n> >>>> 1 file changed, 36 insertions(+), 39 deletions(-)\n> >>>>\n> >>>> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> >>>> index 050f8512b..722ff231c 100644\n> >>>> --- a/src/libcamera/control_serializer.cpp\n> >>>> +++ b/src/libcamera/control_serializer.cpp\n> >>>> @@ -427,22 +427,22 @@ ControlInfo ControlSerializer::loadControlInfo(ByteStreamBuffer &b)\n> >>>> template<>\n> >>>> ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &buffer)\n> >>>> {\n> >>>> - const struct ipa_controls_header *hdr = buffer.read();\n> >>>> - if (!hdr) {\n> >>>> + struct ipa_controls_header hdr;\n> >>>> + if (buffer.read(&hdr) < 0) {\n> >>>\n> >>> Aha, but the key difference is that before it was 'zero copy' just\n> >>> pointing at the data, while now we're making a copy ?\n> >>\n> >> Yes.\n> >>\n> >>> I wonder if we can detect the alignment and only do the copy if\n> >>> unaligned? Or maybe that's just not worth it and we should just accept\n> >>> the copy - this shouldn't be a high data volume for the most parts...\n> >>\n> >> I feel like the control id/info map/list/value allocation easily dominates the\n> >> cost of these small copies. These are all <=32 bytes.\n> > \n> > Hrm.. indeed - we don't normally expect ControlLists to contain large\n> > data.\n> > \n> > I thnik CCMs with a 3x3 matrix is 'hopefully' the biggest we expect for\n> > the most part and things like lens shading tables shouldn't be\n> > popultated through a control list.\n> \n> This change only affects the headers, e.g. the bytes of the control value\n> data is still read the same way, directly from the buffer.\n> \n> > So ... yes a copy seems quick and trivial...\n> > \n> > I'll put this in already:\n> > \n> > Reviewed-by: Kieran Bingham \n> > \n> > But probably want this also checked by either or both of Laurent and\n> > Paul who know the IPC mechanisms well.\n> > \n> >>> But maybe its something we want to be able to profile/monitor in the\n> >>> future.\n> >>>\n> >>> Or perhaps can we find a way to align data when it goes into the\n> >>> ByteStreamBuffer with some alignment padding sections somehow?\n> >>\n> >> I could be missing something, but given how ipc serialization is done\n> >> at the moment, it does not seem entirely trivial.\n\nWhat's our alignment issue here, what field(s) are unaligned ? The\nheader contains 32-bit fields, are we serializing data in such a way\nthat causes ipa_controls_header instances to not have a 4 bytes\nalignment (as in hdr.size not being a multiple of 4) ? Or is the buffer\nitself not aligned ?\n\n> >>> If fixing/specifying the alignment in the buffer isn't possible easy -\n> >>> then I could certainly imagine accepting this as a current possible solution ...\n> >>>\n> >>>> LOG(Serializer, Error) << \"Out of data\";\n> >>>> return {};\n> >>>> }\n> >>>> \n> >>>> - auto iter = infoMaps_.find(hdr->handle);\n> >>>> + auto iter = infoMaps_.find(hdr.handle);\n> >>>> if (iter != infoMaps_.end()) {\n> >>>> LOG(Serializer, Debug) << \"Use cached ControlInfoMap\";\n> >>>> return iter->second;\n> >>>> }\n> >>>> \n> >>>> - if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) {\n> >>>> + if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) {\n> >>>> LOG(Serializer, Error)\n> >>>> << \"Unsupported controls format version \"\n> >>>> - << hdr->version;\n> >>>> + << hdr.version;\n> >>>> return {};\n> >>>> }\n> >>>> \n> >>>> @@ -455,7 +455,7 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> >>>> */\n> >>>> const ControlIdMap *idMap = nullptr;\n> >>>> ControlIdMap *localIdMap = nullptr;\n> >>>> - switch (hdr->id_map_type) {\n> >>>> + switch (hdr.id_map_type) {\n> >>>> case IPA_CONTROL_ID_MAP_CONTROLS:\n> >>>> idMap = &controls::controls;\n> >>>> break;\n> >>>> @@ -469,12 +469,12 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> >>>> break;\n> >>>> default:\n> >>>> LOG(Serializer, Error)\n> >>>> - << \"Unknown id map type: \" << hdr->id_map_type;\n> >>>> + << \"Unknown id map type: \" << hdr.id_map_type;\n> >>>> return {};\n> >>>> }\n> >>>> \n> >>>> - ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr));\n> >>>> - ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset);\n> >>>> + ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr));\n> >>>> + ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset);\n> >>>> \n> >>>> if (buffer.overflow()) {\n> >>>> LOG(Serializer, Error) << \"Out of data\";\n> >>>> @@ -482,36 +482,35 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> >>>> }\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> >>>> + for (unsigned int i = 0; i < hdr.entries; ++i) {\n> >>>> + struct ipa_control_info_entry entry;\n> >>>> + if (entries.read(&entry) < 0) {\n> >>>> LOG(Serializer, Error) << \"Out of data\";\n> >>>> return {};\n> >>>> }\n> >>>> \n> >>>> - ControlType type = static_cast(entry->type);\n> >>>> + ControlType type = static_cast(entry.type);\n> >>>> \n> >>>> /* If we're using a local id map, populate it. */\n> >>>> if (localIdMap) {\n> >>>> ControlId::DirectionFlags flags{\n> >>>> - static_cast(entry->direction)\n> >>>> + static_cast(entry.direction)\n> >>>> };\n> >>>> \n> >>>> /**\n> >>>> * \\todo Find a way to preserve the control name for\n> >>>> * debugging purpose.\n> >>>> */\n> >>>> - controlIds_.emplace_back(std::make_unique(entry->id,\n> >>>> + controlIds_.emplace_back(std::make_unique(entry.id,\n> >>>> \"\", \"local\", type,\n> >>>> flags));\n> >>>> - (*localIdMap)[entry->id] = controlIds_.back().get();\n> >>>> + (*localIdMap)[entry.id] = controlIds_.back().get();\n> >>>> }\n> >>>> \n> >>>> - const ControlId *controlId = idMap->at(entry->id);\n> >>>> + const ControlId *controlId = idMap->at(entry.id);\n> >>>> ASSERT(controlId);\n> >>>> \n> >>>> - if (entry->offset != values.offset()) {\n> >>>> + if (entry.offset != values.offset()) {\n> >>>> LOG(Serializer, Error)\n> >>>> << \"Bad data, entry offset mismatch (entry \"\n> >>>> << i << \")\";\n> >>>> @@ -526,9 +525,9 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> >>>> * Create the ControlInfoMap in the cache, and store the map to handle\n> >>>> * association.\n> >>>> */\n> >>>> - infoMaps_[hdr->handle] = ControlInfoMap(std::move(ctrls), *idMap);\n> >>>> - ControlInfoMap &map = infoMaps_[hdr->handle];\n> >>>> - infoMapHandles_[&map] = hdr->handle;\n> >>>> + infoMaps_[hdr.handle] = ControlInfoMap(std::move(ctrls), *idMap);\n> >>>> + ControlInfoMap &map = infoMaps_[hdr.handle];\n> >>>> + infoMapHandles_[&map] = hdr.handle;\n> >>>> \n> >>>> return map;\n> >>>> }\n> >>>> @@ -545,21 +544,21 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> >>>> template<>\n> >>>> ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer)\n> >>>> {\n> >>>> - const struct ipa_controls_header *hdr = buffer.read();\n> >>>> - if (!hdr) {\n> >>>> + struct ipa_controls_header hdr;\n> >>>> + if (buffer.read(&hdr) < 0) {\n> >>>> LOG(Serializer, Error) << \"Out of data\";\n> >>>> return {};\n> >>>> }\n> >>>> \n> >>>> - if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) {\n> >>>> + if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) {\n> >>>> LOG(Serializer, Error)\n> >>>> << \"Unsupported controls format version \"\n> >>>> - << hdr->version;\n> >>>> + << hdr.version;\n> >>>> return {};\n> >>>> }\n> >>>> \n> >>>> - ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr));\n> >>>> - ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset);\n> >>>> + ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr));\n> >>>> + ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset);\n> >>>> \n> >>>> if (buffer.overflow()) {\n> >>>> LOG(Serializer, Error) << \"Out of data\";\n> >>>> @@ -576,10 +575,10 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> >>>> * ControlInfoMap is available.\n> >>>> */\n> >>>> const ControlIdMap *idMap;\n> >>>> - if (hdr->handle) {\n> >>>> + if (hdr.handle) {\n> >>>> auto iter = std::find_if(infoMapHandles_.begin(), infoMapHandles_.end(),\n> >>>> [&](decltype(infoMapHandles_)::value_type &entry) {\n> >>>> - return entry.second == hdr->handle;\n> >>>> + return entry.second == hdr.handle;\n> >>>> });\n> >>>> if (iter == infoMapHandles_.end()) {\n> >>>> LOG(Serializer, Error)\n> >>>> @@ -590,7 +589,7 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> >>>> const ControlInfoMap *infoMap = iter->first;\n> >>>> idMap = &infoMap->idmap();\n> >>>> } else {\n> >>>> - switch (hdr->id_map_type) {\n> >>>> + switch (hdr.id_map_type) {\n> >>>> case IPA_CONTROL_ID_MAP_CONTROLS:\n> >>>> idMap = &controls::controls;\n> >>>> break;\n> >>>> @@ -601,7 +600,7 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> >>>> \n> >>>> case IPA_CONTROL_ID_MAP_V4L2:\n> >>>> default:\n> >>>> - if (hdr->entries > 0)\n> >>>> + if (hdr.entries > 0)\n> >>>> LOG(Serializer, Fatal)\n> >>>> << \"A list of V4L2 controls requires a ControlInfoMap\";\n> >>>> \n> >>>> @@ -617,23 +616,21 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> >>>> */\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> >>>> + for (unsigned int i = 0; i < hdr.entries; ++i) {\n> >>>> + struct ipa_control_value_entry entry;\n> >>>> + if (entries.read(&entry) < 0) {\n> >>>> LOG(Serializer, Error) << \"Out of data\";\n> >>>> return {};\n> >>>> }\n> >>>> \n> >>>> - if (entry->offset != values.offset()) {\n> >>>> + if (entry.offset != values.offset()) {\n> >>>> LOG(Serializer, Error)\n> >>>> << \"Bad data, entry offset mismatch (entry \"\n> >>>> << i << \")\";\n> >>>> return {};\n> >>>> }\n> >>>> \n> >>>> - ctrls.set(entry->id,\n> >>>> - loadControlValue(values, entry->is_array, entry->count));\n> >>>> + ctrls.set(entry.id, loadControlValue(values, entry.is_array, entry.count));\n> >>>> }\n> >>>> \n> >>>> return ctrls;","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 CFC80BDCC1\n\tfor ;\n\tMon, 11 Aug 2025 14:38:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 07C3369242;\n\tMon, 11 Aug 2025 16:38:23 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EBF0969233\n\tfor ;\n\tMon, 11 Aug 2025 16:38:21 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id A67F982A;\n\tMon, 11 Aug 2025 16:37:29 +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=\"dWVnpPPY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1754923049;\n\tbh=Ysx+m/XD2Zoq3Ee/S0RvFGjIeSHWP6tTQyDVRsjmVas=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=dWVnpPPYWbXXkF7zT37MFACK8rmlpePIxBwcja2ll+LcuvXeHRIozDhfGr1MbDdB8\n\t0pGhKJCLagaYBQm/kXnEqVK2HhFwFNG9JhQddgeDtKmDeGID5ZLF3W/u5F2IBJPPcB\n\teJQGLvhEQFkRRg/lb3R+gJAEMv/3snqMPM7sy8eo=","Date":"Mon, 11 Aug 2025 17:38:03 +0300","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1] libcamera: control_serializer: Do not alias byte\n\tbuffer","Message-ID":"<20250811143803.GI21313@pendragon.ideasonboard.com>","References":"<20250811131159.1342981-1-barnabas.pocze@ideasonboard.com>\n\t<175492155549.1641235.14089828007550134576@ping.linuxembedded.co.uk>\n\t<333a28b4-f758-4e57-a13e-bf7f9ee42755@ideasonboard.com>\n\t<175492223974.1641235.4760089293007493036@ping.linuxembedded.co.uk>\n\t<3c6c6b31-5af8-4201-923c-d749c4ad6e51@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<3c6c6b31-5af8-4201-923c-d749c4ad6e51@ideasonboard.com>","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":35345,"web_url":"https://patchwork.libcamera.org/comment/35345/","msgid":"","date":"2025-08-11T14:52:45","subject":"Re: [PATCH v1] libcamera: control_serializer: Do not alias byte\n\tbuffer","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"2025. 08. 11. 16:38 keltezéssel, Laurent Pinchart írta:\n> On Mon, Aug 11, 2025 at 04:28:10PM +0200, Barnabás Pőcze wrote:\n>> 2025. 08. 11. 16:23 keltezéssel, Kieran Bingham írta:\n>>> Quoting Barnabás Pőcze (2025-08-11 15:18:57)\n>>>> 2025. 08. 11. 16:12 keltezéssel, Kieran Bingham írta:\n>>>>> Quoting Barnabás Pőcze (2025-08-11 14:11:59)\n>>>>>> Aliasing an arbitrary buffer with some `T *` is unlikely to yield good\n>>>>>> results, primarily due to potential alignment issues. So don't do it,\n>>>>>> instead create the objects on the stack and read into them, this\n>>>>>> removes any and all strict aliasing or alignment concerns.\n>>>>>>\n>>>>>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=221\n>>>>>> Signed-off-by: Barnabás Pőcze \n>>>>>> ---\n>>>>>> src/libcamera/control_serializer.cpp | 75 +++++++++++++---------------\n>>>>>> 1 file changed, 36 insertions(+), 39 deletions(-)\n>>>>>>\n>>>>>> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n>>>>>> index 050f8512b..722ff231c 100644\n>>>>>> --- a/src/libcamera/control_serializer.cpp\n>>>>>> +++ b/src/libcamera/control_serializer.cpp\n>>>>>> @@ -427,22 +427,22 @@ ControlInfo ControlSerializer::loadControlInfo(ByteStreamBuffer &b)\n>>>>>> template<>\n>>>>>> ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &buffer)\n>>>>>> {\n>>>>>> - const struct ipa_controls_header *hdr = buffer.read();\n>>>>>> - if (!hdr) {\n>>>>>> + struct ipa_controls_header hdr;\n>>>>>> + if (buffer.read(&hdr) < 0) {\n>>>>>\n>>>>> Aha, but the key difference is that before it was 'zero copy' just\n>>>>> pointing at the data, while now we're making a copy ?\n>>>>\n>>>> Yes.\n>>>>\n>>>>> I wonder if we can detect the alignment and only do the copy if\n>>>>> unaligned? Or maybe that's just not worth it and we should just accept\n>>>>> the copy - this shouldn't be a high data volume for the most parts...\n>>>>\n>>>> I feel like the control id/info map/list/value allocation easily dominates the\n>>>> cost of these small copies. These are all <=32 bytes.\n>>>\n>>> Hrm.. indeed - we don't normally expect ControlLists to contain large\n>>> data.\n>>>\n>>> I thnik CCMs with a 3x3 matrix is 'hopefully' the biggest we expect for\n>>> the most part and things like lens shading tables shouldn't be\n>>> popultated through a control list.\n>>\n>> This change only affects the headers, e.g. the bytes of the control value\n>> data is still read the same way, directly from the buffer.\n>>\n>>> So ... yes a copy seems quick and trivial...\n>>>\n>>> I'll put this in already:\n>>>\n>>> Reviewed-by: Kieran Bingham \n>>>\n>>> But probably want this also checked by either or both of Laurent and\n>>> Paul who know the IPC mechanisms well.\n>>>\n>>>>> But maybe its something we want to be able to profile/monitor in the\n>>>>> future.\n>>>>>\n>>>>> Or perhaps can we find a way to align data when it goes into the\n>>>>> ByteStreamBuffer with some alignment padding sections somehow?\n>>>>\n>>>> I could be missing something, but given how ipc serialization is done\n>>>> at the moment, it does not seem entirely trivial.\n> \n> What's our alignment issue here, what field(s) are unaligned ? The\n> header contains 32-bit fields, are we serializing data in such a way\n> that causes ipa_controls_header instances to not have a 4 bytes\n> alignment (as in hdr.size not being a multiple of 4) ? Or is the buffer\n> itself not aligned ?\n\nI will turn the question around: what would guarantee the alignment?\nAs far as I can tell, there is nothing. `ControlSerializer` does not guarantee\nany alignment in and of itself (e.g. `ipa_controls_header::size` can easily be\nnot divisible by 4, e.g. `ControlSerializer::binarySize()` of a uint32_t is 5).\nAnd even if it did guarantee alignment, an appropriately sized string serialized\nbefore it can easily throw the final alignment off.\n\n\n> \n>>>>> If fixing/specifying the alignment in the buffer isn't possible easy -\n>>>>> then I could certainly imagine accepting this as a current possible solution ...\n>>>>>\n>>>>>> LOG(Serializer, Error) << \"Out of data\";\n>>>>>> return {};\n>>>>>> }\n>>>>>> \n>>>>>> - auto iter = infoMaps_.find(hdr->handle);\n>>>>>> + auto iter = infoMaps_.find(hdr.handle);\n>>>>>> if (iter != infoMaps_.end()) {\n>>>>>> LOG(Serializer, Debug) << \"Use cached ControlInfoMap\";\n>>>>>> return iter->second;\n>>>>>> }\n>>>>>> \n>>>>>> - if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) {\n>>>>>> + if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) {\n>>>>>> LOG(Serializer, Error)\n>>>>>> << \"Unsupported controls format version \"\n>>>>>> - << hdr->version;\n>>>>>> + << hdr.version;\n>>>>>> return {};\n>>>>>> }\n>>>>>> \n>>>>>> @@ -455,7 +455,7 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n>>>>>> */\n>>>>>> const ControlIdMap *idMap = nullptr;\n>>>>>> ControlIdMap *localIdMap = nullptr;\n>>>>>> - switch (hdr->id_map_type) {\n>>>>>> + switch (hdr.id_map_type) {\n>>>>>> case IPA_CONTROL_ID_MAP_CONTROLS:\n>>>>>> idMap = &controls::controls;\n>>>>>> break;\n>>>>>> @@ -469,12 +469,12 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n>>>>>> break;\n>>>>>> default:\n>>>>>> LOG(Serializer, Error)\n>>>>>> - << \"Unknown id map type: \" << hdr->id_map_type;\n>>>>>> + << \"Unknown id map type: \" << hdr.id_map_type;\n>>>>>> return {};\n>>>>>> }\n>>>>>> \n>>>>>> - ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr));\n>>>>>> - ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset);\n>>>>>> + ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr));\n>>>>>> + ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset);\n>>>>>> \n>>>>>> if (buffer.overflow()) {\n>>>>>> LOG(Serializer, Error) << \"Out of data\";\n>>>>>> @@ -482,36 +482,35 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n>>>>>> }\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>>>>>> + for (unsigned int i = 0; i < hdr.entries; ++i) {\n>>>>>> + struct ipa_control_info_entry entry;\n>>>>>> + if (entries.read(&entry) < 0) {\n>>>>>> LOG(Serializer, Error) << \"Out of data\";\n>>>>>> return {};\n>>>>>> }\n>>>>>> \n>>>>>> - ControlType type = static_cast(entry->type);\n>>>>>> + ControlType type = static_cast(entry.type);\n>>>>>> \n>>>>>> /* If we're using a local id map, populate it. */\n>>>>>> if (localIdMap) {\n>>>>>> ControlId::DirectionFlags flags{\n>>>>>> - static_cast(entry->direction)\n>>>>>> + static_cast(entry.direction)\n>>>>>> };\n>>>>>> \n>>>>>> /**\n>>>>>> * \\todo Find a way to preserve the control name for\n>>>>>> * debugging purpose.\n>>>>>> */\n>>>>>> - controlIds_.emplace_back(std::make_unique(entry->id,\n>>>>>> + controlIds_.emplace_back(std::make_unique(entry.id,\n>>>>>> \"\", \"local\", type,\n>>>>>> flags));\n>>>>>> - (*localIdMap)[entry->id] = controlIds_.back().get();\n>>>>>> + (*localIdMap)[entry.id] = controlIds_.back().get();\n>>>>>> }\n>>>>>> \n>>>>>> - const ControlId *controlId = idMap->at(entry->id);\n>>>>>> + const ControlId *controlId = idMap->at(entry.id);\n>>>>>> ASSERT(controlId);\n>>>>>> \n>>>>>> - if (entry->offset != values.offset()) {\n>>>>>> + if (entry.offset != values.offset()) {\n>>>>>> LOG(Serializer, Error)\n>>>>>> << \"Bad data, entry offset mismatch (entry \"\n>>>>>> << i << \")\";\n>>>>>> @@ -526,9 +525,9 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n>>>>>> * Create the ControlInfoMap in the cache, and store the map to handle\n>>>>>> * association.\n>>>>>> */\n>>>>>> - infoMaps_[hdr->handle] = ControlInfoMap(std::move(ctrls), *idMap);\n>>>>>> - ControlInfoMap &map = infoMaps_[hdr->handle];\n>>>>>> - infoMapHandles_[&map] = hdr->handle;\n>>>>>> + infoMaps_[hdr.handle] = ControlInfoMap(std::move(ctrls), *idMap);\n>>>>>> + ControlInfoMap &map = infoMaps_[hdr.handle];\n>>>>>> + infoMapHandles_[&map] = hdr.handle;\n>>>>>> \n>>>>>> return map;\n>>>>>> }\n>>>>>> @@ -545,21 +544,21 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n>>>>>> template<>\n>>>>>> ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer)\n>>>>>> {\n>>>>>> - const struct ipa_controls_header *hdr = buffer.read();\n>>>>>> - if (!hdr) {\n>>>>>> + struct ipa_controls_header hdr;\n>>>>>> + if (buffer.read(&hdr) < 0) {\n>>>>>> LOG(Serializer, Error) << \"Out of data\";\n>>>>>> return {};\n>>>>>> }\n>>>>>> \n>>>>>> - if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) {\n>>>>>> + if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) {\n>>>>>> LOG(Serializer, Error)\n>>>>>> << \"Unsupported controls format version \"\n>>>>>> - << hdr->version;\n>>>>>> + << hdr.version;\n>>>>>> return {};\n>>>>>> }\n>>>>>> \n>>>>>> - ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr));\n>>>>>> - ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset);\n>>>>>> + ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr));\n>>>>>> + ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset);\n>>>>>> \n>>>>>> if (buffer.overflow()) {\n>>>>>> LOG(Serializer, Error) << \"Out of data\";\n>>>>>> @@ -576,10 +575,10 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n>>>>>> * ControlInfoMap is available.\n>>>>>> */\n>>>>>> const ControlIdMap *idMap;\n>>>>>> - if (hdr->handle) {\n>>>>>> + if (hdr.handle) {\n>>>>>> auto iter = std::find_if(infoMapHandles_.begin(), infoMapHandles_.end(),\n>>>>>> [&](decltype(infoMapHandles_)::value_type &entry) {\n>>>>>> - return entry.second == hdr->handle;\n>>>>>> + return entry.second == hdr.handle;\n>>>>>> });\n>>>>>> if (iter == infoMapHandles_.end()) {\n>>>>>> LOG(Serializer, Error)\n>>>>>> @@ -590,7 +589,7 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n>>>>>> const ControlInfoMap *infoMap = iter->first;\n>>>>>> idMap = &infoMap->idmap();\n>>>>>> } else {\n>>>>>> - switch (hdr->id_map_type) {\n>>>>>> + switch (hdr.id_map_type) {\n>>>>>> case IPA_CONTROL_ID_MAP_CONTROLS:\n>>>>>> idMap = &controls::controls;\n>>>>>> break;\n>>>>>> @@ -601,7 +600,7 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n>>>>>> \n>>>>>> case IPA_CONTROL_ID_MAP_V4L2:\n>>>>>> default:\n>>>>>> - if (hdr->entries > 0)\n>>>>>> + if (hdr.entries > 0)\n>>>>>> LOG(Serializer, Fatal)\n>>>>>> << \"A list of V4L2 controls requires a ControlInfoMap\";\n>>>>>> \n>>>>>> @@ -617,23 +616,21 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n>>>>>> */\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>>>>>> + for (unsigned int i = 0; i < hdr.entries; ++i) {\n>>>>>> + struct ipa_control_value_entry entry;\n>>>>>> + if (entries.read(&entry) < 0) {\n>>>>>> LOG(Serializer, Error) << \"Out of data\";\n>>>>>> return {};\n>>>>>> }\n>>>>>> \n>>>>>> - if (entry->offset != values.offset()) {\n>>>>>> + if (entry.offset != values.offset()) {\n>>>>>> LOG(Serializer, Error)\n>>>>>> << \"Bad data, entry offset mismatch (entry \"\n>>>>>> << i << \")\";\n>>>>>> return {};\n>>>>>> }\n>>>>>> \n>>>>>> - ctrls.set(entry->id,\n>>>>>> - loadControlValue(values, entry->is_array, entry->count));\n>>>>>> + ctrls.set(entry.id, loadControlValue(values, entry.is_array, entry.count));\n>>>>>> }\n>>>>>> \n>>>>>> return ctrls;\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 C42FDBDCC1\n\tfor ;\n\tMon, 11 Aug 2025 14:52:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DC59F6923C;\n\tMon, 11 Aug 2025 16:52:50 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 69FB069233\n\tfor ;\n\tMon, 11 Aug 2025 16:52:49 +0200 (CEST)","from [192.168.33.20] (185.221.140.182.nat.pool.zt.hu\n\t[185.221.140.182])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 115FA446;\n\tMon, 11 Aug 2025 16:51:57 +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=\"jaFPu8cI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1754923917;\n\tbh=Fcc5EVrKOSaATN/0VUph6Qhe9RkfLSC6GX2yht8d1r0=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=jaFPu8cIdXK70S+cynOfLGS/n6gMZu32rPCzYAW7J1rUgDsKUhdM1mzZqn7PH+ugi\n\t8WSV+Nn3rldPGbLB64fd9p/frLDe3ioI3xB/pYNJeXv7aIVPfNbnLm7QUiF+vGPSfa\n\tqslY/G+5D19Cm0X2b3Dh4e+KaqkqmnGswOoZoNEg=","Message-ID":"","Date":"Mon, 11 Aug 2025 16:52:45 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v1] libcamera: control_serializer: Do not alias byte\n\tbuffer","To":"Laurent Pinchart ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20250811131159.1342981-1-barnabas.pocze@ideasonboard.com>\n\t<175492155549.1641235.14089828007550134576@ping.linuxembedded.co.uk>\n\t<333a28b4-f758-4e57-a13e-bf7f9ee42755@ideasonboard.com>\n\t<175492223974.1641235.4760089293007493036@ping.linuxembedded.co.uk>\n\t<3c6c6b31-5af8-4201-923c-d749c4ad6e51@ideasonboard.com>\n\t<20250811143803.GI21313@pendragon.ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20250811143803.GI21313@pendragon.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":35348,"web_url":"https://patchwork.libcamera.org/comment/35348/","msgid":"<20250811152132.GB30054@pendragon.ideasonboard.com>","date":"2025-08-11T15:21:32","subject":"Re: [PATCH v1] libcamera: control_serializer: Do not alias byte\n\tbuffer","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Aug 11, 2025 at 04:52:45PM +0200, Barnabás Pőcze wrote:\n> 2025. 08. 11. 16:38 keltezéssel, Laurent Pinchart írta:\n> > On Mon, Aug 11, 2025 at 04:28:10PM +0200, Barnabás Pőcze wrote:\n> >> 2025. 08. 11. 16:23 keltezéssel, Kieran Bingham írta:\n> >>> Quoting Barnabás Pőcze (2025-08-11 15:18:57)\n> >>>> 2025. 08. 11. 16:12 keltezéssel, Kieran Bingham írta:\n> >>>>> Quoting Barnabás Pőcze (2025-08-11 14:11:59)\n> >>>>>> Aliasing an arbitrary buffer with some `T *` is unlikely to yield good\n> >>>>>> results, primarily due to potential alignment issues. So don't do it,\n> >>>>>> instead create the objects on the stack and read into them, this\n> >>>>>> removes any and all strict aliasing or alignment concerns.\n> >>>>>>\n> >>>>>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=221\n> >>>>>> Signed-off-by: Barnabás Pőcze \n> >>>>>> ---\n> >>>>>> src/libcamera/control_serializer.cpp | 75 +++++++++++++---------------\n> >>>>>> 1 file changed, 36 insertions(+), 39 deletions(-)\n> >>>>>>\n> >>>>>> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> >>>>>> index 050f8512b..722ff231c 100644\n> >>>>>> --- a/src/libcamera/control_serializer.cpp\n> >>>>>> +++ b/src/libcamera/control_serializer.cpp\n> >>>>>> @@ -427,22 +427,22 @@ ControlInfo ControlSerializer::loadControlInfo(ByteStreamBuffer &b)\n> >>>>>> template<>\n> >>>>>> ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &buffer)\n> >>>>>> {\n> >>>>>> - const struct ipa_controls_header *hdr = buffer.read();\n> >>>>>> - if (!hdr) {\n> >>>>>> + struct ipa_controls_header hdr;\n> >>>>>> + if (buffer.read(&hdr) < 0) {\n> >>>>>\n> >>>>> Aha, but the key difference is that before it was 'zero copy' just\n> >>>>> pointing at the data, while now we're making a copy ?\n> >>>>\n> >>>> Yes.\n> >>>>\n> >>>>> I wonder if we can detect the alignment and only do the copy if\n> >>>>> unaligned? Or maybe that's just not worth it and we should just accept\n> >>>>> the copy - this shouldn't be a high data volume for the most parts...\n> >>>>\n> >>>> I feel like the control id/info map/list/value allocation easily dominates the\n> >>>> cost of these small copies. These are all <=32 bytes.\n> >>>\n> >>> Hrm.. indeed - we don't normally expect ControlLists to contain large\n> >>> data.\n> >>>\n> >>> I thnik CCMs with a 3x3 matrix is 'hopefully' the biggest we expect for\n> >>> the most part and things like lens shading tables shouldn't be\n> >>> popultated through a control list.\n> >>\n> >> This change only affects the headers, e.g. the bytes of the control value\n> >> data is still read the same way, directly from the buffer.\n> >>\n> >>> So ... yes a copy seems quick and trivial...\n> >>>\n> >>> I'll put this in already:\n> >>>\n> >>> Reviewed-by: Kieran Bingham \n> >>>\n> >>> But probably want this also checked by either or both of Laurent and\n> >>> Paul who know the IPC mechanisms well.\n> >>>\n> >>>>> But maybe its something we want to be able to profile/monitor in the\n> >>>>> future.\n> >>>>>\n> >>>>> Or perhaps can we find a way to align data when it goes into the\n> >>>>> ByteStreamBuffer with some alignment padding sections somehow?\n> >>>>\n> >>>> I could be missing something, but given how ipc serialization is done\n> >>>> at the moment, it does not seem entirely trivial.\n> > \n> > What's our alignment issue here, what field(s) are unaligned ? The\n> > header contains 32-bit fields, are we serializing data in such a way\n> > that causes ipa_controls_header instances to not have a 4 bytes\n> > alignment (as in hdr.size not being a multiple of 4) ? Or is the buffer\n> > itself not aligned ?\n> \n> I will turn the question around: what would guarantee the alignment?\n> As far as I can tell, there is nothing. `ControlSerializer` does not guarantee\n> any alignment in and of itself (e.g. `ipa_controls_header::size` can easily be\n> not divisible by 4, e.g. `ControlSerializer::binarySize()` of a uint32_t is 5).\n> And even if it did guarantee alignment, an appropriately sized string serialized\n> before it can easily throw the final alignment off.\n\nI thought we were supposed to pad control packets to multiples of 8\nbytes, as described in ipa_controls.cpp. That's what surprises me here.\n\n> >>>>> If fixing/specifying the alignment in the buffer isn't possible easy -\n> >>>>> then I could certainly imagine accepting this as a current possible solution ...\n> >>>>>\n> >>>>>> LOG(Serializer, Error) << \"Out of data\";\n> >>>>>> return {};\n> >>>>>> }\n> >>>>>> \n> >>>>>> - auto iter = infoMaps_.find(hdr->handle);\n> >>>>>> + auto iter = infoMaps_.find(hdr.handle);\n> >>>>>> if (iter != infoMaps_.end()) {\n> >>>>>> LOG(Serializer, Debug) << \"Use cached ControlInfoMap\";\n> >>>>>> return iter->second;\n> >>>>>> }\n> >>>>>> \n> >>>>>> - if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) {\n> >>>>>> + if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) {\n> >>>>>> LOG(Serializer, Error)\n> >>>>>> << \"Unsupported controls format version \"\n> >>>>>> - << hdr->version;\n> >>>>>> + << hdr.version;\n> >>>>>> return {};\n> >>>>>> }\n> >>>>>> \n> >>>>>> @@ -455,7 +455,7 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> >>>>>> */\n> >>>>>> const ControlIdMap *idMap = nullptr;\n> >>>>>> ControlIdMap *localIdMap = nullptr;\n> >>>>>> - switch (hdr->id_map_type) {\n> >>>>>> + switch (hdr.id_map_type) {\n> >>>>>> case IPA_CONTROL_ID_MAP_CONTROLS:\n> >>>>>> idMap = &controls::controls;\n> >>>>>> break;\n> >>>>>> @@ -469,12 +469,12 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> >>>>>> break;\n> >>>>>> default:\n> >>>>>> LOG(Serializer, Error)\n> >>>>>> - << \"Unknown id map type: \" << hdr->id_map_type;\n> >>>>>> + << \"Unknown id map type: \" << hdr.id_map_type;\n> >>>>>> return {};\n> >>>>>> }\n> >>>>>> \n> >>>>>> - ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr));\n> >>>>>> - ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset);\n> >>>>>> + ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr));\n> >>>>>> + ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset);\n> >>>>>> \n> >>>>>> if (buffer.overflow()) {\n> >>>>>> LOG(Serializer, Error) << \"Out of data\";\n> >>>>>> @@ -482,36 +482,35 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> >>>>>> }\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> >>>>>> + for (unsigned int i = 0; i < hdr.entries; ++i) {\n> >>>>>> + struct ipa_control_info_entry entry;\n> >>>>>> + if (entries.read(&entry) < 0) {\n> >>>>>> LOG(Serializer, Error) << \"Out of data\";\n> >>>>>> return {};\n> >>>>>> }\n> >>>>>> \n> >>>>>> - ControlType type = static_cast(entry->type);\n> >>>>>> + ControlType type = static_cast(entry.type);\n> >>>>>> \n> >>>>>> /* If we're using a local id map, populate it. */\n> >>>>>> if (localIdMap) {\n> >>>>>> ControlId::DirectionFlags flags{\n> >>>>>> - static_cast(entry->direction)\n> >>>>>> + static_cast(entry.direction)\n> >>>>>> };\n> >>>>>> \n> >>>>>> /**\n> >>>>>> * \\todo Find a way to preserve the control name for\n> >>>>>> * debugging purpose.\n> >>>>>> */\n> >>>>>> - controlIds_.emplace_back(std::make_unique(entry->id,\n> >>>>>> + controlIds_.emplace_back(std::make_unique(entry.id,\n> >>>>>> \"\", \"local\", type,\n> >>>>>> flags));\n> >>>>>> - (*localIdMap)[entry->id] = controlIds_.back().get();\n> >>>>>> + (*localIdMap)[entry.id] = controlIds_.back().get();\n> >>>>>> }\n> >>>>>> \n> >>>>>> - const ControlId *controlId = idMap->at(entry->id);\n> >>>>>> + const ControlId *controlId = idMap->at(entry.id);\n> >>>>>> ASSERT(controlId);\n> >>>>>> \n> >>>>>> - if (entry->offset != values.offset()) {\n> >>>>>> + if (entry.offset != values.offset()) {\n> >>>>>> LOG(Serializer, Error)\n> >>>>>> << \"Bad data, entry offset mismatch (entry \"\n> >>>>>> << i << \")\";\n> >>>>>> @@ -526,9 +525,9 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> >>>>>> * Create the ControlInfoMap in the cache, and store the map to handle\n> >>>>>> * association.\n> >>>>>> */\n> >>>>>> - infoMaps_[hdr->handle] = ControlInfoMap(std::move(ctrls), *idMap);\n> >>>>>> - ControlInfoMap &map = infoMaps_[hdr->handle];\n> >>>>>> - infoMapHandles_[&map] = hdr->handle;\n> >>>>>> + infoMaps_[hdr.handle] = ControlInfoMap(std::move(ctrls), *idMap);\n> >>>>>> + ControlInfoMap &map = infoMaps_[hdr.handle];\n> >>>>>> + infoMapHandles_[&map] = hdr.handle;\n> >>>>>> \n> >>>>>> return map;\n> >>>>>> }\n> >>>>>> @@ -545,21 +544,21 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> >>>>>> template<>\n> >>>>>> ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer)\n> >>>>>> {\n> >>>>>> - const struct ipa_controls_header *hdr = buffer.read();\n> >>>>>> - if (!hdr) {\n> >>>>>> + struct ipa_controls_header hdr;\n> >>>>>> + if (buffer.read(&hdr) < 0) {\n> >>>>>> LOG(Serializer, Error) << \"Out of data\";\n> >>>>>> return {};\n> >>>>>> }\n> >>>>>> \n> >>>>>> - if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) {\n> >>>>>> + if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) {\n> >>>>>> LOG(Serializer, Error)\n> >>>>>> << \"Unsupported controls format version \"\n> >>>>>> - << hdr->version;\n> >>>>>> + << hdr.version;\n> >>>>>> return {};\n> >>>>>> }\n> >>>>>> \n> >>>>>> - ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr));\n> >>>>>> - ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset);\n> >>>>>> + ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr));\n> >>>>>> + ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset);\n> >>>>>> \n> >>>>>> if (buffer.overflow()) {\n> >>>>>> LOG(Serializer, Error) << \"Out of data\";\n> >>>>>> @@ -576,10 +575,10 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> >>>>>> * ControlInfoMap is available.\n> >>>>>> */\n> >>>>>> const ControlIdMap *idMap;\n> >>>>>> - if (hdr->handle) {\n> >>>>>> + if (hdr.handle) {\n> >>>>>> auto iter = std::find_if(infoMapHandles_.begin(), infoMapHandles_.end(),\n> >>>>>> [&](decltype(infoMapHandles_)::value_type &entry) {\n> >>>>>> - return entry.second == hdr->handle;\n> >>>>>> + return entry.second == hdr.handle;\n> >>>>>> });\n> >>>>>> if (iter == infoMapHandles_.end()) {\n> >>>>>> LOG(Serializer, Error)\n> >>>>>> @@ -590,7 +589,7 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> >>>>>> const ControlInfoMap *infoMap = iter->first;\n> >>>>>> idMap = &infoMap->idmap();\n> >>>>>> } else {\n> >>>>>> - switch (hdr->id_map_type) {\n> >>>>>> + switch (hdr.id_map_type) {\n> >>>>>> case IPA_CONTROL_ID_MAP_CONTROLS:\n> >>>>>> idMap = &controls::controls;\n> >>>>>> break;\n> >>>>>> @@ -601,7 +600,7 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> >>>>>> \n> >>>>>> case IPA_CONTROL_ID_MAP_V4L2:\n> >>>>>> default:\n> >>>>>> - if (hdr->entries > 0)\n> >>>>>> + if (hdr.entries > 0)\n> >>>>>> LOG(Serializer, Fatal)\n> >>>>>> << \"A list of V4L2 controls requires a ControlInfoMap\";\n> >>>>>> \n> >>>>>> @@ -617,23 +616,21 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> >>>>>> */\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> >>>>>> + for (unsigned int i = 0; i < hdr.entries; ++i) {\n> >>>>>> + struct ipa_control_value_entry entry;\n> >>>>>> + if (entries.read(&entry) < 0) {\n> >>>>>> LOG(Serializer, Error) << \"Out of data\";\n> >>>>>> return {};\n> >>>>>> }\n> >>>>>> \n> >>>>>> - if (entry->offset != values.offset()) {\n> >>>>>> + if (entry.offset != values.offset()) {\n> >>>>>> LOG(Serializer, Error)\n> >>>>>> << \"Bad data, entry offset mismatch (entry \"\n> >>>>>> << i << \")\";\n> >>>>>> return {};\n> >>>>>> }\n> >>>>>> \n> >>>>>> - ctrls.set(entry->id,\n> >>>>>> - loadControlValue(values, entry->is_array, entry->count));\n> >>>>>> + ctrls.set(entry.id, loadControlValue(values, entry.is_array, entry.count));\n> >>>>>> }\n> >>>>>> \n> >>>>>> return ctrls;\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 1CC6FBDCC1\n\tfor ;\n\tMon, 11 Aug 2025 15:21:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2E0A46923F;\n\tMon, 11 Aug 2025 17:21:54 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 48E9969233\n\tfor ;\n\tMon, 11 Aug 2025 17:21:52 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id B82EA4A4;\n\tMon, 11 Aug 2025 17:20:59 +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=\"V2D/JG+3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1754925659;\n\tbh=cxywHqaSog67hhnNJAQ8N6BDA5iX2oSHijcbJErxHXI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=V2D/JG+3AC5kwJqSrHMdSU4q79dvRLgW5jWbBIzTy1KsID4c9kkPzARulL4o1uTG8\n\t1YFvQoJN82nTa7/3uAE6AITJiiEFUCsTEDDfAxhCZX3cIWClN1QnDUC8UVVL261YnN\n\tN3nDLX7OTLye4VnSeIrPfrRpS8Aboej3vhmS8Wv0=","Date":"Mon, 11 Aug 2025 18:21:32 +0300","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1] libcamera: control_serializer: Do not alias byte\n\tbuffer","Message-ID":"<20250811152132.GB30054@pendragon.ideasonboard.com>","References":"<20250811131159.1342981-1-barnabas.pocze@ideasonboard.com>\n\t<175492155549.1641235.14089828007550134576@ping.linuxembedded.co.uk>\n\t<333a28b4-f758-4e57-a13e-bf7f9ee42755@ideasonboard.com>\n\t<175492223974.1641235.4760089293007493036@ping.linuxembedded.co.uk>\n\t<3c6c6b31-5af8-4201-923c-d749c4ad6e51@ideasonboard.com>\n\t<20250811143803.GI21313@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","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":35369,"web_url":"https://patchwork.libcamera.org/comment/35369/","msgid":"","date":"2025-08-13T10:32:29","subject":"Re: [PATCH v1] libcamera: control_serializer: Do not alias byte\n\tbuffer","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"2025. 08. 11. 17:21 keltezéssel, Laurent Pinchart írta:\n> On Mon, Aug 11, 2025 at 04:52:45PM +0200, Barnabás Pőcze wrote:\n>> 2025. 08. 11. 16:38 keltezéssel, Laurent Pinchart írta:\n>>> On Mon, Aug 11, 2025 at 04:28:10PM +0200, Barnabás Pőcze wrote:\n>>>> 2025. 08. 11. 16:23 keltezéssel, Kieran Bingham írta:\n>>>>> Quoting Barnabás Pőcze (2025-08-11 15:18:57)\n>>>>>> 2025. 08. 11. 16:12 keltezéssel, Kieran Bingham írta:\n>>>>>>> Quoting Barnabás Pőcze (2025-08-11 14:11:59)\n>>>>>>>> Aliasing an arbitrary buffer with some `T *` is unlikely to yield good\n>>>>>>>> results, primarily due to potential alignment issues. So don't do it,\n>>>>>>>> instead create the objects on the stack and read into them, this\n>>>>>>>> removes any and all strict aliasing or alignment concerns.\n>>>>>>>>\n>>>>>>>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=221\n>>>>>>>> Signed-off-by: Barnabás Pőcze \n>>>>>>>> ---\n>>>>>>>> src/libcamera/control_serializer.cpp | 75 +++++++++++++---------------\n>>>>>>>> 1 file changed, 36 insertions(+), 39 deletions(-)\n>>>>>>>>\n>>>>>>>> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n>>>>>>>> index 050f8512b..722ff231c 100644\n>>>>>>>> --- a/src/libcamera/control_serializer.cpp\n>>>>>>>> +++ b/src/libcamera/control_serializer.cpp\n>>>>>>>> @@ -427,22 +427,22 @@ ControlInfo ControlSerializer::loadControlInfo(ByteStreamBuffer &b)\n>>>>>>>> template<>\n>>>>>>>> ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &buffer)\n>>>>>>>> {\n>>>>>>>> - const struct ipa_controls_header *hdr = buffer.read();\n>>>>>>>> - if (!hdr) {\n>>>>>>>> + struct ipa_controls_header hdr;\n>>>>>>>> + if (buffer.read(&hdr) < 0) {\n>>>>>>>\n>>>>>>> Aha, but the key difference is that before it was 'zero copy' just\n>>>>>>> pointing at the data, while now we're making a copy ?\n>>>>>>\n>>>>>> Yes.\n>>>>>>\n>>>>>>> I wonder if we can detect the alignment and only do the copy if\n>>>>>>> unaligned? Or maybe that's just not worth it and we should just accept\n>>>>>>> the copy - this shouldn't be a high data volume for the most parts...\n>>>>>>\n>>>>>> I feel like the control id/info map/list/value allocation easily dominates the\n>>>>>> cost of these small copies. These are all <=32 bytes.\n>>>>>\n>>>>> Hrm.. indeed - we don't normally expect ControlLists to contain large\n>>>>> data.\n>>>>>\n>>>>> I thnik CCMs with a 3x3 matrix is 'hopefully' the biggest we expect for\n>>>>> the most part and things like lens shading tables shouldn't be\n>>>>> popultated through a control list.\n>>>>\n>>>> This change only affects the headers, e.g. the bytes of the control value\n>>>> data is still read the same way, directly from the buffer.\n>>>>\n>>>>> So ... yes a copy seems quick and trivial...\n>>>>>\n>>>>> I'll put this in already:\n>>>>>\n>>>>> Reviewed-by: Kieran Bingham \n>>>>>\n>>>>> But probably want this also checked by either or both of Laurent and\n>>>>> Paul who know the IPC mechanisms well.\n>>>>>\n>>>>>>> But maybe its something we want to be able to profile/monitor in the\n>>>>>>> future.\n>>>>>>>\n>>>>>>> Or perhaps can we find a way to align data when it goes into the\n>>>>>>> ByteStreamBuffer with some alignment padding sections somehow?\n>>>>>>\n>>>>>> I could be missing something, but given how ipc serialization is done\n>>>>>> at the moment, it does not seem entirely trivial.\n>>>\n>>> What's our alignment issue here, what field(s) are unaligned ? The\n>>> header contains 32-bit fields, are we serializing data in such a way\n>>> that causes ipa_controls_header instances to not have a 4 bytes\n>>> alignment (as in hdr.size not being a multiple of 4) ? Or is the buffer\n>>> itself not aligned ?\n>>\n>> I will turn the question around: what would guarantee the alignment?\n>> As far as I can tell, there is nothing. `ControlSerializer` does not guarantee\n>> any alignment in and of itself (e.g. `ipa_controls_header::size` can easily be\n>> not divisible by 4, e.g. `ControlSerializer::binarySize()` of a uint32_t is 5).\n>> And even if it did guarantee alignment, an appropriately sized string serialized\n>> before it can easily throw the final alignment off.\n> \n> I thought we were supposed to pad control packets to multiples of 8\n> bytes, as described in ipa_controls.cpp. That's what surprises me here.\n\nAhh, that's true. I haven't noticed that. How should we proceed then? I would like to\ngo ahead with this change now for two reasons:\n\n * the way serialization is done does not seem to guarantee sufficient alignment even\n if the control lists, etc. were serialized respecting the alignment requirements;\n * I would like to incorporate https://patchwork.libcamera.org/patch/23373/ which\n changes at the least code appreciably, so I think it would be better to return\n to the question of alignment after that.\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \n>>>>>>> If fixing/specifying the alignment in the buffer isn't possible easy -\n>>>>>>> then I could certainly imagine accepting this as a current possible solution ...\n>>>>>>>\n>>>>>>>> LOG(Serializer, Error) << \"Out of data\";\n>>>>>>>> return {};\n>>>>>>>> }\n>>>>>>>> \n>>>>>>>> - auto iter = infoMaps_.find(hdr->handle);\n>>>>>>>> + auto iter = infoMaps_.find(hdr.handle);\n>>>>>>>> if (iter != infoMaps_.end()) {\n>>>>>>>> LOG(Serializer, Debug) << \"Use cached ControlInfoMap\";\n>>>>>>>> return iter->second;\n>>>>>>>> }\n>>>>>>>> \n>>>>>>>> - if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) {\n>>>>>>>> + if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) {\n>>>>>>>> LOG(Serializer, Error)\n>>>>>>>> << \"Unsupported controls format version \"\n>>>>>>>> - << hdr->version;\n>>>>>>>> + << hdr.version;\n>>>>>>>> return {};\n>>>>>>>> }\n>>>>>>>> \n>>>>>>>> @@ -455,7 +455,7 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n>>>>>>>> */\n>>>>>>>> const ControlIdMap *idMap = nullptr;\n>>>>>>>> ControlIdMap *localIdMap = nullptr;\n>>>>>>>> - switch (hdr->id_map_type) {\n>>>>>>>> + switch (hdr.id_map_type) {\n>>>>>>>> case IPA_CONTROL_ID_MAP_CONTROLS:\n>>>>>>>> idMap = &controls::controls;\n>>>>>>>> break;\n>>>>>>>> @@ -469,12 +469,12 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n>>>>>>>> break;\n>>>>>>>> default:\n>>>>>>>> LOG(Serializer, Error)\n>>>>>>>> - << \"Unknown id map type: \" << hdr->id_map_type;\n>>>>>>>> + << \"Unknown id map type: \" << hdr.id_map_type;\n>>>>>>>> return {};\n>>>>>>>> }\n>>>>>>>> \n>>>>>>>> - ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr));\n>>>>>>>> - ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset);\n>>>>>>>> + ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr));\n>>>>>>>> + ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset);\n>>>>>>>> \n>>>>>>>> if (buffer.overflow()) {\n>>>>>>>> LOG(Serializer, Error) << \"Out of data\";\n>>>>>>>> @@ -482,36 +482,35 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n>>>>>>>> }\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>>>>>>>> + for (unsigned int i = 0; i < hdr.entries; ++i) {\n>>>>>>>> + struct ipa_control_info_entry entry;\n>>>>>>>> + if (entries.read(&entry) < 0) {\n>>>>>>>> LOG(Serializer, Error) << \"Out of data\";\n>>>>>>>> return {};\n>>>>>>>> }\n>>>>>>>> \n>>>>>>>> - ControlType type = static_cast(entry->type);\n>>>>>>>> + ControlType type = static_cast(entry.type);\n>>>>>>>> \n>>>>>>>> /* If we're using a local id map, populate it. */\n>>>>>>>> if (localIdMap) {\n>>>>>>>> ControlId::DirectionFlags flags{\n>>>>>>>> - static_cast(entry->direction)\n>>>>>>>> + static_cast(entry.direction)\n>>>>>>>> };\n>>>>>>>> \n>>>>>>>> /**\n>>>>>>>> * \\todo Find a way to preserve the control name for\n>>>>>>>> * debugging purpose.\n>>>>>>>> */\n>>>>>>>> - controlIds_.emplace_back(std::make_unique(entry->id,\n>>>>>>>> + controlIds_.emplace_back(std::make_unique(entry.id,\n>>>>>>>> \"\", \"local\", type,\n>>>>>>>> flags));\n>>>>>>>> - (*localIdMap)[entry->id] = controlIds_.back().get();\n>>>>>>>> + (*localIdMap)[entry.id] = controlIds_.back().get();\n>>>>>>>> }\n>>>>>>>> \n>>>>>>>> - const ControlId *controlId = idMap->at(entry->id);\n>>>>>>>> + const ControlId *controlId = idMap->at(entry.id);\n>>>>>>>> ASSERT(controlId);\n>>>>>>>> \n>>>>>>>> - if (entry->offset != values.offset()) {\n>>>>>>>> + if (entry.offset != values.offset()) {\n>>>>>>>> LOG(Serializer, Error)\n>>>>>>>> << \"Bad data, entry offset mismatch (entry \"\n>>>>>>>> << i << \")\";\n>>>>>>>> @@ -526,9 +525,9 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n>>>>>>>> * Create the ControlInfoMap in the cache, and store the map to handle\n>>>>>>>> * association.\n>>>>>>>> */\n>>>>>>>> - infoMaps_[hdr->handle] = ControlInfoMap(std::move(ctrls), *idMap);\n>>>>>>>> - ControlInfoMap &map = infoMaps_[hdr->handle];\n>>>>>>>> - infoMapHandles_[&map] = hdr->handle;\n>>>>>>>> + infoMaps_[hdr.handle] = ControlInfoMap(std::move(ctrls), *idMap);\n>>>>>>>> + ControlInfoMap &map = infoMaps_[hdr.handle];\n>>>>>>>> + infoMapHandles_[&map] = hdr.handle;\n>>>>>>>> \n>>>>>>>> return map;\n>>>>>>>> }\n>>>>>>>> @@ -545,21 +544,21 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n>>>>>>>> template<>\n>>>>>>>> ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer)\n>>>>>>>> {\n>>>>>>>> - const struct ipa_controls_header *hdr = buffer.read();\n>>>>>>>> - if (!hdr) {\n>>>>>>>> + struct ipa_controls_header hdr;\n>>>>>>>> + if (buffer.read(&hdr) < 0) {\n>>>>>>>> LOG(Serializer, Error) << \"Out of data\";\n>>>>>>>> return {};\n>>>>>>>> }\n>>>>>>>> \n>>>>>>>> - if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) {\n>>>>>>>> + if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) {\n>>>>>>>> LOG(Serializer, Error)\n>>>>>>>> << \"Unsupported controls format version \"\n>>>>>>>> - << hdr->version;\n>>>>>>>> + << hdr.version;\n>>>>>>>> return {};\n>>>>>>>> }\n>>>>>>>> \n>>>>>>>> - ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr));\n>>>>>>>> - ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset);\n>>>>>>>> + ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr));\n>>>>>>>> + ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset);\n>>>>>>>> \n>>>>>>>> if (buffer.overflow()) {\n>>>>>>>> LOG(Serializer, Error) << \"Out of data\";\n>>>>>>>> @@ -576,10 +575,10 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n>>>>>>>> * ControlInfoMap is available.\n>>>>>>>> */\n>>>>>>>> const ControlIdMap *idMap;\n>>>>>>>> - if (hdr->handle) {\n>>>>>>>> + if (hdr.handle) {\n>>>>>>>> auto iter = std::find_if(infoMapHandles_.begin(), infoMapHandles_.end(),\n>>>>>>>> [&](decltype(infoMapHandles_)::value_type &entry) {\n>>>>>>>> - return entry.second == hdr->handle;\n>>>>>>>> + return entry.second == hdr.handle;\n>>>>>>>> });\n>>>>>>>> if (iter == infoMapHandles_.end()) {\n>>>>>>>> LOG(Serializer, Error)\n>>>>>>>> @@ -590,7 +589,7 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n>>>>>>>> const ControlInfoMap *infoMap = iter->first;\n>>>>>>>> idMap = &infoMap->idmap();\n>>>>>>>> } else {\n>>>>>>>> - switch (hdr->id_map_type) {\n>>>>>>>> + switch (hdr.id_map_type) {\n>>>>>>>> case IPA_CONTROL_ID_MAP_CONTROLS:\n>>>>>>>> idMap = &controls::controls;\n>>>>>>>> break;\n>>>>>>>> @@ -601,7 +600,7 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n>>>>>>>> \n>>>>>>>> case IPA_CONTROL_ID_MAP_V4L2:\n>>>>>>>> default:\n>>>>>>>> - if (hdr->entries > 0)\n>>>>>>>> + if (hdr.entries > 0)\n>>>>>>>> LOG(Serializer, Fatal)\n>>>>>>>> << \"A list of V4L2 controls requires a ControlInfoMap\";\n>>>>>>>> \n>>>>>>>> @@ -617,23 +616,21 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n>>>>>>>> */\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>>>>>>>> + for (unsigned int i = 0; i < hdr.entries; ++i) {\n>>>>>>>> + struct ipa_control_value_entry entry;\n>>>>>>>> + if (entries.read(&entry) < 0) {\n>>>>>>>> LOG(Serializer, Error) << \"Out of data\";\n>>>>>>>> return {};\n>>>>>>>> }\n>>>>>>>> \n>>>>>>>> - if (entry->offset != values.offset()) {\n>>>>>>>> + if (entry.offset != values.offset()) {\n>>>>>>>> LOG(Serializer, Error)\n>>>>>>>> << \"Bad data, entry offset mismatch (entry \"\n>>>>>>>> << i << \")\";\n>>>>>>>> return {};\n>>>>>>>> }\n>>>>>>>> \n>>>>>>>> - ctrls.set(entry->id,\n>>>>>>>> - loadControlValue(values, entry->is_array, entry->count));\n>>>>>>>> + ctrls.set(entry.id, loadControlValue(values, entry.is_array, entry.count));\n>>>>>>>> }\n>>>>>>>> \n>>>>>>>> return ctrls;\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 5A768BDCC1\n\tfor ;\n\tWed, 13 Aug 2025 10:32:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 82E0F6924D;\n\tWed, 13 Aug 2025 12:32:33 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E6BE269249\n\tfor ;\n\tWed, 13 Aug 2025 12:32:31 +0200 (CEST)","from [192.168.33.21] (185.221.141.188.nat.pool.zt.hu\n\t[185.221.141.188])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2FCF92EC;\n\tWed, 13 Aug 2025 12:31:38 +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=\"nh+rhCVl\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1755081098;\n\tbh=J0RUvLiyfcGK5t+vApEW7hhDIta7ZXmUyTnGIkGt0/4=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=nh+rhCVln2yga5dNQtzKB7aTu0ibRQSCdG/EKWjQLYoWownP7Z+IHX+6C68m5Qrs3\n\tu6QdVbIzxFwfMFP2r5YHq5IQcAhskLMUNdYrY4Fz9eAOvDzqAszlepYwj4Ik8CEWRQ\n\t5CkabouxyVMbJKRRQuTqILZ3vZylBATeaYyMBAhE=","Message-ID":"","Date":"Wed, 13 Aug 2025 12:32:29 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v1] libcamera: control_serializer: Do not alias byte\n\tbuffer","To":"Laurent Pinchart ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20250811131159.1342981-1-barnabas.pocze@ideasonboard.com>\n\t<175492155549.1641235.14089828007550134576@ping.linuxembedded.co.uk>\n\t<333a28b4-f758-4e57-a13e-bf7f9ee42755@ideasonboard.com>\n\t<175492223974.1641235.4760089293007493036@ping.linuxembedded.co.uk>\n\t<3c6c6b31-5af8-4201-923c-d749c4ad6e51@ideasonboard.com>\n\t<20250811143803.GI21313@pendragon.ideasonboard.com>\n\t\n\t<20250811152132.GB30054@pendragon.ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20250811152132.GB30054@pendragon.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":35371,"web_url":"https://patchwork.libcamera.org/comment/35371/","msgid":"<20250813103825.GB6440@pendragon.ideasonboard.com>","date":"2025-08-13T10:38:25","subject":"Re: [PATCH v1] libcamera: control_serializer: Do not alias byte\n\tbuffer","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Aug 13, 2025 at 12:32:29PM +0200, Barnabás Pőcze wrote:\n> 2025. 08. 11. 17:21 keltezéssel, Laurent Pinchart írta:\n> > On Mon, Aug 11, 2025 at 04:52:45PM +0200, Barnabás Pőcze wrote:\n> >> 2025. 08. 11. 16:38 keltezéssel, Laurent Pinchart írta:\n> >>> On Mon, Aug 11, 2025 at 04:28:10PM +0200, Barnabás Pőcze wrote:\n> >>>> 2025. 08. 11. 16:23 keltezéssel, Kieran Bingham írta:\n> >>>>> Quoting Barnabás Pőcze (2025-08-11 15:18:57)\n> >>>>>> 2025. 08. 11. 16:12 keltezéssel, Kieran Bingham írta:\n> >>>>>>> Quoting Barnabás Pőcze (2025-08-11 14:11:59)\n> >>>>>>>> Aliasing an arbitrary buffer with some `T *` is unlikely to yield good\n> >>>>>>>> results, primarily due to potential alignment issues. So don't do it,\n> >>>>>>>> instead create the objects on the stack and read into them, this\n> >>>>>>>> removes any and all strict aliasing or alignment concerns.\n> >>>>>>>>\n> >>>>>>>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=221\n> >>>>>>>> Signed-off-by: Barnabás Pőcze \n> >>>>>>>> ---\n> >>>>>>>> src/libcamera/control_serializer.cpp | 75 +++++++++++++---------------\n> >>>>>>>> 1 file changed, 36 insertions(+), 39 deletions(-)\n> >>>>>>>>\n> >>>>>>>> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> >>>>>>>> index 050f8512b..722ff231c 100644\n> >>>>>>>> --- a/src/libcamera/control_serializer.cpp\n> >>>>>>>> +++ b/src/libcamera/control_serializer.cpp\n> >>>>>>>> @@ -427,22 +427,22 @@ ControlInfo ControlSerializer::loadControlInfo(ByteStreamBuffer &b)\n> >>>>>>>> template<>\n> >>>>>>>> ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &buffer)\n> >>>>>>>> {\n> >>>>>>>> - const struct ipa_controls_header *hdr = buffer.read();\n> >>>>>>>> - if (!hdr) {\n> >>>>>>>> + struct ipa_controls_header hdr;\n> >>>>>>>> + if (buffer.read(&hdr) < 0) {\n> >>>>>>>\n> >>>>>>> Aha, but the key difference is that before it was 'zero copy' just\n> >>>>>>> pointing at the data, while now we're making a copy ?\n> >>>>>>\n> >>>>>> Yes.\n> >>>>>>\n> >>>>>>> I wonder if we can detect the alignment and only do the copy if\n> >>>>>>> unaligned? Or maybe that's just not worth it and we should just accept\n> >>>>>>> the copy - this shouldn't be a high data volume for the most parts...\n> >>>>>>\n> >>>>>> I feel like the control id/info map/list/value allocation easily dominates the\n> >>>>>> cost of these small copies. These are all <=32 bytes.\n> >>>>>\n> >>>>> Hrm.. indeed - we don't normally expect ControlLists to contain large\n> >>>>> data.\n> >>>>>\n> >>>>> I thnik CCMs with a 3x3 matrix is 'hopefully' the biggest we expect for\n> >>>>> the most part and things like lens shading tables shouldn't be\n> >>>>> popultated through a control list.\n> >>>>\n> >>>> This change only affects the headers, e.g. the bytes of the control value\n> >>>> data is still read the same way, directly from the buffer.\n> >>>>\n> >>>>> So ... yes a copy seems quick and trivial...\n> >>>>>\n> >>>>> I'll put this in already:\n> >>>>>\n> >>>>> Reviewed-by: Kieran Bingham \n> >>>>>\n> >>>>> But probably want this also checked by either or both of Laurent and\n> >>>>> Paul who know the IPC mechanisms well.\n> >>>>>\n> >>>>>>> But maybe its something we want to be able to profile/monitor in the\n> >>>>>>> future.\n> >>>>>>>\n> >>>>>>> Or perhaps can we find a way to align data when it goes into the\n> >>>>>>> ByteStreamBuffer with some alignment padding sections somehow?\n> >>>>>>\n> >>>>>> I could be missing something, but given how ipc serialization is done\n> >>>>>> at the moment, it does not seem entirely trivial.\n> >>>\n> >>> What's our alignment issue here, what field(s) are unaligned ? The\n> >>> header contains 32-bit fields, are we serializing data in such a way\n> >>> that causes ipa_controls_header instances to not have a 4 bytes\n> >>> alignment (as in hdr.size not being a multiple of 4) ? Or is the buffer\n> >>> itself not aligned ?\n> >>\n> >> I will turn the question around: what would guarantee the alignment?\n> >> As far as I can tell, there is nothing. `ControlSerializer` does not guarantee\n> >> any alignment in and of itself (e.g. `ipa_controls_header::size` can easily be\n> >> not divisible by 4, e.g. `ControlSerializer::binarySize()` of a uint32_t is 5).\n> >> And even if it did guarantee alignment, an appropriately sized string serialized\n> >> before it can easily throw the final alignment off.\n> > \n> > I thought we were supposed to pad control packets to multiples of 8\n> > bytes, as described in ipa_controls.cpp. That's what surprises me here.\n> \n> Ahh, that's true. I haven't noticed that. How should we proceed then? I would like to\n> go ahead with this change now for two reasons:\n> \n> * the way serialization is done does not seem to guarantee sufficient alignment even\n> if the control lists, etc. were serialized respecting the alignment requirements;\n\nDoes that mean there's a bug in the serialization code that we should\nfix ?\n\n> * I would like to incorporate https://patchwork.libcamera.org/patch/23373/ which\n> changes at the least code appreciably, so I think it would be better to return\n> to the question of alignment after that.\n\nI agree that this patch will not significantly hinder performance as it\nonly copies the headers. However, with the documented alignment, this\ncopy shouldn't be needed, and avoiding a copy would still be best. Are\nyou proposing merging this patch and https://patchwork.libcamera.org/patch/23373/,\nand then fix the alignment problem and remove the copy on top ?\n\n> >>>>>>> If fixing/specifying the alignment in the buffer isn't possible easy -\n> >>>>>>> then I could certainly imagine accepting this as a current possible solution ...\n> >>>>>>>\n> >>>>>>>> LOG(Serializer, Error) << \"Out of data\";\n> >>>>>>>> return {};\n> >>>>>>>> }\n> >>>>>>>> \n> >>>>>>>> - auto iter = infoMaps_.find(hdr->handle);\n> >>>>>>>> + auto iter = infoMaps_.find(hdr.handle);\n> >>>>>>>> if (iter != infoMaps_.end()) {\n> >>>>>>>> LOG(Serializer, Debug) << \"Use cached ControlInfoMap\";\n> >>>>>>>> return iter->second;\n> >>>>>>>> }\n> >>>>>>>> \n> >>>>>>>> - if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) {\n> >>>>>>>> + if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) {\n> >>>>>>>> LOG(Serializer, Error)\n> >>>>>>>> << \"Unsupported controls format version \"\n> >>>>>>>> - << hdr->version;\n> >>>>>>>> + << hdr.version;\n> >>>>>>>> return {};\n> >>>>>>>> }\n> >>>>>>>> \n> >>>>>>>> @@ -455,7 +455,7 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> >>>>>>>> */\n> >>>>>>>> const ControlIdMap *idMap = nullptr;\n> >>>>>>>> ControlIdMap *localIdMap = nullptr;\n> >>>>>>>> - switch (hdr->id_map_type) {\n> >>>>>>>> + switch (hdr.id_map_type) {\n> >>>>>>>> case IPA_CONTROL_ID_MAP_CONTROLS:\n> >>>>>>>> idMap = &controls::controls;\n> >>>>>>>> break;\n> >>>>>>>> @@ -469,12 +469,12 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> >>>>>>>> break;\n> >>>>>>>> default:\n> >>>>>>>> LOG(Serializer, Error)\n> >>>>>>>> - << \"Unknown id map type: \" << hdr->id_map_type;\n> >>>>>>>> + << \"Unknown id map type: \" << hdr.id_map_type;\n> >>>>>>>> return {};\n> >>>>>>>> }\n> >>>>>>>> \n> >>>>>>>> - ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr));\n> >>>>>>>> - ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset);\n> >>>>>>>> + ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr));\n> >>>>>>>> + ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset);\n> >>>>>>>> \n> >>>>>>>> if (buffer.overflow()) {\n> >>>>>>>> LOG(Serializer, Error) << \"Out of data\";\n> >>>>>>>> @@ -482,36 +482,35 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> >>>>>>>> }\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> >>>>>>>> + for (unsigned int i = 0; i < hdr.entries; ++i) {\n> >>>>>>>> + struct ipa_control_info_entry entry;\n> >>>>>>>> + if (entries.read(&entry) < 0) {\n> >>>>>>>> LOG(Serializer, Error) << \"Out of data\";\n> >>>>>>>> return {};\n> >>>>>>>> }\n> >>>>>>>> \n> >>>>>>>> - ControlType type = static_cast(entry->type);\n> >>>>>>>> + ControlType type = static_cast(entry.type);\n> >>>>>>>> \n> >>>>>>>> /* If we're using a local id map, populate it. */\n> >>>>>>>> if (localIdMap) {\n> >>>>>>>> ControlId::DirectionFlags flags{\n> >>>>>>>> - static_cast(entry->direction)\n> >>>>>>>> + static_cast(entry.direction)\n> >>>>>>>> };\n> >>>>>>>> \n> >>>>>>>> /**\n> >>>>>>>> * \\todo Find a way to preserve the control name for\n> >>>>>>>> * debugging purpose.\n> >>>>>>>> */\n> >>>>>>>> - controlIds_.emplace_back(std::make_unique(entry->id,\n> >>>>>>>> + controlIds_.emplace_back(std::make_unique(entry.id,\n> >>>>>>>> \"\", \"local\", type,\n> >>>>>>>> flags));\n> >>>>>>>> - (*localIdMap)[entry->id] = controlIds_.back().get();\n> >>>>>>>> + (*localIdMap)[entry.id] = controlIds_.back().get();\n> >>>>>>>> }\n> >>>>>>>> \n> >>>>>>>> - const ControlId *controlId = idMap->at(entry->id);\n> >>>>>>>> + const ControlId *controlId = idMap->at(entry.id);\n> >>>>>>>> ASSERT(controlId);\n> >>>>>>>> \n> >>>>>>>> - if (entry->offset != values.offset()) {\n> >>>>>>>> + if (entry.offset != values.offset()) {\n> >>>>>>>> LOG(Serializer, Error)\n> >>>>>>>> << \"Bad data, entry offset mismatch (entry \"\n> >>>>>>>> << i << \")\";\n> >>>>>>>> @@ -526,9 +525,9 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> >>>>>>>> * Create the ControlInfoMap in the cache, and store the map to handle\n> >>>>>>>> * association.\n> >>>>>>>> */\n> >>>>>>>> - infoMaps_[hdr->handle] = ControlInfoMap(std::move(ctrls), *idMap);\n> >>>>>>>> - ControlInfoMap &map = infoMaps_[hdr->handle];\n> >>>>>>>> - infoMapHandles_[&map] = hdr->handle;\n> >>>>>>>> + infoMaps_[hdr.handle] = ControlInfoMap(std::move(ctrls), *idMap);\n> >>>>>>>> + ControlInfoMap &map = infoMaps_[hdr.handle];\n> >>>>>>>> + infoMapHandles_[&map] = hdr.handle;\n> >>>>>>>> \n> >>>>>>>> return map;\n> >>>>>>>> }\n> >>>>>>>> @@ -545,21 +544,21 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> >>>>>>>> template<>\n> >>>>>>>> ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer)\n> >>>>>>>> {\n> >>>>>>>> - const struct ipa_controls_header *hdr = buffer.read();\n> >>>>>>>> - if (!hdr) {\n> >>>>>>>> + struct ipa_controls_header hdr;\n> >>>>>>>> + if (buffer.read(&hdr) < 0) {\n> >>>>>>>> LOG(Serializer, Error) << \"Out of data\";\n> >>>>>>>> return {};\n> >>>>>>>> }\n> >>>>>>>> \n> >>>>>>>> - if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) {\n> >>>>>>>> + if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) {\n> >>>>>>>> LOG(Serializer, Error)\n> >>>>>>>> << \"Unsupported controls format version \"\n> >>>>>>>> - << hdr->version;\n> >>>>>>>> + << hdr.version;\n> >>>>>>>> return {};\n> >>>>>>>> }\n> >>>>>>>> \n> >>>>>>>> - ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr));\n> >>>>>>>> - ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset);\n> >>>>>>>> + ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr));\n> >>>>>>>> + ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset);\n> >>>>>>>> \n> >>>>>>>> if (buffer.overflow()) {\n> >>>>>>>> LOG(Serializer, Error) << \"Out of data\";\n> >>>>>>>> @@ -576,10 +575,10 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> >>>>>>>> * ControlInfoMap is available.\n> >>>>>>>> */\n> >>>>>>>> const ControlIdMap *idMap;\n> >>>>>>>> - if (hdr->handle) {\n> >>>>>>>> + if (hdr.handle) {\n> >>>>>>>> auto iter = std::find_if(infoMapHandles_.begin(), infoMapHandles_.end(),\n> >>>>>>>> [&](decltype(infoMapHandles_)::value_type &entry) {\n> >>>>>>>> - return entry.second == hdr->handle;\n> >>>>>>>> + return entry.second == hdr.handle;\n> >>>>>>>> });\n> >>>>>>>> if (iter == infoMapHandles_.end()) {\n> >>>>>>>> LOG(Serializer, Error)\n> >>>>>>>> @@ -590,7 +589,7 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> >>>>>>>> const ControlInfoMap *infoMap = iter->first;\n> >>>>>>>> idMap = &infoMap->idmap();\n> >>>>>>>> } else {\n> >>>>>>>> - switch (hdr->id_map_type) {\n> >>>>>>>> + switch (hdr.id_map_type) {\n> >>>>>>>> case IPA_CONTROL_ID_MAP_CONTROLS:\n> >>>>>>>> idMap = &controls::controls;\n> >>>>>>>> break;\n> >>>>>>>> @@ -601,7 +600,7 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> >>>>>>>> \n> >>>>>>>> case IPA_CONTROL_ID_MAP_V4L2:\n> >>>>>>>> default:\n> >>>>>>>> - if (hdr->entries > 0)\n> >>>>>>>> + if (hdr.entries > 0)\n> >>>>>>>> LOG(Serializer, Fatal)\n> >>>>>>>> << \"A list of V4L2 controls requires a ControlInfoMap\";\n> >>>>>>>> \n> >>>>>>>> @@ -617,23 +616,21 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> >>>>>>>> */\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> >>>>>>>> + for (unsigned int i = 0; i < hdr.entries; ++i) {\n> >>>>>>>> + struct ipa_control_value_entry entry;\n> >>>>>>>> + if (entries.read(&entry) < 0) {\n> >>>>>>>> LOG(Serializer, Error) << \"Out of data\";\n> >>>>>>>> return {};\n> >>>>>>>> }\n> >>>>>>>> \n> >>>>>>>> - if (entry->offset != values.offset()) {\n> >>>>>>>> + if (entry.offset != values.offset()) {\n> >>>>>>>> LOG(Serializer, Error)\n> >>>>>>>> << \"Bad data, entry offset mismatch (entry \"\n> >>>>>>>> << i << \")\";\n> >>>>>>>> return {};\n> >>>>>>>> }\n> >>>>>>>> \n> >>>>>>>> - ctrls.set(entry->id,\n> >>>>>>>> - loadControlValue(values, entry->is_array, entry->count));\n> >>>>>>>> + ctrls.set(entry.id, loadControlValue(values, entry.is_array, entry.count));\n> >>>>>>>> }\n> >>>>>>>> \n> >>>>>>>> return ctrls;","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 9CD91BEFBE\n\tfor ;\n\tWed, 13 Aug 2025 10:38:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CB47B69249;\n\tWed, 13 Aug 2025 12: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 37D2269249\n\tfor ;\n\tWed, 13 Aug 2025 12:38:44 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 8723B2EC;\n\tWed, 13 Aug 2025 12:37:50 +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=\"VkT0TLaL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1755081470;\n\tbh=oDkwQKGZLpKN7I9azd/6MmOmQ0oSi9fnwnGzMaEiLME=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=VkT0TLaLpTXSWP2rqcO8T5rT1XpS0+URjQ8Y48r3TmcG4wBwNH7zYoKjhTmAMqYp2\n\t9ikpS356snc02a6AfUE+O2/jWWSmtten/+IwlV2KR+U8Tc/qf8IvYu5/pHfW0gARpd\n\tMCF13n/fsMNbSpe7IYSzzSg2EDq7VubEjj4dJ91U=","Date":"Wed, 13 Aug 2025 13:38:25 +0300","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1] libcamera: control_serializer: Do not alias byte\n\tbuffer","Message-ID":"<20250813103825.GB6440@pendragon.ideasonboard.com>","References":"<20250811131159.1342981-1-barnabas.pocze@ideasonboard.com>\n\t<175492155549.1641235.14089828007550134576@ping.linuxembedded.co.uk>\n\t<333a28b4-f758-4e57-a13e-bf7f9ee42755@ideasonboard.com>\n\t<175492223974.1641235.4760089293007493036@ping.linuxembedded.co.uk>\n\t<3c6c6b31-5af8-4201-923c-d749c4ad6e51@ideasonboard.com>\n\t<20250811143803.GI21313@pendragon.ideasonboard.com>\n\t\n\t<20250811152132.GB30054@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","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":35374,"web_url":"https://patchwork.libcamera.org/comment/35374/","msgid":"<72575c14-ac7a-4cef-a6e7-647b717c40d7@ideasonboard.com>","date":"2025-08-13T10:48:20","subject":"Re: [PATCH v1] libcamera: control_serializer: Do not alias byte\n\tbuffer","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"2025. 08. 13. 12:38 keltezéssel, Laurent Pinchart írta:\n> On Wed, Aug 13, 2025 at 12:32:29PM +0200, Barnabás Pőcze wrote:\n>> 2025. 08. 11. 17:21 keltezéssel, Laurent Pinchart írta:\n>>> On Mon, Aug 11, 2025 at 04:52:45PM +0200, Barnabás Pőcze wrote:\n>>>> 2025. 08. 11. 16:38 keltezéssel, Laurent Pinchart írta:\n>>>>> On Mon, Aug 11, 2025 at 04:28:10PM +0200, Barnabás Pőcze wrote:\n>>>>>> 2025. 08. 11. 16:23 keltezéssel, Kieran Bingham írta:\n>>>>>>> Quoting Barnabás Pőcze (2025-08-11 15:18:57)\n>>>>>>>> 2025. 08. 11. 16:12 keltezéssel, Kieran Bingham írta:\n>>>>>>>>> Quoting Barnabás Pőcze (2025-08-11 14:11:59)\n>>>>>>>>>> Aliasing an arbitrary buffer with some `T *` is unlikely to yield good\n>>>>>>>>>> results, primarily due to potential alignment issues. So don't do it,\n>>>>>>>>>> instead create the objects on the stack and read into them, this\n>>>>>>>>>> removes any and all strict aliasing or alignment concerns.\n>>>>>>>>>>\n>>>>>>>>>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=221\n>>>>>>>>>> Signed-off-by: Barnabás Pőcze \n>>>>>>>>>> ---\n>>>>>>>>>> src/libcamera/control_serializer.cpp | 75 +++++++++++++---------------\n>>>>>>>>>> 1 file changed, 36 insertions(+), 39 deletions(-)\n>>>>>>>>>>\n>>>>>>>>>> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n>>>>>>>>>> index 050f8512b..722ff231c 100644\n>>>>>>>>>> --- a/src/libcamera/control_serializer.cpp\n>>>>>>>>>> +++ b/src/libcamera/control_serializer.cpp\n>>>>>>>>>> @@ -427,22 +427,22 @@ ControlInfo ControlSerializer::loadControlInfo(ByteStreamBuffer &b)\n>>>>>>>>>> template<>\n>>>>>>>>>> ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &buffer)\n>>>>>>>>>> {\n>>>>>>>>>> - const struct ipa_controls_header *hdr = buffer.read();\n>>>>>>>>>> - if (!hdr) {\n>>>>>>>>>> + struct ipa_controls_header hdr;\n>>>>>>>>>> + if (buffer.read(&hdr) < 0) {\n>>>>>>>>>\n>>>>>>>>> Aha, but the key difference is that before it was 'zero copy' just\n>>>>>>>>> pointing at the data, while now we're making a copy ?\n>>>>>>>>\n>>>>>>>> Yes.\n>>>>>>>>\n>>>>>>>>> I wonder if we can detect the alignment and only do the copy if\n>>>>>>>>> unaligned? Or maybe that's just not worth it and we should just accept\n>>>>>>>>> the copy - this shouldn't be a high data volume for the most parts...\n>>>>>>>>\n>>>>>>>> I feel like the control id/info map/list/value allocation easily dominates the\n>>>>>>>> cost of these small copies. These are all <=32 bytes.\n>>>>>>>\n>>>>>>> Hrm.. indeed - we don't normally expect ControlLists to contain large\n>>>>>>> data.\n>>>>>>>\n>>>>>>> I thnik CCMs with a 3x3 matrix is 'hopefully' the biggest we expect for\n>>>>>>> the most part and things like lens shading tables shouldn't be\n>>>>>>> popultated through a control list.\n>>>>>>\n>>>>>> This change only affects the headers, e.g. the bytes of the control value\n>>>>>> data is still read the same way, directly from the buffer.\n>>>>>>\n>>>>>>> So ... yes a copy seems quick and trivial...\n>>>>>>>\n>>>>>>> I'll put this in already:\n>>>>>>>\n>>>>>>> Reviewed-by: Kieran Bingham \n>>>>>>>\n>>>>>>> But probably want this also checked by either or both of Laurent and\n>>>>>>> Paul who know the IPC mechanisms well.\n>>>>>>>\n>>>>>>>>> But maybe its something we want to be able to profile/monitor in the\n>>>>>>>>> future.\n>>>>>>>>>\n>>>>>>>>> Or perhaps can we find a way to align data when it goes into the\n>>>>>>>>> ByteStreamBuffer with some alignment padding sections somehow?\n>>>>>>>>\n>>>>>>>> I could be missing something, but given how ipc serialization is done\n>>>>>>>> at the moment, it does not seem entirely trivial.\n>>>>>\n>>>>> What's our alignment issue here, what field(s) are unaligned ? The\n>>>>> header contains 32-bit fields, are we serializing data in such a way\n>>>>> that causes ipa_controls_header instances to not have a 4 bytes\n>>>>> alignment (as in hdr.size not being a multiple of 4) ? Or is the buffer\n>>>>> itself not aligned ?\n>>>>\n>>>> I will turn the question around: what would guarantee the alignment?\n>>>> As far as I can tell, there is nothing. `ControlSerializer` does not guarantee\n>>>> any alignment in and of itself (e.g. `ipa_controls_header::size` can easily be\n>>>> not divisible by 4, e.g. `ControlSerializer::binarySize()` of a uint32_t is 5).\n>>>> And even if it did guarantee alignment, an appropriately sized string serialized\n>>>> before it can easily throw the final alignment off.\n>>>\n>>> I thought we were supposed to pad control packets to multiples of 8\n>>> bytes, as described in ipa_controls.cpp. That's what surprises me here.\n>>\n>> Ahh, that's true. I haven't noticed that. How should we proceed then? I would like to\n>> go ahead with this change now for two reasons:\n>>\n>> * the way serialization is done does not seem to guarantee sufficient alignment even\n>> if the control lists, etc. were serialized respecting the alignment requirements;\n> \n> Does that mean there's a bug in the serialization code that we should\n> fix ?\n\nIt does not match the documentation so I suppose at least one of them has to be fixed.\n\n\n> \n>> * I would like to incorporate https://patchwork.libcamera.org/patch/23373/ which\n>> changes at the least code appreciably, so I think it would be better to return\n>> to the question of alignment after that.\n> \n> I agree that this patch will not significantly hinder performance as it\n> only copies the headers. However, with the documented alignment, this\n> copy shouldn't be needed, and avoiding a copy would still be best. Are\n> you proposing merging this patch and https://patchwork.libcamera.org/patch/23373/,\n> and then fix the alignment problem and remove the copy on top ?\n\nSomething like that, yes. My first impression is that getting the alignment right will\nneed non-trivial changes, so I would postpone it until later (at least until after the\nother serialization related patch).\n\nRegards,\nBarnabás Pőcze\n\n\n\n> \n>>>>>>>>> If fixing/specifying the alignment in the buffer isn't possible easy -\n>>>>>>>>> then I could certainly imagine accepting this as a current possible solution ...\n>>>>>>>>>\n>>>>>>>>>> LOG(Serializer, Error) << \"Out of data\";\n>>>>>>>>>> return {};\n>>>>>>>>>> }\n>>>>>>>>>> \n>>>>>>>>>> - auto iter = infoMaps_.find(hdr->handle);\n>>>>>>>>>> + auto iter = infoMaps_.find(hdr.handle);\n>>>>>>>>>> if (iter != infoMaps_.end()) {\n>>>>>>>>>> LOG(Serializer, Debug) << \"Use cached ControlInfoMap\";\n>>>>>>>>>> return iter->second;\n>>>>>>>>>> }\n>>>>>>>>>> \n>>>>>>>>>> - if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) {\n>>>>>>>>>> + if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) {\n>>>>>>>>>> LOG(Serializer, Error)\n>>>>>>>>>> << \"Unsupported controls format version \"\n>>>>>>>>>> - << hdr->version;\n>>>>>>>>>> + << hdr.version;\n>>>>>>>>>> return {};\n>>>>>>>>>> }\n>>>>>>>>>> \n>>>>>>>>>> @@ -455,7 +455,7 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n>>>>>>>>>> */\n>>>>>>>>>> const ControlIdMap *idMap = nullptr;\n>>>>>>>>>> ControlIdMap *localIdMap = nullptr;\n>>>>>>>>>> - switch (hdr->id_map_type) {\n>>>>>>>>>> + switch (hdr.id_map_type) {\n>>>>>>>>>> case IPA_CONTROL_ID_MAP_CONTROLS:\n>>>>>>>>>> idMap = &controls::controls;\n>>>>>>>>>> break;\n>>>>>>>>>> @@ -469,12 +469,12 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n>>>>>>>>>> break;\n>>>>>>>>>> default:\n>>>>>>>>>> LOG(Serializer, Error)\n>>>>>>>>>> - << \"Unknown id map type: \" << hdr->id_map_type;\n>>>>>>>>>> + << \"Unknown id map type: \" << hdr.id_map_type;\n>>>>>>>>>> return {};\n>>>>>>>>>> }\n>>>>>>>>>> \n>>>>>>>>>> - ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr));\n>>>>>>>>>> - ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset);\n>>>>>>>>>> + ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr));\n>>>>>>>>>> + ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset);\n>>>>>>>>>> \n>>>>>>>>>> if (buffer.overflow()) {\n>>>>>>>>>> LOG(Serializer, Error) << \"Out of data\";\n>>>>>>>>>> @@ -482,36 +482,35 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n>>>>>>>>>> }\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>>>>>>>>>> + for (unsigned int i = 0; i < hdr.entries; ++i) {\n>>>>>>>>>> + struct ipa_control_info_entry entry;\n>>>>>>>>>> + if (entries.read(&entry) < 0) {\n>>>>>>>>>> LOG(Serializer, Error) << \"Out of data\";\n>>>>>>>>>> return {};\n>>>>>>>>>> }\n>>>>>>>>>> \n>>>>>>>>>> - ControlType type = static_cast(entry->type);\n>>>>>>>>>> + ControlType type = static_cast(entry.type);\n>>>>>>>>>> \n>>>>>>>>>> /* If we're using a local id map, populate it. */\n>>>>>>>>>> if (localIdMap) {\n>>>>>>>>>> ControlId::DirectionFlags flags{\n>>>>>>>>>> - static_cast(entry->direction)\n>>>>>>>>>> + static_cast(entry.direction)\n>>>>>>>>>> };\n>>>>>>>>>> \n>>>>>>>>>> /**\n>>>>>>>>>> * \\todo Find a way to preserve the control name for\n>>>>>>>>>> * debugging purpose.\n>>>>>>>>>> */\n>>>>>>>>>> - controlIds_.emplace_back(std::make_unique(entry->id,\n>>>>>>>>>> + controlIds_.emplace_back(std::make_unique(entry.id,\n>>>>>>>>>> \"\", \"local\", type,\n>>>>>>>>>> flags));\n>>>>>>>>>> - (*localIdMap)[entry->id] = controlIds_.back().get();\n>>>>>>>>>> + (*localIdMap)[entry.id] = controlIds_.back().get();\n>>>>>>>>>> }\n>>>>>>>>>> \n>>>>>>>>>> - const ControlId *controlId = idMap->at(entry->id);\n>>>>>>>>>> + const ControlId *controlId = idMap->at(entry.id);\n>>>>>>>>>> ASSERT(controlId);\n>>>>>>>>>> \n>>>>>>>>>> - if (entry->offset != values.offset()) {\n>>>>>>>>>> + if (entry.offset != values.offset()) {\n>>>>>>>>>> LOG(Serializer, Error)\n>>>>>>>>>> << \"Bad data, entry offset mismatch (entry \"\n>>>>>>>>>> << i << \")\";\n>>>>>>>>>> @@ -526,9 +525,9 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n>>>>>>>>>> * Create the ControlInfoMap in the cache, and store the map to handle\n>>>>>>>>>> * association.\n>>>>>>>>>> */\n>>>>>>>>>> - infoMaps_[hdr->handle] = ControlInfoMap(std::move(ctrls), *idMap);\n>>>>>>>>>> - ControlInfoMap &map = infoMaps_[hdr->handle];\n>>>>>>>>>> - infoMapHandles_[&map] = hdr->handle;\n>>>>>>>>>> + infoMaps_[hdr.handle] = ControlInfoMap(std::move(ctrls), *idMap);\n>>>>>>>>>> + ControlInfoMap &map = infoMaps_[hdr.handle];\n>>>>>>>>>> + infoMapHandles_[&map] = hdr.handle;\n>>>>>>>>>> \n>>>>>>>>>> return map;\n>>>>>>>>>> }\n>>>>>>>>>> @@ -545,21 +544,21 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n>>>>>>>>>> template<>\n>>>>>>>>>> ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer)\n>>>>>>>>>> {\n>>>>>>>>>> - const struct ipa_controls_header *hdr = buffer.read();\n>>>>>>>>>> - if (!hdr) {\n>>>>>>>>>> + struct ipa_controls_header hdr;\n>>>>>>>>>> + if (buffer.read(&hdr) < 0) {\n>>>>>>>>>> LOG(Serializer, Error) << \"Out of data\";\n>>>>>>>>>> return {};\n>>>>>>>>>> }\n>>>>>>>>>> \n>>>>>>>>>> - if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) {\n>>>>>>>>>> + if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) {\n>>>>>>>>>> LOG(Serializer, Error)\n>>>>>>>>>> << \"Unsupported controls format version \"\n>>>>>>>>>> - << hdr->version;\n>>>>>>>>>> + << hdr.version;\n>>>>>>>>>> return {};\n>>>>>>>>>> }\n>>>>>>>>>> \n>>>>>>>>>> - ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr));\n>>>>>>>>>> - ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset);\n>>>>>>>>>> + ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr));\n>>>>>>>>>> + ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset);\n>>>>>>>>>> \n>>>>>>>>>> if (buffer.overflow()) {\n>>>>>>>>>> LOG(Serializer, Error) << \"Out of data\";\n>>>>>>>>>> @@ -576,10 +575,10 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n>>>>>>>>>> * ControlInfoMap is available.\n>>>>>>>>>> */\n>>>>>>>>>> const ControlIdMap *idMap;\n>>>>>>>>>> - if (hdr->handle) {\n>>>>>>>>>> + if (hdr.handle) {\n>>>>>>>>>> auto iter = std::find_if(infoMapHandles_.begin(), infoMapHandles_.end(),\n>>>>>>>>>> [&](decltype(infoMapHandles_)::value_type &entry) {\n>>>>>>>>>> - return entry.second == hdr->handle;\n>>>>>>>>>> + return entry.second == hdr.handle;\n>>>>>>>>>> });\n>>>>>>>>>> if (iter == infoMapHandles_.end()) {\n>>>>>>>>>> LOG(Serializer, Error)\n>>>>>>>>>> @@ -590,7 +589,7 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n>>>>>>>>>> const ControlInfoMap *infoMap = iter->first;\n>>>>>>>>>> idMap = &infoMap->idmap();\n>>>>>>>>>> } else {\n>>>>>>>>>> - switch (hdr->id_map_type) {\n>>>>>>>>>> + switch (hdr.id_map_type) {\n>>>>>>>>>> case IPA_CONTROL_ID_MAP_CONTROLS:\n>>>>>>>>>> idMap = &controls::controls;\n>>>>>>>>>> break;\n>>>>>>>>>> @@ -601,7 +600,7 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n>>>>>>>>>> \n>>>>>>>>>> case IPA_CONTROL_ID_MAP_V4L2:\n>>>>>>>>>> default:\n>>>>>>>>>> - if (hdr->entries > 0)\n>>>>>>>>>> + if (hdr.entries > 0)\n>>>>>>>>>> LOG(Serializer, Fatal)\n>>>>>>>>>> << \"A list of V4L2 controls requires a ControlInfoMap\";\n>>>>>>>>>> \n>>>>>>>>>> @@ -617,23 +616,21 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n>>>>>>>>>> */\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>>>>>>>>>> + for (unsigned int i = 0; i < hdr.entries; ++i) {\n>>>>>>>>>> + struct ipa_control_value_entry entry;\n>>>>>>>>>> + if (entries.read(&entry) < 0) {\n>>>>>>>>>> LOG(Serializer, Error) << \"Out of data\";\n>>>>>>>>>> return {};\n>>>>>>>>>> }\n>>>>>>>>>> \n>>>>>>>>>> - if (entry->offset != values.offset()) {\n>>>>>>>>>> + if (entry.offset != values.offset()) {\n>>>>>>>>>> LOG(Serializer, Error)\n>>>>>>>>>> << \"Bad data, entry offset mismatch (entry \"\n>>>>>>>>>> << i << \")\";\n>>>>>>>>>> return {};\n>>>>>>>>>> }\n>>>>>>>>>> \n>>>>>>>>>> - ctrls.set(entry->id,\n>>>>>>>>>> - loadControlValue(values, entry->is_array, entry->count));\n>>>>>>>>>> + ctrls.set(entry.id, loadControlValue(values, entry.is_array, entry.count));\n>>>>>>>>>> }\n>>>>>>>>>> \n>>>>>>>>>> return ctrls;\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 48713BEFBE\n\tfor ;\n\tWed, 13 Aug 2025 10:48:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 803D36924C;\n\tWed, 13 Aug 2025 12:48:26 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 17EC16922C\n\tfor ;\n\tWed, 13 Aug 2025 12:48:25 +0200 (CEST)","from [192.168.33.21] (185.221.141.188.nat.pool.zt.hu\n\t[185.221.141.188])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 34927169;\n\tWed, 13 Aug 2025 12:47:30 +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=\"VdoOG3SH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1755082051;\n\tbh=raqG+JAmRdfJukT/Lhsk8l+wCR+bCWK9jq1cNWh7PwI=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=VdoOG3SHm8XXk1m0UqSAn6jHB1sLksOu6g0J9yB0WHAIAb5j4qHuMe6NRTxGIHExD\n\tGEOjq58bk8Rku/gkzDLBidJ1BYnWWl2yJL7neno/DkxNM6fmk/bX716cj3z8i7j9Qu\n\tymDZxqzxRlik/H4PRrJxDp4+ADobbuar9hXNl0bo=","Message-ID":"<72575c14-ac7a-4cef-a6e7-647b717c40d7@ideasonboard.com>","Date":"Wed, 13 Aug 2025 12:48:20 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v1] libcamera: control_serializer: Do not alias byte\n\tbuffer","To":"Laurent Pinchart ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20250811131159.1342981-1-barnabas.pocze@ideasonboard.com>\n\t<175492155549.1641235.14089828007550134576@ping.linuxembedded.co.uk>\n\t<333a28b4-f758-4e57-a13e-bf7f9ee42755@ideasonboard.com>\n\t<175492223974.1641235.4760089293007493036@ping.linuxembedded.co.uk>\n\t<3c6c6b31-5af8-4201-923c-d749c4ad6e51@ideasonboard.com>\n\t<20250811143803.GI21313@pendragon.ideasonboard.com>\n\t\n\t<20250811152132.GB30054@pendragon.ideasonboard.com>\n\t\n\t<20250813103825.GB6440@pendragon.ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20250813103825.GB6440@pendragon.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":35376,"web_url":"https://patchwork.libcamera.org/comment/35376/","msgid":"<20250813105654.GF6440@pendragon.ideasonboard.com>","date":"2025-08-13T10:56:54","subject":"Re: [PATCH v1] libcamera: control_serializer: Do not alias byte\n\tbuffer","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Aug 13, 2025 at 12:48:20PM +0200, Barnabás Pőcze wrote:\n> 2025. 08. 13. 12:38 keltezéssel, Laurent Pinchart írta:\n> > On Wed, Aug 13, 2025 at 12:32:29PM +0200, Barnabás Pőcze wrote:\n> >> 2025. 08. 11. 17:21 keltezéssel, Laurent Pinchart írta:\n> >>> On Mon, Aug 11, 2025 at 04:52:45PM +0200, Barnabás Pőcze wrote:\n> >>>> 2025. 08. 11. 16:38 keltezéssel, Laurent Pinchart írta:\n> >>>>> On Mon, Aug 11, 2025 at 04:28:10PM +0200, Barnabás Pőcze wrote:\n> >>>>>> 2025. 08. 11. 16:23 keltezéssel, Kieran Bingham írta:\n> >>>>>>> Quoting Barnabás Pőcze (2025-08-11 15:18:57)\n> >>>>>>>> 2025. 08. 11. 16:12 keltezéssel, Kieran Bingham írta:\n> >>>>>>>>> Quoting Barnabás Pőcze (2025-08-11 14:11:59)\n> >>>>>>>>>> Aliasing an arbitrary buffer with some `T *` is unlikely to yield good\n> >>>>>>>>>> results, primarily due to potential alignment issues. So don't do it,\n> >>>>>>>>>> instead create the objects on the stack and read into them, this\n> >>>>>>>>>> removes any and all strict aliasing or alignment concerns.\n> >>>>>>>>>>\n> >>>>>>>>>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=221\n> >>>>>>>>>> Signed-off-by: Barnabás Pőcze \n> >>>>>>>>>> ---\n> >>>>>>>>>> src/libcamera/control_serializer.cpp | 75 +++++++++++++---------------\n> >>>>>>>>>> 1 file changed, 36 insertions(+), 39 deletions(-)\n> >>>>>>>>>>\n> >>>>>>>>>> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> >>>>>>>>>> index 050f8512b..722ff231c 100644\n> >>>>>>>>>> --- a/src/libcamera/control_serializer.cpp\n> >>>>>>>>>> +++ b/src/libcamera/control_serializer.cpp\n> >>>>>>>>>> @@ -427,22 +427,22 @@ ControlInfo ControlSerializer::loadControlInfo(ByteStreamBuffer &b)\n> >>>>>>>>>> template<>\n> >>>>>>>>>> ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &buffer)\n> >>>>>>>>>> {\n> >>>>>>>>>> - const struct ipa_controls_header *hdr = buffer.read();\n> >>>>>>>>>> - if (!hdr) {\n> >>>>>>>>>> + struct ipa_controls_header hdr;\n> >>>>>>>>>> + if (buffer.read(&hdr) < 0) {\n> >>>>>>>>>\n> >>>>>>>>> Aha, but the key difference is that before it was 'zero copy' just\n> >>>>>>>>> pointing at the data, while now we're making a copy ?\n> >>>>>>>>\n> >>>>>>>> Yes.\n> >>>>>>>>\n> >>>>>>>>> I wonder if we can detect the alignment and only do the copy if\n> >>>>>>>>> unaligned? Or maybe that's just not worth it and we should just accept\n> >>>>>>>>> the copy - this shouldn't be a high data volume for the most parts...\n> >>>>>>>>\n> >>>>>>>> I feel like the control id/info map/list/value allocation easily dominates the\n> >>>>>>>> cost of these small copies. These are all <=32 bytes.\n> >>>>>>>\n> >>>>>>> Hrm.. indeed - we don't normally expect ControlLists to contain large\n> >>>>>>> data.\n> >>>>>>>\n> >>>>>>> I thnik CCMs with a 3x3 matrix is 'hopefully' the biggest we expect for\n> >>>>>>> the most part and things like lens shading tables shouldn't be\n> >>>>>>> popultated through a control list.\n> >>>>>>\n> >>>>>> This change only affects the headers, e.g. the bytes of the control value\n> >>>>>> data is still read the same way, directly from the buffer.\n> >>>>>>\n> >>>>>>> So ... yes a copy seems quick and trivial...\n> >>>>>>>\n> >>>>>>> I'll put this in already:\n> >>>>>>>\n> >>>>>>> Reviewed-by: Kieran Bingham \n> >>>>>>>\n> >>>>>>> But probably want this also checked by either or both of Laurent and\n> >>>>>>> Paul who know the IPC mechanisms well.\n> >>>>>>>\n> >>>>>>>>> But maybe its something we want to be able to profile/monitor in the\n> >>>>>>>>> future.\n> >>>>>>>>>\n> >>>>>>>>> Or perhaps can we find a way to align data when it goes into the\n> >>>>>>>>> ByteStreamBuffer with some alignment padding sections somehow?\n> >>>>>>>>\n> >>>>>>>> I could be missing something, but given how ipc serialization is done\n> >>>>>>>> at the moment, it does not seem entirely trivial.\n> >>>>>\n> >>>>> What's our alignment issue here, what field(s) are unaligned ? The\n> >>>>> header contains 32-bit fields, are we serializing data in such a way\n> >>>>> that causes ipa_controls_header instances to not have a 4 bytes\n> >>>>> alignment (as in hdr.size not being a multiple of 4) ? Or is the buffer\n> >>>>> itself not aligned ?\n> >>>>\n> >>>> I will turn the question around: what would guarantee the alignment?\n> >>>> As far as I can tell, there is nothing. `ControlSerializer` does not guarantee\n> >>>> any alignment in and of itself (e.g. `ipa_controls_header::size` can easily be\n> >>>> not divisible by 4, e.g. `ControlSerializer::binarySize()` of a uint32_t is 5).\n> >>>> And even if it did guarantee alignment, an appropriately sized string serialized\n> >>>> before it can easily throw the final alignment off.\n> >>>\n> >>> I thought we were supposed to pad control packets to multiples of 8\n> >>> bytes, as described in ipa_controls.cpp. That's what surprises me here.\n> >>\n> >> Ahh, that's true. I haven't noticed that. How should we proceed then? I would like to\n> >> go ahead with this change now for two reasons:\n> >>\n> >> * the way serialization is done does not seem to guarantee sufficient alignment even\n> >> if the control lists, etc. were serialized respecting the alignment requirements;\n> > \n> > Does that mean there's a bug in the serialization code that we should\n> > fix ?\n> \n> It does not match the documentation so I suppose at least one of them has to be fixed.\n\nIndeed :-)\n\n> >> * I would like to incorporate https://patchwork.libcamera.org/patch/23373/ which\n> >> changes at the least code appreciably, so I think it would be better to return\n> >> to the question of alignment after that.\n> > \n> > I agree that this patch will not significantly hinder performance as it\n> > only copies the headers. However, with the documented alignment, this\n> > copy shouldn't be needed, and avoiding a copy would still be best. Are\n> > you proposing merging this patch and https://patchwork.libcamera.org/patch/23373/,\n> > and then fix the alignment problem and remove the copy on top ?\n> \n> Something like that, yes. My first impression is that getting the alignment right will\n> need non-trivial changes, so I would postpone it until later (at least until after the\n> other serialization related patch).\n\nI haven't looked at what it would take to fix the alignment, so I don't\nhave a clear opinion. If you think it's easier to address it on top, I'm\nOK with that, even if it will cause a bit of churn going back and forth\nwith the copy.\n\n> >>>>>>>>> If fixing/specifying the alignment in the buffer isn't possible easy -\n> >>>>>>>>> then I could certainly imagine accepting this as a current possible solution ...\n> >>>>>>>>>\n> >>>>>>>>>> LOG(Serializer, Error) << \"Out of data\";\n> >>>>>>>>>> return {};\n> >>>>>>>>>> }\n> >>>>>>>>>> \n> >>>>>>>>>> - auto iter = infoMaps_.find(hdr->handle);\n> >>>>>>>>>> + auto iter = infoMaps_.find(hdr.handle);\n> >>>>>>>>>> if (iter != infoMaps_.end()) {\n> >>>>>>>>>> LOG(Serializer, Debug) << \"Use cached ControlInfoMap\";\n> >>>>>>>>>> return iter->second;\n> >>>>>>>>>> }\n> >>>>>>>>>> \n> >>>>>>>>>> - if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) {\n> >>>>>>>>>> + if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) {\n> >>>>>>>>>> LOG(Serializer, Error)\n> >>>>>>>>>> << \"Unsupported controls format version \"\n> >>>>>>>>>> - << hdr->version;\n> >>>>>>>>>> + << hdr.version;\n> >>>>>>>>>> return {};\n> >>>>>>>>>> }\n> >>>>>>>>>> \n> >>>>>>>>>> @@ -455,7 +455,7 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> >>>>>>>>>> */\n> >>>>>>>>>> const ControlIdMap *idMap = nullptr;\n> >>>>>>>>>> ControlIdMap *localIdMap = nullptr;\n> >>>>>>>>>> - switch (hdr->id_map_type) {\n> >>>>>>>>>> + switch (hdr.id_map_type) {\n> >>>>>>>>>> case IPA_CONTROL_ID_MAP_CONTROLS:\n> >>>>>>>>>> idMap = &controls::controls;\n> >>>>>>>>>> break;\n> >>>>>>>>>> @@ -469,12 +469,12 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> >>>>>>>>>> break;\n> >>>>>>>>>> default:\n> >>>>>>>>>> LOG(Serializer, Error)\n> >>>>>>>>>> - << \"Unknown id map type: \" << hdr->id_map_type;\n> >>>>>>>>>> + << \"Unknown id map type: \" << hdr.id_map_type;\n> >>>>>>>>>> return {};\n> >>>>>>>>>> }\n> >>>>>>>>>> \n> >>>>>>>>>> - ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr));\n> >>>>>>>>>> - ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset);\n> >>>>>>>>>> + ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr));\n> >>>>>>>>>> + ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset);\n> >>>>>>>>>> \n> >>>>>>>>>> if (buffer.overflow()) {\n> >>>>>>>>>> LOG(Serializer, Error) << \"Out of data\";\n> >>>>>>>>>> @@ -482,36 +482,35 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> >>>>>>>>>> }\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> >>>>>>>>>> + for (unsigned int i = 0; i < hdr.entries; ++i) {\n> >>>>>>>>>> + struct ipa_control_info_entry entry;\n> >>>>>>>>>> + if (entries.read(&entry) < 0) {\n> >>>>>>>>>> LOG(Serializer, Error) << \"Out of data\";\n> >>>>>>>>>> return {};\n> >>>>>>>>>> }\n> >>>>>>>>>> \n> >>>>>>>>>> - ControlType type = static_cast(entry->type);\n> >>>>>>>>>> + ControlType type = static_cast(entry.type);\n> >>>>>>>>>> \n> >>>>>>>>>> /* If we're using a local id map, populate it. */\n> >>>>>>>>>> if (localIdMap) {\n> >>>>>>>>>> ControlId::DirectionFlags flags{\n> >>>>>>>>>> - static_cast(entry->direction)\n> >>>>>>>>>> + static_cast(entry.direction)\n> >>>>>>>>>> };\n> >>>>>>>>>> \n> >>>>>>>>>> /**\n> >>>>>>>>>> * \\todo Find a way to preserve the control name for\n> >>>>>>>>>> * debugging purpose.\n> >>>>>>>>>> */\n> >>>>>>>>>> - controlIds_.emplace_back(std::make_unique(entry->id,\n> >>>>>>>>>> + controlIds_.emplace_back(std::make_unique(entry.id,\n> >>>>>>>>>> \"\", \"local\", type,\n> >>>>>>>>>> flags));\n> >>>>>>>>>> - (*localIdMap)[entry->id] = controlIds_.back().get();\n> >>>>>>>>>> + (*localIdMap)[entry.id] = controlIds_.back().get();\n> >>>>>>>>>> }\n> >>>>>>>>>> \n> >>>>>>>>>> - const ControlId *controlId = idMap->at(entry->id);\n> >>>>>>>>>> + const ControlId *controlId = idMap->at(entry.id);\n> >>>>>>>>>> ASSERT(controlId);\n> >>>>>>>>>> \n> >>>>>>>>>> - if (entry->offset != values.offset()) {\n> >>>>>>>>>> + if (entry.offset != values.offset()) {\n> >>>>>>>>>> LOG(Serializer, Error)\n> >>>>>>>>>> << \"Bad data, entry offset mismatch (entry \"\n> >>>>>>>>>> << i << \")\";\n> >>>>>>>>>> @@ -526,9 +525,9 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> >>>>>>>>>> * Create the ControlInfoMap in the cache, and store the map to handle\n> >>>>>>>>>> * association.\n> >>>>>>>>>> */\n> >>>>>>>>>> - infoMaps_[hdr->handle] = ControlInfoMap(std::move(ctrls), *idMap);\n> >>>>>>>>>> - ControlInfoMap &map = infoMaps_[hdr->handle];\n> >>>>>>>>>> - infoMapHandles_[&map] = hdr->handle;\n> >>>>>>>>>> + infoMaps_[hdr.handle] = ControlInfoMap(std::move(ctrls), *idMap);\n> >>>>>>>>>> + ControlInfoMap &map = infoMaps_[hdr.handle];\n> >>>>>>>>>> + infoMapHandles_[&map] = hdr.handle;\n> >>>>>>>>>> \n> >>>>>>>>>> return map;\n> >>>>>>>>>> }\n> >>>>>>>>>> @@ -545,21 +544,21 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> >>>>>>>>>> template<>\n> >>>>>>>>>> ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer)\n> >>>>>>>>>> {\n> >>>>>>>>>> - const struct ipa_controls_header *hdr = buffer.read();\n> >>>>>>>>>> - if (!hdr) {\n> >>>>>>>>>> + struct ipa_controls_header hdr;\n> >>>>>>>>>> + if (buffer.read(&hdr) < 0) {\n> >>>>>>>>>> LOG(Serializer, Error) << \"Out of data\";\n> >>>>>>>>>> return {};\n> >>>>>>>>>> }\n> >>>>>>>>>> \n> >>>>>>>>>> - if (hdr->version != IPA_CONTROLS_FORMAT_VERSION) {\n> >>>>>>>>>> + if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) {\n> >>>>>>>>>> LOG(Serializer, Error)\n> >>>>>>>>>> << \"Unsupported controls format version \"\n> >>>>>>>>>> - << hdr->version;\n> >>>>>>>>>> + << hdr.version;\n> >>>>>>>>>> return {};\n> >>>>>>>>>> }\n> >>>>>>>>>> \n> >>>>>>>>>> - ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr));\n> >>>>>>>>>> - ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset);\n> >>>>>>>>>> + ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr));\n> >>>>>>>>>> + ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset);\n> >>>>>>>>>> \n> >>>>>>>>>> if (buffer.overflow()) {\n> >>>>>>>>>> LOG(Serializer, Error) << \"Out of data\";\n> >>>>>>>>>> @@ -576,10 +575,10 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> >>>>>>>>>> * ControlInfoMap is available.\n> >>>>>>>>>> */\n> >>>>>>>>>> const ControlIdMap *idMap;\n> >>>>>>>>>> - if (hdr->handle) {\n> >>>>>>>>>> + if (hdr.handle) {\n> >>>>>>>>>> auto iter = std::find_if(infoMapHandles_.begin(), infoMapHandles_.end(),\n> >>>>>>>>>> [&](decltype(infoMapHandles_)::value_type &entry) {\n> >>>>>>>>>> - return entry.second == hdr->handle;\n> >>>>>>>>>> + return entry.second == hdr.handle;\n> >>>>>>>>>> });\n> >>>>>>>>>> if (iter == infoMapHandles_.end()) {\n> >>>>>>>>>> LOG(Serializer, Error)\n> >>>>>>>>>> @@ -590,7 +589,7 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> >>>>>>>>>> const ControlInfoMap *infoMap = iter->first;\n> >>>>>>>>>> idMap = &infoMap->idmap();\n> >>>>>>>>>> } else {\n> >>>>>>>>>> - switch (hdr->id_map_type) {\n> >>>>>>>>>> + switch (hdr.id_map_type) {\n> >>>>>>>>>> case IPA_CONTROL_ID_MAP_CONTROLS:\n> >>>>>>>>>> idMap = &controls::controls;\n> >>>>>>>>>> break;\n> >>>>>>>>>> @@ -601,7 +600,7 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> >>>>>>>>>> \n> >>>>>>>>>> case IPA_CONTROL_ID_MAP_V4L2:\n> >>>>>>>>>> default:\n> >>>>>>>>>> - if (hdr->entries > 0)\n> >>>>>>>>>> + if (hdr.entries > 0)\n> >>>>>>>>>> LOG(Serializer, Fatal)\n> >>>>>>>>>> << \"A list of V4L2 controls requires a ControlInfoMap\";\n> >>>>>>>>>> \n> >>>>>>>>>> @@ -617,23 +616,21 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> >>>>>>>>>> */\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> >>>>>>>>>> + for (unsigned int i = 0; i < hdr.entries; ++i) {\n> >>>>>>>>>> + struct ipa_control_value_entry entry;\n> >>>>>>>>>> + if (entries.read(&entry) < 0) {\n> >>>>>>>>>> LOG(Serializer, Error) << \"Out of data\";\n> >>>>>>>>>> return {};\n> >>>>>>>>>> }\n> >>>>>>>>>> \n> >>>>>>>>>> - if (entry->offset != values.offset()) {\n> >>>>>>>>>> + if (entry.offset != values.offset()) {\n> >>>>>>>>>> LOG(Serializer, Error)\n> >>>>>>>>>> << \"Bad data, entry offset mismatch (entry \"\n> >>>>>>>>>> << i << \")\";\n> >>>>>>>>>> return {};\n> >>>>>>>>>> }\n> >>>>>>>>>> \n> >>>>>>>>>> - ctrls.set(entry->id,\n> >>>>>>>>>> - loadControlValue(values, entry->is_array, entry->count));\n> >>>>>>>>>> + ctrls.set(entry.id, loadControlValue(values, entry.is_array, entry.count));\n> >>>>>>>>>> }\n> >>>>>>>>>> \n> >>>>>>>>>> return ctrls;","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 4983EBDCC1\n\tfor ;\n\tWed, 13 Aug 2025 10:57:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7168869249;\n\tWed, 13 Aug 2025 12:57:15 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 47D216922C\n\tfor ;\n\tWed, 13 Aug 2025 12:57:14 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 8A2BF379;\n\tWed, 13 Aug 2025 12:56:20 +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=\"hPEalvUZ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1755082580;\n\tbh=wFV8RPJOUsKUXYtRrkm2G2SIHhtNMH9o36zyHJktgcE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hPEalvUZ6Fb9Km7ZCFnIiEkZCyvWBp9kPOdBj/KHhaa5FLMpU+IpJr/cDJmttGrtK\n\tfrYHt8boRtY56IEe7oeqtJ8tcdNfcXnFXp8NLkZQhYdaaiTKIB4N3PNW5A0ecsWf+e\n\twZRRu9qi3QVmIsVZSDIfrANqWx+0z9oSLqQq0NVk=","Date":"Wed, 13 Aug 2025 13:56:54 +0300","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1] libcamera: control_serializer: Do not alias byte\n\tbuffer","Message-ID":"<20250813105654.GF6440@pendragon.ideasonboard.com>","References":"<175492155549.1641235.14089828007550134576@ping.linuxembedded.co.uk>\n\t<333a28b4-f758-4e57-a13e-bf7f9ee42755@ideasonboard.com>\n\t<175492223974.1641235.4760089293007493036@ping.linuxembedded.co.uk>\n\t<3c6c6b31-5af8-4201-923c-d749c4ad6e51@ideasonboard.com>\n\t<20250811143803.GI21313@pendragon.ideasonboard.com>\n\t\n\t<20250811152132.GB30054@pendragon.ideasonboard.com>\n\t\n\t<20250813103825.GB6440@pendragon.ideasonboard.com>\n\t<72575c14-ac7a-4cef-a6e7-647b717c40d7@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<72575c14-ac7a-4cef-a6e7-647b717c40d7@ideasonboard.com>","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\" "}}]