[{"id":13487,"web_url":"https://patchwork.libcamera.org/comment/13487/","msgid":"<20201026160752.dtob6iq2tm5kkwg5@uno.localdomain>","date":"2020-10-26T16:07:52","subject":"Re: [libcamera-devel] [PATCH v3 10/38] libcamera: Add\n\tIPADataSerializer","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul,\n\nOn Fri, Oct 02, 2020 at 11:31:26PM +0900, Paul Elder wrote:\n> Add an IPADataSerializer which implments de/serialization of built-in\n> (PODs, vector, map, string) and libcamera data structures. This is\n> intended to be used by the proxy and the proxy worker in the IPC layer.\n>\n> Signed-off-by: Paul Elder \n>\n> ---\n> Changes in v3:\n> - reimplement append/readUInt with memcpy (intead of bit shifting)\n> - change DECLARE_INTEGRAL_SERIALIZER with DECLARE_POD_SERIALIZER\n> - use this for int64_t, bool, float, and double\n> - fix comment style\n>\n> Changes in v2:\n> - added serializers for all integer types, bool, and string\n> - use string serializer for IPASettings serializer\n> - add documentation\n> - add serializer for const ControlList\n> ---\n> .../libcamera/internal/ipa_data_serializer.h | 1014 +++++++++++++++++\n> src/libcamera/ipa_data_serializer.cpp | 154 +++\n> src/libcamera/meson.build | 1 +\n> 3 files changed, 1169 insertions(+)\n> create mode 100644 include/libcamera/internal/ipa_data_serializer.h\n> create mode 100644 src/libcamera/ipa_data_serializer.cpp\n>\n> diff --git a/include/libcamera/internal/ipa_data_serializer.h b/include/libcamera/internal/ipa_data_serializer.h\n> new file mode 100644\n> index 00000000..9cd35c4c\n> --- /dev/null\n> +++ b/include/libcamera/internal/ipa_data_serializer.h\n> @@ -0,0 +1,1014 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * ipa_data_serializer.h - Image Processing Algorithm data serializer\n> + */\n> +#ifndef __LIBCAMERA_INTERNAL_IPA_DATA_SERIALIZER_H__\n> +#define __LIBCAMERA_INTERNAL_IPA_DATA_SERIALIZER_H__\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \"libcamera/internal/byte_stream_buffer.h\"\n> +#include \"libcamera/internal/camera_sensor.h\"\n> +#include \"libcamera/internal/control_serializer.h\"\n> +#include \"libcamera/internal/log.h\"\n> +\n> +namespace libcamera {\n> +\n> +LOG_DECLARE_CATEGORY(IPADataSerializer)\n> +\n\nshould these be wrapped in an anonymous namespace ? They are used by\nthe generated proxyes, but the header is included. I tried it locally\nand complier seems to digest it\n\n> +template\n> +void appendUInt(std::vector &vec, T val)\n> +{\n> +\tsize_t byteWidth = sizeof(val);\n> +\tstd::vector v(byteWidth);\n> +\tmemcpy(&v[0], &val, byteWidth);\n> +\n> +\tvec.insert(vec.end(), v.begin(), v.end());\n> +}\n> +\n> +template\n> +T readUInt(std::vector &vec, size_t pos)\n> +{\n> +\tT ret = 0;\n> +\tsize_t byteWidth = sizeof(ret);\n> +\tif (pos + byteWidth > vec.size())\n> +\t\treturn ret;\n> +\n> +\tmemcpy(&ret, &vec.data()[pos], byteWidth);\n\nJust &vec[pos] should be enough\n\n> +\treturn ret;\n> +}\n> +\n> +template\n> +T readUInt(std::vector::iterator it)\n> +{\n> +\tT ret = 0;\n> +\tsize_t byteWidth = sizeof(ret);\n> +\n> +\tstd::vector v(it, it + byteWidth);\n> +\tmemcpy(&ret, v.data(), byteWidth);\n> +\treturn ret;\n> +}\n\nThese new versions looks much better!\n\n> +\n> +template\n> +class IPADataSerializer\n> +{\n> +#ifdef __DOXYGEN__\n> +public:\n> +\tstatic std::tuple, std::vector>\n> +\tserialize(T data, ControlSerializer *cs);\n> +\n> +\tstatic T deserialize(std::vector &data,\n> +\t\t\t ControlSerializer *cs);\n> +\tstatic T deserialize(std::vector::iterator it1,\n> +\t\t\t std::vector::iterator it2,\n> +\t\t\t ControlSerializer *cs);\n> +\n\nIntentional empty line ?\n\n> +\tstatic T deserialize(std::vector &data,\n> +\t\t\t std::vector &fds,\n> +\t\t\t ControlSerializer *cs);\n> +\tstatic T deserialize(std::vector::iterator data_it1,\n> +\t\t\t std::vector::iterator data_it2,\n> +\t\t\t std::vector::iterator fds_it1,\n> +\t\t\t std::vector::iterator fds_it2,\n> +\t\t\t ControlSerializer *cs);\n\nAren't these better called data_start, data_end and fds_start, fds_end\n?\n\n> +#endif /* __DOXYGEN__ */\n\nDo you need this ifdef __DOXYGEN__ ? you want these parsed, don't you ?\nIt's usually #ifndef __DOXYGEN__ to exclude from documentation parsing\nsome parts of the code, like you're done below\n\n> +};\n> +\n> +#ifndef __DOXYGEN__\n> +\n> +template\n> +class IPADataSerializer>\n> +{\n> +public:\n> +\tstatic std::tuple, std::vector>\n> +\tserialize(const std::vector &data, ControlSerializer *cs = nullptr)\n> +\t{\n> +\t\tstd::vector data_vec;\n\nWe can reserve data.size() * sizeof(V) in the vector\n\n> +\t\tstd::vector fds_vec;\n> +\n> +\t\t/* Serialize the length. */\n> +\t\tuint32_t vec_len = data.size();\n> +\t\tappendUInt(data_vec, vec_len);\n> +\n> +\t\t/* Serialize the members. */\n> +\t\tfor (auto it = data.begin(); it != data.end(); ++it) {\n\n for (auto const &it : data) ?\n\n> +\t\t\tstd::vector dvec;\n> +\t\t\tstd::vector fvec;\n> +\n> +\t\t\tstd::tie(dvec, fvec) =\n> +\t\t\t\tIPADataSerializer::serialize(*it, cs);\n> +\n> +\t\t\tappendUInt(data_vec, dvec.size());\n\nWhy is the size (in bytes) of each serialized entry recorded in the\nserialized data buffer ? Aren't all the members of the same size ?\n\n> +\t\t\tappendUInt(data_vec, fvec.size());\n\nSame question, more relevant as FileDescriptors are known to be\nuin32_t\n\n> +\n> +\t\t\tdata_vec.insert(data_vec.end(), dvec.begin(), dvec.end());\n> +\t\t\tfds_vec.insert(fds_vec.end(), fvec.begin(), fvec.end());\n> +\t\t}\n> +\n> +\t\treturn {data_vec, fds_vec};\n> +\t}\n> +\n> +\tstatic std::vector deserialize(std::vector &data, ControlSerializer *cs = nullptr)\n> +\t{\n> +\t\treturn IPADataSerializer>::deserialize(data.begin(), data.end(), cs);\n\nquestion: do you need to fully qualify the deserializer() function\ncall ? I tried removing \"IPADataSerializer>::\" and\ncompiler is still happy\n\n> +\t}\n> +\n> +\tstatic std::vector deserialize(std::vector::iterator it1,\n> +\t\t\t\t\t std::vector::iterator it2,\n> +\t\t\t\t\t ControlSerializer *cs = nullptr)\n> +\t{\n> +\t\tstd::vector fds;\n> +\t\treturn IPADataSerializer>::deserialize(it1, it2,\n> +\t\t\t\t\t\t\t\t fds.begin(), fds.end(),\n> +\t\t\t\t\t\t\t\t cs);\n> +\t}\n> +\n> +\tstatic std::vector deserialize(std::vector &data, std::vector &fds,\n> +\t\t\t\t\t ControlSerializer *cs = nullptr)\n> +\t{\n> +\t\treturn IPADataSerializer>::deserialize(data.begin(), data.end(),\n> +\t\t\t\t\t\t\t\t fds.begin(), fds.end(),\n> +\t\t\t\t\t\t\t\t cs);\n> +\t}\n> +\n> +\tstatic std::vector deserialize(std::vector::iterator data_it1,\n> +\t\t\t\t\t [[maybe_unused]] std::vector::iterator data_it2,\n> +\t\t\t\t\t std::vector::iterator fds_it1,\n> +\t\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it2,\n> +\t\t\t\t\t ControlSerializer *cs = nullptr)\n> +\t{\n> +\t\tuint32_t vec_len = readUInt(data_it1);\n> +\t\tstd::vector ret(vec_len);\n> +\n> +\t\tstd::vector::iterator data_it = data_it1 + 4;\n> +\t\tstd::vector::iterator fd_it = fds_it1;\n> +\t\tfor (uint32_t i = 0; i < vec_len; i++) {\n> +\t\t\tuint32_t sizeof_data = readUInt(data_it);\n\nSame questions as above, do we need to insert the entry size in the\nserialized buffer, or can it just be assumed to be sizeof(V) ?\n\n> +\t\t\tuint32_t sizeof_fds = readUInt(data_it + 4);\n\nAnd this one to be 4 bytes ?\n\n> +\n> +\t\t\tret[i] = IPADataSerializer::deserialize(data_it + 8,\n> +\t\t\t\t\t\t\t\t data_it + 8 + sizeof_data,\n> +\t\t\t\t\t\t\t\t fd_it,\n> +\t\t\t\t\t\t\t\t fd_it + sizeof_fds,\n> +\t\t\t\t\t\t\t\t cs);\n> +\n> +\t\t\tdata_it += 8 + sizeof_data;\n> +\t\t\tfd_it += sizeof_fds;\n> +\t\t}\n> +\n> +\t\treturn ret;\n> +\t}\n> +};\n> +\n> +template\n> +class IPADataSerializer>\n> +{\n> +public:\n> +\tstatic std::tuple, std::vector>\n> +\tserialize(const std::map &data, ControlSerializer *cs = nullptr)\n> +\t{\n> +\t\tstd::vector data_vec;\n> +\t\tstd::vector fds_vec;\n> +\n> +\t\t/* Serialize the length. */\n> +\t\tuint32_t map_len = data.size();\n> +\t\tappendUInt(data_vec, map_len);\n> +\n> +\t\t/* Serialize the members. */\n> +\t\tfor (auto it = data.begin(); it != data.end(); ++it) {\n\nSame comment: a range loop might be nicer\n for (auto const &it : data)\n\n> +\t\t\tstd::vector dvec;\n> +\t\t\tstd::vector fvec;\n> +\n> +\t\t\tstd::tie(dvec, fvec) =\n> +\t\t\t\tIPADataSerializer::serialize(it->first, cs);\n> +\n> +\t\t\tappendUInt(data_vec, dvec.size());\n> +\t\t\tappendUInt(data_vec, fvec.size());\n> +\n> +\t\t\tdata_vec.insert(data_vec.end(), dvec.begin(), dvec.end());\n> +\t\t\tfds_vec.insert(fds_vec.end(), fvec.begin(), fvec.end());\n> +\n> +\t\t\tstd::tie(dvec, fvec) =\n> +\t\t\t\tIPADataSerializer::serialize(it->second, cs);\n> +\n> +\t\t\tappendUInt(data_vec, dvec.size());\n> +\t\t\tappendUInt(data_vec, fvec.size());\n> +\n> +\t\t\tdata_vec.insert(data_vec.end(), dvec.begin(), dvec.end());\n> +\t\t\tfds_vec.insert(fds_vec.end(), fvec.begin(), fvec.end());\n> +\t\t}\n> +\n> +\t\treturn {data_vec, fds_vec};\n> +\t}\n> +\n> +\tstatic std::map deserialize(std::vector &data, ControlSerializer *cs = nullptr)\n> +\t{\n> +\t\treturn IPADataSerializer>::deserialize(data.begin(), data.end(), cs);\n> +\t}\n> +\n> +\tstatic std::map deserialize(std::vector::iterator it1,\n> +\t\t\t\t\t std::vector::iterator it2,\n> +\t\t\t\t\t ControlSerializer *cs = nullptr)\n> +\t{\n> +\t\tstd::vector fds;\n> +\t\treturn IPADataSerializer>::deserialize(it1, it2,\n> +\t\t\t\t\t\t\t\t fds.begin(), fds.end(),\n> +\t\t\t\t\t\t\t\t cs);\n> +\t}\n> +\n> +\tstatic std::map deserialize(std::vector &data, std::vector &fds,\n> +\t\t\t\t\t ControlSerializer *cs = nullptr)\n> +\t{\n> +\t\treturn IPADataSerializer>::deserialize(data.begin(), data.end(),\n> +\t\t\t\t\t\t\t\t fds.begin(), fds.end(),\n> +\t\t\t\t\t\t\t\t cs);\n> +\t}\n\nSame comments on the fully qualified path\n\n> +\n> +\tstatic std::map deserialize(std::vector::iterator data_it1,\n> +\t\t\t\t\t [[maybe_unused]] std::vector::iterator data_it2,\n> +\t\t\t\t\t std::vector::iterator fds_it1,\n> +\t\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it2,\n> +\t\t\t\t\t ControlSerializer *cs = nullptr)\n> +\t{\n> +\t\tstd::map ret;\n> +\n> +\t\tuint32_t map_len = readUInt(data_it1);\n> +\n> +\t\tstd::vector::iterator data_it = data_it1 + 4;\n> +\t\tstd::vector::iterator fd_it = fds_it1;\n> +\t\tfor (uint32_t i = 0; i < map_len; i++) {\n> +\t\t\tuint32_t sizeof_data = readUInt(data_it);\n> +\t\t\tuint32_t sizeof_fds = readUInt(data_it + 4);\n> +\n> +\t\t\tK key = IPADataSerializer::deserialize(data_it + 8,\n> +\t\t\t\t\t\t\t\t data_it + 8 + sizeof_data,\n> +\t\t\t\t\t\t\t\t fd_it,\n> +\t\t\t\t\t\t\t\t fd_it + sizeof_fds,\n> +\t\t\t\t\t\t\t\t cs);\n> +\n> +\t\t\tdata_it += 8 + sizeof_data;\n> +\t\t\tfd_it += sizeof_fds;\n> +\t\t\tsizeof_data = readUInt(data_it);\n> +\t\t\tsizeof_fds = readUInt(data_it + 4);\n> +\n> +\t\t\tconst V value = IPADataSerializer::deserialize(data_it + 8,\n> +\t\t\t\t\t\t\t\t\t data_it + 8 + sizeof_data,\n> +\t\t\t\t\t\t\t\t\t fd_it,\n> +\t\t\t\t\t\t\t\t\t fd_it + sizeof_fds,\n> +\t\t\t\t\t\t\t\t\t cs);\n> +\t\t\tret.insert({key, value});\n> +\n> +\t\t\tdata_it += 8 + sizeof_data;\n> +\t\t\tfd_it += sizeof_fds;\n> +\t\t}\n> +\n> +\t\treturn ret;\n> +\t}\n> +};\n> +\n> +#define DECLARE_POD_SERIALIZER(type)\t\t\t\t\t\\\n> +template<>\t\t\t\t\t\t\t\t\\\n> +class IPADataSerializer\t\t\t\t\t\t\\\n> +{\t\t\t\t\t\t\t\t\t\\\n> +public:\t\t\t\t\t\t\t\t\t\\\n> +\tstatic std::tuple, std::vector>\t\\\n> +\tserialize(const type data,\t\t\t\t\t\\\n> +\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\t\\\n> +\t{\t\t\t\t\t\t\t\t\\\n> +\t\tstd::vector data_vec;\t\t\t\t\\\n\nDo you think pre-allocating sizeof(type) in the vector might help ?\n\n> +\t\tappendUInt(data_vec, data);\t\t\t\\\n> +\t\t\t\t\t\t\t\t\t\\\n> +\t\treturn {data_vec, {}};\t\t\t\t\t\\\n> +\t}\t\t\t\t\t\t\t\t\\\n> +\t\t\t\t\t\t\t\t\t\\\n> +\tstatic type deserialize(std::vector &data,\t\t\\\n> +\t\t\t\t[[maybe_unused]] ControlSerializer *cs = nullptr)\\\n> +\t{\t\t\t\t\t\t\t\t\\\n> +\t\treturn IPADataSerializer::deserialize(data.begin(),\\\n> +\t\t\t\t\t\t\t data.end());\\\n> +\t}\t\t\t\t\t\t\t\t\\\n> +\t\t\t\t\t\t\t\t\t\\\n> +\tstatic type deserialize(std::vector::iterator it1,\t\\\n> +\t\t\t\t[[maybe_unused]] std::vector::iterator it2,\\\n> +\t\t\t\t[[maybe_unused]] ControlSerializer *cs = nullptr)\\\n> +\t{\t\t\t\t\t\t\t\t\\\n> +\t\treturn readUInt(it1);\t\t\t\t\\\n> +\t}\t\t\t\t\t\t\t\t\\\n> +\t\t\t\t\t\t\t\t\t\\\n> +\tstatic type deserialize(std::vector &data,\t\t\\\n> +\t\t\t\t[[maybe_unused]] std::vector &fds,\\\n> +\t\t\t\t[[maybe_unused]] ControlSerializer *cs = nullptr)\\\n> +\t{\t\t\t\t\t\t\t\t\\\n> +\t\treturn IPADataSerializer::deserialize(data.begin(),\\\n> +\t\t\t\t\t\t\t data.end());\\\n> +\t}\t\t\t\t\t\t\t\t\\\n> +\t\t\t\t\t\t\t\t\t\\\n> +\tstatic type deserialize(std::vector::iterator data_it1,\\\n> +\t\t\t\tstd::vector::iterator data_it2,\\\n> +\t\t\t\t[[maybe_unused]] std::vector::iterator fds_it1,\\\n> +\t\t\t\t[[maybe_unused]] std::vector::iterator fds_it2,\\\n> +\t\t\t\t[[maybe_unused]] ControlSerializer *cs = nullptr)\\\n> +\t{\t\t\t\t\t\t\t\t\\\n> +\t\treturn IPADataSerializer::deserialize(data_it1,\t\\\n> +\t\t\t\t\t\t\t data_it2);\t\\\n> +\t}\t\t\t\t\t\t\t\t\\\n> +};\n> +\n> +DECLARE_POD_SERIALIZER(bool)\n> +DECLARE_POD_SERIALIZER(uint8_t)\n> +DECLARE_POD_SERIALIZER(uint16_t)\n> +DECLARE_POD_SERIALIZER(uint32_t)\n> +DECLARE_POD_SERIALIZER(uint64_t)\n> +DECLARE_POD_SERIALIZER(int8_t)\n> +DECLARE_POD_SERIALIZER(int16_t)\n> +DECLARE_POD_SERIALIZER(int32_t)\n> +DECLARE_POD_SERIALIZER(int64_t)\n\nThe I wonder if keeping here and in the documentation the term 'uint'\nis correct and it shouldn't just be replaced with 'int'\n\n> +DECLARE_POD_SERIALIZER(float)\n> +DECLARE_POD_SERIALIZER(double)\n\nOr even POD\n\n> +\n> +template<>\n> +class IPADataSerializer\n> +{\n> +public:\n> +\tstatic std::tuple, std::vector>\n> +\tserialize(const std::string &data, [[maybe_unused]] ControlSerializer *cs = nullptr)\n> +\t{\n> +\t\tstd::vector data_vec(data.begin(), data.end());\n> +\n> +\t\treturn {data_vec, {}};\n> +\t}\n> +\n> +\tstatic std::string deserialize(std::vector &data,\n> +\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> +\t{\n> +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end());\n> +\t}\n> +\n> +\tstatic std::string deserialize(std::vector::iterator it1,\n> +\t\t\t\t std::vector::iterator it2,\n> +\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> +\t{\n> +\t\tstd::string str(it1, it2);\n> +\n> +\t\treturn str;\n\nCan you just\n return {it1, it2};\n?\n\n> +\t}\n> +\n> +\tstatic std::string deserialize(std::vector &data,\n> +\t\t\t\t [[maybe_unused]] std::vector &fds,\n> +\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> +\t{\n> +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end());\n\nMaybe\n return {data.being(), data.end()};\n\nwould save a function call ?\nSame for other overloaded methods (and possibly not only in the\n specialization).\n\n\n> +\t}\n> +\n> +\tstatic std::string deserialize(std::vector::iterator data_it1,\n> +\t\t\t\t std::vector::iterator data_it2,\n> +\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it1,\n> +\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it2,\n> +\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> +\t{\n> +\t\treturn IPADataSerializer::deserialize(data_it1, data_it2);\n> +\t}\n> +};\n> +\n> +template<>\n> +class IPADataSerializer\n> +{\n> +public:\n> +\tstatic std::tuple, std::vector>\n> +\tserialize(const FileDescriptor &data, [[maybe_unused]] ControlSerializer *cs = nullptr)\n> +\t{\n> +\t\tstd::vector data_vec = { data.isValid() };\n> +\t\tstd::vector fd_vec;\n> +\t\tif (data.isValid())\n> +\t\t\tfd_vec.push_back(data.fd());\n> +\n> +\t\treturn {data_vec, fd_vec};\n> +\t}\n> +\n> +\tstatic FileDescriptor deserialize(std::vector &data, std::vector &fds,\n> +\t\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> +\t{\n> +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end(),\n> +\t\t\t\t\t\t\t\t fds.begin(), fds.end());\n> +\t}\n> +\n> +\tstatic FileDescriptor deserialize(std::vector::iterator data_it1,\n> +\t\t\t\t\t std::vector::iterator data_it2,\n> +\t\t\t\t\t std::vector::iterator fds_it1,\n> +\t\t\t\t\t std::vector::iterator fds_it2,\n> +\t\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> +\t{\n> +\t\tif (std::distance(data_it1, data_it2) < 1)\n> +\t\t\tLOG(IPADataSerializer, Fatal)\n> +\t\t\t\t<< \"Invalid data to deserialize FileDescriptor\";\n> +\n> +\t\tbool valid = *data_it1;\n\nin C this would be\n bool valid = !!*data_it1;\nnot sure about C++ and not even sure if the !! is just a paranoid\nkernel convention, as I don't think even C requires that\n\n> +\n> +\t\tif (valid && std::distance(fds_it1, fds_it2) < 1)\n\n\t\tif (valid && std::distance(fds_it1, fds_it2) < 1) {\n\t\t\tLOG(IPADataSerializer, Fatal)\n\t\t\t\t<< \"Invalid fds to deserialize FileDescriptor\";\n return {}\n }\n\n return FileDescriptor(*fds_it1);\n\nOr is returning {} when std::distance() < 1 wrong ? I don't think so,\nas even if it is \"valid\" there's not fds serialized.\n\n\n> +\t\t\tLOG(IPADataSerializer, Fatal)\n> +\t\t\t\t<< \"Invalid fds to deserialize FileDescriptor\";\n> +\n> +\t\treturn valid ? FileDescriptor(*fds_it1) : FileDescriptor();\n> +\t}\n> +};\n> +\n> +template<>\n> +class IPADataSerializer\n> +{\n> +public:\n> +\tstatic std::tuple, std::vector>\n> +\tserialize(const IPASettings &data, [[maybe_unused]] ControlSerializer *cs = nullptr)\n> +\t{\n> +\t\treturn IPADataSerializer::serialize(data.configurationFile);\n> +\t}\n> +\n> +\tstatic IPASettings deserialize(std::vector &data,\n> +\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> +\t{\n> +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end());\n> +\t}\n> +\n> +\tstatic IPASettings deserialize(std::vector::iterator it1,\n> +\t\t\t\t std::vector::iterator it2,\n> +\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> +\t{\n> +\t\tIPASettings ret;\n> +\t\tret.configurationFile = IPADataSerializer::deserialize(it1, it2);\n> +\n> +\t\treturn ret;\n\n return { IPADataSerializer::deserialize(it1, it2) };\n\n?\n\n> +\t}\n> +\n> +\tstatic IPASettings deserialize(std::vector &data,\n> +\t\t\t\t [[maybe_unused]] std::vector &fds,\n> +\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> +\t{\n> +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end());\n\n return { IPADataSerializer::deserialize(data.begin(), data.end() };\n\n?\n\n> +\t}\n> +\n> +\tstatic IPASettings deserialize(std::vector::iterator data_it1,\n> +\t\t\t\t std::vector::iterator data_it2,\n> +\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it1,\n> +\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it2,\n> +\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> +\t{\n> +\t\treturn IPADataSerializer::deserialize(data_it1, data_it2);\n\nsame\n\n> +\t}\n> +};\n> +\n> +template<>\n> +class IPADataSerializer\n> +{\n> +public:\n> +\tstatic std::tuple, std::vector>\n> +\tserialize(const CameraSensorInfo &data, [[maybe_unused]] ControlSerializer *cs = nullptr)\n> +\t{\n> +\t\tstd::vector data_vec;\n\nReserving sizeof(CameraSensorInfo) might help\n\nI just noticed and I wonder why all this code uses this_naming_style\nin place of the libcamera standard namingStyle\n\n> +\n> +\t\tuint32_t str_len = data.model.size();\n> +\t\tappendUInt(data_vec, str_len);\n> +\n> +\t\tdata_vec.insert(data_vec.end(), data.model.begin(), data.model.end());\n\nShouldn't this be serialized using the IPADataSerializer\nif it's not needed, do we need the specialization then ?\n\n> +\n> +\t\tappendUInt(data_vec, data.bitsPerPixel);\n> +\n> +\t\tappendUInt(data_vec, data.activeAreaSize.width);\n> +\t\tappendUInt(data_vec, data.activeAreaSize.height);\n> +\n> +\t\tappendUInt(data_vec, static_cast(data.analogCrop.x));\n> +\t\tappendUInt(data_vec, static_cast(data.analogCrop.y));\n> +\t\tappendUInt(data_vec, data.analogCrop.width);\n> +\t\tappendUInt(data_vec, data.analogCrop.height);\n> +\n> +\t\tappendUInt(data_vec, data.outputSize.width);\n> +\t\tappendUInt(data_vec, data.outputSize.height);\n> +\n> +\t\tappendUInt(data_vec, data.pixelRate);\n> +\n> +\t\tappendUInt(data_vec, data.lineLength);\n> +\n> +\t\treturn {data_vec, {}};\n> +\t}\n> +\n> +\tstatic CameraSensorInfo deserialize(std::vector &data,\n> +\t\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> +\t{\n> +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end());\n> +\t}\n> +\n> +\tstatic CameraSensorInfo deserialize(std::vector::iterator it1,\n> +\t\t\t\t\t [[maybe_unused]] std::vector::iterator it2,\n> +\t\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> +\t{\n> +\t\tCameraSensorInfo ret;\n CameraSensorInfo ret{};\n\nAlso for other statically allocated fields that are returned by other\nspecializations.\n\n> +\n> +\t\tuint32_t str_len = readUInt(it1);\n> +\t\tstd::string str(it1 + 4, it1 + 4 + str_len);\n> +\t\tret.model = str;\n> +\n> +\t\tstd::vector::iterator it = it1 + 4 + str_len;\n> +\n> +\t\tret.bitsPerPixel = readUInt(it);\n> +\n> +\t\tret.activeAreaSize.width = readUInt(it + 4);\n> +\t\tret.activeAreaSize.height = readUInt(it + 8);\n> +\n> +\t\tret.analogCrop.x = static_cast(readUInt(it + 12));\n> +\t\tret.analogCrop.y = static_cast(readUInt(it + 16));\n> +\t\tret.analogCrop.width = readUInt(it + 20);\n> +\t\tret.analogCrop.height = readUInt(it + 24);\n> +\n> +\t\tret.outputSize.width = readUInt(it + 28);\n> +\t\tret.outputSize.height = readUInt(it + 32);\n> +\n> +\t\tret.pixelRate = readUInt(it + 36);\n> +\n> +\t\tret.lineLength = readUInt(it + 44);\n\nlineLength is a 32 bits integer\n\n> +\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tstatic CameraSensorInfo deserialize(std::vector &data,\n> +\t\t\t\t\t [[maybe_unused]] std::vector &fds,\n> +\t\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> +\t{\n> +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end());\n> +\t}\n> +\n> +\tstatic CameraSensorInfo deserialize(std::vector::iterator data_it1,\n> +\t\t\t\t\t std::vector::iterator data_it2,\n> +\t\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it1,\n> +\t\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it2,\n> +\t\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> +\t{\n> +\t\treturn IPADataSerializer::deserialize(data_it1, data_it2);\n> +\t}\n> +};\n> +\n> +template<>\n> +class IPADataSerializer\n> +{\n> +public:\n> +\tstatic std::tuple, std::vector>\n> +\tserialize(const IPAStream &data, [[maybe_unused]] ControlSerializer *cs = nullptr)\n> +\t{\n> +\t\tstd::vector data_vec;\n> +\n> +\t\tappendUInt(data_vec, data.pixelFormat);\n> +\n> +\t\tappendUInt(data_vec, data.size.width);\n> +\t\tappendUInt(data_vec, data.size.height);\n> +\n> +\t\treturn {data_vec, {}};\n> +\t}\n> +\n> +\tstatic IPAStream deserialize(std::vector &data,\n> +\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> +\t{\n> +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end());\n> +\t}\n> +\n> +\tstatic IPAStream deserialize(std::vector::iterator it1,\n> +\t\t\t\t [[maybe_unused]] std::vector::iterator it2,\n> +\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> +\t{\n> +\t\tIPAStream ret;\n> +\n> +\t\tret.pixelFormat = readUInt(it1);\n> +\n> +\t\tret.size.width = readUInt(it1 + 4);\n> +\t\tret.size.height = readUInt(it1 + 8);\n> +\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tstatic IPAStream deserialize(std::vector &data,\n> +\t\t\t\t [[maybe_unused]] std::vector &fds,\n> +\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> +\t{\n> +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end());\n> +\t}\n> +\n> +\tstatic IPAStream deserialize(std::vector::iterator data_it1,\n> +\t\t\t\t std::vector::iterator data_it2,\n> +\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it1,\n> +\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it2,\n> +\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> +\t{\n> +\t\treturn IPADataSerializer::deserialize(data_it1, data_it2);\n> +\t}\n> +};\n> +\n> +template<>\n> +class IPADataSerializer\n> +{\n> +public:\n> +\t/* map arg will be generated, since it's per-pipeline anyway. */\n> +\tstatic std::tuple, std::vector>\n> +\tserialize(const ControlList &data, const ControlInfoMap &map,\n> +\t\t ControlSerializer *cs)\n> +\t{\n> +\t\tif (!cs)\n> +\t\t\tLOG(IPADataSerializer, Fatal)\n> +\t\t\t\t<< \"ControlSerializer not provided for serialization of ControlList\";\n> +\n> +\t\tsize_t size = cs->binarySize(map);\n> +\t\tstd::vector infoData(size);\n> +\t\tByteStreamBuffer buffer(infoData.data(), infoData.size());\n> +\t\tint ret = cs->serialize(map, buffer);\n> +\n> +\t\tif (ret < 0 || buffer.overflow()) {\n> +\t\t\tLOG(IPADataSerializer, Error) << \"Failed to serialize ControlList's ControlInfoMap\";\n> +\t\t\treturn {{}, {}};\n> +\t\t}\n> +\n> +\t\tsize = cs->binarySize(data);\n> +\t\tstd::vector listData(size);\n> +\t\tbuffer = ByteStreamBuffer(listData.data(), listData.size());\n> +\t\tret = cs->serialize(data, buffer);\n> +\n> +\t\tif (ret < 0 || buffer.overflow()) {\n> +\t\t\tLOG(IPADataSerializer, Error) << \"Failed to serialize ControlList\";\n> +\t\t\treturn {{}, {}};\n> +\t\t}\n> +\n> +\t\tstd::vector data_vec;\n\nYou know the sizes and can reserve ?\n\n> +\t\tappendUInt(data_vec, infoData.size());\n> +\t\tappendUInt(data_vec, listData.size());\n> +\t\tdata_vec.insert(data_vec.end(), infoData.begin(), infoData.end());\n> +\t\tdata_vec.insert(data_vec.end(), listData.begin(), listData.end());\n> +\n> +\t\treturn {data_vec, {}};\n> +\t}\n> +\n> +\tstatic const ControlList deserialize(std::vector &data, ControlSerializer *cs)\n> +\t{\n> +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end(), cs);\n> +\t}\n> +\n> +\tstatic const ControlList deserialize(std::vector::iterator it1,\n> +\t\t\t\t [[maybe_unused]] std::vector::iterator it2,\n> +\t\t\t\t ControlSerializer *cs)\n> +\t{\n> +\t\tif (!cs)\n> +\t\t\tLOG(IPADataSerializer, Fatal)\n> +\t\t\t\t<< \"ControlSerializer not provided for deserialization of ControlList\";\n> +\n> +\t\tuint32_t infoDataSize = readUInt(it1);\n> +\t\tuint32_t listDataSize = readUInt(it1 + 4);\n> +\n> +\t\tstd::vector::iterator it = it1 + 8;\n> +\n> +\t\tstd::vector infoData(it, it + infoDataSize);\n> +\t\tstd::vector listData(it + infoDataSize, it + infoDataSize + listDataSize);\n> +\n> +\t\tByteStreamBuffer buffer(const_cast(infoData.data()), infoData.size());\n> +\t\tControlInfoMap map = cs->deserialize(buffer);\n> +\t\t/* It's fine if map is empty. */\n> +\t\tif (buffer.overflow()) {\n> +\t\t\tLOG(IPADataSerializer, Error)\n> +\t\t\t\t<< \"Failed to deserialize ControlLists's ControlInfoMap: buffer overflow\";\n> +\t\t\treturn ControlList();\n> +\t\t}\n> +\n> +\t\tbuffer = ByteStreamBuffer(const_cast(listData.data()), listData.size());\n> +\t\tControlList list = cs->deserialize(buffer);\n> +\t\tif (buffer.overflow())\n> +\t\t\tLOG(IPADataSerializer, Error) << \"Failed to deserialize ControlList: buffer overflow\";\n> +\t\tif (list.empty())\n> +\t\t\tLOG(IPADataSerializer, Error) << \"Failed to deserialize ControlList: empty list\";\n\nShould we return {} in these cases or is it fine ?\n\n> +\n> +\t\treturn list;\n> +\t}\n> +\n> +\tstatic const ControlList deserialize(std::vector &data,\n> +\t\t\t\t [[maybe_unused]] std::vector &fds,\n> +\t\t\t\t ControlSerializer *cs)\n> +\t{\n> +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end(), cs);\n> +\t}\n> +\n> +\tstatic const ControlList deserialize(std::vector::iterator data_it1,\n> +\t\t\t\t std::vector::iterator data_it2,\n> +\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it1,\n> +\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it2,\n> +\t\t\t\t ControlSerializer *cs)\n> +\t{\n> +\t\treturn IPADataSerializer::deserialize(data_it1, data_it2, cs);\n> +\t}\n> +};\n> +\n> +template<>\n> +class IPADataSerializer\n> +{\n> +public:\n> +\tstatic std::tuple, std::vector>\n> +\tserialize(const ControlList &data, const ControlInfoMap &map,\n> +\t\t ControlSerializer *cs)\n> +\t{\n> +\t\tconst ControlList &list_const = const_cast(data);\n> +\n> +\t\tstd::vector data_vec;\n> +\t\tstd::tie(data_vec, std::ignore) =\n> +\t\t\tIPADataSerializer::serialize(list_const, map, cs);\n> +\n> +\t\treturn {data_vec, {}};\n> +\t}\n> +\n> +\tstatic ControlList deserialize(std::vector &data,\n> +\t\t\t\t ControlSerializer *cs)\n> +\t{\n> +\t\tControlList ret;\n> +\t\tconst ControlList &list = ret;\n> +\t\tconst_cast(list) =\n> +\t\t\tIPADataSerializer::deserialize(data, cs);\n> +\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tstatic ControlList deserialize(std::vector::iterator it1,\n> +\t\t\t\t [[maybe_unused]] std::vector::iterator it2,\n> +\t\t\t\t ControlSerializer *cs)\n> +\t{\n> +\t\tControlList ret;\n> +\t\tconst ControlList &list = ret;\n> +\t\tconst_cast(list) =\n> +\t\t\tIPADataSerializer::deserialize(it1, it2, cs);\n\nWhy not\n const ControlList list =\n\t\t\tIPADataSerializer::deserialize(it1, it2, cs);\n return const_cast(list)\n\n> +\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tstatic ControlList deserialize(std::vector &data,\n> +\t\t\t\t [[maybe_unused]] std::vector &fds,\n> +\t\t\t\t ControlSerializer *cs)\n> +\t{\n> +\t\tControlList ret;\n> +\t\tconst ControlList &list = ret;\n> +\t\tconst_cast(list) =\n> +\t\t\tIPADataSerializer::deserialize(data, fds, cs);\n> +\n> +\t\treturn ret;\n\nsame question\n\n> +\t}\n> +\n> +\tstatic ControlList deserialize(std::vector::iterator data_it1,\n> +\t\t\t\t std::vector::iterator data_it2,\n> +\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it1,\n> +\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it2,\n> +\t\t\t\t ControlSerializer *cs)\n> +\t{\n> +\t\tControlList ret;\n> +\t\tconst ControlList &list = ret;\n> +\t\tconst_cast(list) =\n> +\t\t\tIPADataSerializer::deserialize(data_it1, data_it2,\n> +\t\t\t\t\t\t\t\t\t fds_it1, fds_it2, cs);\n\nsame question\n\n> +\n> +\t\treturn ret;\n> +\t}\n> +};\n> +\n> +template<>\n> +class IPADataSerializer\n> +{\n> +public:\n> +\tstatic std::tuple, std::vector>\n> +\tserialize(const ControlInfoMap &map, ControlSerializer *cs)\n> +\t{\n> +\t\tif (!cs)\n> +\t\t\tLOG(IPADataSerializer, Fatal)\n> +\t\t\t\t<< \"ControlSerializer not provided for serialization of ControlInfoMap\";\n> +\n> +\t\tsize_t size = cs->binarySize(map);\n> +\t\tstd::vector infoData(size);\n> +\t\tByteStreamBuffer buffer(infoData.data(), infoData.size());\n> +\t\tint ret = cs->serialize(map, buffer);\n> +\n> +\t\tif (ret < 0 || buffer.overflow())\n> +\t\t\treturn {{}, {}};\n> +\n> +\t\tstd::vector data_vec;\n> +\t\tappendUInt(data_vec, infoData.size());\n> +\t\tdata_vec.insert(data_vec.end(), infoData.begin(), infoData.end());\n> +\n> +\t\treturn {data_vec, {}};\n> +\t}\n> +\n> +\tstatic const ControlInfoMap deserialize(std::vector &data,\n> +\t\t\t\t\t\tControlSerializer *cs)\n> +\t{\n> +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end(), cs);\n> +\t}\n> +\n> +\tstatic const ControlInfoMap deserialize(std::vector::iterator it1,\n> +\t\t\t\t\t\t[[maybe_unused]] std::vector::iterator it2,\n> +\t\t\t\t\t\tControlSerializer *cs)\n> +\t{\n> +\t\tif (!cs)\n> +\t\t\tLOG(IPADataSerializer, Fatal)\n> +\t\t\t\t<< \"ControlSerializer not provided for deserialization of ControlInfoMap\";\n> +\n> +\t\tuint32_t infoDataSize = readUInt(it1);\n> +\n> +\t\tstd::vector::iterator it = it1 + 4;\n> +\n> +\t\tstd::vector infoData(it, it + infoDataSize);\n> +\n> +\t\tByteStreamBuffer buffer(const_cast(infoData.data()), infoData.size());\n> +\t\tconst ControlInfoMap map = cs->deserialize(buffer);\n> +\n> +\t\treturn map;\n> +\t}\n> +\n> +\tstatic const ControlInfoMap deserialize(std::vector &data,\n> +\t\t\t\t\t\t[[maybe_unused]] std::vector &fds,\n> +\t\t\t\t\t\tControlSerializer *cs)\n> +\t{\n> +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end(), cs);\n> +\t}\n> +\n> +\tstatic const ControlInfoMap deserialize(std::vector::iterator data_it1,\n> +\t\t\t\t\t std::vector::iterator data_it2,\n> +\t\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it1,\n> +\t\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it2,\n> +\t\t\t\t\t ControlSerializer *cs)\n> +\t{\n> +\t\treturn IPADataSerializer::deserialize(data_it1, data_it2, cs);\n> +\t}\n> +};\n> +\n> +template<>\n> +class IPADataSerializer\n> +{\n> +public:\n> +\tstatic std::tuple, std::vector>\n> +\tserialize(const ControlInfoMap &map, [[maybe_unused]] ControlSerializer *cs)\n> +\t{\n> +\t\tconst ControlInfoMap &map_const = const_cast(map);\n> +\n> +\t\tstd::vector data_vec;\n> +\t\tstd::tie(data_vec, std::ignore) =\n> +\t\t\tIPADataSerializer::serialize(map_const, cs);\n> +\n> +\t\treturn {data_vec, {}};\n> +\t}\n> +\n> +\tstatic ControlInfoMap deserialize(std::vector &data,\n> +\t\t\t\t\t ControlSerializer *cs)\n> +\t{\n> +\t\tControlInfoMap ret;\n> +\t\tconst ControlInfoMap &map = ret;\n> +\t\tconst_cast(map) =\n> +\t\t\tIPADataSerializer::deserialize(data, cs);\n> +\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tstatic ControlInfoMap deserialize(std::vector::iterator it1,\n> +\t\t\t\t\t\t[[maybe_unused]] std::vector::iterator it2,\n> +\t\t\t\t\t\tControlSerializer *cs)\n> +\t{\n> +\t\tControlInfoMap ret;\n> +\t\tconst ControlInfoMap &map = ret;\n> +\t\tconst_cast(map) =\n> +\t\t\tIPADataSerializer::deserialize(it1, it2, cs);\n> +\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tstatic ControlInfoMap deserialize(std::vector &data,\n> +\t\t\t\t\t\t[[maybe_unused]] std::vector &fds,\n> +\t\t\t\t\t\tControlSerializer *cs)\n> +\t{\n> +\t\tControlInfoMap ret;\n> +\t\tconst ControlInfoMap &map = ret;\n> +\t\tconst_cast(map) =\n> +\t\t\tIPADataSerializer::deserialize(data, fds, cs);\n> +\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tstatic ControlInfoMap deserialize(std::vector::iterator data_it1,\n> +\t\t\t\t\t std::vector::iterator data_it2,\n> +\t\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it1,\n> +\t\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it2,\n> +\t\t\t\t\t ControlSerializer *cs)\n> +\t{\n> +\t\tControlInfoMap ret;\n> +\t\tconst ControlInfoMap &map = ret;\n> +\t\tconst_cast(map) =\n> +\t\t\tIPADataSerializer::deserialize(data_it1, data_it2,\n> +\t\t\t\t\t\t\t\t\t fds_it1, fds_it2, cs);\n\nVery similar comment as per the ControlList implementation\n\n> +\n> +\t\treturn ret;\n> +\t}\n> +};\n> +\n> +template<>\n> +class IPADataSerializer\n> +{\n> +public:\n> +\tstatic std::tuple, std::vector>\n> +\tserialize(const FrameBuffer::Plane &data, [[maybe_unused]] ControlSerializer *cs = nullptr)\n> +\t{\n> +\t\tstd::vector data_vec;\n> +\t\tstd::vector fds_vec;\n> +\n> +\t\tstd::vector fdBuf;\n> +\t\tstd::vector fdFds;\n> +\t\tstd::tie(fdBuf, fdFds) =\n> +\t\t\tIPADataSerializer::serialize(data.fd);\n> +\t\tdata_vec.insert(data_vec.end(), fdBuf.begin(), fdBuf.end());\n> +\t\tfds_vec.insert(fds_vec.end(), fdFds.begin(), fdFds.end());\n> +\n> +\t\tappendUInt(data_vec, data.length);\n> +\n> +\t\treturn {data_vec, fds_vec};\n> +\t}\n> +\n> +\tstatic FrameBuffer::Plane deserialize(std::vector &data,\n> +\t\t\t\t\t std::vector &fds,\n> +\t\t\t\t\t ControlSerializer *cs = nullptr)\n> +\t{\n> +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end(),\n> +\t\t\t\t\t\t\t\t\t fds.begin(), fds.end(),\n> +\t\t\t\t\t\t\t\t\t cs);\n> +\t}\n> +\n> +\tstatic FrameBuffer::Plane deserialize(std::vector::iterator data_it1,\n> +\t\t\t\t\t [[maybe_unused]] std::vector::iterator data_it2,\n> +\t\t\t\t\t std::vector::iterator fds_it1,\n> +\t\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it2,\n> +\t\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> +\t{\n> +\t\tFrameBuffer::Plane ret;\n> +\n> +\t\tret.fd = IPADataSerializer::deserialize(data_it1, data_it1 + 1,\n> +\t\t\t\t\t\t\t\t\tfds_it1, fds_it1 + 1);\n> +\t\tret.length = readUInt(data_it1 + 1);\n> +\n> +\t\treturn ret;\n> +\t}\n> +};\n> +\n> +\n> +template<>\n> +class IPADataSerializer\n> +{\n> +public:\n> +\tstatic std::tuple, std::vector>\n> +\tserialize(const IPABuffer &data, ControlSerializer *cs = nullptr)\n> +\t{\n> +\t\tstd::vector data_vec;\n> +\n> +\t\tappendUInt(data_vec, data.id);\n> +\n> +\t\tstd::vector planes_data_vec;\n> +\t\tstd::vector planes_fds_vec;\n\nCalculating in advance the occupation might help but it's not trivial\n\n> +\t\tstd::tie(planes_data_vec, planes_fds_vec) =\n> +\t\t\tIPADataSerializer>::serialize(data.planes, cs);\n> +\n> +\t\tdata_vec.insert(data_vec.end(), planes_data_vec.begin(), planes_data_vec.end());\n> +\n> +\t\treturn {data_vec, planes_fds_vec};\n> +\t}\n> +\n> +\tstatic IPABuffer deserialize(std::vector &data,\n> +\t\t\t\t std::vector &fds,\n> +\t\t\t\t ControlSerializer *cs = nullptr)\n> +\t{\n> +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end(),\n> +\t\t\t\t\t\t\t\t fds.begin(), fds.end(), cs);\n> +\t}\n> +\n> +\tstatic IPABuffer deserialize(std::vector::iterator data_it1,\n> +\t\t\t\t std::vector::iterator data_it2,\n> +\t\t\t\t std::vector::iterator fds_it1,\n> +\t\t\t\t std::vector::iterator fds_it2,\n> +\t\t\t\t ControlSerializer *cs = nullptr)\n> +\t{\n> +\t\tIPABuffer ret;\n> +\n> +\t\tret.id = readUInt(data_it1);\n> +\n> +\t\tret.planes =\n> +\t\t\tIPADataSerializer>::deserialize(\n> +\t\t\t\tdata_it1 + 4, data_it2, fds_it1, fds_it2, cs);\n> +\n> +\t\treturn ret;\n> +\t}\n> +};\n> +\n> +#endif /* __DOXYGEN__ */\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_INTERNAL_IPA_DATA_SERIALIZER_H__ */\n> diff --git a/src/libcamera/ipa_data_serializer.cpp b/src/libcamera/ipa_data_serializer.cpp\n> new file mode 100644\n> index 00000000..5029cdf6\n> --- /dev/null\n> +++ b/src/libcamera/ipa_data_serializer.cpp\n> @@ -0,0 +1,154 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * ipa_data_serializer.cpp - Image Processing Algorithm data serializer\n> + */\n> +\n> +#include \"libcamera/internal/ipa_data_serializer.h\"\n> +\n> +#include \"libcamera/internal/log.h\"\n> +\n> +/**\n> + * \\file ipa_data_serializer.h\n> + * \\brief IPA Data Serializer\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(IPADataSerializer)\n> +\n> +/**\n> + * \\class IPADataSerializer\n> + * \\brief IPA Data Serializer\n> + *\n> + * Static template class that provides functions for serializing and\n> + * deserializing IPA data.\n> + */\n> +\n> +/**\n> + * \\fn template void appendUInt(std::vector &vec, T val)\n> + * \\brief Append uint to end of byte vector, in little-endian order\n> + * \\tparam T Type of uint to append\n> + * \\param[in] vec Byte vector to append to\n> + * \\param[in] val Value of uint to append\n\nAs said, this works for uint, int, float and double now\n\n> + */\n> +\n> +/**\n> + * \\fn template T readUInt(std::vector &vec, size_t pos)\n> + * \\brief Read uint from byte vector, in little-endian order\n> + * \\tparam T Type of uint to read\n> + * \\param[in] vec Byte vector to read from\n> + * \\param[in] pos Index in vec to start reading from\n> + *\n\nNo need for an empty line if no additional description is provided\n\n> + * \\return The uint read from \\a vec, or 0 if reading goes past end of \\a vec\n\nI wonder if we should not pass a T * as an output parameter and return\nan error code. Returning 0 makes it impossible to detect failure, as 0\nis a valid value\n\n> + */\n> +\n> +/**\n> + * \\fn template T readUInt(std::vector::iterator it)\n> + * \\brief Read uint from byte vector, in little-endian order\n> + * \\tparam T Type of uint to read\n> + * \\param[in] it Iterator of byte vector to read from\n> + *\n\nNo need to empty line\n\n> + * \\return The uint read from \\a vec\n> + */\n> +\n> +/**\n> + * \\fn template IPADataSerializer::serialize(\n> + * \tT data,\n> + * \tControlSerializer *cs = nullptr)\n> + * \\brief Serialize an object into byte vector and fd vector\n> + * \\tparam T Type of object to serialize\n> + * \\param[in] data Object to serialize\n> + * \\param[in] cs ControlSerializer\n> + *\n> + * \\a cs is only necessary if the object type \\a T or its members contain\n> + * ControlList or ControlInfoMap.\n> + *\n> + * \\return Tuple of byte vector and fd vector, that is the serialized form\n> + * of \\a data\n\nMaybe later, but I think for each template specialization, we want to\ndocument the serialization format, in example that for vectors, the\nlength is the first 4 bytes and so on (even more for custom libcamera\ndata types)\n\n> + */\n> +\n> +/**\n> + * \\fn template IPADataSerializer::deserialize(\n> + * \tstd::vector &data,\n> + * \tControlSerializer *cs = nullptr)\n> + * \\brief Deserialize byte vector into an object\n> + * \\tparam T Type of object to deserialize to\n> + * \\param[in] data Byte vector to deserialize from\n> + * \\param[in] cs ControlSerializer\n> + *\n> + * This version of deserialize() can be used if the object type \\a T and its\n> + * members don't have any FileDescriptor.\n> + *\n> + * \\a cs is only necessary if the object type \\a T or its members contain\n> + * ControlList or ControlInfoMap.\n> + *\n> + * \\return The deserialized object\n> + */\n> +\n> +/**\n> + * \\fn template IPADataSerializer::deserialize(\n> + * \tstd::vector::iterator it1,\n> + * \tstd::vector::iterator it2,\n> + * \tControlSerializer *cs = nullptr)\n> + * \\brief Deserialize byte vector into an object\n> + * \\tparam T Type of object to deserialize to\n> + * \\param[in] it1 Begin iterator of byte vector to deserialize from\n> + * \\param[in] it2 End iterator of byte vector to deserialize from\n> + * \\param[in] cs ControlSerializer\n> + *\n> + * This version of deserialize() can be used if the object type \\a T and its\n> + * members don't have any FileDescriptor.\n> + *\n> + * \\a cs is only necessary if the object type \\a T or its members contain\n> + * ControlList or ControlInfoMap.\n> + *\n> + * \\return The deserialized object\n> + */\n> +\n> +/**\n> + * \\fn template IPADataSerializer::deserialize(\n> + * \tstd::vector &data,\n> + * \tstd::vector &fds,\n> + * \tControlSerializer *cs = nullptr)\n> + * \\brief Deserialize byte vector and fd vector into an object\n> + * \\tparam T Type of object to deserialize to\n> + * \\param[in] data Byte vector to deserialize from\n> + * \\param[in] fds Fd vector to deserialize from\n> + * \\param[in] cs ControlSerializer\n> + *\n> + * This version of deserialize() (or the iterator version) must be used if\n> + * the object type \\a T or its members contain FileDescriptor.\n> + *\n> + * \\a cs is only necessary if the object type \\a T or its members contain\n> + * ControlList or ControlInfoMap.\n> + *\n> + * \\return The deserialized object\n> + */\n> +\n> +/**\n> + * \\fn template IPADataSerializer::deserialize(\n> + * \tstd::vector::iterator data_it1,\n> + * \tstd::vector::iterator data_it2,\n> + * \tstd::vector::iterator fds_it1,\n> + * \tstd::vector::iterator fds_it2,\n> + * \tControlSerializer *cs = nullptr)\n> + * \\brief Deserialize byte vector and fd vector into an object\n> + * \\tparam T Type of object to deserialize to\n> + * \\param[in] data_it1 Begin iterator of byte vector to deserialize from\n> + * \\param[in] data_it2 End iterator of byte vector to deserialize from\n> + * \\param[in] fds_it1 Begin iterator of fd vector to deserialize from\n> + * \\param[in] fds_it2 End iterator of fd vector to deserialize from\n> + * \\param[in] cs ControlSerializer\n> + *\n> + * This version of deserialize() (or the vector version) must be used if\n> + * the object type \\a T or its members contain FileDescriptor.\n> + *\n> + * \\a cs is only necessary if the object type \\a T or its members contain\n> + * ControlList or ControlInfoMap.\n> + *\n> + * \\return The deserialized object\n> + */\n> +\n\nPuff.. pant.. a lot of code to review work here! Nice one Paul!\n\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 07711b5f..190d7490 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -24,6 +24,7 @@ libcamera_sources = files([\n> 'geometry.cpp',\n> 'ipa_context_wrapper.cpp',\n> 'ipa_controls.cpp',\n> + 'ipa_data_serializer.cpp',\n> 'ipa_interface.cpp',\n> 'ipa_manager.cpp',\n> 'ipa_module.cpp',\n> --\n> 2.27.0\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 4D0DAC3B5C\n\tfor ;\n\tMon, 26 Oct 2020 16:07:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 99D1F61E6B;\n\tMon, 26 Oct 2020 17:07:55 +0100 (CET)","from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[217.70.183.200])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 927886034E\n\tfor ;\n\tMon, 26 Oct 2020 17:07:54 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay7-d.mail.gandi.net (Postfix) with ESMTPSA id EB05320003;\n\tMon, 26 Oct 2020 16:07:53 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Mon, 26 Oct 2020 17:07:52 +0100","From":"Jacopo Mondi ","To":"Paul Elder ","Message-ID":"<20201026160752.dtob6iq2tm5kkwg5@uno.localdomain>","References":"<20201002143154.468162-1-paul.elder@ideasonboard.com>\n\t<20201002143154.468162-11-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201002143154.468162-11-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 10/38] libcamera: Add\n\tIPADataSerializer","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13510,"web_url":"https://patchwork.libcamera.org/comment/13510/","msgid":"<20201027101403.GA2236@pyrite.rasen.tech>","date":"2020-10-27T10:14:03","subject":"Re: [libcamera-devel] [PATCH v3 10/38] libcamera: Add\n\tIPADataSerializer","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the thorough review!\n\nOn Mon, Oct 26, 2020 at 05:07:52PM +0100, Jacopo Mondi wrote:\n> Hi Paul,\n> \n> On Fri, Oct 02, 2020 at 11:31:26PM +0900, Paul Elder wrote:\n> > Add an IPADataSerializer which implments de/serialization of built-in\n> > (PODs, vector, map, string) and libcamera data structures. This is\n> > intended to be used by the proxy and the proxy worker in the IPC layer.\n> >\n> > Signed-off-by: Paul Elder \n> >\n> > ---\n> > Changes in v3:\n> > - reimplement append/readUInt with memcpy (intead of bit shifting)\n> > - change DECLARE_INTEGRAL_SERIALIZER with DECLARE_POD_SERIALIZER\n> > - use this for int64_t, bool, float, and double\n> > - fix comment style\n> >\n> > Changes in v2:\n> > - added serializers for all integer types, bool, and string\n> > - use string serializer for IPASettings serializer\n> > - add documentation\n> > - add serializer for const ControlList\n> > ---\n> > .../libcamera/internal/ipa_data_serializer.h | 1014 +++++++++++++++++\n> > src/libcamera/ipa_data_serializer.cpp | 154 +++\n> > src/libcamera/meson.build | 1 +\n> > 3 files changed, 1169 insertions(+)\n> > create mode 100644 include/libcamera/internal/ipa_data_serializer.h\n> > create mode 100644 src/libcamera/ipa_data_serializer.cpp\n> >\n> > diff --git a/include/libcamera/internal/ipa_data_serializer.h b/include/libcamera/internal/ipa_data_serializer.h\n> > new file mode 100644\n> > index 00000000..9cd35c4c\n> > --- /dev/null\n> > +++ b/include/libcamera/internal/ipa_data_serializer.h\n> > @@ -0,0 +1,1014 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Google Inc.\n> > + *\n> > + * ipa_data_serializer.h - Image Processing Algorithm data serializer\n> > + */\n> > +#ifndef __LIBCAMERA_INTERNAL_IPA_DATA_SERIALIZER_H__\n> > +#define __LIBCAMERA_INTERNAL_IPA_DATA_SERIALIZER_H__\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \"libcamera/internal/byte_stream_buffer.h\"\n> > +#include \"libcamera/internal/camera_sensor.h\"\n> > +#include \"libcamera/internal/control_serializer.h\"\n> > +#include \"libcamera/internal/log.h\"\n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DECLARE_CATEGORY(IPADataSerializer)\n> > +\n> \n> should these be wrapped in an anonymous namespace ? They are used by\n> the generated proxyes, but the header is included. I tried it locally\n> and complier seems to digest it\n\nAh yes, probably.\n\n> > +template\n> > +void appendUInt(std::vector &vec, T val)\n> > +{\n> > +\tsize_t byteWidth = sizeof(val);\n> > +\tstd::vector v(byteWidth);\n> > +\tmemcpy(&v[0], &val, byteWidth);\n> > +\n> > +\tvec.insert(vec.end(), v.begin(), v.end());\n> > +}\n> > +\n> > +template\n> > +T readUInt(std::vector &vec, size_t pos)\n> > +{\n> > +\tT ret = 0;\n> > +\tsize_t byteWidth = sizeof(ret);\n> > +\tif (pos + byteWidth > vec.size())\n> > +\t\treturn ret;\n> > +\n> > +\tmemcpy(&ret, &vec.data()[pos], byteWidth);\n> \n> Just &vec[pos] should be enough\n\nIt's fine even for memcpying?\n\n> > +\treturn ret;\n> > +}\n> > +\n> > +template\n> > +T readUInt(std::vector::iterator it)\n> > +{\n> > +\tT ret = 0;\n> > +\tsize_t byteWidth = sizeof(ret);\n> > +\n> > +\tstd::vector v(it, it + byteWidth);\n> > +\tmemcpy(&ret, v.data(), byteWidth);\n> > +\treturn ret;\n> > +}\n> \n> These new versions looks much better!\n\n\\o/\n\nAh, so later on you're saying that I should rename these, since they're\nnot limited to uints?\n\n> > +\n> > +template\n> > +class IPADataSerializer\n> > +{\n> > +#ifdef __DOXYGEN__\n> > +public:\n> > +\tstatic std::tuple, std::vector>\n> > +\tserialize(T data, ControlSerializer *cs);\n> > +\n> > +\tstatic T deserialize(std::vector &data,\n> > +\t\t\t ControlSerializer *cs);\n> > +\tstatic T deserialize(std::vector::iterator it1,\n> > +\t\t\t std::vector::iterator it2,\n> > +\t\t\t ControlSerializer *cs);\n> > +\n> \n> Intentional empty line ?\n\nYes, the first two are data only, and the bottom two are data+fd.\n\n> > +\tstatic T deserialize(std::vector &data,\n> > +\t\t\t std::vector &fds,\n> > +\t\t\t ControlSerializer *cs);\n> > +\tstatic T deserialize(std::vector::iterator data_it1,\n> > +\t\t\t std::vector::iterator data_it2,\n> > +\t\t\t std::vector::iterator fds_it1,\n> > +\t\t\t std::vector::iterator fds_it2,\n> > +\t\t\t ControlSerializer *cs);\n> \n> Aren't these better called data_start, data_end and fds_start, fds_end\n> ?\n\nYeah, that's better.\n\n> > +#endif /* __DOXYGEN__ */\n> \n> Do you need this ifdef __DOXYGEN__ ? you want these parsed, don't you ?\n> It's usually #ifndef __DOXYGEN__ to exclude from documentation parsing\n> some parts of the code, like you're done below\n\nI do need this ifdef. I want doxygen to document just the functions in\nthe base serializer, since it's the same interface for all serializers,\nbut I don't want the base serializer template to be compiled.\n\n> > +};\n> > +\n> > +#ifndef __DOXYGEN__\n> > +\n> > +template\n> > +class IPADataSerializer>\n> > +{\n> > +public:\n> > +\tstatic std::tuple, std::vector>\n> > +\tserialize(const std::vector &data, ControlSerializer *cs = nullptr)\n> > +\t{\n> > +\t\tstd::vector data_vec;\n> \n> We can reserve data.size() * sizeof(V) in the vector\n\nI'm not sure we can do that...\n\n> > +\t\tstd::vector fds_vec;\n> > +\n> > +\t\t/* Serialize the length. */\n> > +\t\tuint32_t vec_len = data.size();\n> > +\t\tappendUInt(data_vec, vec_len);\n> > +\n> > +\t\t/* Serialize the members. */\n> > +\t\tfor (auto it = data.begin(); it != data.end(); ++it) {\n> \n> for (auto const &it : data) ?\n\nack\n\n> > +\t\t\tstd::vector dvec;\n> > +\t\t\tstd::vector fvec;\n> > +\n> > +\t\t\tstd::tie(dvec, fvec) =\n> > +\t\t\t\tIPADataSerializer::serialize(*it, cs);\n> > +\n> > +\t\t\tappendUInt(data_vec, dvec.size());\n> \n> Why is the size (in bytes) of each serialized entry recorded in the\n> serialized data buffer ? Aren't all the members of the same size ?\n\n...because the members of the vector aren't necessarily the same size.\nThe vector could contains variable-size objects, like other vectors, or\nmaps, or strings, etc, so we need to tag the size of every element.\n\n> > +\t\t\tappendUInt(data_vec, fvec.size());\n> \n> Same question, more relevant as FileDescriptors are known to be\n> uin32_t\n\nSame reason, the elements of the vector could contain a variable number\nof FileDescriptors.\n\n> > +\n> > +\t\t\tdata_vec.insert(data_vec.end(), dvec.begin(), dvec.end());\n> > +\t\t\tfds_vec.insert(fds_vec.end(), fvec.begin(), fvec.end());\n> > +\t\t}\n> > +\n> > +\t\treturn {data_vec, fds_vec};\n> > +\t}\n> > +\n> > +\tstatic std::vector deserialize(std::vector &data, ControlSerializer *cs = nullptr)\n> > +\t{\n> > +\t\treturn IPADataSerializer>::deserialize(data.begin(), data.end(), cs);\n> \n> question: do you need to fully qualify the deserializer() function\n> call ? I tried removing \"IPADataSerializer>::\" and\n> compiler is still happy\n\nOh, maybe not. I remember I was having some confusion about which\ntemplate function gets called without the full qualification.\n\n> > +\t}\n> > +\n> > +\tstatic std::vector deserialize(std::vector::iterator it1,\n> > +\t\t\t\t\t std::vector::iterator it2,\n> > +\t\t\t\t\t ControlSerializer *cs = nullptr)\n> > +\t{\n> > +\t\tstd::vector fds;\n> > +\t\treturn IPADataSerializer>::deserialize(it1, it2,\n> > +\t\t\t\t\t\t\t\t fds.begin(), fds.end(),\n> > +\t\t\t\t\t\t\t\t cs);\n> > +\t}\n> > +\n> > +\tstatic std::vector deserialize(std::vector &data, std::vector &fds,\n> > +\t\t\t\t\t ControlSerializer *cs = nullptr)\n> > +\t{\n> > +\t\treturn IPADataSerializer>::deserialize(data.begin(), data.end(),\n> > +\t\t\t\t\t\t\t\t fds.begin(), fds.end(),\n> > +\t\t\t\t\t\t\t\t cs);\n> > +\t}\n> > +\n> > +\tstatic std::vector deserialize(std::vector::iterator data_it1,\n> > +\t\t\t\t\t [[maybe_unused]] std::vector::iterator data_it2,\n> > +\t\t\t\t\t std::vector::iterator fds_it1,\n> > +\t\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it2,\n> > +\t\t\t\t\t ControlSerializer *cs = nullptr)\n> > +\t{\n> > +\t\tuint32_t vec_len = readUInt(data_it1);\n> > +\t\tstd::vector ret(vec_len);\n> > +\n> > +\t\tstd::vector::iterator data_it = data_it1 + 4;\n> > +\t\tstd::vector::iterator fd_it = fds_it1;\n> > +\t\tfor (uint32_t i = 0; i < vec_len; i++) {\n> > +\t\t\tuint32_t sizeof_data = readUInt(data_it);\n> \n> Same questions as above, do we need to insert the entry size in the\n> serialized buffer, or can it just be assumed to be sizeof(V) ?\n\nSame answer as above, sizeof(V) might not be constant.\n\n> > +\t\t\tuint32_t sizeof_fds = readUInt(data_it + 4);\n> \n> And this one to be 4 bytes ?\n> \n> > +\n> > +\t\t\tret[i] = IPADataSerializer::deserialize(data_it + 8,\n> > +\t\t\t\t\t\t\t\t data_it + 8 + sizeof_data,\n> > +\t\t\t\t\t\t\t\t fd_it,\n> > +\t\t\t\t\t\t\t\t fd_it + sizeof_fds,\n> > +\t\t\t\t\t\t\t\t cs);\n> > +\n> > +\t\t\tdata_it += 8 + sizeof_data;\n> > +\t\t\tfd_it += sizeof_fds;\n> > +\t\t}\n> > +\n> > +\t\treturn ret;\n> > +\t}\n> > +};\n> > +\n> > +template\n> > +class IPADataSerializer>\n> > +{\n> > +public:\n> > +\tstatic std::tuple, std::vector>\n> > +\tserialize(const std::map &data, ControlSerializer *cs = nullptr)\n> > +\t{\n> > +\t\tstd::vector data_vec;\n> > +\t\tstd::vector fds_vec;\n> > +\n> > +\t\t/* Serialize the length. */\n> > +\t\tuint32_t map_len = data.size();\n> > +\t\tappendUInt(data_vec, map_len);\n> > +\n> > +\t\t/* Serialize the members. */\n> > +\t\tfor (auto it = data.begin(); it != data.end(); ++it) {\n> \n> Same comment: a range loop might be nicer\n> for (auto const &it : data)\n\nack\n\n> > +\t\t\tstd::vector dvec;\n> > +\t\t\tstd::vector fvec;\n> > +\n> > +\t\t\tstd::tie(dvec, fvec) =\n> > +\t\t\t\tIPADataSerializer::serialize(it->first, cs);\n> > +\n> > +\t\t\tappendUInt(data_vec, dvec.size());\n> > +\t\t\tappendUInt(data_vec, fvec.size());\n> > +\n> > +\t\t\tdata_vec.insert(data_vec.end(), dvec.begin(), dvec.end());\n> > +\t\t\tfds_vec.insert(fds_vec.end(), fvec.begin(), fvec.end());\n> > +\n> > +\t\t\tstd::tie(dvec, fvec) =\n> > +\t\t\t\tIPADataSerializer::serialize(it->second, cs);\n> > +\n> > +\t\t\tappendUInt(data_vec, dvec.size());\n> > +\t\t\tappendUInt(data_vec, fvec.size());\n> > +\n> > +\t\t\tdata_vec.insert(data_vec.end(), dvec.begin(), dvec.end());\n> > +\t\t\tfds_vec.insert(fds_vec.end(), fvec.begin(), fvec.end());\n> > +\t\t}\n> > +\n> > +\t\treturn {data_vec, fds_vec};\n> > +\t}\n> > +\n> > +\tstatic std::map deserialize(std::vector &data, ControlSerializer *cs = nullptr)\n> > +\t{\n> > +\t\treturn IPADataSerializer>::deserialize(data.begin(), data.end(), cs);\n> > +\t}\n> > +\n> > +\tstatic std::map deserialize(std::vector::iterator it1,\n> > +\t\t\t\t\t std::vector::iterator it2,\n> > +\t\t\t\t\t ControlSerializer *cs = nullptr)\n> > +\t{\n> > +\t\tstd::vector fds;\n> > +\t\treturn IPADataSerializer>::deserialize(it1, it2,\n> > +\t\t\t\t\t\t\t\t fds.begin(), fds.end(),\n> > +\t\t\t\t\t\t\t\t cs);\n> > +\t}\n> > +\n> > +\tstatic std::map deserialize(std::vector &data, std::vector &fds,\n> > +\t\t\t\t\t ControlSerializer *cs = nullptr)\n> > +\t{\n> > +\t\treturn IPADataSerializer>::deserialize(data.begin(), data.end(),\n> > +\t\t\t\t\t\t\t\t fds.begin(), fds.end(),\n> > +\t\t\t\t\t\t\t\t cs);\n> > +\t}\n> \n> Same comments on the fully qualified path\n\nack\n\n> > +\n> > +\tstatic std::map deserialize(std::vector::iterator data_it1,\n> > +\t\t\t\t\t [[maybe_unused]] std::vector::iterator data_it2,\n> > +\t\t\t\t\t std::vector::iterator fds_it1,\n> > +\t\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it2,\n> > +\t\t\t\t\t ControlSerializer *cs = nullptr)\n> > +\t{\n> > +\t\tstd::map ret;\n> > +\n> > +\t\tuint32_t map_len = readUInt(data_it1);\n> > +\n> > +\t\tstd::vector::iterator data_it = data_it1 + 4;\n> > +\t\tstd::vector::iterator fd_it = fds_it1;\n> > +\t\tfor (uint32_t i = 0; i < map_len; i++) {\n> > +\t\t\tuint32_t sizeof_data = readUInt(data_it);\n> > +\t\t\tuint32_t sizeof_fds = readUInt(data_it + 4);\n> > +\n> > +\t\t\tK key = IPADataSerializer::deserialize(data_it + 8,\n> > +\t\t\t\t\t\t\t\t data_it + 8 + sizeof_data,\n> > +\t\t\t\t\t\t\t\t fd_it,\n> > +\t\t\t\t\t\t\t\t fd_it + sizeof_fds,\n> > +\t\t\t\t\t\t\t\t cs);\n> > +\n> > +\t\t\tdata_it += 8 + sizeof_data;\n> > +\t\t\tfd_it += sizeof_fds;\n> > +\t\t\tsizeof_data = readUInt(data_it);\n> > +\t\t\tsizeof_fds = readUInt(data_it + 4);\n> > +\n> > +\t\t\tconst V value = IPADataSerializer::deserialize(data_it + 8,\n> > +\t\t\t\t\t\t\t\t\t data_it + 8 + sizeof_data,\n> > +\t\t\t\t\t\t\t\t\t fd_it,\n> > +\t\t\t\t\t\t\t\t\t fd_it + sizeof_fds,\n> > +\t\t\t\t\t\t\t\t\t cs);\n> > +\t\t\tret.insert({key, value});\n> > +\n> > +\t\t\tdata_it += 8 + sizeof_data;\n> > +\t\t\tfd_it += sizeof_fds;\n> > +\t\t}\n> > +\n> > +\t\treturn ret;\n> > +\t}\n> > +};\n> > +\n> > +#define DECLARE_POD_SERIALIZER(type)\t\t\t\t\t\\\n> > +template<>\t\t\t\t\t\t\t\t\\\n> > +class IPADataSerializer\t\t\t\t\t\t\\\n> > +{\t\t\t\t\t\t\t\t\t\\\n> > +public:\t\t\t\t\t\t\t\t\t\\\n> > +\tstatic std::tuple, std::vector>\t\\\n> > +\tserialize(const type data,\t\t\t\t\t\\\n> > +\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\t\\\n> > +\t{\t\t\t\t\t\t\t\t\\\n> > +\t\tstd::vector data_vec;\t\t\t\t\\\n> \n> Do you think pre-allocating sizeof(type) in the vector might help ?\n\ntbh, yeah.\n\n> > +\t\tappendUInt(data_vec, data);\t\t\t\\\n\nOh, then this needs a writeUInt cousin.\n\n> > +\t\t\t\t\t\t\t\t\t\\\n> > +\t\treturn {data_vec, {}};\t\t\t\t\t\\\n> > +\t}\t\t\t\t\t\t\t\t\\\n> > +\t\t\t\t\t\t\t\t\t\\\n> > +\tstatic type deserialize(std::vector &data,\t\t\\\n> > +\t\t\t\t[[maybe_unused]] ControlSerializer *cs = nullptr)\\\n> > +\t{\t\t\t\t\t\t\t\t\\\n> > +\t\treturn IPADataSerializer::deserialize(data.begin(),\\\n> > +\t\t\t\t\t\t\t data.end());\\\n> > +\t}\t\t\t\t\t\t\t\t\\\n> > +\t\t\t\t\t\t\t\t\t\\\n> > +\tstatic type deserialize(std::vector::iterator it1,\t\\\n> > +\t\t\t\t[[maybe_unused]] std::vector::iterator it2,\\\n> > +\t\t\t\t[[maybe_unused]] ControlSerializer *cs = nullptr)\\\n> > +\t{\t\t\t\t\t\t\t\t\\\n> > +\t\treturn readUInt(it1);\t\t\t\t\\\n> > +\t}\t\t\t\t\t\t\t\t\\\n> > +\t\t\t\t\t\t\t\t\t\\\n> > +\tstatic type deserialize(std::vector &data,\t\t\\\n> > +\t\t\t\t[[maybe_unused]] std::vector &fds,\\\n> > +\t\t\t\t[[maybe_unused]] ControlSerializer *cs = nullptr)\\\n> > +\t{\t\t\t\t\t\t\t\t\\\n> > +\t\treturn IPADataSerializer::deserialize(data.begin(),\\\n> > +\t\t\t\t\t\t\t data.end());\\\n> > +\t}\t\t\t\t\t\t\t\t\\\n> > +\t\t\t\t\t\t\t\t\t\\\n> > +\tstatic type deserialize(std::vector::iterator data_it1,\\\n> > +\t\t\t\tstd::vector::iterator data_it2,\\\n> > +\t\t\t\t[[maybe_unused]] std::vector::iterator fds_it1,\\\n> > +\t\t\t\t[[maybe_unused]] std::vector::iterator fds_it2,\\\n> > +\t\t\t\t[[maybe_unused]] ControlSerializer *cs = nullptr)\\\n> > +\t{\t\t\t\t\t\t\t\t\\\n> > +\t\treturn IPADataSerializer::deserialize(data_it1,\t\\\n> > +\t\t\t\t\t\t\t data_it2);\t\\\n> > +\t}\t\t\t\t\t\t\t\t\\\n> > +};\n> > +\n> > +DECLARE_POD_SERIALIZER(bool)\n> > +DECLARE_POD_SERIALIZER(uint8_t)\n> > +DECLARE_POD_SERIALIZER(uint16_t)\n> > +DECLARE_POD_SERIALIZER(uint32_t)\n> > +DECLARE_POD_SERIALIZER(uint64_t)\n> > +DECLARE_POD_SERIALIZER(int8_t)\n> > +DECLARE_POD_SERIALIZER(int16_t)\n> > +DECLARE_POD_SERIALIZER(int32_t)\n> > +DECLARE_POD_SERIALIZER(int64_t)\n> \n> The I wonder if keeping here and in the documentation the term 'uint'\n> is correct and it shouldn't just be replaced with 'int'\n\nI'm guessing you're referring to {append,read}UInt()?\n\n> > +DECLARE_POD_SERIALIZER(float)\n> > +DECLARE_POD_SERIALIZER(double)\n> \n> Or even POD\n\nI'm not sure what you're referring to here :/\n\n> > +\n> > +template<>\n> > +class IPADataSerializer\n> > +{\n> > +public:\n> > +\tstatic std::tuple, std::vector>\n> > +\tserialize(const std::string &data, [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > +\t{\n> > +\t\tstd::vector data_vec(data.begin(), data.end());\n> > +\n> > +\t\treturn {data_vec, {}};\n> > +\t}\n> > +\n> > +\tstatic std::string deserialize(std::vector &data,\n> > +\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > +\t{\n> > +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end());\n> > +\t}\n> > +\n> > +\tstatic std::string deserialize(std::vector::iterator it1,\n> > +\t\t\t\t std::vector::iterator it2,\n> > +\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > +\t{\n> > +\t\tstd::string str(it1, it2);\n> > +\n> > +\t\treturn str;\n> \n> Can you just\n> return {it1, it2};\n> ?\n\nAh, yes. I had it (and everything below) implemented in this manner\nbecause I wanted to algorithmize the serdes code generation, so I was\npracticing it on myself here. I guess it's about time for optimizations;\nthank you for the pointers.\n\n> > +\t}\n> > +\n> > +\tstatic std::string deserialize(std::vector &data,\n> > +\t\t\t\t [[maybe_unused]] std::vector &fds,\n> > +\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > +\t{\n> > +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end());\n> \n> Maybe\n> return {data.being(), data.end()};\n> \n> would save a function call ?\n> Same for other overloaded methods (and possibly not only in the\n> specialization).\n> \n\nack\n\n> > +\t}\n> > +\n> > +\tstatic std::string deserialize(std::vector::iterator data_it1,\n> > +\t\t\t\t std::vector::iterator data_it2,\n> > +\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it1,\n> > +\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it2,\n> > +\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > +\t{\n> > +\t\treturn IPADataSerializer::deserialize(data_it1, data_it2);\n> > +\t}\n> > +};\n> > +\n> > +template<>\n> > +class IPADataSerializer\n> > +{\n> > +public:\n> > +\tstatic std::tuple, std::vector>\n> > +\tserialize(const FileDescriptor &data, [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > +\t{\n> > +\t\tstd::vector data_vec = { data.isValid() };\n> > +\t\tstd::vector fd_vec;\n> > +\t\tif (data.isValid())\n> > +\t\t\tfd_vec.push_back(data.fd());\n> > +\n> > +\t\treturn {data_vec, fd_vec};\n> > +\t}\n> > +\n> > +\tstatic FileDescriptor deserialize(std::vector &data, std::vector &fds,\n> > +\t\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > +\t{\n> > +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end(),\n> > +\t\t\t\t\t\t\t\t fds.begin(), fds.end());\n> > +\t}\n> > +\n> > +\tstatic FileDescriptor deserialize(std::vector::iterator data_it1,\n> > +\t\t\t\t\t std::vector::iterator data_it2,\n> > +\t\t\t\t\t std::vector::iterator fds_it1,\n> > +\t\t\t\t\t std::vector::iterator fds_it2,\n> > +\t\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > +\t{\n> > +\t\tif (std::distance(data_it1, data_it2) < 1)\n> > +\t\t\tLOG(IPADataSerializer, Fatal)\n> > +\t\t\t\t<< \"Invalid data to deserialize FileDescriptor\";\n> > +\n> > +\t\tbool valid = *data_it1;\n> \n> in C this would be\n> bool valid = !!*data_it1;\n> not sure about C++ and not even sure if the !! is just a paranoid\n> kernel convention, as I don't think even C requires that\n\nOh... I'm not sure. It passed my de/serialization tests.\n\n> > +\n> > +\t\tif (valid && std::distance(fds_it1, fds_it2) < 1)\n> \n> \t\tif (valid && std::distance(fds_it1, fds_it2) < 1) {\n> \t\t\tLOG(IPADataSerializer, Fatal)\n> \t\t\t\t<< \"Invalid fds to deserialize FileDescriptor\";\n> return {}\n> }\n> \n> return FileDescriptor(*fds_it1);\n> \n> Or is returning {} when std::distance() < 1 wrong ? I don't think so,\n> as even if it is \"valid\" there's not fds serialized.\n\nIf std::distance() < 1 then *fds_it1 will segfault. So if valid and\nstd::distance() < 1, then there's a big problem -> fatal. If not valid,\nthen std::distance() doesn't matter, and we return a new uninitialized\nFileDescriptor, but we can't construct it using *fds_it1, since the\niterator shouldn't be valid.\n\nIt's perfectly fine to send an invalid FileDescriptor, for example if\nthe FileDescriptor field isn't used, or uninitialized. The problem with\nsending it directly is that sendmsg() EINVALs if we try to send -1, so\nwe need this valid flag in the data vector.\n\n> \n> > +\t\t\tLOG(IPADataSerializer, Fatal)\n> > +\t\t\t\t<< \"Invalid fds to deserialize FileDescriptor\";\n> > +\n> > +\t\treturn valid ? FileDescriptor(*fds_it1) : FileDescriptor();\n> > +\t}\n> > +};\n> > +\n> > +template<>\n> > +class IPADataSerializer\n> > +{\n> > +public:\n> > +\tstatic std::tuple, std::vector>\n> > +\tserialize(const IPASettings &data, [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > +\t{\n> > +\t\treturn IPADataSerializer::serialize(data.configurationFile);\n> > +\t}\n> > +\n> > +\tstatic IPASettings deserialize(std::vector &data,\n> > +\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > +\t{\n> > +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end());\n> > +\t}\n> > +\n> > +\tstatic IPASettings deserialize(std::vector::iterator it1,\n> > +\t\t\t\t std::vector::iterator it2,\n> > +\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > +\t{\n> > +\t\tIPASettings ret;\n> > +\t\tret.configurationFile = IPADataSerializer::deserialize(it1, it2);\n> > +\n> > +\t\treturn ret;\n> \n> return { IPADataSerializer::deserialize(it1, it2) };\n> \n> ?\n\nAh yes, much needed optimization.\n\n> > +\t}\n> > +\n> > +\tstatic IPASettings deserialize(std::vector &data,\n> > +\t\t\t\t [[maybe_unused]] std::vector &fds,\n> > +\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > +\t{\n> > +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end());\n> \n> return { IPADataSerializer::deserialize(data.begin(), data.end() };\n> \n> ?\n\nack\n\n> > +\t}\n> > +\n> > +\tstatic IPASettings deserialize(std::vector::iterator data_it1,\n> > +\t\t\t\t std::vector::iterator data_it2,\n> > +\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it1,\n> > +\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it2,\n> > +\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > +\t{\n> > +\t\treturn IPADataSerializer::deserialize(data_it1, data_it2);\n> \n> same\n\nack\n\n> > +\t}\n> > +};\n> > +\n> > +template<>\n> > +class IPADataSerializer\n> > +{\n> > +public:\n> > +\tstatic std::tuple, std::vector>\n> > +\tserialize(const CameraSensorInfo &data, [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > +\t{\n> > +\t\tstd::vector data_vec;\n> \n> Reserving sizeof(CameraSensorInfo) might help\n\nIt's got a string in it though, so we don't know sizeof(CameraSensorInfo).\n\n> I just noticed and I wonder why all this code uses this_naming_style\n> in place of the libcamera standard namingStyle\n\nAh yes, I should probably fix that...\n\n> > +\n> > +\t\tuint32_t str_len = data.model.size();\n> > +\t\tappendUInt(data_vec, str_len);\n> > +\n> > +\t\tdata_vec.insert(data_vec.end(), data.model.begin(), data.model.end());\n> \n> Shouldn't this be serialized using the IPADataSerializer\n> if it's not needed, do we need the specialization then ?\n\nIt should be :)\n\n> > +\n> > +\t\tappendUInt(data_vec, data.bitsPerPixel);\n> > +\n> > +\t\tappendUInt(data_vec, data.activeAreaSize.width);\n> > +\t\tappendUInt(data_vec, data.activeAreaSize.height);\n> > +\n> > +\t\tappendUInt(data_vec, static_cast(data.analogCrop.x));\n> > +\t\tappendUInt(data_vec, static_cast(data.analogCrop.y));\n> > +\t\tappendUInt(data_vec, data.analogCrop.width);\n> > +\t\tappendUInt(data_vec, data.analogCrop.height);\n> > +\n> > +\t\tappendUInt(data_vec, data.outputSize.width);\n> > +\t\tappendUInt(data_vec, data.outputSize.height);\n> > +\n> > +\t\tappendUInt(data_vec, data.pixelRate);\n> > +\n> > +\t\tappendUInt(data_vec, data.lineLength);\n> > +\n> > +\t\treturn {data_vec, {}};\n> > +\t}\n> > +\n> > +\tstatic CameraSensorInfo deserialize(std::vector &data,\n> > +\t\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > +\t{\n> > +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end());\n> > +\t}\n> > +\n> > +\tstatic CameraSensorInfo deserialize(std::vector::iterator it1,\n> > +\t\t\t\t\t [[maybe_unused]] std::vector::iterator it2,\n> > +\t\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > +\t{\n> > +\t\tCameraSensorInfo ret;\n> CameraSensorInfo ret{};\n> \n> Also for other statically allocated fields that are returned by other\n> specializations.\n\nDo we need to, since all the fields are populated below?\n\n> > +\n> > +\t\tuint32_t str_len = readUInt(it1);\n> > +\t\tstd::string str(it1 + 4, it1 + 4 + str_len);\n> > +\t\tret.model = str;\n> > +\n> > +\t\tstd::vector::iterator it = it1 + 4 + str_len;\n> > +\n> > +\t\tret.bitsPerPixel = readUInt(it);\n> > +\n> > +\t\tret.activeAreaSize.width = readUInt(it + 4);\n> > +\t\tret.activeAreaSize.height = readUInt(it + 8);\n> > +\n> > +\t\tret.analogCrop.x = static_cast(readUInt(it + 12));\n> > +\t\tret.analogCrop.y = static_cast(readUInt(it + 16));\n> > +\t\tret.analogCrop.width = readUInt(it + 20);\n> > +\t\tret.analogCrop.height = readUInt(it + 24);\n> > +\n> > +\t\tret.outputSize.width = readUInt(it + 28);\n> > +\t\tret.outputSize.height = readUInt(it + 32);\n> > +\n> > +\t\tret.pixelRate = readUInt(it + 36);\n> > +\n> > +\t\tret.lineLength = readUInt(it + 44);\n> \n> lineLength is a 32 bits integer\n\nAh, yes.\n\n> > +\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\tstatic CameraSensorInfo deserialize(std::vector &data,\n> > +\t\t\t\t\t [[maybe_unused]] std::vector &fds,\n> > +\t\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > +\t{\n> > +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end());\n> > +\t}\n> > +\n> > +\tstatic CameraSensorInfo deserialize(std::vector::iterator data_it1,\n> > +\t\t\t\t\t std::vector::iterator data_it2,\n> > +\t\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it1,\n> > +\t\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it2,\n> > +\t\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > +\t{\n> > +\t\treturn IPADataSerializer::deserialize(data_it1, data_it2);\n> > +\t}\n> > +};\n> > +\n> > +template<>\n> > +class IPADataSerializer\n> > +{\n> > +public:\n> > +\tstatic std::tuple, std::vector>\n> > +\tserialize(const IPAStream &data, [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > +\t{\n> > +\t\tstd::vector data_vec;\n> > +\n> > +\t\tappendUInt(data_vec, data.pixelFormat);\n> > +\n> > +\t\tappendUInt(data_vec, data.size.width);\n> > +\t\tappendUInt(data_vec, data.size.height);\n> > +\n> > +\t\treturn {data_vec, {}};\n> > +\t}\n> > +\n> > +\tstatic IPAStream deserialize(std::vector &data,\n> > +\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > +\t{\n> > +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end());\n> > +\t}\n> > +\n> > +\tstatic IPAStream deserialize(std::vector::iterator it1,\n> > +\t\t\t\t [[maybe_unused]] std::vector::iterator it2,\n> > +\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > +\t{\n> > +\t\tIPAStream ret;\n> > +\n> > +\t\tret.pixelFormat = readUInt(it1);\n> > +\n> > +\t\tret.size.width = readUInt(it1 + 4);\n> > +\t\tret.size.height = readUInt(it1 + 8);\n> > +\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\tstatic IPAStream deserialize(std::vector &data,\n> > +\t\t\t\t [[maybe_unused]] std::vector &fds,\n> > +\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > +\t{\n> > +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end());\n> > +\t}\n> > +\n> > +\tstatic IPAStream deserialize(std::vector::iterator data_it1,\n> > +\t\t\t\t std::vector::iterator data_it2,\n> > +\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it1,\n> > +\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it2,\n> > +\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > +\t{\n> > +\t\treturn IPADataSerializer::deserialize(data_it1, data_it2);\n> > +\t}\n> > +};\n> > +\n> > +template<>\n> > +class IPADataSerializer\n> > +{\n> > +public:\n> > +\t/* map arg will be generated, since it's per-pipeline anyway. */\n> > +\tstatic std::tuple, std::vector>\n> > +\tserialize(const ControlList &data, const ControlInfoMap &map,\n> > +\t\t ControlSerializer *cs)\n> > +\t{\n> > +\t\tif (!cs)\n> > +\t\t\tLOG(IPADataSerializer, Fatal)\n> > +\t\t\t\t<< \"ControlSerializer not provided for serialization of ControlList\";\n> > +\n> > +\t\tsize_t size = cs->binarySize(map);\n> > +\t\tstd::vector infoData(size);\n> > +\t\tByteStreamBuffer buffer(infoData.data(), infoData.size());\n> > +\t\tint ret = cs->serialize(map, buffer);\n> > +\n> > +\t\tif (ret < 0 || buffer.overflow()) {\n> > +\t\t\tLOG(IPADataSerializer, Error) << \"Failed to serialize ControlList's ControlInfoMap\";\n> > +\t\t\treturn {{}, {}};\n> > +\t\t}\n> > +\n> > +\t\tsize = cs->binarySize(data);\n> > +\t\tstd::vector listData(size);\n> > +\t\tbuffer = ByteStreamBuffer(listData.data(), listData.size());\n> > +\t\tret = cs->serialize(data, buffer);\n> > +\n> > +\t\tif (ret < 0 || buffer.overflow()) {\n> > +\t\t\tLOG(IPADataSerializer, Error) << \"Failed to serialize ControlList\";\n> > +\t\t\treturn {{}, {}};\n> > +\t\t}\n> > +\n> > +\t\tstd::vector data_vec;\n> \n> You know the sizes and can reserve ?\n\nAt this point. yes.\n\n> > +\t\tappendUInt(data_vec, infoData.size());\n> > +\t\tappendUInt(data_vec, listData.size());\n> > +\t\tdata_vec.insert(data_vec.end(), infoData.begin(), infoData.end());\n> > +\t\tdata_vec.insert(data_vec.end(), listData.begin(), listData.end());\n> > +\n> > +\t\treturn {data_vec, {}};\n> > +\t}\n> > +\n> > +\tstatic const ControlList deserialize(std::vector &data, ControlSerializer *cs)\n> > +\t{\n> > +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end(), cs);\n> > +\t}\n> > +\n> > +\tstatic const ControlList deserialize(std::vector::iterator it1,\n> > +\t\t\t\t [[maybe_unused]] std::vector::iterator it2,\n> > +\t\t\t\t ControlSerializer *cs)\n> > +\t{\n> > +\t\tif (!cs)\n> > +\t\t\tLOG(IPADataSerializer, Fatal)\n> > +\t\t\t\t<< \"ControlSerializer not provided for deserialization of ControlList\";\n> > +\n> > +\t\tuint32_t infoDataSize = readUInt(it1);\n> > +\t\tuint32_t listDataSize = readUInt(it1 + 4);\n> > +\n> > +\t\tstd::vector::iterator it = it1 + 8;\n> > +\n> > +\t\tstd::vector infoData(it, it + infoDataSize);\n> > +\t\tstd::vector listData(it + infoDataSize, it + infoDataSize + listDataSize);\n> > +\n> > +\t\tByteStreamBuffer buffer(const_cast(infoData.data()), infoData.size());\n> > +\t\tControlInfoMap map = cs->deserialize(buffer);\n> > +\t\t/* It's fine if map is empty. */\n> > +\t\tif (buffer.overflow()) {\n> > +\t\t\tLOG(IPADataSerializer, Error)\n> > +\t\t\t\t<< \"Failed to deserialize ControlLists's ControlInfoMap: buffer overflow\";\n> > +\t\t\treturn ControlList();\n> > +\t\t}\n> > +\n> > +\t\tbuffer = ByteStreamBuffer(const_cast(listData.data()), listData.size());\n> > +\t\tControlList list = cs->deserialize(buffer);\n> > +\t\tif (buffer.overflow())\n> > +\t\t\tLOG(IPADataSerializer, Error) << \"Failed to deserialize ControlList: buffer overflow\";\n> > +\t\tif (list.empty())\n> > +\t\t\tLOG(IPADataSerializer, Error) << \"Failed to deserialize ControlList: empty list\";\n> \n> Should we return {} in these cases or is it fine ?\n\nControlSerializer::deserialize() returns {} in these cases anyway, so it\nshould be fine.\n\n> > +\n> > +\t\treturn list;\n> > +\t}\n> > +\n> > +\tstatic const ControlList deserialize(std::vector &data,\n> > +\t\t\t\t [[maybe_unused]] std::vector &fds,\n> > +\t\t\t\t ControlSerializer *cs)\n> > +\t{\n> > +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end(), cs);\n> > +\t}\n> > +\n> > +\tstatic const ControlList deserialize(std::vector::iterator data_it1,\n> > +\t\t\t\t std::vector::iterator data_it2,\n> > +\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it1,\n> > +\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it2,\n> > +\t\t\t\t ControlSerializer *cs)\n> > +\t{\n> > +\t\treturn IPADataSerializer::deserialize(data_it1, data_it2, cs);\n> > +\t}\n> > +};\n> > +\n> > +template<>\n> > +class IPADataSerializer\n> > +{\n> > +public:\n> > +\tstatic std::tuple, std::vector>\n> > +\tserialize(const ControlList &data, const ControlInfoMap &map,\n> > +\t\t ControlSerializer *cs)\n> > +\t{\n> > +\t\tconst ControlList &list_const = const_cast(data);\n> > +\n> > +\t\tstd::vector data_vec;\n> > +\t\tstd::tie(data_vec, std::ignore) =\n> > +\t\t\tIPADataSerializer::serialize(list_const, map, cs);\n> > +\n> > +\t\treturn {data_vec, {}};\n> > +\t}\n> > +\n> > +\tstatic ControlList deserialize(std::vector &data,\n> > +\t\t\t\t ControlSerializer *cs)\n> > +\t{\n> > +\t\tControlList ret;\n> > +\t\tconst ControlList &list = ret;\n> > +\t\tconst_cast(list) =\n> > +\t\t\tIPADataSerializer::deserialize(data, cs);\n> > +\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\tstatic ControlList deserialize(std::vector::iterator it1,\n> > +\t\t\t\t [[maybe_unused]] std::vector::iterator it2,\n> > +\t\t\t\t ControlSerializer *cs)\n> > +\t{\n> > +\t\tControlList ret;\n> > +\t\tconst ControlList &list = ret;\n> > +\t\tconst_cast(list) =\n> > +\t\t\tIPADataSerializer::deserialize(it1, it2, cs);\n> \n> Why not\n> const ControlList list =\n> \t\t\tIPADataSerializer::deserialize(it1, it2, cs);\n> return const_cast(list)\n\nMy understanding is that to remove a const with const_cast, the original\nvariable must not be const.\n\n> > +\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\tstatic ControlList deserialize(std::vector &data,\n> > +\t\t\t\t [[maybe_unused]] std::vector &fds,\n> > +\t\t\t\t ControlSerializer *cs)\n> > +\t{\n> > +\t\tControlList ret;\n> > +\t\tconst ControlList &list = ret;\n> > +\t\tconst_cast(list) =\n> > +\t\t\tIPADataSerializer::deserialize(data, fds, cs);\n> > +\n> > +\t\treturn ret;\n> \n> same question\n> \n> > +\t}\n> > +\n> > +\tstatic ControlList deserialize(std::vector::iterator data_it1,\n> > +\t\t\t\t std::vector::iterator data_it2,\n> > +\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it1,\n> > +\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it2,\n> > +\t\t\t\t ControlSerializer *cs)\n> > +\t{\n> > +\t\tControlList ret;\n> > +\t\tconst ControlList &list = ret;\n> > +\t\tconst_cast(list) =\n> > +\t\t\tIPADataSerializer::deserialize(data_it1, data_it2,\n> > +\t\t\t\t\t\t\t\t\t fds_it1, fds_it2, cs);\n> \n> same question\n> \n> > +\n> > +\t\treturn ret;\n> > +\t}\n> > +};\n> > +\n> > +template<>\n> > +class IPADataSerializer\n> > +{\n> > +public:\n> > +\tstatic std::tuple, std::vector>\n> > +\tserialize(const ControlInfoMap &map, ControlSerializer *cs)\n> > +\t{\n> > +\t\tif (!cs)\n> > +\t\t\tLOG(IPADataSerializer, Fatal)\n> > +\t\t\t\t<< \"ControlSerializer not provided for serialization of ControlInfoMap\";\n> > +\n> > +\t\tsize_t size = cs->binarySize(map);\n> > +\t\tstd::vector infoData(size);\n> > +\t\tByteStreamBuffer buffer(infoData.data(), infoData.size());\n> > +\t\tint ret = cs->serialize(map, buffer);\n> > +\n> > +\t\tif (ret < 0 || buffer.overflow())\n> > +\t\t\treturn {{}, {}};\n> > +\n> > +\t\tstd::vector data_vec;\n> > +\t\tappendUInt(data_vec, infoData.size());\n> > +\t\tdata_vec.insert(data_vec.end(), infoData.begin(), infoData.end());\n> > +\n> > +\t\treturn {data_vec, {}};\n> > +\t}\n> > +\n> > +\tstatic const ControlInfoMap deserialize(std::vector &data,\n> > +\t\t\t\t\t\tControlSerializer *cs)\n> > +\t{\n> > +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end(), cs);\n> > +\t}\n> > +\n> > +\tstatic const ControlInfoMap deserialize(std::vector::iterator it1,\n> > +\t\t\t\t\t\t[[maybe_unused]] std::vector::iterator it2,\n> > +\t\t\t\t\t\tControlSerializer *cs)\n> > +\t{\n> > +\t\tif (!cs)\n> > +\t\t\tLOG(IPADataSerializer, Fatal)\n> > +\t\t\t\t<< \"ControlSerializer not provided for deserialization of ControlInfoMap\";\n> > +\n> > +\t\tuint32_t infoDataSize = readUInt(it1);\n> > +\n> > +\t\tstd::vector::iterator it = it1 + 4;\n> > +\n> > +\t\tstd::vector infoData(it, it + infoDataSize);\n> > +\n> > +\t\tByteStreamBuffer buffer(const_cast(infoData.data()), infoData.size());\n> > +\t\tconst ControlInfoMap map = cs->deserialize(buffer);\n> > +\n> > +\t\treturn map;\n> > +\t}\n> > +\n> > +\tstatic const ControlInfoMap deserialize(std::vector &data,\n> > +\t\t\t\t\t\t[[maybe_unused]] std::vector &fds,\n> > +\t\t\t\t\t\tControlSerializer *cs)\n> > +\t{\n> > +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end(), cs);\n> > +\t}\n> > +\n> > +\tstatic const ControlInfoMap deserialize(std::vector::iterator data_it1,\n> > +\t\t\t\t\t std::vector::iterator data_it2,\n> > +\t\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it1,\n> > +\t\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it2,\n> > +\t\t\t\t\t ControlSerializer *cs)\n> > +\t{\n> > +\t\treturn IPADataSerializer::deserialize(data_it1, data_it2, cs);\n> > +\t}\n> > +};\n> > +\n> > +template<>\n> > +class IPADataSerializer\n> > +{\n> > +public:\n> > +\tstatic std::tuple, std::vector>\n> > +\tserialize(const ControlInfoMap &map, [[maybe_unused]] ControlSerializer *cs)\n> > +\t{\n> > +\t\tconst ControlInfoMap &map_const = const_cast(map);\n> > +\n> > +\t\tstd::vector data_vec;\n> > +\t\tstd::tie(data_vec, std::ignore) =\n> > +\t\t\tIPADataSerializer::serialize(map_const, cs);\n> > +\n> > +\t\treturn {data_vec, {}};\n> > +\t}\n> > +\n> > +\tstatic ControlInfoMap deserialize(std::vector &data,\n> > +\t\t\t\t\t ControlSerializer *cs)\n> > +\t{\n> > +\t\tControlInfoMap ret;\n> > +\t\tconst ControlInfoMap &map = ret;\n> > +\t\tconst_cast(map) =\n> > +\t\t\tIPADataSerializer::deserialize(data, cs);\n> > +\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\tstatic ControlInfoMap deserialize(std::vector::iterator it1,\n> > +\t\t\t\t\t\t[[maybe_unused]] std::vector::iterator it2,\n> > +\t\t\t\t\t\tControlSerializer *cs)\n> > +\t{\n> > +\t\tControlInfoMap ret;\n> > +\t\tconst ControlInfoMap &map = ret;\n> > +\t\tconst_cast(map) =\n> > +\t\t\tIPADataSerializer::deserialize(it1, it2, cs);\n> > +\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\tstatic ControlInfoMap deserialize(std::vector &data,\n> > +\t\t\t\t\t\t[[maybe_unused]] std::vector &fds,\n> > +\t\t\t\t\t\tControlSerializer *cs)\n> > +\t{\n> > +\t\tControlInfoMap ret;\n> > +\t\tconst ControlInfoMap &map = ret;\n> > +\t\tconst_cast(map) =\n> > +\t\t\tIPADataSerializer::deserialize(data, fds, cs);\n> > +\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\tstatic ControlInfoMap deserialize(std::vector::iterator data_it1,\n> > +\t\t\t\t\t std::vector::iterator data_it2,\n> > +\t\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it1,\n> > +\t\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it2,\n> > +\t\t\t\t\t ControlSerializer *cs)\n> > +\t{\n> > +\t\tControlInfoMap ret;\n> > +\t\tconst ControlInfoMap &map = ret;\n> > +\t\tconst_cast(map) =\n> > +\t\t\tIPADataSerializer::deserialize(data_it1, data_it2,\n> > +\t\t\t\t\t\t\t\t\t fds_it1, fds_it2, cs);\n> \n> Very similar comment as per the ControlList implementation\n> \n> > +\n> > +\t\treturn ret;\n> > +\t}\n> > +};\n> > +\n> > +template<>\n> > +class IPADataSerializer\n> > +{\n> > +public:\n> > +\tstatic std::tuple, std::vector>\n> > +\tserialize(const FrameBuffer::Plane &data, [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > +\t{\n> > +\t\tstd::vector data_vec;\n> > +\t\tstd::vector fds_vec;\n> > +\n> > +\t\tstd::vector fdBuf;\n> > +\t\tstd::vector fdFds;\n> > +\t\tstd::tie(fdBuf, fdFds) =\n> > +\t\t\tIPADataSerializer::serialize(data.fd);\n> > +\t\tdata_vec.insert(data_vec.end(), fdBuf.begin(), fdBuf.end());\n> > +\t\tfds_vec.insert(fds_vec.end(), fdFds.begin(), fdFds.end());\n> > +\n> > +\t\tappendUInt(data_vec, data.length);\n> > +\n> > +\t\treturn {data_vec, fds_vec};\n> > +\t}\n> > +\n> > +\tstatic FrameBuffer::Plane deserialize(std::vector &data,\n> > +\t\t\t\t\t std::vector &fds,\n> > +\t\t\t\t\t ControlSerializer *cs = nullptr)\n> > +\t{\n> > +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end(),\n> > +\t\t\t\t\t\t\t\t\t fds.begin(), fds.end(),\n> > +\t\t\t\t\t\t\t\t\t cs);\n> > +\t}\n> > +\n> > +\tstatic FrameBuffer::Plane deserialize(std::vector::iterator data_it1,\n> > +\t\t\t\t\t [[maybe_unused]] std::vector::iterator data_it2,\n> > +\t\t\t\t\t std::vector::iterator fds_it1,\n> > +\t\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it2,\n> > +\t\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > +\t{\n> > +\t\tFrameBuffer::Plane ret;\n> > +\n> > +\t\tret.fd = IPADataSerializer::deserialize(data_it1, data_it1 + 1,\n> > +\t\t\t\t\t\t\t\t\tfds_it1, fds_it1 + 1);\n> > +\t\tret.length = readUInt(data_it1 + 1);\n> > +\n> > +\t\treturn ret;\n> > +\t}\n> > +};\n> > +\n> > +\n> > +template<>\n> > +class IPADataSerializer\n> > +{\n> > +public:\n> > +\tstatic std::tuple, std::vector>\n> > +\tserialize(const IPABuffer &data, ControlSerializer *cs = nullptr)\n> > +\t{\n> > +\t\tstd::vector data_vec;\n> > +\n> > +\t\tappendUInt(data_vec, data.id);\n> > +\n> > +\t\tstd::vector planes_data_vec;\n> > +\t\tstd::vector planes_fds_vec;\n> \n> Calculating in advance the occupation might help but it's not trivial\n\nYeah, that's what IPADataSerializer is for :)\n\n> > +\t\tstd::tie(planes_data_vec, planes_fds_vec) =\n> > +\t\t\tIPADataSerializer>::serialize(data.planes, cs);\n> > +\n> > +\t\tdata_vec.insert(data_vec.end(), planes_data_vec.begin(), planes_data_vec.end());\n> > +\n> > +\t\treturn {data_vec, planes_fds_vec};\n> > +\t}\n> > +\n> > +\tstatic IPABuffer deserialize(std::vector &data,\n> > +\t\t\t\t std::vector &fds,\n> > +\t\t\t\t ControlSerializer *cs = nullptr)\n> > +\t{\n> > +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end(),\n> > +\t\t\t\t\t\t\t\t fds.begin(), fds.end(), cs);\n> > +\t}\n> > +\n> > +\tstatic IPABuffer deserialize(std::vector::iterator data_it1,\n> > +\t\t\t\t std::vector::iterator data_it2,\n> > +\t\t\t\t std::vector::iterator fds_it1,\n> > +\t\t\t\t std::vector::iterator fds_it2,\n> > +\t\t\t\t ControlSerializer *cs = nullptr)\n> > +\t{\n> > +\t\tIPABuffer ret;\n> > +\n> > +\t\tret.id = readUInt(data_it1);\n> > +\n> > +\t\tret.planes =\n> > +\t\t\tIPADataSerializer>::deserialize(\n> > +\t\t\t\tdata_it1 + 4, data_it2, fds_it1, fds_it2, cs);\n> > +\n> > +\t\treturn ret;\n> > +\t}\n> > +};\n> > +\n> > +#endif /* __DOXYGEN__ */\n> > +\n> > +} /* namespace libcamera */\n> > +\n> > +#endif /* __LIBCAMERA_INTERNAL_IPA_DATA_SERIALIZER_H__ */\n> > diff --git a/src/libcamera/ipa_data_serializer.cpp b/src/libcamera/ipa_data_serializer.cpp\n> > new file mode 100644\n> > index 00000000..5029cdf6\n> > --- /dev/null\n> > +++ b/src/libcamera/ipa_data_serializer.cpp\n> > @@ -0,0 +1,154 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Google Inc.\n> > + *\n> > + * ipa_data_serializer.cpp - Image Processing Algorithm data serializer\n> > + */\n> > +\n> > +#include \"libcamera/internal/ipa_data_serializer.h\"\n> > +\n> > +#include \"libcamera/internal/log.h\"\n> > +\n> > +/**\n> > + * \\file ipa_data_serializer.h\n> > + * \\brief IPA Data Serializer\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DEFINE_CATEGORY(IPADataSerializer)\n> > +\n> > +/**\n> > + * \\class IPADataSerializer\n> > + * \\brief IPA Data Serializer\n> > + *\n> > + * Static template class that provides functions for serializing and\n> > + * deserializing IPA data.\n> > + */\n> > +\n> > +/**\n> > + * \\fn template void appendUInt(std::vector &vec, T val)\n> > + * \\brief Append uint to end of byte vector, in little-endian order\n> > + * \\tparam T Type of uint to append\n> > + * \\param[in] vec Byte vector to append to\n> > + * \\param[in] val Value of uint to append\n> \n> As said, this works for uint, int, float and double now\n\nAh okay, I'll rename this then.\n\n> > + */\n> > +\n> > +/**\n> > + * \\fn template T readUInt(std::vector &vec, size_t pos)\n> > + * \\brief Read uint from byte vector, in little-endian order\n> > + * \\tparam T Type of uint to read\n> > + * \\param[in] vec Byte vector to read from\n> > + * \\param[in] pos Index in vec to start reading from\n> > + *\n> \n> No need for an empty line if no additional description is provided\n\nack\n\n> > + * \\return The uint read from \\a vec, or 0 if reading goes past end of \\a vec\n> \n> I wonder if we should not pass a T * as an output parameter and return\n> an error code. Returning 0 makes it impossible to detect failure, as 0\n> is a valid value\n\nI was actually wondering about that since when I wrote it in the first\nplace. This might be a good solution.\n\n> > + */\n> > +\n> > +/**\n> > + * \\fn template T readUInt(std::vector::iterator it)\n> > + * \\brief Read uint from byte vector, in little-endian order\n> > + * \\tparam T Type of uint to read\n> > + * \\param[in] it Iterator of byte vector to read from\n> > + *\n> \n> No need to empty line\n\nack\n\n> > + * \\return The uint read from \\a vec\n> > + */\n> > +\n> > +/**\n> > + * \\fn template IPADataSerializer::serialize(\n> > + * \tT data,\n> > + * \tControlSerializer *cs = nullptr)\n> > + * \\brief Serialize an object into byte vector and fd vector\n> > + * \\tparam T Type of object to serialize\n> > + * \\param[in] data Object to serialize\n> > + * \\param[in] cs ControlSerializer\n> > + *\n> > + * \\a cs is only necessary if the object type \\a T or its members contain\n> > + * ControlList or ControlInfoMap.\n> > + *\n> > + * \\return Tuple of byte vector and fd vector, that is the serialized form\n> > + * of \\a data\n> \n> Maybe later, but I think for each template specialization, we want to\n> document the serialization format, in example that for vectors, the\n> length is the first 4 bytes and so on (even more for custom libcamera\n> data types)\n\nGood idea.\n\n> > + */\n> > +\n> > +/**\n> > + * \\fn template IPADataSerializer::deserialize(\n> > + * \tstd::vector &data,\n> > + * \tControlSerializer *cs = nullptr)\n> > + * \\brief Deserialize byte vector into an object\n> > + * \\tparam T Type of object to deserialize to\n> > + * \\param[in] data Byte vector to deserialize from\n> > + * \\param[in] cs ControlSerializer\n> > + *\n> > + * This version of deserialize() can be used if the object type \\a T and its\n> > + * members don't have any FileDescriptor.\n> > + *\n> > + * \\a cs is only necessary if the object type \\a T or its members contain\n> > + * ControlList or ControlInfoMap.\n> > + *\n> > + * \\return The deserialized object\n> > + */\n> > +\n> > +/**\n> > + * \\fn template IPADataSerializer::deserialize(\n> > + * \tstd::vector::iterator it1,\n> > + * \tstd::vector::iterator it2,\n> > + * \tControlSerializer *cs = nullptr)\n> > + * \\brief Deserialize byte vector into an object\n> > + * \\tparam T Type of object to deserialize to\n> > + * \\param[in] it1 Begin iterator of byte vector to deserialize from\n> > + * \\param[in] it2 End iterator of byte vector to deserialize from\n> > + * \\param[in] cs ControlSerializer\n> > + *\n> > + * This version of deserialize() can be used if the object type \\a T and its\n> > + * members don't have any FileDescriptor.\n> > + *\n> > + * \\a cs is only necessary if the object type \\a T or its members contain\n> > + * ControlList or ControlInfoMap.\n> > + *\n> > + * \\return The deserialized object\n> > + */\n> > +\n> > +/**\n> > + * \\fn template IPADataSerializer::deserialize(\n> > + * \tstd::vector &data,\n> > + * \tstd::vector &fds,\n> > + * \tControlSerializer *cs = nullptr)\n> > + * \\brief Deserialize byte vector and fd vector into an object\n> > + * \\tparam T Type of object to deserialize to\n> > + * \\param[in] data Byte vector to deserialize from\n> > + * \\param[in] fds Fd vector to deserialize from\n> > + * \\param[in] cs ControlSerializer\n> > + *\n> > + * This version of deserialize() (or the iterator version) must be used if\n> > + * the object type \\a T or its members contain FileDescriptor.\n> > + *\n> > + * \\a cs is only necessary if the object type \\a T or its members contain\n> > + * ControlList or ControlInfoMap.\n> > + *\n> > + * \\return The deserialized object\n> > + */\n> > +\n> > +/**\n> > + * \\fn template IPADataSerializer::deserialize(\n> > + * \tstd::vector::iterator data_it1,\n> > + * \tstd::vector::iterator data_it2,\n> > + * \tstd::vector::iterator fds_it1,\n> > + * \tstd::vector::iterator fds_it2,\n> > + * \tControlSerializer *cs = nullptr)\n> > + * \\brief Deserialize byte vector and fd vector into an object\n> > + * \\tparam T Type of object to deserialize to\n> > + * \\param[in] data_it1 Begin iterator of byte vector to deserialize from\n> > + * \\param[in] data_it2 End iterator of byte vector to deserialize from\n> > + * \\param[in] fds_it1 Begin iterator of fd vector to deserialize from\n> > + * \\param[in] fds_it2 End iterator of fd vector to deserialize from\n> > + * \\param[in] cs ControlSerializer\n> > + *\n> > + * This version of deserialize() (or the vector version) must be used if\n> > + * the object type \\a T or its members contain FileDescriptor.\n> > + *\n> > + * \\a cs is only necessary if the object type \\a T or its members contain\n> > + * ControlList or ControlInfoMap.\n> > + *\n> > + * \\return The deserialized object\n> > + */\n> > +\n> \n> Puff.. pant.. a lot of code to review work here! Nice one Paul!\n\nThank you for the reviewing the whole thing :)\n\n\nPaul\n\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index 07711b5f..190d7490 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -24,6 +24,7 @@ libcamera_sources = files([\n> > 'geometry.cpp',\n> > 'ipa_context_wrapper.cpp',\n> > 'ipa_controls.cpp',\n> > + 'ipa_data_serializer.cpp',\n> > 'ipa_interface.cpp',\n> > 'ipa_manager.cpp',\n> > 'ipa_module.cpp',\n> > --\n> > 2.27.0\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel","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 C4249BDB1E\n\tfor ;\n\tTue, 27 Oct 2020 10:14:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4F4FC620B9;\n\tTue, 27 Oct 2020 11:14:12 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1B4AD6034C\n\tfor ;\n\tTue, 27 Oct 2020 11:14:11 +0100 (CET)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2DE1581;\n\tTue, 27 Oct 2020 11:14:08 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"stEfcwb5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1603793650;\n\tbh=+2yZm9OMEu0NsDXobF7NEFLSXeP7eyYifolSO9swFwA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=stEfcwb5USFX8vusyqeyJ6fsZbZQGiD825ZPcR8nuYQh+7a7HhsK6FilNCW4eWABm\n\t5t5/cQSUCBN7xqF11lUNk5nauRxuhaZlIxFkSDot9JyPGb0MeDoGU3N4+V/HEl7368\n\tRP9u/9yyVLXee2DaRWZGDQgr9qgRlYt4w4m0r44E=","Date":"Tue, 27 Oct 2020 19:14:03 +0900","From":"paul.elder@ideasonboard.com","To":"Jacopo Mondi ","Message-ID":"<20201027101403.GA2236@pyrite.rasen.tech>","References":"<20201002143154.468162-1-paul.elder@ideasonboard.com>\n\t<20201002143154.468162-11-paul.elder@ideasonboard.com>\n\t<20201026160752.dtob6iq2tm5kkwg5@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201026160752.dtob6iq2tm5kkwg5@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 10/38] libcamera: Add\n\tIPADataSerializer","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13513,"web_url":"https://patchwork.libcamera.org/comment/13513/","msgid":"<20201027113019.ifdwfbqzl5qtu7ph@uno.localdomain>","date":"2020-10-27T11:30:19","subject":"Re: [libcamera-devel] [PATCH v3 10/38] libcamera: Add\n\tIPADataSerializer","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul,\n\nOn Tue, Oct 27, 2020 at 07:14:03PM +0900, paul.elder@ideasonboard.com wrote:\n> Hi Jacopo,\n>\n> Thank you for the thorough review!\n>\n\nThank you for the enormous work\n\n> On Mon, Oct 26, 2020 at 05:07:52PM +0100, Jacopo Mondi wrote:\n> > Hi Paul,\n> >\n> > On Fri, Oct 02, 2020 at 11:31:26PM +0900, Paul Elder wrote:\n> > > Add an IPADataSerializer which implments de/serialization of built-in\n> > > (PODs, vector, map, string) and libcamera data structures. This is\n> > > intended to be used by the proxy and the proxy worker in the IPC layer.\n> > >\n> > > Signed-off-by: Paul Elder \n> > >\n> > > ---\n> > > Changes in v3:\n> > > - reimplement append/readUInt with memcpy (intead of bit shifting)\n> > > - change DECLARE_INTEGRAL_SERIALIZER with DECLARE_POD_SERIALIZER\n> > > - use this for int64_t, bool, float, and double\n> > > - fix comment style\n> > >\n> > > Changes in v2:\n> > > - added serializers for all integer types, bool, and string\n> > > - use string serializer for IPASettings serializer\n> > > - add documentation\n> > > - add serializer for const ControlList\n> > > ---\n> > > .../libcamera/internal/ipa_data_serializer.h | 1014 +++++++++++++++++\n> > > src/libcamera/ipa_data_serializer.cpp | 154 +++\n> > > src/libcamera/meson.build | 1 +\n> > > 3 files changed, 1169 insertions(+)\n> > > create mode 100644 include/libcamera/internal/ipa_data_serializer.h\n> > > create mode 100644 src/libcamera/ipa_data_serializer.cpp\n> > >\n> > > diff --git a/include/libcamera/internal/ipa_data_serializer.h b/include/libcamera/internal/ipa_data_serializer.h\n> > > new file mode 100644\n> > > index 00000000..9cd35c4c\n> > > --- /dev/null\n> > > +++ b/include/libcamera/internal/ipa_data_serializer.h\n> > > @@ -0,0 +1,1014 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2020, Google Inc.\n> > > + *\n> > > + * ipa_data_serializer.h - Image Processing Algorithm data serializer\n> > > + */\n> > > +#ifndef __LIBCAMERA_INTERNAL_IPA_DATA_SERIALIZER_H__\n> > > +#define __LIBCAMERA_INTERNAL_IPA_DATA_SERIALIZER_H__\n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \"libcamera/internal/byte_stream_buffer.h\"\n> > > +#include \"libcamera/internal/camera_sensor.h\"\n> > > +#include \"libcamera/internal/control_serializer.h\"\n> > > +#include \"libcamera/internal/log.h\"\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +LOG_DECLARE_CATEGORY(IPADataSerializer)\n> > > +\n> >\n> > should these be wrapped in an anonymous namespace ? They are used by\n> > the generated proxyes, but the header is included. I tried it locally\n> > and complier seems to digest it\n>\n> Ah yes, probably.\n>\n> > > +template\n> > > +void appendUInt(std::vector &vec, T val)\n> > > +{\n> > > +\tsize_t byteWidth = sizeof(val);\n> > > +\tstd::vector v(byteWidth);\n> > > +\tmemcpy(&v[0], &val, byteWidth);\n> > > +\n> > > +\tvec.insert(vec.end(), v.begin(), v.end());\n> > > +}\n> > > +\n> > > +template\n> > > +T readUInt(std::vector &vec, size_t pos)\n> > > +{\n> > > +\tT ret = 0;\n> > > +\tsize_t byteWidth = sizeof(ret);\n> > > +\tif (pos + byteWidth > vec.size())\n> > > +\t\treturn ret;\n> > > +\n> > > +\tmemcpy(&ret, &vec.data()[pos], byteWidth);\n> >\n> > Just &vec[pos] should be enough\n>\n> It's fine even for memcpying?\n>\n\nstd::vector::operator[] returns a T&, so taking its address should be\nequivalent to &std::vector::data()[pos]\n\n> > > +\treturn ret;\n> > > +}\n> > > +\n> > > +template\n> > > +T readUInt(std::vector::iterator it)\n> > > +{\n> > > +\tT ret = 0;\n> > > +\tsize_t byteWidth = sizeof(ret);\n> > > +\n> > > +\tstd::vector v(it, it + byteWidth);\n> > > +\tmemcpy(&ret, v.data(), byteWidth);\n> > > +\treturn ret;\n> > > +}\n> >\n> > These new versions looks much better!\n>\n> \\o/\n>\n> Ah, so later on you're saying that I should rename these, since they're\n> not limited to uints?\n>\n\nYes, I think removing UInt from the names might be useful.\nAs said below this apply to most POD types, so appendPOD() and\nreadPOD() might be more appropriate. But even just 'append()' and\n'read()' might be enough and nicer to read (assuming they're wrapped\nin an anonymous namespace). Ah wait, 'read()' -might- already be used :)\n\nWe could live with that, as the read syscall shall be prefixed with ::\n\nThis, in example, works with g++ 10.2.0 and clang++ 10.0.1\n\n#include \n#include \n\nnamespace {\nvoid read(char b)\n{\n\tstd::cout << b << std::endl;\n}\n};\n\nint main()\n{\n\tchar a;\n\n\t::read(1, &a, 1);\n\tread(a);\n\n\treturn 0;\n}\n\n\nI'm not sure we want to go there though.\n\nOtherwise, name the namespace that wraps readUInt() ? You can use\n'detail' as the namespace name as it's done for Span<> ?\n\n> > > +\n> > > +template\n> > > +class IPADataSerializer\n> > > +{\n> > > +#ifdef __DOXYGEN__\n> > > +public:\n> > > +\tstatic std::tuple, std::vector>\n> > > +\tserialize(T data, ControlSerializer *cs);\n> > > +\n> > > +\tstatic T deserialize(std::vector &data,\n> > > +\t\t\t ControlSerializer *cs);\n> > > +\tstatic T deserialize(std::vector::iterator it1,\n> > > +\t\t\t std::vector::iterator it2,\n> > > +\t\t\t ControlSerializer *cs);\n> > > +\n> >\n> > Intentional empty line ?\n>\n> Yes, the first two are data only, and the bottom two are data+fd.\n>\n> > > +\tstatic T deserialize(std::vector &data,\n> > > +\t\t\t std::vector &fds,\n> > > +\t\t\t ControlSerializer *cs);\n> > > +\tstatic T deserialize(std::vector::iterator data_it1,\n> > > +\t\t\t std::vector::iterator data_it2,\n> > > +\t\t\t std::vector::iterator fds_it1,\n> > > +\t\t\t std::vector::iterator fds_it2,\n> > > +\t\t\t ControlSerializer *cs);\n> >\n> > Aren't these better called data_start, data_end and fds_start, fds_end\n> > ?\n>\n> Yeah, that's better.\n>\n> > > +#endif /* __DOXYGEN__ */\n> >\n> > Do you need this ifdef __DOXYGEN__ ? you want these parsed, don't you ?\n> > It's usually #ifndef __DOXYGEN__ to exclude from documentation parsing\n> > some parts of the code, like you're done below\n>\n> I do need this ifdef. I want doxygen to document just the functions in\n> the base serializer, since it's the same interface for all serializers,\n\nBut won't doxygen parse it anyway. I tried removing the #ifdef and I\nhave documentation generated for IPADataSerializer::serialize() and\nfriends generated.\n\n> but I don't want the base serializer template to be compiled.\n\nThat's ok. And that's why you need the below #ifndef\nOr didn't I get what you mean with 'base serializer' ?\n\n>\n> > > +};\n> > > +\n> > > +#ifndef __DOXYGEN__\n> > > +\n> > > +template\n> > > +class IPADataSerializer>\n> > > +{\n> > > +public:\n> > > +\tstatic std::tuple, std::vector>\n> > > +\tserialize(const std::vector &data, ControlSerializer *cs = nullptr)\n> > > +\t{\n> > > +\t\tstd::vector data_vec;\n> >\n> > We can reserve data.size() * sizeof(V) in the vector\n>\n> I'm not sure we can do that...\n>\n> > > +\t\tstd::vector fds_vec;\n> > > +\n> > > +\t\t/* Serialize the length. */\n> > > +\t\tuint32_t vec_len = data.size();\n> > > +\t\tappendUInt(data_vec, vec_len);\n> > > +\n> > > +\t\t/* Serialize the members. */\n> > > +\t\tfor (auto it = data.begin(); it != data.end(); ++it) {\n> >\n> > for (auto const &it : data) ?\n>\n> ack\n>\n> > > +\t\t\tstd::vector dvec;\n> > > +\t\t\tstd::vector fvec;\n> > > +\n> > > +\t\t\tstd::tie(dvec, fvec) =\n> > > +\t\t\t\tIPADataSerializer::serialize(*it, cs);\n> > > +\n> > > +\t\t\tappendUInt(data_vec, dvec.size());\n> >\n> > Why is the size (in bytes) of each serialized entry recorded in the\n> > serialized data buffer ? Aren't all the members of the same size ?\n>\n> ...because the members of the vector aren't necessarily the same size.\n> The vector could contains variable-size objects, like other vectors, or\n> maps, or strings, etc, so we need to tag the size of every element.\n\nThe fact you've pointed out that, in example, CameraSensor contains a\nstring of variable length makes me agree with you and the fact we have\nto prefix the actual data with the expected size in the serialized\nformat. The same for pre-reserving the vectors, sizeof() won't surely\nhelp with !POD data.\n\n>\n> > > +\t\t\tappendUInt(data_vec, fvec.size());\n> >\n\n[snip]\n\n> > > +#define DECLARE_POD_SERIALIZER(type)\t\t\t\t\t\\\n> > > +template<>\t\t\t\t\t\t\t\t\\\n> > > +class IPADataSerializer\t\t\t\t\t\t\\\n> > > +{\t\t\t\t\t\t\t\t\t\\\n> > > +public:\t\t\t\t\t\t\t\t\t\\\n> > > +\tstatic std::tuple, std::vector>\t\\\n> > > +\tserialize(const type data,\t\t\t\t\t\\\n> > > +\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\t\\\n> > > +\t{\t\t\t\t\t\t\t\t\\\n> > > +\t\tstd::vector data_vec;\t\t\t\t\\\n> >\n> > Do you think pre-allocating sizeof(type) in the vector might help ?\n>\n> tbh, yeah.\n>\n> > > +\t\tappendUInt(data_vec, data);\t\t\t\\\n>\n> Oh, then this needs a writeUInt cousin.\n>\n\nWhy so ?\n\n> > > +\t\t\t\t\t\t\t\t\t\\\n> > > +\t\treturn {data_vec, {}};\t\t\t\t\t\\\n> > > +\t}\t\t\t\t\t\t\t\t\\\n> > > +\t\t\t\t\t\t\t\t\t\\\n> > > +\tstatic type deserialize(std::vector &data,\t\t\\\n> > > +\t\t\t\t[[maybe_unused]] ControlSerializer *cs = nullptr)\\\n> > > +\t{\t\t\t\t\t\t\t\t\\\n> > > +\t\treturn IPADataSerializer::deserialize(data.begin(),\\\n> > > +\t\t\t\t\t\t\t data.end());\\\n> > > +\t}\t\t\t\t\t\t\t\t\\\n> > > +\t\t\t\t\t\t\t\t\t\\\n> > > +\tstatic type deserialize(std::vector::iterator it1,\t\\\n> > > +\t\t\t\t[[maybe_unused]] std::vector::iterator it2,\\\n> > > +\t\t\t\t[[maybe_unused]] ControlSerializer *cs = nullptr)\\\n> > > +\t{\t\t\t\t\t\t\t\t\\\n> > > +\t\treturn readUInt(it1);\t\t\t\t\\\n> > > +\t}\t\t\t\t\t\t\t\t\\\n> > > +\t\t\t\t\t\t\t\t\t\\\n> > > +\tstatic type deserialize(std::vector &data,\t\t\\\n> > > +\t\t\t\t[[maybe_unused]] std::vector &fds,\\\n> > > +\t\t\t\t[[maybe_unused]] ControlSerializer *cs = nullptr)\\\n> > > +\t{\t\t\t\t\t\t\t\t\\\n> > > +\t\treturn IPADataSerializer::deserialize(data.begin(),\\\n> > > +\t\t\t\t\t\t\t data.end());\\\n> > > +\t}\t\t\t\t\t\t\t\t\\\n> > > +\t\t\t\t\t\t\t\t\t\\\n> > > +\tstatic type deserialize(std::vector::iterator data_it1,\\\n> > > +\t\t\t\tstd::vector::iterator data_it2,\\\n> > > +\t\t\t\t[[maybe_unused]] std::vector::iterator fds_it1,\\\n> > > +\t\t\t\t[[maybe_unused]] std::vector::iterator fds_it2,\\\n> > > +\t\t\t\t[[maybe_unused]] ControlSerializer *cs = nullptr)\\\n> > > +\t{\t\t\t\t\t\t\t\t\\\n> > > +\t\treturn IPADataSerializer::deserialize(data_it1,\t\\\n> > > +\t\t\t\t\t\t\t data_it2);\t\\\n> > > +\t}\t\t\t\t\t\t\t\t\\\n> > > +};\n> > > +\n> > > +DECLARE_POD_SERIALIZER(bool)\n> > > +DECLARE_POD_SERIALIZER(uint8_t)\n> > > +DECLARE_POD_SERIALIZER(uint16_t)\n> > > +DECLARE_POD_SERIALIZER(uint32_t)\n> > > +DECLARE_POD_SERIALIZER(uint64_t)\n> > > +DECLARE_POD_SERIALIZER(int8_t)\n> > > +DECLARE_POD_SERIALIZER(int16_t)\n> > > +DECLARE_POD_SERIALIZER(int32_t)\n> > > +DECLARE_POD_SERIALIZER(int64_t)\n> >\n> > The I wonder if keeping here and in the documentation the term 'uint'\n> > is correct and it shouldn't just be replaced with 'int'\n>\n> I'm guessing you're referring to {append,read}UInt()?\n>\n\nYes\n\n> > > +DECLARE_POD_SERIALIZER(float)\n> > > +DECLARE_POD_SERIALIZER(double)\n> >\n> > Or even POD\n>\n> I'm not sure what you're referring to here :/\n>\n\nStill on the name, see above for the readPOD() appendPOD() discussion\n(these two are awful names tbh)\n\n> > > +\n> > > +template<>\n> > > +class IPADataSerializer\n> > > +{\n> > > +public:\n> > > +\tstatic std::tuple, std::vector>\n> > > +\tserialize(const std::string &data, [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > +\t{\n> > > +\t\tstd::vector data_vec(data.begin(), data.end());\n> > > +\n> > > +\t\treturn {data_vec, {}};\n> > > +\t}\n> > > +\n> > > +\tstatic std::string deserialize(std::vector &data,\n> > > +\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > +\t{\n> > > +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end());\n> > > +\t}\n> > > +\n> > > +\tstatic std::string deserialize(std::vector::iterator it1,\n> > > +\t\t\t\t std::vector::iterator it2,\n> > > +\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > +\t{\n> > > +\t\tstd::string str(it1, it2);\n> > > +\n> > > +\t\treturn str;\n> >\n> > Can you just\n> > return {it1, it2};\n> > ?\n>\n> Ah, yes. I had it (and everything below) implemented in this manner\n> because I wanted to algorithmize the serdes code generation, so I was\n> practicing it on myself here. I guess it's about time for optimizations;\n> thank you for the pointers.\n>\n> > > +\t}\n> > > +\n> > > +\tstatic std::string deserialize(std::vector &data,\n> > > +\t\t\t\t [[maybe_unused]] std::vector &fds,\n> > > +\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > +\t{\n> > > +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end());\n> >\n> > Maybe\n> > return {data.being(), data.end()};\n> >\n> > would save a function call ?\n> > Same for other overloaded methods (and possibly not only in the\n> > specialization).\n> >\n>\n> ack\n>\n> > > +\t}\n> > > +\n> > > +\tstatic std::string deserialize(std::vector::iterator data_it1,\n> > > +\t\t\t\t std::vector::iterator data_it2,\n> > > +\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it1,\n> > > +\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it2,\n> > > +\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > +\t{\n> > > +\t\treturn IPADataSerializer::deserialize(data_it1, data_it2);\n> > > +\t}\n> > > +};\n> > > +\n> > > +template<>\n> > > +class IPADataSerializer\n> > > +{\n> > > +public:\n> > > +\tstatic std::tuple, std::vector>\n> > > +\tserialize(const FileDescriptor &data, [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > +\t{\n> > > +\t\tstd::vector data_vec = { data.isValid() };\n> > > +\t\tstd::vector fd_vec;\n> > > +\t\tif (data.isValid())\n> > > +\t\t\tfd_vec.push_back(data.fd());\n> > > +\n> > > +\t\treturn {data_vec, fd_vec};\n> > > +\t}\n> > > +\n> > > +\tstatic FileDescriptor deserialize(std::vector &data, std::vector &fds,\n> > > +\t\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > +\t{\n> > > +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end(),\n> > > +\t\t\t\t\t\t\t\t fds.begin(), fds.end());\n> > > +\t}\n> > > +\n> > > +\tstatic FileDescriptor deserialize(std::vector::iterator data_it1,\n> > > +\t\t\t\t\t std::vector::iterator data_it2,\n> > > +\t\t\t\t\t std::vector::iterator fds_it1,\n> > > +\t\t\t\t\t std::vector::iterator fds_it2,\n> > > +\t\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > +\t{\n> > > +\t\tif (std::distance(data_it1, data_it2) < 1)\n> > > +\t\t\tLOG(IPADataSerializer, Fatal)\n> > > +\t\t\t\t<< \"Invalid data to deserialize FileDescriptor\";\n> > > +\n> > > +\t\tbool valid = *data_it1;\n> >\n> > in C this would be\n> > bool valid = !!*data_it1;\n> > not sure about C++ and not even sure if the !! is just a paranoid\n> > kernel convention, as I don't think even C requires that\n>\n> Oh... I'm not sure. It passed my de/serialization tests.\n>\n> > > +\n> > > +\t\tif (valid && std::distance(fds_it1, fds_it2) < 1)\n> >\n> > \t\tif (valid && std::distance(fds_it1, fds_it2) < 1) {\n> > \t\t\tLOG(IPADataSerializer, Fatal)\n> > \t\t\t\t<< \"Invalid fds to deserialize FileDescriptor\";\n> > return {}\n> > }\n> >\n> > return FileDescriptor(*fds_it1);\n> >\n> > Or is returning {} when std::distance() < 1 wrong ? I don't think so,\n> > as even if it is \"valid\" there's not fds serialized.\n>\n> If std::distance() < 1 then *fds_it1 will segfault. So if valid and\n> std::distance() < 1, then there's a big problem -> fatal. If not valid,\n> then std::distance() doesn't matter, and we return a new uninitialized\n> FileDescriptor, but we can't construct it using *fds_it1, since the\n> iterator shouldn't be valid.\n>\n\nI've missed 'Fatal' in the LOG() call. Sorry about this.\n\n> It's perfectly fine to send an invalid FileDescriptor, for example if\n> the FileDescriptor field isn't used, or uninitialized. The problem with\n> sending it directly is that sendmsg() EINVALs if we try to send -1, so\n> we need this valid flag in the data vector.\n>\n> >\n> > > +\t\t\tLOG(IPADataSerializer, Fatal)\n> > > +\t\t\t\t<< \"Invalid fds to deserialize FileDescriptor\";\n> > > +\n> > > +\t\treturn valid ? FileDescriptor(*fds_it1) : FileDescriptor();\n> > > +\t}\n> > > +};\n> > > +\n> > > +template<>\n> > > +class IPADataSerializer\n> > > +{\n> > > +public:\n> > > +\tstatic std::tuple, std::vector>\n> > > +\tserialize(const IPASettings &data, [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > +\t{\n> > > +\t\treturn IPADataSerializer::serialize(data.configurationFile);\n> > > +\t}\n> > > +\n> > > +\tstatic IPASettings deserialize(std::vector &data,\n> > > +\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > +\t{\n> > > +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end());\n> > > +\t}\n> > > +\n> > > +\tstatic IPASettings deserialize(std::vector::iterator it1,\n> > > +\t\t\t\t std::vector::iterator it2,\n> > > +\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > +\t{\n> > > +\t\tIPASettings ret;\n> > > +\t\tret.configurationFile = IPADataSerializer::deserialize(it1, it2);\n> > > +\n> > > +\t\treturn ret;\n> >\n> > return { IPADataSerializer::deserialize(it1, it2) };\n> >\n> > ?\n>\n> Ah yes, much needed optimization.\n>\n> > > +\t}\n> > > +\n> > > +\tstatic IPASettings deserialize(std::vector &data,\n> > > +\t\t\t\t [[maybe_unused]] std::vector &fds,\n> > > +\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > +\t{\n> > > +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end());\n> >\n> > return { IPADataSerializer::deserialize(data.begin(), data.end() };\n> >\n> > ?\n>\n> ack\n>\n> > > +\t}\n> > > +\n> > > +\tstatic IPASettings deserialize(std::vector::iterator data_it1,\n> > > +\t\t\t\t std::vector::iterator data_it2,\n> > > +\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it1,\n> > > +\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it2,\n> > > +\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > +\t{\n> > > +\t\treturn IPADataSerializer::deserialize(data_it1, data_it2);\n> >\n> > same\n>\n> ack\n>\n> > > +\t}\n> > > +};\n> > > +\n> > > +template<>\n> > > +class IPADataSerializer\n> > > +{\n> > > +public:\n> > > +\tstatic std::tuple, std::vector>\n> > > +\tserialize(const CameraSensorInfo &data, [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > +\t{\n> > > +\t\tstd::vector data_vec;\n> >\n> > Reserving sizeof(CameraSensorInfo) might help\n>\n> It's got a string in it though, so we don't know sizeof(CameraSensorInfo).\n>\n\nIt could be calculated though. Nevermind\n\n> > I just noticed and I wonder why all this code uses this_naming_style\n> > in place of the libcamera standard namingStyle\n>\n> Ah yes, I should probably fix that...\n>\n\nThanks\n\n> > > +\n> > > +\t\tuint32_t str_len = data.model.size();\n> > > +\t\tappendUInt(data_vec, str_len);\n> > > +\n> > > +\t\tdata_vec.insert(data_vec.end(), data.model.begin(), data.model.end());\n> >\n> > Shouldn't this be serialized using the IPADataSerializer\n> > if it's not needed, do we need the specialization then ?\n>\n> It should be :)\n>\n> > > +\n> > > +\t\tappendUInt(data_vec, data.bitsPerPixel);\n> > > +\n> > > +\t\tappendUInt(data_vec, data.activeAreaSize.width);\n> > > +\t\tappendUInt(data_vec, data.activeAreaSize.height);\n> > > +\n> > > +\t\tappendUInt(data_vec, static_cast(data.analogCrop.x));\n> > > +\t\tappendUInt(data_vec, static_cast(data.analogCrop.y));\n> > > +\t\tappendUInt(data_vec, data.analogCrop.width);\n> > > +\t\tappendUInt(data_vec, data.analogCrop.height);\n> > > +\n> > > +\t\tappendUInt(data_vec, data.outputSize.width);\n> > > +\t\tappendUInt(data_vec, data.outputSize.height);\n> > > +\n> > > +\t\tappendUInt(data_vec, data.pixelRate);\n> > > +\n> > > +\t\tappendUInt(data_vec, data.lineLength);\n> > > +\n> > > +\t\treturn {data_vec, {}};\n> > > +\t}\n> > > +\n> > > +\tstatic CameraSensorInfo deserialize(std::vector &data,\n> > > +\t\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > +\t{\n> > > +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end());\n> > > +\t}\n> > > +\n> > > +\tstatic CameraSensorInfo deserialize(std::vector::iterator it1,\n> > > +\t\t\t\t\t [[maybe_unused]] std::vector::iterator it2,\n> > > +\t\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > +\t{\n> > > +\t\tCameraSensorInfo ret;\n> > CameraSensorInfo ret{};\n> >\n> > Also for other statically allocated fields that are returned by other\n> > specializations.\n>\n> Do we need to, since all the fields are populated below?\n>\n\nTechnically not, and for all the supported types you fill in all\nfields, you're right. But to be honest I won't mind a\n0-initialization anyway, mostly because if we happen to add support\nfor new types, and if they won't be fully-filled, we keep the good\npractice of {} and we're safe. A bit paranoid maybe :)\n\n> > > +\n> > > +\t\tuint32_t str_len = readUInt(it1);\n> > > +\t\tstd::string str(it1 + 4, it1 + 4 + str_len);\n> > > +\t\tret.model = str;\n> > > +\n> > > +\t\tstd::vector::iterator it = it1 + 4 + str_len;\n> > > +\n> > > +\t\tret.bitsPerPixel = readUInt(it);\n> > > +\n> > > +\t\tret.activeAreaSize.width = readUInt(it + 4);\n> > > +\t\tret.activeAreaSize.height = readUInt(it + 8);\n> > > +\n> > > +\t\tret.analogCrop.x = static_cast(readUInt(it + 12));\n> > > +\t\tret.analogCrop.y = static_cast(readUInt(it + 16));\n> > > +\t\tret.analogCrop.width = readUInt(it + 20);\n> > > +\t\tret.analogCrop.height = readUInt(it + 24);\n> > > +\n> > > +\t\tret.outputSize.width = readUInt(it + 28);\n> > > +\t\tret.outputSize.height = readUInt(it + 32);\n> > > +\n> > > +\t\tret.pixelRate = readUInt(it + 36);\n> > > +\n> > > +\t\tret.lineLength = readUInt(it + 44);\n> >\n> > lineLength is a 32 bits integer\n>\n> Ah, yes.\n>\n> > > +\n> > > +\t\treturn ret;\n> > > +\t}\n> > > +\n> > > +\tstatic CameraSensorInfo deserialize(std::vector &data,\n> > > +\t\t\t\t\t [[maybe_unused]] std::vector &fds,\n> > > +\t\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > +\t{\n> > > +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end());\n> > > +\t}\n> > > +\n> > > +\tstatic CameraSensorInfo deserialize(std::vector::iterator data_it1,\n> > > +\t\t\t\t\t std::vector::iterator data_it2,\n> > > +\t\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it1,\n> > > +\t\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it2,\n> > > +\t\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > +\t{\n> > > +\t\treturn IPADataSerializer::deserialize(data_it1, data_it2);\n> > > +\t}\n> > > +};\n> > > +\n> > > +template<>\n> > > +class IPADataSerializer\n> > > +{\n> > > +public:\n> > > +\tstatic std::tuple, std::vector>\n> > > +\tserialize(const IPAStream &data, [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > +\t{\n> > > +\t\tstd::vector data_vec;\n> > > +\n> > > +\t\tappendUInt(data_vec, data.pixelFormat);\n> > > +\n> > > +\t\tappendUInt(data_vec, data.size.width);\n> > > +\t\tappendUInt(data_vec, data.size.height);\n> > > +\n> > > +\t\treturn {data_vec, {}};\n> > > +\t}\n> > > +\n> > > +\tstatic IPAStream deserialize(std::vector &data,\n> > > +\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > +\t{\n> > > +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end());\n> > > +\t}\n> > > +\n> > > +\tstatic IPAStream deserialize(std::vector::iterator it1,\n> > > +\t\t\t\t [[maybe_unused]] std::vector::iterator it2,\n> > > +\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > +\t{\n> > > +\t\tIPAStream ret;\n> > > +\n> > > +\t\tret.pixelFormat = readUInt(it1);\n> > > +\n> > > +\t\tret.size.width = readUInt(it1 + 4);\n> > > +\t\tret.size.height = readUInt(it1 + 8);\n> > > +\n> > > +\t\treturn ret;\n> > > +\t}\n> > > +\n> > > +\tstatic IPAStream deserialize(std::vector &data,\n> > > +\t\t\t\t [[maybe_unused]] std::vector &fds,\n> > > +\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > +\t{\n> > > +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end());\n> > > +\t}\n> > > +\n> > > +\tstatic IPAStream deserialize(std::vector::iterator data_it1,\n> > > +\t\t\t\t std::vector::iterator data_it2,\n> > > +\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it1,\n> > > +\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it2,\n> > > +\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > +\t{\n> > > +\t\treturn IPADataSerializer::deserialize(data_it1, data_it2);\n> > > +\t}\n> > > +};\n> > > +\n> > > +template<>\n> > > +class IPADataSerializer\n> > > +{\n> > > +public:\n> > > +\t/* map arg will be generated, since it's per-pipeline anyway. */\n> > > +\tstatic std::tuple, std::vector>\n> > > +\tserialize(const ControlList &data, const ControlInfoMap &map,\n> > > +\t\t ControlSerializer *cs)\n> > > +\t{\n> > > +\t\tif (!cs)\n> > > +\t\t\tLOG(IPADataSerializer, Fatal)\n> > > +\t\t\t\t<< \"ControlSerializer not provided for serialization of ControlList\";\n> > > +\n> > > +\t\tsize_t size = cs->binarySize(map);\n> > > +\t\tstd::vector infoData(size);\n> > > +\t\tByteStreamBuffer buffer(infoData.data(), infoData.size());\n> > > +\t\tint ret = cs->serialize(map, buffer);\n> > > +\n> > > +\t\tif (ret < 0 || buffer.overflow()) {\n> > > +\t\t\tLOG(IPADataSerializer, Error) << \"Failed to serialize ControlList's ControlInfoMap\";\n> > > +\t\t\treturn {{}, {}};\n> > > +\t\t}\n> > > +\n> > > +\t\tsize = cs->binarySize(data);\n> > > +\t\tstd::vector listData(size);\n> > > +\t\tbuffer = ByteStreamBuffer(listData.data(), listData.size());\n> > > +\t\tret = cs->serialize(data, buffer);\n> > > +\n> > > +\t\tif (ret < 0 || buffer.overflow()) {\n> > > +\t\t\tLOG(IPADataSerializer, Error) << \"Failed to serialize ControlList\";\n> > > +\t\t\treturn {{}, {}};\n> > > +\t\t}\n> > > +\n> > > +\t\tstd::vector data_vec;\n> >\n> > You know the sizes and can reserve ?\n>\n> At this point. yes.\n>\n> > > +\t\tappendUInt(data_vec, infoData.size());\n> > > +\t\tappendUInt(data_vec, listData.size());\n> > > +\t\tdata_vec.insert(data_vec.end(), infoData.begin(), infoData.end());\n> > > +\t\tdata_vec.insert(data_vec.end(), listData.begin(), listData.end());\n> > > +\n> > > +\t\treturn {data_vec, {}};\n> > > +\t}\n> > > +\n> > > +\tstatic const ControlList deserialize(std::vector &data, ControlSerializer *cs)\n> > > +\t{\n> > > +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end(), cs);\n> > > +\t}\n> > > +\n> > > +\tstatic const ControlList deserialize(std::vector::iterator it1,\n> > > +\t\t\t\t [[maybe_unused]] std::vector::iterator it2,\n> > > +\t\t\t\t ControlSerializer *cs)\n> > > +\t{\n> > > +\t\tif (!cs)\n> > > +\t\t\tLOG(IPADataSerializer, Fatal)\n> > > +\t\t\t\t<< \"ControlSerializer not provided for deserialization of ControlList\";\n> > > +\n> > > +\t\tuint32_t infoDataSize = readUInt(it1);\n> > > +\t\tuint32_t listDataSize = readUInt(it1 + 4);\n> > > +\n> > > +\t\tstd::vector::iterator it = it1 + 8;\n> > > +\n> > > +\t\tstd::vector infoData(it, it + infoDataSize);\n> > > +\t\tstd::vector listData(it + infoDataSize, it + infoDataSize + listDataSize);\n> > > +\n> > > +\t\tByteStreamBuffer buffer(const_cast(infoData.data()), infoData.size());\n> > > +\t\tControlInfoMap map = cs->deserialize(buffer);\n> > > +\t\t/* It's fine if map is empty. */\n> > > +\t\tif (buffer.overflow()) {\n> > > +\t\t\tLOG(IPADataSerializer, Error)\n> > > +\t\t\t\t<< \"Failed to deserialize ControlLists's ControlInfoMap: buffer overflow\";\n> > > +\t\t\treturn ControlList();\n> > > +\t\t}\n> > > +\n> > > +\t\tbuffer = ByteStreamBuffer(const_cast(listData.data()), listData.size());\n> > > +\t\tControlList list = cs->deserialize(buffer);\n> > > +\t\tif (buffer.overflow())\n> > > +\t\t\tLOG(IPADataSerializer, Error) << \"Failed to deserialize ControlList: buffer overflow\";\n> > > +\t\tif (list.empty())\n> > > +\t\t\tLOG(IPADataSerializer, Error) << \"Failed to deserialize ControlList: empty list\";\n> >\n> > Should we return {} in these cases or is it fine ?\n>\n> ControlSerializer::deserialize() returns {} in these cases anyway, so it\n> should be fine.\n>\n> > > +\n> > > +\t\treturn list;\n> > > +\t}\n> > > +\n> > > +\tstatic const ControlList deserialize(std::vector &data,\n> > > +\t\t\t\t [[maybe_unused]] std::vector &fds,\n> > > +\t\t\t\t ControlSerializer *cs)\n> > > +\t{\n> > > +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end(), cs);\n> > > +\t}\n> > > +\n> > > +\tstatic const ControlList deserialize(std::vector::iterator data_it1,\n> > > +\t\t\t\t std::vector::iterator data_it2,\n> > > +\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it1,\n> > > +\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it2,\n> > > +\t\t\t\t ControlSerializer *cs)\n> > > +\t{\n> > > +\t\treturn IPADataSerializer::deserialize(data_it1, data_it2, cs);\n> > > +\t}\n> > > +};\n> > > +\n> > > +template<>\n> > > +class IPADataSerializer\n> > > +{\n> > > +public:\n> > > +\tstatic std::tuple, std::vector>\n> > > +\tserialize(const ControlList &data, const ControlInfoMap &map,\n> > > +\t\t ControlSerializer *cs)\n> > > +\t{\n> > > +\t\tconst ControlList &list_const = const_cast(data);\n> > > +\n> > > +\t\tstd::vector data_vec;\n> > > +\t\tstd::tie(data_vec, std::ignore) =\n> > > +\t\t\tIPADataSerializer::serialize(list_const, map, cs);\n> > > +\n> > > +\t\treturn {data_vec, {}};\n> > > +\t}\n> > > +\n> > > +\tstatic ControlList deserialize(std::vector &data,\n> > > +\t\t\t\t ControlSerializer *cs)\n> > > +\t{\n> > > +\t\tControlList ret;\n> > > +\t\tconst ControlList &list = ret;\n> > > +\t\tconst_cast(list) =\n> > > +\t\t\tIPADataSerializer::deserialize(data, cs);\n> > > +\n> > > +\t\treturn ret;\n> > > +\t}\n> > > +\n> > > +\tstatic ControlList deserialize(std::vector::iterator it1,\n> > > +\t\t\t\t [[maybe_unused]] std::vector::iterator it2,\n> > > +\t\t\t\t ControlSerializer *cs)\n> > > +\t{\n> > > +\t\tControlList ret;\n> > > +\t\tconst ControlList &list = ret;\n> > > +\t\tconst_cast(list) =\n> > > +\t\t\tIPADataSerializer::deserialize(it1, it2, cs);\n> >\n> > Why not\n> > const ControlList list =\n> > \t\t\tIPADataSerializer::deserialize(it1, it2, cs);\n> > return const_cast(list)\n>\n> My understanding is that to remove a const with const_cast, the original\n> variable must not be const.\n\nYou're right, this won't work, cast should happen on a pointer or\nreference.\n\nThis works with gcc and clang:\n\n\t\tconst ControlList &list =\n\t\t\tIPADataSerializer::deserialize(data, cs);\n\n\t\treturn const_cast(list);\n\nAs the function returns by value it should be ok.\nTests pass as well, but I'm not sure if this case is tested though.\n\n>\n> > > +\n> > > +\t\treturn ret;\n> > > +\t}\n> > > +\n> > > + */\n\n[snip]\n\n> > > +\n> > > +/**\n> > > + * \\fn template T readUInt(std::vector &vec, size_t pos)\n> > > + * \\brief Read uint from byte vector, in little-endian order\n> > > + * \\tparam T Type of uint to read\n> > > + * \\param[in] vec Byte vector to read from\n> > > + * \\param[in] pos Index in vec to start reading from\n> > > + *\n> >\n> > No need for an empty line if no additional description is provided\n>\n> ack\n>\n> > > + * \\return The uint read from \\a vec, or 0 if reading goes past end of \\a vec\n> >\n> > I wonder if we should not pass a T * as an output parameter and return\n> > an error code. Returning 0 makes it impossible to detect failure, as 0\n> > is a valid value\n>\n> I was actually wondering about that since when I wrote it in the first\n> place. This might be a good solution.\n\nUp to you. Do you check for the return value or do you need to do so ?\n\n>\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn template T readUInt(std::vector::iterator it)\n> > > + * \\brief Read uint from byte vector, in little-endian order\n> > > + * \\tparam T Type of uint to read\n> > > + * \\param[in] it Iterator of byte vector to read from\n> > > + *\n> >\n> > No need to empty line\n>\n> ack\n>\n> > > + * \\return The uint read from \\a vec\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn template IPADataSerializer::serialize(\n> > > + * \tT data,\n> > > + * \tControlSerializer *cs = nullptr)\n> > > + * \\brief Serialize an object into byte vector and fd vector\n> > > + * \\tparam T Type of object to serialize\n> > > + * \\param[in] data Object to serialize\n> > > + * \\param[in] cs ControlSerializer\n> > > + *\n> > > + * \\a cs is only necessary if the object type \\a T or its members contain\n> > > + * ControlList or ControlInfoMap.\n> > > + *\n> > > + * \\return Tuple of byte vector and fd vector, that is the serialized form\n> > > + * of \\a data\n> >\n> > Maybe later, but I think for each template specialization, we want to\n> > document the serialization format, in example that for vectors, the\n> > length is the first 4 bytes and so on (even more for custom libcamera\n> > data types)\n>\n> Good idea.\n\nAlthough it won't be parsed by doxygen and will remain in-code only.\nBut knowing how each container will be serialized might help\nvalidating it.\n\nActually, how does it work for closed source IPAs ? Before this series\nthe pipeline handler and the IPA were in charge of\nserializing/deserializing with custom code, as the close source blob,\npossibly written in C, wouldn't be able to link against libcamera and\nuse its helpers (better, they might chose not to, as from a compliance\npov it's fine linking a blob against L-GPL code). Is it still possible\n? In that case, documenting how things gets serialized might be\neven more important.\n\nThanks\n j","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 0119BBDB1E\n\tfor ;\n\tTue, 27 Oct 2020 11:30:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5EEC8620C2;\n\tTue, 27 Oct 2020 12:30:23 +0100 (CET)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 41E6E61E57\n\tfor ;\n\tTue, 27 Oct 2020 12:30:22 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id B25B01BF20C;\n\tTue, 27 Oct 2020 11:30:21 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Tue, 27 Oct 2020 12:30:19 +0100","From":"Jacopo Mondi ","To":"paul.elder@ideasonboard.com","Message-ID":"<20201027113019.ifdwfbqzl5qtu7ph@uno.localdomain>","References":"<20201002143154.468162-1-paul.elder@ideasonboard.com>\n\t<20201002143154.468162-11-paul.elder@ideasonboard.com>\n\t<20201026160752.dtob6iq2tm5kkwg5@uno.localdomain>\n\t<20201027101403.GA2236@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201027101403.GA2236@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [PATCH v3 10/38] libcamera: Add\n\tIPADataSerializer","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13526,"web_url":"https://patchwork.libcamera.org/comment/13526/","msgid":"<20201028025249.GA2119@pyrite.rasen.tech>","date":"2020-10-28T02:52:49","subject":"Re: [libcamera-devel] [PATCH v3 10/38] libcamera: Add\n\tIPADataSerializer","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Tue, Oct 27, 2020 at 12:30:19PM +0100, Jacopo Mondi wrote:\n> Hi Paul,\n> \n> On Tue, Oct 27, 2020 at 07:14:03PM +0900, paul.elder@ideasonboard.com wrote:\n> > Hi Jacopo,\n> >\n> > Thank you for the thorough review!\n> >\n> \n> Thank you for the enormous work\n> \n> > On Mon, Oct 26, 2020 at 05:07:52PM +0100, Jacopo Mondi wrote:\n> > > Hi Paul,\n> > >\n> > > On Fri, Oct 02, 2020 at 11:31:26PM +0900, Paul Elder wrote:\n> > > > Add an IPADataSerializer which implments de/serialization of built-in\n> > > > (PODs, vector, map, string) and libcamera data structures. This is\n> > > > intended to be used by the proxy and the proxy worker in the IPC layer.\n> > > >\n> > > > Signed-off-by: Paul Elder \n> > > >\n> > > > ---\n> > > > Changes in v3:\n> > > > - reimplement append/readUInt with memcpy (intead of bit shifting)\n> > > > - change DECLARE_INTEGRAL_SERIALIZER with DECLARE_POD_SERIALIZER\n> > > > - use this for int64_t, bool, float, and double\n> > > > - fix comment style\n> > > >\n> > > > Changes in v2:\n> > > > - added serializers for all integer types, bool, and string\n> > > > - use string serializer for IPASettings serializer\n> > > > - add documentation\n> > > > - add serializer for const ControlList\n> > > > ---\n> > > > .../libcamera/internal/ipa_data_serializer.h | 1014 +++++++++++++++++\n> > > > src/libcamera/ipa_data_serializer.cpp | 154 +++\n> > > > src/libcamera/meson.build | 1 +\n> > > > 3 files changed, 1169 insertions(+)\n> > > > create mode 100644 include/libcamera/internal/ipa_data_serializer.h\n> > > > create mode 100644 src/libcamera/ipa_data_serializer.cpp\n> > > >\n> > > > diff --git a/include/libcamera/internal/ipa_data_serializer.h b/include/libcamera/internal/ipa_data_serializer.h\n> > > > new file mode 100644\n> > > > index 00000000..9cd35c4c\n> > > > --- /dev/null\n> > > > +++ b/include/libcamera/internal/ipa_data_serializer.h\n> > > > @@ -0,0 +1,1014 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2020, Google Inc.\n> > > > + *\n> > > > + * ipa_data_serializer.h - Image Processing Algorithm data serializer\n> > > > + */\n> > > > +#ifndef __LIBCAMERA_INTERNAL_IPA_DATA_SERIALIZER_H__\n> > > > +#define __LIBCAMERA_INTERNAL_IPA_DATA_SERIALIZER_H__\n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +#include \"libcamera/internal/byte_stream_buffer.h\"\n> > > > +#include \"libcamera/internal/camera_sensor.h\"\n> > > > +#include \"libcamera/internal/control_serializer.h\"\n> > > > +#include \"libcamera/internal/log.h\"\n> > > > +\n> > > > +namespace libcamera {\n> > > > +\n> > > > +LOG_DECLARE_CATEGORY(IPADataSerializer)\n> > > > +\n> > >\n> > > should these be wrapped in an anonymous namespace ? They are used by\n> > > the generated proxyes, but the header is included. I tried it locally\n> > > and complier seems to digest it\n> >\n> > Ah yes, probably.\n> >\n> > > > +template\n> > > > +void appendUInt(std::vector &vec, T val)\n> > > > +{\n> > > > +\tsize_t byteWidth = sizeof(val);\n> > > > +\tstd::vector v(byteWidth);\n> > > > +\tmemcpy(&v[0], &val, byteWidth);\n> > > > +\n> > > > +\tvec.insert(vec.end(), v.begin(), v.end());\n> > > > +}\n> > > > +\n> > > > +template\n> > > > +T readUInt(std::vector &vec, size_t pos)\n> > > > +{\n> > > > +\tT ret = 0;\n> > > > +\tsize_t byteWidth = sizeof(ret);\n> > > > +\tif (pos + byteWidth > vec.size())\n> > > > +\t\treturn ret;\n> > > > +\n> > > > +\tmemcpy(&ret, &vec.data()[pos], byteWidth);\n> > >\n> > > Just &vec[pos] should be enough\n> >\n> > It's fine even for memcpying?\n> >\n> \n> std::vector::operator[] returns a T&, so taking its address should be\n> equivalent to &std::vector::data()[pos]\n\nAh, I see.\n\n> > > > +\treturn ret;\n> > > > +}\n> > > > +\n> > > > +template\n> > > > +T readUInt(std::vector::iterator it)\n> > > > +{\n> > > > +\tT ret = 0;\n> > > > +\tsize_t byteWidth = sizeof(ret);\n> > > > +\n> > > > +\tstd::vector v(it, it + byteWidth);\n> > > > +\tmemcpy(&ret, v.data(), byteWidth);\n> > > > +\treturn ret;\n> > > > +}\n> > >\n> > > These new versions looks much better!\n> >\n> > \\o/\n> >\n> > Ah, so later on you're saying that I should rename these, since they're\n> > not limited to uints?\n> >\n> \n> Yes, I think removing UInt from the names might be useful.\n> As said below this apply to most POD types, so appendPOD() and\n> readPOD() might be more appropriate. But even just 'append()' and\n> 'read()' might be enough and nicer to read (assuming they're wrapped\n> in an anonymous namespace). Ah wait, 'read()' -might- already be used :)\n> \n> We could live with that, as the read syscall shall be prefixed with ::\n> \n> This, in example, works with g++ 10.2.0 and clang++ 10.0.1\n> \n> #include \n> #include \n> \n> namespace {\n> void read(char b)\n> {\n> \tstd::cout << b << std::endl;\n> }\n> };\n> \n> int main()\n> {\n> \tchar a;\n> \n> \t::read(1, &a, 1);\n> \tread(a);\n> \n> \treturn 0;\n> }\n> \n> \n> I'm not sure we want to go there though.\n> \n> Otherwise, name the namespace that wraps readUInt() ? You can use\n> 'detail' as the namespace name as it's done for Span<> ?\n\nEh... I'll probably just go with {read,append}POD().\n\n> > > > +\n> > > > +template\n> > > > +class IPADataSerializer\n> > > > +{\n> > > > +#ifdef __DOXYGEN__\n> > > > +public:\n> > > > +\tstatic std::tuple, std::vector>\n> > > > +\tserialize(T data, ControlSerializer *cs);\n> > > > +\n> > > > +\tstatic T deserialize(std::vector &data,\n> > > > +\t\t\t ControlSerializer *cs);\n> > > > +\tstatic T deserialize(std::vector::iterator it1,\n> > > > +\t\t\t std::vector::iterator it2,\n> > > > +\t\t\t ControlSerializer *cs);\n> > > > +\n> > >\n> > > Intentional empty line ?\n> >\n> > Yes, the first two are data only, and the bottom two are data+fd.\n> >\n> > > > +\tstatic T deserialize(std::vector &data,\n> > > > +\t\t\t std::vector &fds,\n> > > > +\t\t\t ControlSerializer *cs);\n> > > > +\tstatic T deserialize(std::vector::iterator data_it1,\n> > > > +\t\t\t std::vector::iterator data_it2,\n> > > > +\t\t\t std::vector::iterator fds_it1,\n> > > > +\t\t\t std::vector::iterator fds_it2,\n> > > > +\t\t\t ControlSerializer *cs);\n> > >\n> > > Aren't these better called data_start, data_end and fds_start, fds_end\n> > > ?\n> >\n> > Yeah, that's better.\n> >\n> > > > +#endif /* __DOXYGEN__ */\n> > >\n> > > Do you need this ifdef __DOXYGEN__ ? you want these parsed, don't you ?\n> > > It's usually #ifndef __DOXYGEN__ to exclude from documentation parsing\n> > > some parts of the code, like you're done below\n> >\n> > I do need this ifdef. I want doxygen to document just the functions in\n> > the base serializer, since it's the same interface for all serializers,\n> \n> But won't doxygen parse it anyway. I tried removing the #ifdef and I\n> have documentation generated for IPADataSerializer::serialize() and\n> friends generated.\n> \n> > but I don't want the base serializer template to be compiled.\n> \n> That's ok. And that's why you need the below #ifndef\n> Or didn't I get what you mean with 'base serializer' ?\n\nNo, I mean I don't want the unspecialized serializer template to be\ncompiled. It's there only for generating documentation.\n\n> >\n> > > > +};\n> > > > +\n> > > > +#ifndef __DOXYGEN__\n> > > > +\n> > > > +template\n> > > > +class IPADataSerializer>\n> > > > +{\n> > > > +public:\n> > > > +\tstatic std::tuple, std::vector>\n> > > > +\tserialize(const std::vector &data, ControlSerializer *cs = nullptr)\n> > > > +\t{\n> > > > +\t\tstd::vector data_vec;\n> > >\n> > > We can reserve data.size() * sizeof(V) in the vector\n> >\n> > I'm not sure we can do that...\n> >\n> > > > +\t\tstd::vector fds_vec;\n> > > > +\n> > > > +\t\t/* Serialize the length. */\n> > > > +\t\tuint32_t vec_len = data.size();\n> > > > +\t\tappendUInt(data_vec, vec_len);\n> > > > +\n> > > > +\t\t/* Serialize the members. */\n> > > > +\t\tfor (auto it = data.begin(); it != data.end(); ++it) {\n> > >\n> > > for (auto const &it : data) ?\n> >\n> > ack\n> >\n> > > > +\t\t\tstd::vector dvec;\n> > > > +\t\t\tstd::vector fvec;\n> > > > +\n> > > > +\t\t\tstd::tie(dvec, fvec) =\n> > > > +\t\t\t\tIPADataSerializer::serialize(*it, cs);\n> > > > +\n> > > > +\t\t\tappendUInt(data_vec, dvec.size());\n> > >\n> > > Why is the size (in bytes) of each serialized entry recorded in the\n> > > serialized data buffer ? Aren't all the members of the same size ?\n> >\n> > ...because the members of the vector aren't necessarily the same size.\n> > The vector could contains variable-size objects, like other vectors, or\n> > maps, or strings, etc, so we need to tag the size of every element.\n> \n> The fact you've pointed out that, in example, CameraSensor contains a\n> string of variable length makes me agree with you and the fact we have\n> to prefix the actual data with the expected size in the serialized\n> format. The same for pre-reserving the vectors, sizeof() won't surely\n> help with !POD data.\n> \n> >\n> > > > +\t\t\tappendUInt(data_vec, fvec.size());\n> > >\n> \n> [snip]\n> \n> > > > +#define DECLARE_POD_SERIALIZER(type)\t\t\t\t\t\\\n> > > > +template<>\t\t\t\t\t\t\t\t\\\n> > > > +class IPADataSerializer\t\t\t\t\t\t\\\n> > > > +{\t\t\t\t\t\t\t\t\t\\\n> > > > +public:\t\t\t\t\t\t\t\t\t\\\n> > > > +\tstatic std::tuple, std::vector>\t\\\n> > > > +\tserialize(const type data,\t\t\t\t\t\\\n> > > > +\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\t\\\n> > > > +\t{\t\t\t\t\t\t\t\t\\\n> > > > +\t\tstd::vector data_vec;\t\t\t\t\\\n> > >\n> > > Do you think pre-allocating sizeof(type) in the vector might help ?\n> >\n> > tbh, yeah.\n> >\n> > > > +\t\tappendUInt(data_vec, data);\t\t\t\\\n> >\n> > Oh, then this needs a writeUInt cousin.\n> >\n> \n> Why so ?\n\nOh nvm, I thought that reserve would put empty space.\n\n> > > > +\t\t\t\t\t\t\t\t\t\\\n> > > > +\t\treturn {data_vec, {}};\t\t\t\t\t\\\n> > > > +\t}\t\t\t\t\t\t\t\t\\\n> > > > +\t\t\t\t\t\t\t\t\t\\\n> > > > +\tstatic type deserialize(std::vector &data,\t\t\\\n> > > > +\t\t\t\t[[maybe_unused]] ControlSerializer *cs = nullptr)\\\n> > > > +\t{\t\t\t\t\t\t\t\t\\\n> > > > +\t\treturn IPADataSerializer::deserialize(data.begin(),\\\n> > > > +\t\t\t\t\t\t\t data.end());\\\n> > > > +\t}\t\t\t\t\t\t\t\t\\\n> > > > +\t\t\t\t\t\t\t\t\t\\\n> > > > +\tstatic type deserialize(std::vector::iterator it1,\t\\\n> > > > +\t\t\t\t[[maybe_unused]] std::vector::iterator it2,\\\n> > > > +\t\t\t\t[[maybe_unused]] ControlSerializer *cs = nullptr)\\\n> > > > +\t{\t\t\t\t\t\t\t\t\\\n> > > > +\t\treturn readUInt(it1);\t\t\t\t\\\n> > > > +\t}\t\t\t\t\t\t\t\t\\\n> > > > +\t\t\t\t\t\t\t\t\t\\\n> > > > +\tstatic type deserialize(std::vector &data,\t\t\\\n> > > > +\t\t\t\t[[maybe_unused]] std::vector &fds,\\\n> > > > +\t\t\t\t[[maybe_unused]] ControlSerializer *cs = nullptr)\\\n> > > > +\t{\t\t\t\t\t\t\t\t\\\n> > > > +\t\treturn IPADataSerializer::deserialize(data.begin(),\\\n> > > > +\t\t\t\t\t\t\t data.end());\\\n> > > > +\t}\t\t\t\t\t\t\t\t\\\n> > > > +\t\t\t\t\t\t\t\t\t\\\n> > > > +\tstatic type deserialize(std::vector::iterator data_it1,\\\n> > > > +\t\t\t\tstd::vector::iterator data_it2,\\\n> > > > +\t\t\t\t[[maybe_unused]] std::vector::iterator fds_it1,\\\n> > > > +\t\t\t\t[[maybe_unused]] std::vector::iterator fds_it2,\\\n> > > > +\t\t\t\t[[maybe_unused]] ControlSerializer *cs = nullptr)\\\n> > > > +\t{\t\t\t\t\t\t\t\t\\\n> > > > +\t\treturn IPADataSerializer::deserialize(data_it1,\t\\\n> > > > +\t\t\t\t\t\t\t data_it2);\t\\\n> > > > +\t}\t\t\t\t\t\t\t\t\\\n> > > > +};\n> > > > +\n> > > > +DECLARE_POD_SERIALIZER(bool)\n> > > > +DECLARE_POD_SERIALIZER(uint8_t)\n> > > > +DECLARE_POD_SERIALIZER(uint16_t)\n> > > > +DECLARE_POD_SERIALIZER(uint32_t)\n> > > > +DECLARE_POD_SERIALIZER(uint64_t)\n> > > > +DECLARE_POD_SERIALIZER(int8_t)\n> > > > +DECLARE_POD_SERIALIZER(int16_t)\n> > > > +DECLARE_POD_SERIALIZER(int32_t)\n> > > > +DECLARE_POD_SERIALIZER(int64_t)\n> > >\n> > > The I wonder if keeping here and in the documentation the term 'uint'\n> > > is correct and it shouldn't just be replaced with 'int'\n> >\n> > I'm guessing you're referring to {append,read}UInt()?\n> >\n> \n> Yes\n> \n> > > > +DECLARE_POD_SERIALIZER(float)\n> > > > +DECLARE_POD_SERIALIZER(double)\n> > >\n> > > Or even POD\n> >\n> > I'm not sure what you're referring to here :/\n> >\n> \n> Still on the name, see above for the readPOD() appendPOD() discussion\n> (these two are awful names tbh)\n\nI think they're good enough :p\n\n> > > > +\n> > > > +template<>\n> > > > +class IPADataSerializer\n> > > > +{\n> > > > +public:\n> > > > +\tstatic std::tuple, std::vector>\n> > > > +\tserialize(const std::string &data, [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > > +\t{\n> > > > +\t\tstd::vector data_vec(data.begin(), data.end());\n> > > > +\n> > > > +\t\treturn {data_vec, {}};\n> > > > +\t}\n> > > > +\n> > > > +\tstatic std::string deserialize(std::vector &data,\n> > > > +\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > > +\t{\n> > > > +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end());\n> > > > +\t}\n> > > > +\n> > > > +\tstatic std::string deserialize(std::vector::iterator it1,\n> > > > +\t\t\t\t std::vector::iterator it2,\n> > > > +\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > > +\t{\n> > > > +\t\tstd::string str(it1, it2);\n> > > > +\n> > > > +\t\treturn str;\n> > >\n> > > Can you just\n> > > return {it1, it2};\n> > > ?\n> >\n> > Ah, yes. I had it (and everything below) implemented in this manner\n> > because I wanted to algorithmize the serdes code generation, so I was\n> > practicing it on myself here. I guess it's about time for optimizations;\n> > thank you for the pointers.\n> >\n> > > > +\t}\n> > > > +\n> > > > +\tstatic std::string deserialize(std::vector &data,\n> > > > +\t\t\t\t [[maybe_unused]] std::vector &fds,\n> > > > +\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > > +\t{\n> > > > +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end());\n> > >\n> > > Maybe\n> > > return {data.being(), data.end()};\n> > >\n> > > would save a function call ?\n> > > Same for other overloaded methods (and possibly not only in the\n> > > specialization).\n> > >\n> >\n> > ack\n> >\n> > > > +\t}\n> > > > +\n> > > > +\tstatic std::string deserialize(std::vector::iterator data_it1,\n> > > > +\t\t\t\t std::vector::iterator data_it2,\n> > > > +\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it1,\n> > > > +\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it2,\n> > > > +\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > > +\t{\n> > > > +\t\treturn IPADataSerializer::deserialize(data_it1, data_it2);\n> > > > +\t}\n> > > > +};\n> > > > +\n> > > > +template<>\n> > > > +class IPADataSerializer\n> > > > +{\n> > > > +public:\n> > > > +\tstatic std::tuple, std::vector>\n> > > > +\tserialize(const FileDescriptor &data, [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > > +\t{\n> > > > +\t\tstd::vector data_vec = { data.isValid() };\n> > > > +\t\tstd::vector fd_vec;\n> > > > +\t\tif (data.isValid())\n> > > > +\t\t\tfd_vec.push_back(data.fd());\n> > > > +\n> > > > +\t\treturn {data_vec, fd_vec};\n> > > > +\t}\n> > > > +\n> > > > +\tstatic FileDescriptor deserialize(std::vector &data, std::vector &fds,\n> > > > +\t\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > > +\t{\n> > > > +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end(),\n> > > > +\t\t\t\t\t\t\t\t fds.begin(), fds.end());\n> > > > +\t}\n> > > > +\n> > > > +\tstatic FileDescriptor deserialize(std::vector::iterator data_it1,\n> > > > +\t\t\t\t\t std::vector::iterator data_it2,\n> > > > +\t\t\t\t\t std::vector::iterator fds_it1,\n> > > > +\t\t\t\t\t std::vector::iterator fds_it2,\n> > > > +\t\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > > +\t{\n> > > > +\t\tif (std::distance(data_it1, data_it2) < 1)\n> > > > +\t\t\tLOG(IPADataSerializer, Fatal)\n> > > > +\t\t\t\t<< \"Invalid data to deserialize FileDescriptor\";\n> > > > +\n> > > > +\t\tbool valid = *data_it1;\n> > >\n> > > in C this would be\n> > > bool valid = !!*data_it1;\n> > > not sure about C++ and not even sure if the !! is just a paranoid\n> > > kernel convention, as I don't think even C requires that\n> >\n> > Oh... I'm not sure. It passed my de/serialization tests.\n> >\n> > > > +\n> > > > +\t\tif (valid && std::distance(fds_it1, fds_it2) < 1)\n> > >\n> > > \t\tif (valid && std::distance(fds_it1, fds_it2) < 1) {\n> > > \t\t\tLOG(IPADataSerializer, Fatal)\n> > > \t\t\t\t<< \"Invalid fds to deserialize FileDescriptor\";\n> > > return {}\n> > > }\n> > >\n> > > return FileDescriptor(*fds_it1);\n> > >\n> > > Or is returning {} when std::distance() < 1 wrong ? I don't think so,\n> > > as even if it is \"valid\" there's not fds serialized.\n> >\n> > If std::distance() < 1 then *fds_it1 will segfault. So if valid and\n> > std::distance() < 1, then there's a big problem -> fatal. If not valid,\n> > then std::distance() doesn't matter, and we return a new uninitialized\n> > FileDescriptor, but we can't construct it using *fds_it1, since the\n> > iterator shouldn't be valid.\n> >\n> \n> I've missed 'Fatal' in the LOG() call. Sorry about this.\n> \n> > It's perfectly fine to send an invalid FileDescriptor, for example if\n> > the FileDescriptor field isn't used, or uninitialized. The problem with\n> > sending it directly is that sendmsg() EINVALs if we try to send -1, so\n> > we need this valid flag in the data vector.\n> >\n> > >\n> > > > +\t\t\tLOG(IPADataSerializer, Fatal)\n> > > > +\t\t\t\t<< \"Invalid fds to deserialize FileDescriptor\";\n> > > > +\n> > > > +\t\treturn valid ? FileDescriptor(*fds_it1) : FileDescriptor();\n> > > > +\t}\n> > > > +};\n> > > > +\n> > > > +template<>\n> > > > +class IPADataSerializer\n> > > > +{\n> > > > +public:\n> > > > +\tstatic std::tuple, std::vector>\n> > > > +\tserialize(const IPASettings &data, [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > > +\t{\n> > > > +\t\treturn IPADataSerializer::serialize(data.configurationFile);\n> > > > +\t}\n> > > > +\n> > > > +\tstatic IPASettings deserialize(std::vector &data,\n> > > > +\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > > +\t{\n> > > > +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end());\n> > > > +\t}\n> > > > +\n> > > > +\tstatic IPASettings deserialize(std::vector::iterator it1,\n> > > > +\t\t\t\t std::vector::iterator it2,\n> > > > +\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > > +\t{\n> > > > +\t\tIPASettings ret;\n> > > > +\t\tret.configurationFile = IPADataSerializer::deserialize(it1, it2);\n> > > > +\n> > > > +\t\treturn ret;\n> > >\n> > > return { IPADataSerializer::deserialize(it1, it2) };\n> > >\n> > > ?\n> >\n> > Ah yes, much needed optimization.\n> >\n> > > > +\t}\n> > > > +\n> > > > +\tstatic IPASettings deserialize(std::vector &data,\n> > > > +\t\t\t\t [[maybe_unused]] std::vector &fds,\n> > > > +\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > > +\t{\n> > > > +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end());\n> > >\n> > > return { IPADataSerializer::deserialize(data.begin(), data.end() };\n> > >\n> > > ?\n> >\n> > ack\n> >\n> > > > +\t}\n> > > > +\n> > > > +\tstatic IPASettings deserialize(std::vector::iterator data_it1,\n> > > > +\t\t\t\t std::vector::iterator data_it2,\n> > > > +\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it1,\n> > > > +\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it2,\n> > > > +\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > > +\t{\n> > > > +\t\treturn IPADataSerializer::deserialize(data_it1, data_it2);\n> > >\n> > > same\n> >\n> > ack\n> >\n> > > > +\t}\n> > > > +};\n> > > > +\n> > > > +template<>\n> > > > +class IPADataSerializer\n> > > > +{\n> > > > +public:\n> > > > +\tstatic std::tuple, std::vector>\n> > > > +\tserialize(const CameraSensorInfo &data, [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > > +\t{\n> > > > +\t\tstd::vector data_vec;\n> > >\n> > > Reserving sizeof(CameraSensorInfo) might help\n> >\n> > It's got a string in it though, so we don't know sizeof(CameraSensorInfo).\n> >\n> \n> It could be calculated though. Nevermind\n> \n> > > I just noticed and I wonder why all this code uses this_naming_style\n> > > in place of the libcamera standard namingStyle\n> >\n> > Ah yes, I should probably fix that...\n> >\n> \n> Thanks\n> \n> > > > +\n> > > > +\t\tuint32_t str_len = data.model.size();\n> > > > +\t\tappendUInt(data_vec, str_len);\n> > > > +\n> > > > +\t\tdata_vec.insert(data_vec.end(), data.model.begin(), data.model.end());\n> > >\n> > > Shouldn't this be serialized using the IPADataSerializer\n> > > if it's not needed, do we need the specialization then ?\n> >\n> > It should be :)\n> >\n> > > > +\n> > > > +\t\tappendUInt(data_vec, data.bitsPerPixel);\n> > > > +\n> > > > +\t\tappendUInt(data_vec, data.activeAreaSize.width);\n> > > > +\t\tappendUInt(data_vec, data.activeAreaSize.height);\n> > > > +\n> > > > +\t\tappendUInt(data_vec, static_cast(data.analogCrop.x));\n> > > > +\t\tappendUInt(data_vec, static_cast(data.analogCrop.y));\n> > > > +\t\tappendUInt(data_vec, data.analogCrop.width);\n> > > > +\t\tappendUInt(data_vec, data.analogCrop.height);\n> > > > +\n> > > > +\t\tappendUInt(data_vec, data.outputSize.width);\n> > > > +\t\tappendUInt(data_vec, data.outputSize.height);\n> > > > +\n> > > > +\t\tappendUInt(data_vec, data.pixelRate);\n> > > > +\n> > > > +\t\tappendUInt(data_vec, data.lineLength);\n> > > > +\n> > > > +\t\treturn {data_vec, {}};\n> > > > +\t}\n> > > > +\n> > > > +\tstatic CameraSensorInfo deserialize(std::vector &data,\n> > > > +\t\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > > +\t{\n> > > > +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end());\n> > > > +\t}\n> > > > +\n> > > > +\tstatic CameraSensorInfo deserialize(std::vector::iterator it1,\n> > > > +\t\t\t\t\t [[maybe_unused]] std::vector::iterator it2,\n> > > > +\t\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > > +\t{\n> > > > +\t\tCameraSensorInfo ret;\n> > > CameraSensorInfo ret{};\n> > >\n> > > Also for other statically allocated fields that are returned by other\n> > > specializations.\n> >\n> > Do we need to, since all the fields are populated below?\n> >\n> \n> Technically not, and for all the supported types you fill in all\n> fields, you're right. But to be honest I won't mind a\n> 0-initialization anyway, mostly because if we happen to add support\n> for new types, and if they won't be fully-filled, we keep the good\n> practice of {} and we're safe. A bit paranoid maybe :)\n\nHm maybe better paranoid than segfault?\n\nIf we add new fields, the de/serializer will be updated to serialize\nfrom the field, and deserialize to the field. All the fields will be\nwritten to, and they'll be zero-initialized by readPOD().\n\nWhich reminds me... if readPOD() can return -1 and error then we'll\nhave to check every single readPOD()... that's going to make serdes code\nhuge...\n\nMaybe that's why I just had it return zero, so that we can just continue\ninstead of fataling. I think I'll just print an error and continue,\ninstead of silently returning zero.\n\n> > > > +\n> > > > +\t\tuint32_t str_len = readUInt(it1);\n> > > > +\t\tstd::string str(it1 + 4, it1 + 4 + str_len);\n> > > > +\t\tret.model = str;\n> > > > +\n> > > > +\t\tstd::vector::iterator it = it1 + 4 + str_len;\n> > > > +\n> > > > +\t\tret.bitsPerPixel = readUInt(it);\n> > > > +\n> > > > +\t\tret.activeAreaSize.width = readUInt(it + 4);\n> > > > +\t\tret.activeAreaSize.height = readUInt(it + 8);\n> > > > +\n> > > > +\t\tret.analogCrop.x = static_cast(readUInt(it + 12));\n> > > > +\t\tret.analogCrop.y = static_cast(readUInt(it + 16));\n> > > > +\t\tret.analogCrop.width = readUInt(it + 20);\n> > > > +\t\tret.analogCrop.height = readUInt(it + 24);\n> > > > +\n> > > > +\t\tret.outputSize.width = readUInt(it + 28);\n> > > > +\t\tret.outputSize.height = readUInt(it + 32);\n> > > > +\n> > > > +\t\tret.pixelRate = readUInt(it + 36);\n> > > > +\n> > > > +\t\tret.lineLength = readUInt(it + 44);\n> > >\n> > > lineLength is a 32 bits integer\n> >\n> > Ah, yes.\n> >\n> > > > +\n> > > > +\t\treturn ret;\n> > > > +\t}\n> > > > +\n> > > > +\tstatic CameraSensorInfo deserialize(std::vector &data,\n> > > > +\t\t\t\t\t [[maybe_unused]] std::vector &fds,\n> > > > +\t\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > > +\t{\n> > > > +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end());\n> > > > +\t}\n> > > > +\n> > > > +\tstatic CameraSensorInfo deserialize(std::vector::iterator data_it1,\n> > > > +\t\t\t\t\t std::vector::iterator data_it2,\n> > > > +\t\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it1,\n> > > > +\t\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it2,\n> > > > +\t\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > > +\t{\n> > > > +\t\treturn IPADataSerializer::deserialize(data_it1, data_it2);\n> > > > +\t}\n> > > > +};\n> > > > +\n> > > > +template<>\n> > > > +class IPADataSerializer\n> > > > +{\n> > > > +public:\n> > > > +\tstatic std::tuple, std::vector>\n> > > > +\tserialize(const IPAStream &data, [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > > +\t{\n> > > > +\t\tstd::vector data_vec;\n> > > > +\n> > > > +\t\tappendUInt(data_vec, data.pixelFormat);\n> > > > +\n> > > > +\t\tappendUInt(data_vec, data.size.width);\n> > > > +\t\tappendUInt(data_vec, data.size.height);\n> > > > +\n> > > > +\t\treturn {data_vec, {}};\n> > > > +\t}\n> > > > +\n> > > > +\tstatic IPAStream deserialize(std::vector &data,\n> > > > +\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > > +\t{\n> > > > +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end());\n> > > > +\t}\n> > > > +\n> > > > +\tstatic IPAStream deserialize(std::vector::iterator it1,\n> > > > +\t\t\t\t [[maybe_unused]] std::vector::iterator it2,\n> > > > +\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > > +\t{\n> > > > +\t\tIPAStream ret;\n> > > > +\n> > > > +\t\tret.pixelFormat = readUInt(it1);\n> > > > +\n> > > > +\t\tret.size.width = readUInt(it1 + 4);\n> > > > +\t\tret.size.height = readUInt(it1 + 8);\n> > > > +\n> > > > +\t\treturn ret;\n> > > > +\t}\n> > > > +\n> > > > +\tstatic IPAStream deserialize(std::vector &data,\n> > > > +\t\t\t\t [[maybe_unused]] std::vector &fds,\n> > > > +\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > > +\t{\n> > > > +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end());\n> > > > +\t}\n> > > > +\n> > > > +\tstatic IPAStream deserialize(std::vector::iterator data_it1,\n> > > > +\t\t\t\t std::vector::iterator data_it2,\n> > > > +\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it1,\n> > > > +\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it2,\n> > > > +\t\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > > +\t{\n> > > > +\t\treturn IPADataSerializer::deserialize(data_it1, data_it2);\n> > > > +\t}\n> > > > +};\n> > > > +\n> > > > +template<>\n> > > > +class IPADataSerializer\n> > > > +{\n> > > > +public:\n> > > > +\t/* map arg will be generated, since it's per-pipeline anyway. */\n> > > > +\tstatic std::tuple, std::vector>\n> > > > +\tserialize(const ControlList &data, const ControlInfoMap &map,\n> > > > +\t\t ControlSerializer *cs)\n> > > > +\t{\n> > > > +\t\tif (!cs)\n> > > > +\t\t\tLOG(IPADataSerializer, Fatal)\n> > > > +\t\t\t\t<< \"ControlSerializer not provided for serialization of ControlList\";\n> > > > +\n> > > > +\t\tsize_t size = cs->binarySize(map);\n> > > > +\t\tstd::vector infoData(size);\n> > > > +\t\tByteStreamBuffer buffer(infoData.data(), infoData.size());\n> > > > +\t\tint ret = cs->serialize(map, buffer);\n> > > > +\n> > > > +\t\tif (ret < 0 || buffer.overflow()) {\n> > > > +\t\t\tLOG(IPADataSerializer, Error) << \"Failed to serialize ControlList's ControlInfoMap\";\n> > > > +\t\t\treturn {{}, {}};\n> > > > +\t\t}\n> > > > +\n> > > > +\t\tsize = cs->binarySize(data);\n> > > > +\t\tstd::vector listData(size);\n> > > > +\t\tbuffer = ByteStreamBuffer(listData.data(), listData.size());\n> > > > +\t\tret = cs->serialize(data, buffer);\n> > > > +\n> > > > +\t\tif (ret < 0 || buffer.overflow()) {\n> > > > +\t\t\tLOG(IPADataSerializer, Error) << \"Failed to serialize ControlList\";\n> > > > +\t\t\treturn {{}, {}};\n> > > > +\t\t}\n> > > > +\n> > > > +\t\tstd::vector data_vec;\n> > >\n> > > You know the sizes and can reserve ?\n> >\n> > At this point. yes.\n> >\n> > > > +\t\tappendUInt(data_vec, infoData.size());\n> > > > +\t\tappendUInt(data_vec, listData.size());\n> > > > +\t\tdata_vec.insert(data_vec.end(), infoData.begin(), infoData.end());\n> > > > +\t\tdata_vec.insert(data_vec.end(), listData.begin(), listData.end());\n> > > > +\n> > > > +\t\treturn {data_vec, {}};\n> > > > +\t}\n> > > > +\n> > > > +\tstatic const ControlList deserialize(std::vector &data, ControlSerializer *cs)\n> > > > +\t{\n> > > > +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end(), cs);\n> > > > +\t}\n> > > > +\n> > > > +\tstatic const ControlList deserialize(std::vector::iterator it1,\n> > > > +\t\t\t\t [[maybe_unused]] std::vector::iterator it2,\n> > > > +\t\t\t\t ControlSerializer *cs)\n> > > > +\t{\n> > > > +\t\tif (!cs)\n> > > > +\t\t\tLOG(IPADataSerializer, Fatal)\n> > > > +\t\t\t\t<< \"ControlSerializer not provided for deserialization of ControlList\";\n> > > > +\n> > > > +\t\tuint32_t infoDataSize = readUInt(it1);\n> > > > +\t\tuint32_t listDataSize = readUInt(it1 + 4);\n> > > > +\n> > > > +\t\tstd::vector::iterator it = it1 + 8;\n> > > > +\n> > > > +\t\tstd::vector infoData(it, it + infoDataSize);\n> > > > +\t\tstd::vector listData(it + infoDataSize, it + infoDataSize + listDataSize);\n> > > > +\n> > > > +\t\tByteStreamBuffer buffer(const_cast(infoData.data()), infoData.size());\n> > > > +\t\tControlInfoMap map = cs->deserialize(buffer);\n> > > > +\t\t/* It's fine if map is empty. */\n> > > > +\t\tif (buffer.overflow()) {\n> > > > +\t\t\tLOG(IPADataSerializer, Error)\n> > > > +\t\t\t\t<< \"Failed to deserialize ControlLists's ControlInfoMap: buffer overflow\";\n> > > > +\t\t\treturn ControlList();\n> > > > +\t\t}\n> > > > +\n> > > > +\t\tbuffer = ByteStreamBuffer(const_cast(listData.data()), listData.size());\n> > > > +\t\tControlList list = cs->deserialize(buffer);\n> > > > +\t\tif (buffer.overflow())\n> > > > +\t\t\tLOG(IPADataSerializer, Error) << \"Failed to deserialize ControlList: buffer overflow\";\n> > > > +\t\tif (list.empty())\n> > > > +\t\t\tLOG(IPADataSerializer, Error) << \"Failed to deserialize ControlList: empty list\";\n> > >\n> > > Should we return {} in these cases or is it fine ?\n> >\n> > ControlSerializer::deserialize() returns {} in these cases anyway, so it\n> > should be fine.\n> >\n> > > > +\n> > > > +\t\treturn list;\n> > > > +\t}\n> > > > +\n> > > > +\tstatic const ControlList deserialize(std::vector &data,\n> > > > +\t\t\t\t [[maybe_unused]] std::vector &fds,\n> > > > +\t\t\t\t ControlSerializer *cs)\n> > > > +\t{\n> > > > +\t\treturn IPADataSerializer::deserialize(data.begin(), data.end(), cs);\n> > > > +\t}\n> > > > +\n> > > > +\tstatic const ControlList deserialize(std::vector::iterator data_it1,\n> > > > +\t\t\t\t std::vector::iterator data_it2,\n> > > > +\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it1,\n> > > > +\t\t\t\t [[maybe_unused]] std::vector::iterator fds_it2,\n> > > > +\t\t\t\t ControlSerializer *cs)\n> > > > +\t{\n> > > > +\t\treturn IPADataSerializer::deserialize(data_it1, data_it2, cs);\n> > > > +\t}\n> > > > +};\n> > > > +\n> > > > +template<>\n> > > > +class IPADataSerializer\n> > > > +{\n> > > > +public:\n> > > > +\tstatic std::tuple, std::vector>\n> > > > +\tserialize(const ControlList &data, const ControlInfoMap &map,\n> > > > +\t\t ControlSerializer *cs)\n> > > > +\t{\n> > > > +\t\tconst ControlList &list_const = const_cast(data);\n> > > > +\n> > > > +\t\tstd::vector data_vec;\n> > > > +\t\tstd::tie(data_vec, std::ignore) =\n> > > > +\t\t\tIPADataSerializer::serialize(list_const, map, cs);\n> > > > +\n> > > > +\t\treturn {data_vec, {}};\n> > > > +\t}\n> > > > +\n> > > > +\tstatic ControlList deserialize(std::vector &data,\n> > > > +\t\t\t\t ControlSerializer *cs)\n> > > > +\t{\n> > > > +\t\tControlList ret;\n> > > > +\t\tconst ControlList &list = ret;\n> > > > +\t\tconst_cast(list) =\n> > > > +\t\t\tIPADataSerializer::deserialize(data, cs);\n> > > > +\n> > > > +\t\treturn ret;\n> > > > +\t}\n> > > > +\n> > > > +\tstatic ControlList deserialize(std::vector::iterator it1,\n> > > > +\t\t\t\t [[maybe_unused]] std::vector::iterator it2,\n> > > > +\t\t\t\t ControlSerializer *cs)\n> > > > +\t{\n> > > > +\t\tControlList ret;\n> > > > +\t\tconst ControlList &list = ret;\n> > > > +\t\tconst_cast(list) =\n> > > > +\t\t\tIPADataSerializer::deserialize(it1, it2, cs);\n> > >\n> > > Why not\n> > > const ControlList list =\n> > > \t\t\tIPADataSerializer::deserialize(it1, it2, cs);\n> > > return const_cast(list)\n> >\n> > My understanding is that to remove a const with const_cast, the original\n> > variable must not be const.\n> \n> You're right, this won't work, cast should happen on a pointer or\n> reference.\n> \n> This works with gcc and clang:\n> \n> \t\tconst ControlList &list =\n> \t\t\tIPADataSerializer::deserialize(data, cs);\n> \n> \t\treturn const_cast(list);\n> \n> As the function returns by value it should be ok.\n> Tests pass as well, but I'm not sure if this case is tested though.\n> \n> >\n> > > > +\n> > > > +\t\treturn ret;\n> > > > +\t}\n> > > > +\n> > > > + */\n> \n> [snip]\n> \n> > > > +\n> > > > +/**\n> > > > + * \\fn template T readUInt(std::vector &vec, size_t pos)\n> > > > + * \\brief Read uint from byte vector, in little-endian order\n> > > > + * \\tparam T Type of uint to read\n> > > > + * \\param[in] vec Byte vector to read from\n> > > > + * \\param[in] pos Index in vec to start reading from\n> > > > + *\n> > >\n> > > No need for an empty line if no additional description is provided\n> >\n> > ack\n> >\n> > > > + * \\return The uint read from \\a vec, or 0 if reading goes past end of \\a vec\n> > >\n> > > I wonder if we should not pass a T * as an output parameter and return\n> > > an error code. Returning 0 makes it impossible to detect failure, as 0\n> > > is a valid value\n> >\n> > I was actually wondering about that since when I wrote it in the first\n> > place. This might be a good solution.\n> \n> Up to you. Do you check for the return value or do you need to do so ?\n\nNow that I think about it, I don't think we should return error. I think\njust logging error and return default value and continuing is fine.\n\nAlthough maybe it would be better to use the default value instead of\nzero, if a default value is specified.\n\nAh, that's why I used default constructor, rather than zero constructor\nin the deserializer! Well, that's the case for the generated\nde/serializer at least. ...which still doesn't work because this will\nreturn zero and it'll get written...\n\nBut also, if this errors, it means something is *really* wrong with\ndeserialization, as in, we didn't receive enough data to deserialize the\nexpected object. So maybe we should be fataling instead of\ncontinuing...?\n\nNot sure which way to go...\n\n> >\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn template T readUInt(std::vector::iterator it)\n> > > > + * \\brief Read uint from byte vector, in little-endian order\n> > > > + * \\tparam T Type of uint to read\n> > > > + * \\param[in] it Iterator of byte vector to read from\n> > > > + *\n> > >\n> > > No need to empty line\n> >\n> > ack\n> >\n> > > > + * \\return The uint read from \\a vec\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn template IPADataSerializer::serialize(\n> > > > + * \tT data,\n> > > > + * \tControlSerializer *cs = nullptr)\n> > > > + * \\brief Serialize an object into byte vector and fd vector\n> > > > + * \\tparam T Type of object to serialize\n> > > > + * \\param[in] data Object to serialize\n> > > > + * \\param[in] cs ControlSerializer\n> > > > + *\n> > > > + * \\a cs is only necessary if the object type \\a T or its members contain\n> > > > + * ControlList or ControlInfoMap.\n> > > > + *\n> > > > + * \\return Tuple of byte vector and fd vector, that is the serialized form\n> > > > + * of \\a data\n> > >\n> > > Maybe later, but I think for each template specialization, we want to\n> > > document the serialization format, in example that for vectors, the\n> > > length is the first 4 bytes and so on (even more for custom libcamera\n> > > data types)\n> >\n> > Good idea.\n> \n> Although it won't be parsed by doxygen and will remain in-code only.\n> But knowing how each container will be serialized might help\n> validating it.\n\nYeah.\n\n> Actually, how does it work for closed source IPAs ? Before this series\n> the pipeline handler and the IPA were in charge of\n> serializing/deserializing with custom code, as the close source blob,\n> possibly written in C, wouldn't be able to link against libcamera and\n> use its helpers (better, they might chose not to, as from a compliance\n> pov it's fine linking a blob against L-GPL code). Is it still possible\n> ? In that case, documenting how things gets serialized might be\n> even more important.\n\nNow we have the custom IPA interface, and the header that's generated\nfrom it. This header and the public libcamera headers contain the\ndefinitions of all the objects that would be passed between the pipeline\nhandler and the IPA. The proxy worker links to the IPA, which exposes\nipaCreate() (extern C) which returns an IPA{pipeline_name}Interface (C++).\nSo this way, the proxy worker can call the IPA functions in the IPA\ndirectly, while passing C++ objects directly!\n\nAs for the other direction, since Signal is in a public header, simply\nincluding that will allow closed-source IPAs to emit Signals to the\nproxy worker... right?\n\n\nThanks,\n\nPaul","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 4C8CBC3B5C\n\tfor ;\n\tWed, 28 Oct 2020 02:52:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A9DFE62290;\n\tWed, 28 Oct 2020 03:52:58 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E4CE862034\n\tfor ;\n\tWed, 28 Oct 2020 03:52:57 +0100 (CET)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0B4CE99A;\n\tWed, 28 Oct 2020 03:52:55 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"XVJm9rYN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1603853577;\n\tbh=rqHmBtEW0i9IOAAZ0DNX10KhuHCzuelg77k4qBQ7XLg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=XVJm9rYNUOTIdXXV3a6/mZgQgcN/t8Fui0HtpLmS8FtY3kGjARG0jAd3OKRmJahm3\n\tboM3rOIzMvFOJnPfYCNSMq2mnshFMvo7UuL6WZVtZgokC1Z0HBUzQ0dkSCmIG4I+jk\n\tYuOYmTqQW86R6K5X0qESRk+s3mrCiMklB0xL6ELU=","Date":"Wed, 28 Oct 2020 11:52:49 +0900","From":"paul.elder@ideasonboard.com","To":"Jacopo Mondi ","Message-ID":"<20201028025249.GA2119@pyrite.rasen.tech>","References":"<20201002143154.468162-1-paul.elder@ideasonboard.com>\n\t<20201002143154.468162-11-paul.elder@ideasonboard.com>\n\t<20201026160752.dtob6iq2tm5kkwg5@uno.localdomain>\n\t<20201027101403.GA2236@pyrite.rasen.tech>\n\t<20201027113019.ifdwfbqzl5qtu7ph@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201027113019.ifdwfbqzl5qtu7ph@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 10/38] libcamera: Add\n\tIPADataSerializer","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]