[{"id":3079,"web_url":"https://patchwork.libcamera.org/comment/3079/","msgid":"<20191118211404.GK8072@bigcity.dyn.berto.se>","date":"2019-11-18T21:14:04","subject":"Re: [libcamera-devel] [PATCH v2 16/24] libcamera: Add controls\n\tserializer","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your work.\n\nOn 2019-11-08 22:54:01 +0200, Laurent Pinchart wrote:\n> Add a new ControlSerializer helper to serialize and deserialize\n> ControlInfoMap and ControlList instances. This will be used to implement\n> the C IPA protocol and the communication with IPA through IPC.\n> \n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\nReviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n\n> ---\n>  src/libcamera/control_serializer.cpp       | 501 +++++++++++++++++++++\n>  src/libcamera/include/control_serializer.h |  52 +++\n>  src/libcamera/include/meson.build          |   1 +\n>  src/libcamera/meson.build                  |   1 +\n>  4 files changed, 555 insertions(+)\n>  create mode 100644 src/libcamera/control_serializer.cpp\n>  create mode 100644 src/libcamera/include/control_serializer.h\n> \n> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> new file mode 100644\n> index 000000000000..5fe096128e49\n> --- /dev/null\n> +++ b/src/libcamera/control_serializer.cpp\n> @@ -0,0 +1,501 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * control_serializer.cpp - Control (de)serializer\n> + */\n> +\n> +#include \"control_serializer.h\"\n> +\n> +#include <algorithm>\n> +#include <memory>\n> +#include <vector>\n> +\n> +#include <ipa/ipa_controls.h>\n> +#include <libcamera/control_ids.h>\n> +#include <libcamera/controls.h>\n> +\n> +#include \"byte_stream_buffer.h\"\n> +#include \"log.h\"\n> +\n> +/**\n> + * \\file control_serializer.h\n> + * \\brief Serialization and deserialization helpers for controls\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(Serializer)\n> +\n> +namespace {\n> +\n> +static constexpr size_t ControlValueSize[] = {\n> +\t[ControlTypeNone]\t= 1,\n> +\t[ControlTypeBool]\t= sizeof(bool),\n> +\t[ControlTypeInteger32]\t= sizeof(int32_t),\n> +\t[ControlTypeInteger64]\t= sizeof(int64_t),\n> +};\n> +\n> +} /* namespace */\n> +\n> +/**\n> + * \\class ControlSerializer\n> + * \\brief Serializer and deserializer for control-related classes\n> + *\n> + * The control serializer is a helper to serialize and deserialize\n> + * ControlInfoMap and ControlValue instances for the purpose of communication\n> + * with IPA modules.\n> + *\n> + * Neither the ControlInfoMap nor the ControlList are self-contained data\n> + * container. ControlInfoMap references an external ControlId in each of its\n> + * entries, and ControlList references a ControlInfoMap for the purpose of\n> + * validation. Serializing and deserializing those objects thus requires a\n> + * context that maintains the associations between them. The control serializer\n> + * fulfils this task.\n> + *\n> + * ControlInfoMap instances can be serialized on their own, but require\n> + * ControlId instances to be provided at deserialization time. The serializer\n> + * recreates those ControlId instances and stores them in an internal cache,\n> + * from which the ControlInfoMap is populated.\n> + *\n> + * ControlList instances need to be associated with a ControlInfoMap when\n> + * deserialized. To make this possible, the control lists are serialized with a\n> + * handle to their ControlInfoMap, and the map is looked up from the handle at\n> + * deserialization time. To make this possible, the serializer assigns a\n> + * numerical handle to ControlInfoList instances when they are serialized, and\n> + * stores the mapping between handle and ControlInfoList both when serializing\n> + * (for the pipeline handler side) and deserializing (for the IPA side) them.\n> + * This mapping is used when serializing a ControlList to include the\n> + * corresponding ControlInfoMap handle in the binary data, and when\n> + * deserializing to retrieve the corresponding ControlInfoMap.\n> + *\n> + * In order to perform those tasks, the serializer keeps an internal state that\n> + * needs to be properly populated. This mechanism requires the ControlInfoMap\n> + * corresponding to a ControlList to have been serialized or deserialized\n> + * before the ControlList is serialized or deserialized. Failure to comply with\n> + * that constraint results in serialization or deserialization failure of the\n> + * ControlList.\n> + *\n> + * The serializer can be reset() to clear its internal state. This may be\n> + * performed when reconfiguring an IPA to avoid constant growth of the internal\n> + * state, especially if the contents of the ControlInfoMap instances change at\n> + * that time. A reset of the serializer invalidates all ControlList and\n> + * ControlInfoMap that have been previously deserialized. The caller shall thus\n> + * proceed with care to avoid stale references.\n> + */\n> +\n> +/**\n> + * \\brief Reset the serializer\n> + *\n> + * Reset the internal state of the serializer. This invalidates all the\n> + * ControlList and ControlInfoMap that have been previously deserialized.\n> + */\n> +void ControlSerializer::reset()\n> +{\n> +\tserial_ = 0;\n> +\n> +\tinfoMapHandles_.clear();\n> +\tinfoMaps_.clear();\n> +\tcontrolIds_.clear();\n> +}\n> +\n> +size_t ControlSerializer::binarySize(const ControlValue &value)\n> +{\n> +\treturn ControlValueSize[value.type()];\n> +}\n> +\n> +size_t ControlSerializer::binarySize(const ControlRange &range)\n> +{\n> +\treturn binarySize(range.min()) + binarySize(range.max());\n> +}\n> +\n> +/**\n> + * \\brief Retrieve the size in bytes required to serialize a ControlInfoMap\n> + * \\param[in] info The control info map\n> + *\n> + * Compute and return the size in bytes required to store the serialized\n> + * ControlInfoMap.\n> + *\n> + * \\return The size in bytes required to store the serialized ControlInfoMap\n> + */\n> +size_t ControlSerializer::binarySize(const ControlInfoMap &info)\n> +{\n> +\tsize_t size = sizeof(struct ipa_controls_header)\n> +\t\t    + info.size() * sizeof(struct ipa_control_range_entry);\n> +\n> +\tfor (const auto &ctrl : info)\n> +\t\tsize += binarySize(ctrl.second);\n> +\n> +\treturn size;\n> +}\n> +\n> +/**\n> + * \\brief Retrieve the size in bytes required to serialize a ControlList\n> + * \\param[in] list The control list\n> + *\n> + * Compute and return the size in bytes required to store the serialized\n> + * ControlList.\n> + *\n> + * \\return The size in bytes required to store the serialized ControlList\n> + */\n> +size_t ControlSerializer::binarySize(const ControlList &list)\n> +{\n> +\tsize_t size = sizeof(struct ipa_controls_header)\n> +\t\t    + list.size() * sizeof(struct ipa_control_value_entry);\n> +\n> +\tfor (const auto &ctrl : list)\n> +\t\tsize += binarySize(ctrl.second);\n> +\n> +\treturn size;\n> +}\n> +\n> +void ControlSerializer::store(const ControlValue &value,\n> +\t\t\t      ByteStreamBuffer &buffer)\n> +{\n> +\tswitch (value.type()) {\n> +\tcase ControlTypeBool: {\n> +\t\tbool data = value.get<bool>();\n> +\t\tbuffer.write(&data);\n> +\t\tbreak;\n> +\t}\n> +\n> +\tcase ControlTypeInteger32: {\n> +\t\tint32_t data = value.get<int32_t>();\n> +\t\tbuffer.write(&data);\n> +\t\tbreak;\n> +\t}\n> +\n> +\tcase ControlTypeInteger64: {\n> +\t\tuint64_t data = value.get<int64_t>();\n> +\t\tbuffer.write(&data);\n> +\t\tbreak;\n> +\t}\n> +\n> +\tdefault:\n> +\t\tbreak;\n> +\t}\n> +}\n> +\n> +void ControlSerializer::store(const ControlRange &range,\n> +\t\t\t      ByteStreamBuffer &buffer)\n> +{\n> +\tstore(range.min(), buffer);\n> +\tstore(range.max(), buffer);\n> +}\n> +\n> +/**\n> + * \\brief Serialize a ControlInfoMap in a buffer\n> + * \\param[in] info The control info map to serialize\n> + * \\param[in] buffer The memory buffer where to serialize the ControlInfoMap\n> + *\n> + * Serialize the \\a info map into the \\a buffer using the serialization format\n> + * defined by the IPA context interface in ipa_controls.h.\n> + *\n> + * The serializer stores a reference to the \\a info internally. The caller\n> + * shall ensure that \\a info stays valid until the serializer is reset().\n> + *\n> + * \\return 0 on success, a negative error code otherwise\n> + * \\retval -ENOSPC Not enough space is available in the buffer\n> + */\n> +int ControlSerializer::serialize(const ControlInfoMap &info,\n> +\t\t\t\t ByteStreamBuffer &buffer)\n> +{\n> +\t/* Compute entries and data required sizes. */\n> +\tsize_t entriesSize = info.size() * sizeof(struct ipa_control_range_entry);\n> +\tsize_t valuesSize = 0;\n> +\tfor (const auto &ctrl : info)\n> +\t\tvaluesSize += binarySize(ctrl.second);\n> +\n> +\t/* Prepare the packet header, assign a handle to the ControlInfoMap. */\n> +\tstruct ipa_controls_header hdr;\n> +\thdr.version = IPA_CONTROLS_FORMAT_VERSION;\n> +\thdr.handle = ++serial_;\n> +\thdr.entries = info.size();\n> +\thdr.size = sizeof(hdr) + entriesSize + valuesSize;\n> +\thdr.data_offset = sizeof(hdr) + entriesSize;\n> +\n> +\tbuffer.write(&hdr);\n> +\n> +\t/*\n> +\t * Serialize all entries.\n> +\t * \\todo Serialize the control name too\n> +\t */\n> +\tByteStreamBuffer entries = buffer.carveOut(entriesSize);\n> +\tByteStreamBuffer values = buffer.carveOut(valuesSize);\n> +\n> +\tfor (const auto &ctrl : info) {\n> +\t\tconst ControlId *id = ctrl.first;\n> +\t\tconst ControlRange &range = ctrl.second;\n> +\n> +\t\tstruct ipa_control_range_entry entry;\n> +\t\tentry.id = id->id();\n> +\t\tentry.type = id->type();\n> +\t\tentry.offset = values.offset();\n> +\t\tentries.write(&entry);\n> +\n> +\t\tstore(range, values);\n> +\t}\n> +\n> +\tif (buffer.overflow())\n> +\t\treturn -ENOSPC;\n> +\n> +\t/*\n> +\t * Store the map to handle association, to be used to serialize and\n> +\t * deserialize control lists.\n> +\t */\n> +\tinfoMapHandles_[&info] = hdr.handle;\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\brief Serialize a ControlList in a buffer\n> + * \\param[in] list The control list to serialize\n> + * \\param[in] buffer The memory buffer where to serialize the ControlList\n> + *\n> + * Serialize the \\a list into the \\a buffer using the serialization format\n> + * defined by the IPA context interface in ipa_controls.h.\n> + *\n> + * \\return 0 on success, a negative error code otherwise\n> + * \\retval -ENOSPC Not enough space is available in the buffer\n> + */\n> +int ControlSerializer::serialize(const ControlList &list,\n> +\t\t\t\t ByteStreamBuffer &buffer)\n> +{\n> +\t/*\n> +\t * Find the ControlInfoMap handle for the ControlList if it has one, or\n> +\t * use 0 for ControlList without a ControlInfoMap.\n> +\t */\n> +\tunsigned int infoMapHandle;\n> +\tif (list.infoMap()) {\n> +\t\tauto iter = infoMapHandles_.find(list.infoMap());\n> +\t\tif (iter == infoMapHandles_.end()) {\n> +\t\t\tLOG(Serializer, Error)\n> +\t\t\t\t<< \"Can't serialize ControlList: unknown ControlInfoMap\";\n> +\t\t\treturn -ENOENT;\n> +\t\t}\n> +\n> +\t\tinfoMapHandle = iter->second;\n> +\t} else {\n> +\t\tinfoMapHandle = 0;\n> +\t}\n> +\n> +\tsize_t entriesSize = list.size() * sizeof(struct ipa_control_value_entry);\n> +\tsize_t valuesSize = 0;\n> +\tfor (const auto &ctrl : list)\n> +\t\tvaluesSize += binarySize(ctrl.second);\n> +\n> +\t/* Prepare the packet header. */\n> +\tstruct ipa_controls_header hdr;\n> +\thdr.version = IPA_CONTROLS_FORMAT_VERSION;\n> +\thdr.handle = infoMapHandle;\n> +\thdr.entries = list.size();\n> +\thdr.size = sizeof(hdr) + entriesSize + valuesSize;\n> +\thdr.data_offset = sizeof(hdr) + entriesSize;\n> +\n> +\tbuffer.write(&hdr);\n> +\n> +\tByteStreamBuffer entries = buffer.carveOut(entriesSize);\n> +\tByteStreamBuffer values = buffer.carveOut(valuesSize);\n> +\n> +\t/* Serialize all entries. */\n> +\tfor (const auto &ctrl : list) {\n> +\t\tunsigned int id = ctrl.first;\n> +\t\tconst ControlValue &value = ctrl.second;\n> +\n> +\t\tstruct ipa_control_value_entry entry;\n> +\t\tentry.id = id;\n> +\t\tentry.count = 1;\n> +\t\tentry.type = value.type();\n> +\t\tentry.offset = values.offset();\n> +\t\tentries.write(&entry);\n> +\n> +\t\tstore(value, values);\n> +\t}\n> +\n> +\tif (buffer.overflow())\n> +\t\treturn -ENOSPC;\n> +\n> +\treturn 0;\n> +}\n> +\n> +template<>\n> +ControlValue ControlSerializer::load<ControlValue>(ControlType type,\n> +\t\t\t\t\t\t   ByteStreamBuffer &b)\n> +{\n> +\tswitch (type) {\n> +\tcase ControlTypeBool: {\n> +\t\tbool value;\n> +\t\tb.read(&value);\n> +\t\treturn ControlValue(value);\n> +\t}\n> +\n> +\tcase ControlTypeInteger32: {\n> +\t\tint32_t value;\n> +\t\tb.read(&value);\n> +\t\treturn ControlValue(value);\n> +\t}\n> +\n> +\tcase ControlTypeInteger64: {\n> +\t\tint64_t value;\n> +\t\tb.read(&value);\n> +\t\treturn ControlValue(value);\n> +\t}\n> +\n> +\tdefault:\n> +\t\treturn ControlValue();\n> +\t}\n> +}\n> +\n> +template<>\n> +ControlRange ControlSerializer::load<ControlRange>(ControlType type,\n> +\t\t\t\t\t\t   ByteStreamBuffer &b)\n> +{\n> +\tControlValue min = load<ControlValue>(type, b);\n> +\tControlValue max = load<ControlValue>(type, b);\n> +\n> +\treturn ControlRange(min, max);\n> +}\n> +\n> +/**\n> + * \\fn template<typename T> T ControlSerializer::deserialize(ByteStreamBuffer &buffer)\n> + * \\brief Deserialize an object from a binary buffer\n> + * \\param[in] buffer The memory buffer that contains the object\n> + *\n> + * This method is only valid when specialized for ControlInfoMap or\n> + * ControlList. Any other typename \\a T is not supported.\n> + */\n> +\n> +/**\n> + * \\brief Deserialize a ControlInfoMap from a binary buffer\n> + * \\param[in] buffer The memory buffer that contains the serialized map\n> + *\n> + * Re-construct a ControlInfoMap from a binary \\a buffer containing data\n> + * serialized using the serialize() method.\n> + *\n> + * \\return The deserialized ControlInfoMap\n> + */\n> +template<>\n> +ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &buffer)\n> +{\n> +\tstruct ipa_controls_header hdr;\n> +\tbuffer.read(&hdr);\n> +\n> +\tif (hdr.version != IPA_CONTROLS_FORMAT_VERSION) {\n> +\t\tLOG(Serializer, Error)\n> +\t\t\t<< \"Unsupported controls format version \"\n> +\t\t\t<< hdr.version;\n> +\t\treturn {};\n> +\t}\n> +\n> +\tByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr));\n> +\tByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset);\n> +\n> +\tif (buffer.overflow())\n> +\t\treturn {};\n> +\n> +\tControlInfoMap::Map ctrls;\n> +\n> +\tfor (unsigned int i = 0; i < hdr.entries; ++i) {\n> +\t\tstruct ipa_control_range_entry entry;\n> +\t\tentries.read(&entry);\n> +\n> +\t\t/* Create and cache the individual ControlId. */\n> +\t\tControlType type = static_cast<ControlType>(entry.type);\n> +\t\tcontrolIds_.emplace_back(utils::make_unique<ControlId>(entry.id, \"\", type));\n> +\n> +\t\tif (entry.offset != values.offset()) {\n> +\t\t\tLOG(Serializer, Error)\n> +\t\t\t\t<< \"Bad data, entry offset mismatch (entry \"\n> +\t\t\t\t<< i << \")\";\n> +\t\t\treturn {};\n> +\t\t}\n> +\n> +\t\t/* Create and store the ControlRange. */\n> +\t\tctrls.emplace(controlIds_.back().get(),\n> +\t\t\t      load<ControlRange>(type, values));\n> +\t}\n> +\n> +\t/*\n> +\t * Create the ControlInfoMap in the cache, and store the map to handle\n> +\t * association.\n> +\t */\n> +\tControlInfoMap &map = infoMaps_[hdr.handle] = std::move(ctrls);\n> +\tinfoMapHandles_[&map] = hdr.handle;\n> +\n> +\treturn map;\n> +}\n> +\n> +/**\n> + * \\brief Deserialize a ControlList from a binary buffer\n> + * \\param[in] buffer The memory buffer that contains the serialized list\n> + *\n> + * Re-construct a ControlList from a binary \\a buffer containing data\n> + * serialized using the serialize() method.\n> + *\n> + * \\return The deserialized ControlList\n> + */\n> +template<>\n> +ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer)\n> +{\n> +\tstruct ipa_controls_header hdr;\n> +\tbuffer.read(&hdr);\n> +\n> +\tif (hdr.version != IPA_CONTROLS_FORMAT_VERSION) {\n> +\t\tLOG(Serializer, Error)\n> +\t\t\t<< \"Unsupported controls format version \"\n> +\t\t\t<< hdr.version;\n> +\t\treturn {};\n> +\t}\n> +\n> +\tByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr));\n> +\tByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset);\n> +\n> +\tif (buffer.overflow())\n> +\t\treturn {};\n> +\n> +\t/*\n> +\t * Retrieve the ControlInfoMap associated with the ControlList based on\n> +\t * its ID. The mapping between infoMap and ID is set up when serializing\n> +\t * or deserializing ControlInfoMap. If no mapping is found (which is\n> +\t * currently the case for ControlList related to libcamera controls),\n> +\t * use the global control::control idmap.\n> +\t */\n> +\tconst ControlInfoMap *infoMap;\n> +\tif (hdr.handle) {\n> +\t\tauto iter = std::find_if(infoMapHandles_.begin(), infoMapHandles_.end(),\n> +\t\t\t\t\t [&](decltype(infoMapHandles_)::value_type &entry) {\n> +\t\t\t\t\t\t return entry.second == hdr.handle;\n> +\t\t\t\t\t });\n> +\t\tif (iter == infoMapHandles_.end()) {\n> +\t\t\tLOG(Serializer, Error)\n> +\t\t\t\t<< \"Can't deserialize ControlList: unknown ControlInfoMap\";\n> +\t\t\treturn {};\n> +\t\t}\n> +\n> +\t\tinfoMap = iter->first;\n> +\t} else {\n> +\t\tinfoMap = nullptr;\n> +\t}\n> +\n> +\tControlList ctrls(infoMap ? infoMap->idmap() : controls::controls);\n> +\n> +\tfor (unsigned int i = 0; i < hdr.entries; ++i) {\n> +\t\tstruct ipa_control_value_entry entry;\n> +\t\tentries.read(&entry);\n> +\n> +\t\tif (entry.offset != values.offset()) {\n> +\t\t\tLOG(Serializer, Error)\n> +\t\t\t\t<< \"Bad data, entry offset mismatch (entry \"\n> +\t\t\t\t<< i << \")\";\n> +\t\t\treturn {};\n> +\t\t}\n> +\n> +\t\tControlType type = static_cast<ControlType>(entry.type);\n> +\t\tctrls.set(entry.id, load<ControlValue>(type, values));\n> +\t}\n> +\n> +\treturn ctrls;\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/include/control_serializer.h b/src/libcamera/include/control_serializer.h\n> new file mode 100644\n> index 000000000000..bb3cb8e7b904\n> --- /dev/null\n> +++ b/src/libcamera/include/control_serializer.h\n> @@ -0,0 +1,52 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * control_serializer.h - Control (de)serializer\n> + */\n> +#ifndef __LIBCAMERA_CONTROL_SERIALIZER_H__\n> +#define __LIBCAMERA_CONTROL_SERIALIZER_H__\n> +\n> +#include <map>\n> +#include <memory>\n> +#include <vector>\n> +\n> +#include <libcamera/controls.h>\n> +\n> +namespace libcamera {\n> +\n> +class ByteStreamBuffer;\n> +\n> +class ControlSerializer\n> +{\n> +public:\n> +\tvoid reset();\n> +\n> +\tstatic size_t binarySize(const ControlInfoMap &info);\n> +\tstatic size_t binarySize(const ControlList &list);\n> +\n> +\tint serialize(const ControlInfoMap &info, ByteStreamBuffer &buffer);\n> +\tint serialize(const ControlList &list, ByteStreamBuffer &buffer);\n> +\n> +\ttemplate<typename T>\n> +\tT deserialize(ByteStreamBuffer &buffer);\n> +\n> +private:\n> +\tstatic size_t binarySize(const ControlValue &value);\n> +\tstatic size_t binarySize(const ControlRange &range);\n> +\n> +\tstatic void store(const ControlValue &value, ByteStreamBuffer &buffer);\n> +\tstatic void store(const ControlRange &range, ByteStreamBuffer &buffer);\n> +\n> +\ttemplate<typename T>\n> +\tT load(ControlType type, ByteStreamBuffer &b);\n> +\n> +\tunsigned int serial_;\n> +\tstd::vector<std::unique_ptr<ControlId>> controlIds_;\n> +\tstd::map<unsigned int, ControlInfoMap> infoMaps_;\n> +\tstd::map<const ControlInfoMap *, unsigned int> infoMapHandles_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_CONTROL_SERIALIZER_H__ */\n> diff --git a/src/libcamera/include/meson.build b/src/libcamera/include/meson.build\n> index 1ff0198662cc..697294f4b09b 100644\n> --- a/src/libcamera/include/meson.build\n> +++ b/src/libcamera/include/meson.build\n> @@ -2,6 +2,7 @@ libcamera_headers = files([\n>      'byte_stream_buffer.h',\n>      'camera_controls.h',\n>      'camera_sensor.h',\n> +    'control_serializer.h',\n>      'control_validator.h',\n>      'device_enumerator.h',\n>      'device_enumerator_sysfs.h',\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index dab2d8ad2649..59cf582580c4 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -7,6 +7,7 @@ libcamera_sources = files([\n>      'camera_manager.cpp',\n>      'camera_sensor.cpp',\n>      'controls.cpp',\n> +    'control_serializer.cpp',\n>      'control_validator.cpp',\n>      'device_enumerator.cpp',\n>      'device_enumerator_sysfs.cpp',\n> -- \n> Regards,\n> \n> Laurent Pinchart\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<niklas.soderlund@ragnatech.se>","Received":["from mail-lj1-x234.google.com (mail-lj1-x234.google.com\n\t[IPv6:2a00:1450:4864:20::234])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C7C3060F1C\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 18 Nov 2019 22:14:07 +0100 (CET)","by mail-lj1-x234.google.com with SMTP id e9so20637399ljp.13\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 18 Nov 2019 13:14:07 -0800 (PST)","from localhost (h-93-159.A463.priv.bahnhof.se. [46.59.93.159])\n\tby smtp.gmail.com with ESMTPSA id\n\tr4sm8717587ljn.64.2019.11.18.13.14.04\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 18 Nov 2019 13:14:05 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=dF5nBCQxvGnnC1+ER5aboJzwwGOLTZCXw1DxP1/wG2w=;\n\tb=p4eK/GarEW8PaOBFVMeemaU4EWqN7PoCJ0xBX5BiBLEWZawERh3xr+nXQtSr7xbEQD\n\tbvejFKyHZwelC9MWtz47/MzxPg6sNGE0i79Zy9vqqLSYNIFerptFtv3MQVjC4TT1rQq4\n\tToBmDXU2bPmZu3XWiGM37TW5z/1Nozv/d79uTdP4+ocP5OHTD6q4tgCir+94qVDU5zz4\n\t4w2WZQIoD2B5YTY+t0VkI6+GJZ+08RQfEYDDSCVwe7gKSFDZZovyfbqPNV7Ku6Y1gart\n\tdg9tC+SoLjSJPnhabj5yC6brBaZQrVjfYK8/v9ykT79En0UvMnicgqpOutQIBJ1d96Ae\n\t6XXw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=dF5nBCQxvGnnC1+ER5aboJzwwGOLTZCXw1DxP1/wG2w=;\n\tb=Zco9ybU1Sm30lo3jGbiO7f84RJqoDrfC0oVuFot3jm2w4uDRdJxSvfhSoyuIRqSWah\n\tFUSHu+m6p8rnRLPTBhtdEHk0SAUL6FDkgOP0xUQRJKdg+LW8z26ZPB2K5blSkkYY5kS3\n\t4GG/NqyRv5SAUnSgDVbiTLRXuJAMMhZ90FUdEzQLcSTpT4vV037tmUj+g85zJjq/9xzx\n\t7hnfsZRzxZ4QvowJZARXTQezP90BWF9aJ6P1x3kOGCD1Wsnvq+rCZyi+dZp3ar9Cx0Yr\n\t2cB8uvf+D4KQL1CUxv7+p+i+qXxyH9+yNBbonm3Lwxik9gT1m10BHrXhJW+5M2t+8AAp\n\tJ6dA==","X-Gm-Message-State":"APjAAAX4n4auj7tCENw2Tpji3duH2i/PiwOSlWYSezwQJHhOrzIsstiU\n\t47/SCHaTWfSwid8p1bzguQLtpg==","X-Google-Smtp-Source":"APXvYqw9VJEtYUo0vhnoCIwL0b4mhJgeQUGp0zqBtyOL2d/RRkfV7bIZC4AXz7+wmDoffAToJ5ttkg==","X-Received":"by 2002:a2e:85d5:: with SMTP id\n\th21mr1050657ljj.243.1574111646694; \n\tMon, 18 Nov 2019 13:14:06 -0800 (PST)","Date":"Mon, 18 Nov 2019 22:14:04 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191118211404.GK8072@bigcity.dyn.berto.se>","References":"<20191108205409.18845-1-laurent.pinchart@ideasonboard.com>\n\t<20191108205409.18845-17-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20191108205409.18845-17-laurent.pinchart@ideasonboard.com>","User-Agent":"Mutt/1.12.1 (2019-06-15)","Subject":"Re: [libcamera-devel] [PATCH v2 16/24] libcamera: Add controls\n\tserializer","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Mon, 18 Nov 2019 21:14:08 -0000"}},{"id":3095,"web_url":"https://patchwork.libcamera.org/comment/3095/","msgid":"<20191118235000.hkjgsl26s4zurbc6@uno.localdomain>","date":"2019-11-18T23:50:00","subject":"Re: [libcamera-devel] [PATCH v2 16/24] libcamera: Add controls\n\tserializer","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Fri, Nov 08, 2019 at 10:54:01PM +0200, Laurent Pinchart wrote:\n> Add a new ControlSerializer helper to serialize and deserialize\n> ControlInfoMap and ControlList instances. This will be used to implement\n> the C IPA protocol and the communication with IPA through IPC.\n>\n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> ---\n>  src/libcamera/control_serializer.cpp       | 501 +++++++++++++++++++++\n>  src/libcamera/include/control_serializer.h |  52 +++\n>  src/libcamera/include/meson.build          |   1 +\n>  src/libcamera/meson.build                  |   1 +\n>  4 files changed, 555 insertions(+)\n>  create mode 100644 src/libcamera/control_serializer.cpp\n>  create mode 100644 src/libcamera/include/control_serializer.h\n>\n> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> new file mode 100644\n> index 000000000000..5fe096128e49\n> --- /dev/null\n> +++ b/src/libcamera/control_serializer.cpp\n> @@ -0,0 +1,501 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * control_serializer.cpp - Control (de)serializer\n> + */\n> +\n> +#include \"control_serializer.h\"\n> +\n> +#include <algorithm>\n> +#include <memory>\n> +#include <vector>\n> +\n> +#include <ipa/ipa_controls.h>\n> +#include <libcamera/control_ids.h>\n> +#include <libcamera/controls.h>\n> +\n> +#include \"byte_stream_buffer.h\"\n> +#include \"log.h\"\n> +\n> +/**\n> + * \\file control_serializer.h\n> + * \\brief Serialization and deserialization helpers for controls\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(Serializer)\n> +\n> +namespace {\n> +\n> +static constexpr size_t ControlValueSize[] = {\n> +\t[ControlTypeNone]\t= 1,\n> +\t[ControlTypeBool]\t= sizeof(bool),\n> +\t[ControlTypeInteger32]\t= sizeof(int32_t),\n> +\t[ControlTypeInteger64]\t= sizeof(int64_t),\n> +};\n> +\n> +} /* namespace */\n> +\n> +/**\n> + * \\class ControlSerializer\n> + * \\brief Serializer and deserializer for control-related classes\n> + *\n> + * The control serializer is a helper to serialize and deserialize\n> + * ControlInfoMap and ControlValue instances for the purpose of communication\n> + * with IPA modules.\n> + *\n> + * Neither the ControlInfoMap nor the ControlList are self-contained data\n> + * container. ControlInfoMap references an external ControlId in each of its\n> + * entries, and ControlList references a ControlInfoMap for the purpose of\n> + * validation. Serializing and deserializing those objects thus requires a\n> + * context that maintains the associations between them. The control serializer\n> + * fulfils this task.\n> + *\n> + * ControlInfoMap instances can be serialized on their own, but require\n> + * ControlId instances to be provided at deserialization time. The serializer\n> + * recreates those ControlId instances and stores them in an internal cache,\n> + * from which the ControlInfoMap is populated.\n> + *\n> + * ControlList instances need to be associated with a ControlInfoMap when\n> + * deserialized. To make this possible, the control lists are serialized with a\n> + * handle to their ControlInfoMap, and the map is looked up from the handle at\n\ns/from the handle/from the list of handles/ ? or just 'from the list'\n\n> + * deserialization time. To make this possible, the serializer assigns a\n> + * numerical handle to ControlInfoList instances when they are serialized, and\n\ns/ControlInfoList/ControlInfoMap/\n\n> + * stores the mapping between handle and ControlInfoList both when serializing\n\nditto\n\n> + * (for the pipeline handler side) and deserializing (for the IPA side) them.\n> + * This mapping is used when serializing a ControlList to include the\n> + * corresponding ControlInfoMap handle in the binary data, and when\n> + * deserializing to retrieve the corresponding ControlInfoMap.\n\nThat's very nice to know, but this feels more like a comment to the\nimplementation than part of the documentation..\n\n> + *\n> + * In order to perform those tasks, the serializer keeps an internal state that\n> + * needs to be properly populated. This mechanism requires the ControlInfoMap\n> + * corresponding to a ControlList to have been serialized or deserialized\n> + * before the ControlList is serialized or deserialized. Failure to comply with\n> + * that constraint results in serialization or deserialization failure of the\n> + * ControlList.\n> + *\n> + * The serializer can be reset() to clear its internal state. This may be\n> + * performed when reconfiguring an IPA to avoid constant growth of the internal\n> + * state, especially if the contents of the ControlInfoMap instances change at\n> + * that time. A reset of the serializer invalidates all ControlList and\n> + * ControlInfoMap that have been previously deserialized. The caller shall thus\n> + * proceed with care to avoid stale references.\n> + */\n> +\n> +/**\n> + * \\brief Reset the serializer\n> + *\n> + * Reset the internal state of the serializer. This invalidates all the\n> + * ControlList and ControlInfoMap that have been previously deserialized.\n> + */\n> +void ControlSerializer::reset()\n> +{\n> +\tserial_ = 0;\n> +\n> +\tinfoMapHandles_.clear();\n> +\tinfoMaps_.clear();\n> +\tcontrolIds_.clear();\n> +}\n> +\n> +size_t ControlSerializer::binarySize(const ControlValue &value)\n> +{\n> +\treturn ControlValueSize[value.type()];\n> +}\n> +\n> +size_t ControlSerializer::binarySize(const ControlRange &range)\n> +{\n> +\treturn binarySize(range.min()) + binarySize(range.max());\n> +}\n> +\n> +/**\n> + * \\brief Retrieve the size in bytes required to serialize a ControlInfoMap\n> + * \\param[in] info The control info map\n> + *\n> + * Compute and return the size in bytes required to store the serialized\n> + * ControlInfoMap.\n> + *\n> + * \\return The size in bytes required to store the serialized ControlInfoMap\n> + */\n> +size_t ControlSerializer::binarySize(const ControlInfoMap &info)\n> +{\n> +\tsize_t size = sizeof(struct ipa_controls_header)\n> +\t\t    + info.size() * sizeof(struct ipa_control_range_entry);\n> +\n> +\tfor (const auto &ctrl : info)\n> +\t\tsize += binarySize(ctrl.second);\n> +\n> +\treturn size;\n> +}\n> +\n> +/**\n> + * \\brief Retrieve the size in bytes required to serialize a ControlList\n> + * \\param[in] list The control list\n> + *\n> + * Compute and return the size in bytes required to store the serialized\n> + * ControlList.\n> + *\n> + * \\return The size in bytes required to store the serialized ControlList\n> + */\n> +size_t ControlSerializer::binarySize(const ControlList &list)\n> +{\n> +\tsize_t size = sizeof(struct ipa_controls_header)\n> +\t\t    + list.size() * sizeof(struct ipa_control_value_entry);\n> +\n> +\tfor (const auto &ctrl : list)\n> +\t\tsize += binarySize(ctrl.second);\n> +\n> +\treturn size;\n> +}\n> +\n> +void ControlSerializer::store(const ControlValue &value,\n> +\t\t\t      ByteStreamBuffer &buffer)\n> +{\n> +\tswitch (value.type()) {\n> +\tcase ControlTypeBool: {\n> +\t\tbool data = value.get<bool>();\n> +\t\tbuffer.write(&data);\n\nActually it's quite nice how ByteStreamBuffer abstract away memory\nhandling. Now I'm debated...\n\n> +\t\tbreak;\n> +\t}\n> +\n> +\tcase ControlTypeInteger32: {\n> +\t\tint32_t data = value.get<int32_t>();\n> +\t\tbuffer.write(&data);\n> +\t\tbreak;\n> +\t}\n> +\n> +\tcase ControlTypeInteger64: {\n> +\t\tuint64_t data = value.get<int64_t>();\n> +\t\tbuffer.write(&data);\n> +\t\tbreak;\n> +\t}\n> +\n> +\tdefault:\n> +\t\tbreak;\n> +\t}\n> +}\n> +\n> +void ControlSerializer::store(const ControlRange &range,\n> +\t\t\t      ByteStreamBuffer &buffer)\n> +{\n> +\tstore(range.min(), buffer);\n> +\tstore(range.max(), buffer);\n> +}\n> +\n> +/**\n> + * \\brief Serialize a ControlInfoMap in a buffer\n> + * \\param[in] info The control info map to serialize\n> + * \\param[in] buffer The memory buffer where to serialize the ControlInfoMap\n> + *\n> + * Serialize the \\a info map into the \\a buffer using the serialization format\n> + * defined by the IPA context interface in ipa_controls.h.\n> + *\n> + * The serializer stores a reference to the \\a info internally. The caller\n> + * shall ensure that \\a info stays valid until the serializer is reset().\n> + *\n> + * \\return 0 on success, a negative error code otherwise\n> + * \\retval -ENOSPC Not enough space is available in the buffer\n> + */\n> +int ControlSerializer::serialize(const ControlInfoMap &info,\n> +\t\t\t\t ByteStreamBuffer &buffer)\n> +{\n> +\t/* Compute entries and data required sizes. */\n> +\tsize_t entriesSize = info.size() * sizeof(struct ipa_control_range_entry);\n> +\tsize_t valuesSize = 0;\n> +\tfor (const auto &ctrl : info)\n> +\t\tvaluesSize += binarySize(ctrl.second);\n> +\n> +\t/* Prepare the packet header, assign a handle to the ControlInfoMap. */\n> +\tstruct ipa_controls_header hdr;\n> +\thdr.version = IPA_CONTROLS_FORMAT_VERSION;\n> +\thdr.handle = ++serial_;\n> +\thdr.entries = info.size();\n> +\thdr.size = sizeof(hdr) + entriesSize + valuesSize;\n> +\thdr.data_offset = sizeof(hdr) + entriesSize;\n> +\n> +\tbuffer.write(&hdr);\n> +\n> +\t/*\n> +\t * Serialize all entries.\n> +\t * \\todo Serialize the control name too\n> +\t */\n> +\tByteStreamBuffer entries = buffer.carveOut(entriesSize);\n> +\tByteStreamBuffer values = buffer.carveOut(valuesSize);\n> +\n> +\tfor (const auto &ctrl : info) {\n> +\t\tconst ControlId *id = ctrl.first;\n> +\t\tconst ControlRange &range = ctrl.second;\n> +\n> +\t\tstruct ipa_control_range_entry entry;\n> +\t\tentry.id = id->id();\n> +\t\tentry.type = id->type();\n> +\t\tentry.offset = values.offset();\n> +\t\tentries.write(&entry);\n> +\n> +\t\tstore(range, values);\n> +\t}\n> +\n> +\tif (buffer.overflow())\n> +\t\treturn -ENOSPC;\n> +\n> +\t/*\n> +\t * Store the map to handle association, to be used to serialize and\n> +\t * deserialize control lists.\n> +\t */\n> +\tinfoMapHandles_[&info] = hdr.handle;\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\brief Serialize a ControlList in a buffer\n> + * \\param[in] list The control list to serialize\n> + * \\param[in] buffer The memory buffer where to serialize the ControlList\n> + *\n> + * Serialize the \\a list into the \\a buffer using the serialization format\n> + * defined by the IPA context interface in ipa_controls.h.\n> + *\n> + * \\return 0 on success, a negative error code otherwise\n> + * \\retval -ENOSPC Not enough space is available in the buffer\n> + */\n> +int ControlSerializer::serialize(const ControlList &list,\n> +\t\t\t\t ByteStreamBuffer &buffer)\n> +{\n> +\t/*\n> +\t * Find the ControlInfoMap handle for the ControlList if it has one, or\n> +\t * use 0 for ControlList without a ControlInfoMap.\n> +\t */\n> +\tunsigned int infoMapHandle;\n> +\tif (list.infoMap()) {\n> +\t\tauto iter = infoMapHandles_.find(list.infoMap());\n> +\t\tif (iter == infoMapHandles_.end()) {\n> +\t\t\tLOG(Serializer, Error)\n> +\t\t\t\t<< \"Can't serialize ControlList: unknown ControlInfoMap\";\n> +\t\t\treturn -ENOENT;\n\nDocument -ENOENT too ?\n\n> +\t\t}\n> +\n> +\t\tinfoMapHandle = iter->second;\n> +\t} else {\n> +\t\tinfoMapHandle = 0;\n> +\t}\n> +\n> +\tsize_t entriesSize = list.size() * sizeof(struct ipa_control_value_entry);\n> +\tsize_t valuesSize = 0;\n> +\tfor (const auto &ctrl : list)\n> +\t\tvaluesSize += binarySize(ctrl.second);\n> +\n> +\t/* Prepare the packet header. */\n> +\tstruct ipa_controls_header hdr;\n> +\thdr.version = IPA_CONTROLS_FORMAT_VERSION;\n> +\thdr.handle = infoMapHandle;\n> +\thdr.entries = list.size();\n> +\thdr.size = sizeof(hdr) + entriesSize + valuesSize;\n> +\thdr.data_offset = sizeof(hdr) + entriesSize;\n> +\n> +\tbuffer.write(&hdr);\n> +\n> +\tByteStreamBuffer entries = buffer.carveOut(entriesSize);\n> +\tByteStreamBuffer values = buffer.carveOut(valuesSize);\n> +\n> +\t/* Serialize all entries. */\n> +\tfor (const auto &ctrl : list) {\n> +\t\tunsigned int id = ctrl.first;\n> +\t\tconst ControlValue &value = ctrl.second;\n> +\n> +\t\tstruct ipa_control_value_entry entry;\n> +\t\tentry.id = id;\n> +\t\tentry.count = 1;\n> +\t\tentry.type = value.type();\n> +\t\tentry.offset = values.offset();\n> +\t\tentries.write(&entry);\n> +\n> +\t\tstore(value, values);\n\nI would love to see something like\n                values.write(value)\n\nWhat would you think to move the store() (and load?) function to the\nByteStreamBuffer. It's only used here, and making it aware of\nControlValue and ControlRanges would not be a blasphemy... It would\nremove the need to \"store()\" and \"load()\" data here, which basically\nresolves to inspecting the ControlValue/Range type... If we split them\nthen the ControlSerializer would only deal with containers and\nByteStreamBuffer with actual values. Does it make sense ?\n\n> +\t}\n> +\n> +\tif (buffer.overflow())\n> +\t\treturn -ENOSPC;\n\nCan't we check the ByteStreamBuffer::write return value in the loop\nand break earlier ?\n\n> +\n> +\treturn 0;\n> +}\n> +\n> +template<>\n> +ControlValue ControlSerializer::load<ControlValue>(ControlType type,\n> +\t\t\t\t\t\t   ByteStreamBuffer &b)\n> +{\n> +\tswitch (type) {\n> +\tcase ControlTypeBool: {\n> +\t\tbool value;\n> +\t\tb.read(&value);\n> +\t\treturn ControlValue(value);\n> +\t}\n> +\n> +\tcase ControlTypeInteger32: {\n> +\t\tint32_t value;\n> +\t\tb.read(&value);\n> +\t\treturn ControlValue(value);\n> +\t}\n> +\n> +\tcase ControlTypeInteger64: {\n> +\t\tint64_t value;\n> +\t\tb.read(&value);\n> +\t\treturn ControlValue(value);\n> +\t}\n> +\n> +\tdefault:\n> +\t\treturn ControlValue();\n> +\t}\n> +}\n> +\n> +template<>\n> +ControlRange ControlSerializer::load<ControlRange>(ControlType type,\n> +\t\t\t\t\t\t   ByteStreamBuffer &b)\n> +{\n> +\tControlValue min = load<ControlValue>(type, b);\n> +\tControlValue max = load<ControlValue>(type, b);\n> +\n> +\treturn ControlRange(min, max);\n> +}\n> +\n> +/**\n> + * \\fn template<typename T> T ControlSerializer::deserialize(ByteStreamBuffer &buffer)\n> + * \\brief Deserialize an object from a binary buffer\n> + * \\param[in] buffer The memory buffer that contains the object\n> + *\n> + * This method is only valid when specialized for ControlInfoMap or\n> + * ControlList. Any other typename \\a T is not supported.\n> + */\n> +\n> +/**\n> + * \\brief Deserialize a ControlInfoMap from a binary buffer\n> + * \\param[in] buffer The memory buffer that contains the serialized map\n> + *\n> + * Re-construct a ControlInfoMap from a binary \\a buffer containing data\n> + * serialized using the serialize() method.\n> + *\n> + * \\return The deserialized ControlInfoMap\n> + */\n> +template<>\n> +ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &buffer)\n> +{\n> +\tstruct ipa_controls_header hdr;\n> +\tbuffer.read(&hdr);\n> +\n> +\tif (hdr.version != IPA_CONTROLS_FORMAT_VERSION) {\n> +\t\tLOG(Serializer, Error)\n> +\t\t\t<< \"Unsupported controls format version \"\n> +\t\t\t<< hdr.version;\n> +\t\treturn {};\n> +\t}\n> +\n> +\tByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr));\n> +\tByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset);\n> +\n> +\tif (buffer.overflow())\n> +\t\treturn {};\n\nMaybe an error message would help ?\n\n> +\n> +\tControlInfoMap::Map ctrls;\n> +\n> +\tfor (unsigned int i = 0; i < hdr.entries; ++i) {\n> +\t\tstruct ipa_control_range_entry entry;\n> +\t\tentries.read(&entry);\n> +\n> +\t\t/* Create and cache the individual ControlId. */\n> +\t\tControlType type = static_cast<ControlType>(entry.type);\n> +\t\tcontrolIds_.emplace_back(utils::make_unique<ControlId>(entry.id, \"\", type));\n\nI would note with a todo, we've lost the name\n\n> +\n> +\t\tif (entry.offset != values.offset()) {\n> +\t\t\tLOG(Serializer, Error)\n> +\t\t\t\t<< \"Bad data, entry offset mismatch (entry \"\n> +\t\t\t\t<< i << \")\";\n> +\t\t\treturn {};\n> +\t\t}\n> +\n> +\t\t/* Create and store the ControlRange. */\n> +\t\tctrls.emplace(controlIds_.back().get(),\n> +\t\t\t      load<ControlRange>(type, values));\n> +\t}\n> +\n> +\t/*\n> +\t * Create the ControlInfoMap in the cache, and store the map to handle\n> +\t * association.\n> +\t */\n> +\tControlInfoMap &map = infoMaps_[hdr.handle] = std::move(ctrls);\n> +\tinfoMapHandles_[&map] = hdr.handle;\n> +\n> +\treturn map;\n> +}\n> +\n> +/**\n> + * \\brief Deserialize a ControlList from a binary buffer\n> + * \\param[in] buffer The memory buffer that contains the serialized list\n> + *\n> + * Re-construct a ControlList from a binary \\a buffer containing data\n> + * serialized using the serialize() method.\n> + *\n> + * \\return The deserialized ControlList\n> + */\n> +template<>\n> +ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer)\n> +{\n> +\tstruct ipa_controls_header hdr;\n> +\tbuffer.read(&hdr);\n> +\n> +\tif (hdr.version != IPA_CONTROLS_FORMAT_VERSION) {\n> +\t\tLOG(Serializer, Error)\n> +\t\t\t<< \"Unsupported controls format version \"\n> +\t\t\t<< hdr.version;\n> +\t\treturn {};\n> +\t}\n> +\n> +\tByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr));\n> +\tByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset);\n> +\n> +\tif (buffer.overflow())\n> +\t\treturn {};\n> +\n> +\t/*\n> +\t * Retrieve the ControlInfoMap associated with the ControlList based on\n> +\t * its ID. The mapping between infoMap and ID is set up when serializing\n> +\t * or deserializing ControlInfoMap. If no mapping is found (which is\n> +\t * currently the case for ControlList related to libcamera controls),\n> +\t * use the global control::control idmap.\n> +\t */\n> +\tconst ControlInfoMap *infoMap;\n> +\tif (hdr.handle) {\n> +\t\tauto iter = std::find_if(infoMapHandles_.begin(), infoMapHandles_.end(),\n> +\t\t\t\t\t [&](decltype(infoMapHandles_)::value_type &entry) {\n> +\t\t\t\t\t\t return entry.second == hdr.handle;\n> +\t\t\t\t\t });\n> +\t\tif (iter == infoMapHandles_.end()) {\n> +\t\t\tLOG(Serializer, Error)\n> +\t\t\t\t<< \"Can't deserialize ControlList: unknown ControlInfoMap\";\n> +\t\t\treturn {};\n> +\t\t}\n> +\n> +\t\tinfoMap = iter->first;\n> +\t} else {\n> +\t\tinfoMap = nullptr;\n> +\t}\n> +\n> +\tControlList ctrls(infoMap ? infoMap->idmap() : controls::controls);\n> +\n> +\tfor (unsigned int i = 0; i < hdr.entries; ++i) {\n> +\t\tstruct ipa_control_value_entry entry;\n> +\t\tentries.read(&entry);\n> +\n> +\t\tif (entry.offset != values.offset()) {\n> +\t\t\tLOG(Serializer, Error)\n> +\t\t\t\t<< \"Bad data, entry offset mismatch (entry \"\n> +\t\t\t\t<< i << \")\";\n> +\t\t\treturn {};\n> +\t\t}\n> +\n> +\t\tControlType type = static_cast<ControlType>(entry.type);\n> +\t\tctrls.set(entry.id, load<ControlValue>(type, values));\n> +\t}\n> +\n> +\treturn ctrls;\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/include/control_serializer.h b/src/libcamera/include/control_serializer.h\n> new file mode 100644\n> index 000000000000..bb3cb8e7b904\n> --- /dev/null\n> +++ b/src/libcamera/include/control_serializer.h\n> @@ -0,0 +1,52 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * control_serializer.h - Control (de)serializer\n> + */\n> +#ifndef __LIBCAMERA_CONTROL_SERIALIZER_H__\n> +#define __LIBCAMERA_CONTROL_SERIALIZER_H__\n> +\n> +#include <map>\n> +#include <memory>\n> +#include <vector>\n> +\n> +#include <libcamera/controls.h>\n> +\n> +namespace libcamera {\n> +\n> +class ByteStreamBuffer;\n> +\n> +class ControlSerializer\n> +{\n> +public:\n> +\tvoid reset();\n> +\n> +\tstatic size_t binarySize(const ControlInfoMap &info);\n> +\tstatic size_t binarySize(const ControlList &list);\n> +\n> +\tint serialize(const ControlInfoMap &info, ByteStreamBuffer &buffer);\n> +\tint serialize(const ControlList &list, ByteStreamBuffer &buffer);\n> +\n> +\ttemplate<typename T>\n> +\tT deserialize(ByteStreamBuffer &buffer);\n> +\n> +private:\n> +\tstatic size_t binarySize(const ControlValue &value);\n> +\tstatic size_t binarySize(const ControlRange &range);\n> +\n> +\tstatic void store(const ControlValue &value, ByteStreamBuffer &buffer);\n> +\tstatic void store(const ControlRange &range, ByteStreamBuffer &buffer);\n> +\n> +\ttemplate<typename T>\n> +\tT load(ControlType type, ByteStreamBuffer &b);\n> +\n> +\tunsigned int serial_;\n\nIs serial_ a good name ? Shouldn't it be somethinkg like nextHandle ?\n\n> +\tstd::vector<std::unique_ptr<ControlId>> controlIds_;\n> +\tstd::map<unsigned int, ControlInfoMap> infoMaps_;\n> +\tstd::map<const ControlInfoMap *, unsigned int> infoMapHandles_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_CONTROL_SERIALIZER_H__ */\n> diff --git a/src/libcamera/include/meson.build b/src/libcamera/include/meson.build\n> index 1ff0198662cc..697294f4b09b 100644\n> --- a/src/libcamera/include/meson.build\n> +++ b/src/libcamera/include/meson.build\n> @@ -2,6 +2,7 @@ libcamera_headers = files([\n>      'byte_stream_buffer.h',\n>      'camera_controls.h',\n>      'camera_sensor.h',\n> +    'control_serializer.h',\n>      'control_validator.h',\n>      'device_enumerator.h',\n>      'device_enumerator_sysfs.h',\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index dab2d8ad2649..59cf582580c4 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -7,6 +7,7 @@ libcamera_sources = files([\n>      'camera_manager.cpp',\n>      'camera_sensor.cpp',\n>      'controls.cpp',\n> +    'control_serializer.cpp',\n>      'control_validator.cpp',\n>      'device_enumerator.cpp',\n>      'device_enumerator_sysfs.cpp',\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<jacopo@jmondi.org>","Received":["from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0231A60C12\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 19 Nov 2019 00:47:57 +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 relay12.mail.gandi.net (Postfix) with ESMTPSA id 5DC33200002;\n\tMon, 18 Nov 2019 23:47:57 +0000 (UTC)"],"Date":"Tue, 19 Nov 2019 00:50:00 +0100","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191118235000.hkjgsl26s4zurbc6@uno.localdomain>","References":"<20191108205409.18845-1-laurent.pinchart@ideasonboard.com>\n\t<20191108205409.18845-17-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"ah3iawccz6v4akwt\"","Content-Disposition":"inline","In-Reply-To":"<20191108205409.18845-17-laurent.pinchart@ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v2 16/24] libcamera: Add controls\n\tserializer","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Mon, 18 Nov 2019 23:47:58 -0000"}},{"id":3098,"web_url":"https://patchwork.libcamera.org/comment/3098/","msgid":"<20191119024151.GA4950@pendragon.ideasonboard.com>","date":"2019-11-19T02:41:51","subject":"Re: [libcamera-devel] [PATCH v2 16/24] libcamera: Add controls\n\tserializer","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Tue, Nov 19, 2019 at 12:50:00AM +0100, Jacopo Mondi wrote:\n> On Fri, Nov 08, 2019 at 10:54:01PM +0200, Laurent Pinchart wrote:\n> > Add a new ControlSerializer helper to serialize and deserialize\n> > ControlInfoMap and ControlList instances. This will be used to implement\n> > the C IPA protocol and the communication with IPA through IPC.\n> >\n> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> > ---\n> >  src/libcamera/control_serializer.cpp       | 501 +++++++++++++++++++++\n> >  src/libcamera/include/control_serializer.h |  52 +++\n> >  src/libcamera/include/meson.build          |   1 +\n> >  src/libcamera/meson.build                  |   1 +\n> >  4 files changed, 555 insertions(+)\n> >  create mode 100644 src/libcamera/control_serializer.cpp\n> >  create mode 100644 src/libcamera/include/control_serializer.h\n> >\n> > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> > new file mode 100644\n> > index 000000000000..5fe096128e49\n> > --- /dev/null\n> > +++ b/src/libcamera/control_serializer.cpp\n> > @@ -0,0 +1,501 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * control_serializer.cpp - Control (de)serializer\n> > + */\n> > +\n> > +#include \"control_serializer.h\"\n> > +\n> > +#include <algorithm>\n> > +#include <memory>\n> > +#include <vector>\n> > +\n> > +#include <ipa/ipa_controls.h>\n> > +#include <libcamera/control_ids.h>\n> > +#include <libcamera/controls.h>\n> > +\n> > +#include \"byte_stream_buffer.h\"\n> > +#include \"log.h\"\n> > +\n> > +/**\n> > + * \\file control_serializer.h\n> > + * \\brief Serialization and deserialization helpers for controls\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DEFINE_CATEGORY(Serializer)\n> > +\n> > +namespace {\n> > +\n> > +static constexpr size_t ControlValueSize[] = {\n> > +\t[ControlTypeNone]\t= 1,\n> > +\t[ControlTypeBool]\t= sizeof(bool),\n> > +\t[ControlTypeInteger32]\t= sizeof(int32_t),\n> > +\t[ControlTypeInteger64]\t= sizeof(int64_t),\n> > +};\n> > +\n> > +} /* namespace */\n> > +\n> > +/**\n> > + * \\class ControlSerializer\n> > + * \\brief Serializer and deserializer for control-related classes\n> > + *\n> > + * The control serializer is a helper to serialize and deserialize\n> > + * ControlInfoMap and ControlValue instances for the purpose of communication\n> > + * with IPA modules.\n> > + *\n> > + * Neither the ControlInfoMap nor the ControlList are self-contained data\n> > + * container. ControlInfoMap references an external ControlId in each of its\n> > + * entries, and ControlList references a ControlInfoMap for the purpose of\n> > + * validation. Serializing and deserializing those objects thus requires a\n> > + * context that maintains the associations between them. The control serializer\n> > + * fulfils this task.\n> > + *\n> > + * ControlInfoMap instances can be serialized on their own, but require\n> > + * ControlId instances to be provided at deserialization time. The serializer\n> > + * recreates those ControlId instances and stores them in an internal cache,\n> > + * from which the ControlInfoMap is populated.\n> > + *\n> > + * ControlList instances need to be associated with a ControlInfoMap when\n> > + * deserialized. To make this possible, the control lists are serialized with a\n> > + * handle to their ControlInfoMap, and the map is looked up from the handle at\n> \n> s/from the handle/from the list of handles/ ? or just 'from the list'\n\nI meant that the lookup process takes a handle and returns a map. Maybe\n\"looked up based on the handle\" ?\n\n> > + * deserialization time. To make this possible, the serializer assigns a\n> > + * numerical handle to ControlInfoList instances when they are serialized, and\n> \n> s/ControlInfoList/ControlInfoMap/\n> \n> > + * stores the mapping between handle and ControlInfoList both when serializing\n> \n> ditto\n\nFixed.\n\n> > + * (for the pipeline handler side) and deserializing (for the IPA side) them.\n> > + * This mapping is used when serializing a ControlList to include the\n> > + * corresponding ControlInfoMap handle in the binary data, and when\n> > + * deserializing to retrieve the corresponding ControlInfoMap.\n> \n> That's very nice to know, but this feels more like a comment to the\n> implementation than part of the documentation..\n\nDo you mean the last sentence, or the whole paragraph ? Let's not forget\nthat IPA modules are not required to use ControlSerializer, so\ndocumenting the serialization and deserialization process is still\nuseful (although the IPA interface documentation should be\nself-contained in this regard). Do you think we shouldn't document how\nthe ControlSerializer operates ?\n\n> > + *\n> > + * In order to perform those tasks, the serializer keeps an internal state that\n> > + * needs to be properly populated. This mechanism requires the ControlInfoMap\n> > + * corresponding to a ControlList to have been serialized or deserialized\n> > + * before the ControlList is serialized or deserialized. Failure to comply with\n> > + * that constraint results in serialization or deserialization failure of the\n> > + * ControlList.\n> > + *\n> > + * The serializer can be reset() to clear its internal state. This may be\n> > + * performed when reconfiguring an IPA to avoid constant growth of the internal\n> > + * state, especially if the contents of the ControlInfoMap instances change at\n> > + * that time. A reset of the serializer invalidates all ControlList and\n> > + * ControlInfoMap that have been previously deserialized. The caller shall thus\n> > + * proceed with care to avoid stale references.\n> > + */\n> > +\n> > +/**\n> > + * \\brief Reset the serializer\n> > + *\n> > + * Reset the internal state of the serializer. This invalidates all the\n> > + * ControlList and ControlInfoMap that have been previously deserialized.\n> > + */\n> > +void ControlSerializer::reset()\n> > +{\n> > +\tserial_ = 0;\n> > +\n> > +\tinfoMapHandles_.clear();\n> > +\tinfoMaps_.clear();\n> > +\tcontrolIds_.clear();\n> > +}\n> > +\n> > +size_t ControlSerializer::binarySize(const ControlValue &value)\n> > +{\n> > +\treturn ControlValueSize[value.type()];\n> > +}\n> > +\n> > +size_t ControlSerializer::binarySize(const ControlRange &range)\n> > +{\n> > +\treturn binarySize(range.min()) + binarySize(range.max());\n> > +}\n> > +\n> > +/**\n> > + * \\brief Retrieve the size in bytes required to serialize a ControlInfoMap\n> > + * \\param[in] info The control info map\n> > + *\n> > + * Compute and return the size in bytes required to store the serialized\n> > + * ControlInfoMap.\n> > + *\n> > + * \\return The size in bytes required to store the serialized ControlInfoMap\n> > + */\n> > +size_t ControlSerializer::binarySize(const ControlInfoMap &info)\n> > +{\n> > +\tsize_t size = sizeof(struct ipa_controls_header)\n> > +\t\t    + info.size() * sizeof(struct ipa_control_range_entry);\n> > +\n> > +\tfor (const auto &ctrl : info)\n> > +\t\tsize += binarySize(ctrl.second);\n> > +\n> > +\treturn size;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Retrieve the size in bytes required to serialize a ControlList\n> > + * \\param[in] list The control list\n> > + *\n> > + * Compute and return the size in bytes required to store the serialized\n> > + * ControlList.\n> > + *\n> > + * \\return The size in bytes required to store the serialized ControlList\n> > + */\n> > +size_t ControlSerializer::binarySize(const ControlList &list)\n> > +{\n> > +\tsize_t size = sizeof(struct ipa_controls_header)\n> > +\t\t    + list.size() * sizeof(struct ipa_control_value_entry);\n> > +\n> > +\tfor (const auto &ctrl : list)\n> > +\t\tsize += binarySize(ctrl.second);\n> > +\n> > +\treturn size;\n> > +}\n> > +\n> > +void ControlSerializer::store(const ControlValue &value,\n> > +\t\t\t      ByteStreamBuffer &buffer)\n> > +{\n> > +\tswitch (value.type()) {\n> > +\tcase ControlTypeBool: {\n> > +\t\tbool data = value.get<bool>();\n> > +\t\tbuffer.write(&data);\n> \n> Actually it's quite nice how ByteStreamBuffer abstract away memory\n> handling. Now I'm debated...\n\nIt was convenient up to the point when I tried to serialize control\nvalues in the control list entries :-) I think we should keep the\nByteStreamBuffer as-is for now, and revisit it when moving control\nvalues to the entries (if we do so).\n\n> > +\t\tbreak;\n> > +\t}\n> > +\n> > +\tcase ControlTypeInteger32: {\n> > +\t\tint32_t data = value.get<int32_t>();\n> > +\t\tbuffer.write(&data);\n> > +\t\tbreak;\n> > +\t}\n> > +\n> > +\tcase ControlTypeInteger64: {\n> > +\t\tuint64_t data = value.get<int64_t>();\n> > +\t\tbuffer.write(&data);\n> > +\t\tbreak;\n> > +\t}\n> > +\n> > +\tdefault:\n> > +\t\tbreak;\n> > +\t}\n> > +}\n> > +\n> > +void ControlSerializer::store(const ControlRange &range,\n> > +\t\t\t      ByteStreamBuffer &buffer)\n> > +{\n> > +\tstore(range.min(), buffer);\n> > +\tstore(range.max(), buffer);\n> > +}\n> > +\n> > +/**\n> > + * \\brief Serialize a ControlInfoMap in a buffer\n> > + * \\param[in] info The control info map to serialize\n> > + * \\param[in] buffer The memory buffer where to serialize the ControlInfoMap\n> > + *\n> > + * Serialize the \\a info map into the \\a buffer using the serialization format\n> > + * defined by the IPA context interface in ipa_controls.h.\n> > + *\n> > + * The serializer stores a reference to the \\a info internally. The caller\n> > + * shall ensure that \\a info stays valid until the serializer is reset().\n> > + *\n> > + * \\return 0 on success, a negative error code otherwise\n> > + * \\retval -ENOSPC Not enough space is available in the buffer\n> > + */\n> > +int ControlSerializer::serialize(const ControlInfoMap &info,\n> > +\t\t\t\t ByteStreamBuffer &buffer)\n> > +{\n> > +\t/* Compute entries and data required sizes. */\n> > +\tsize_t entriesSize = info.size() * sizeof(struct ipa_control_range_entry);\n> > +\tsize_t valuesSize = 0;\n> > +\tfor (const auto &ctrl : info)\n> > +\t\tvaluesSize += binarySize(ctrl.second);\n> > +\n> > +\t/* Prepare the packet header, assign a handle to the ControlInfoMap. */\n> > +\tstruct ipa_controls_header hdr;\n> > +\thdr.version = IPA_CONTROLS_FORMAT_VERSION;\n> > +\thdr.handle = ++serial_;\n> > +\thdr.entries = info.size();\n> > +\thdr.size = sizeof(hdr) + entriesSize + valuesSize;\n> > +\thdr.data_offset = sizeof(hdr) + entriesSize;\n> > +\n> > +\tbuffer.write(&hdr);\n> > +\n> > +\t/*\n> > +\t * Serialize all entries.\n> > +\t * \\todo Serialize the control name too\n> > +\t */\n> > +\tByteStreamBuffer entries = buffer.carveOut(entriesSize);\n> > +\tByteStreamBuffer values = buffer.carveOut(valuesSize);\n> > +\n> > +\tfor (const auto &ctrl : info) {\n> > +\t\tconst ControlId *id = ctrl.first;\n> > +\t\tconst ControlRange &range = ctrl.second;\n> > +\n> > +\t\tstruct ipa_control_range_entry entry;\n> > +\t\tentry.id = id->id();\n> > +\t\tentry.type = id->type();\n> > +\t\tentry.offset = values.offset();\n> > +\t\tentries.write(&entry);\n> > +\n> > +\t\tstore(range, values);\n> > +\t}\n> > +\n> > +\tif (buffer.overflow())\n> > +\t\treturn -ENOSPC;\n> > +\n> > +\t/*\n> > +\t * Store the map to handle association, to be used to serialize and\n> > +\t * deserialize control lists.\n> > +\t */\n> > +\tinfoMapHandles_[&info] = hdr.handle;\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Serialize a ControlList in a buffer\n> > + * \\param[in] list The control list to serialize\n> > + * \\param[in] buffer The memory buffer where to serialize the ControlList\n> > + *\n> > + * Serialize the \\a list into the \\a buffer using the serialization format\n> > + * defined by the IPA context interface in ipa_controls.h.\n> > + *\n> > + * \\return 0 on success, a negative error code otherwise\n> > + * \\retval -ENOSPC Not enough space is available in the buffer\n> > + */\n> > +int ControlSerializer::serialize(const ControlList &list,\n> > +\t\t\t\t ByteStreamBuffer &buffer)\n> > +{\n> > +\t/*\n> > +\t * Find the ControlInfoMap handle for the ControlList if it has one, or\n> > +\t * use 0 for ControlList without a ControlInfoMap.\n> > +\t */\n> > +\tunsigned int infoMapHandle;\n> > +\tif (list.infoMap()) {\n> > +\t\tauto iter = infoMapHandles_.find(list.infoMap());\n> > +\t\tif (iter == infoMapHandles_.end()) {\n> > +\t\t\tLOG(Serializer, Error)\n> > +\t\t\t\t<< \"Can't serialize ControlList: unknown ControlInfoMap\";\n> > +\t\t\treturn -ENOENT;\n> \n> Document -ENOENT too ?\n\nGood point. How about\n\n * \\retval -ENOENT The ControlList is related to an unknown ControlInfoMap\n\n> > +\t\t}\n> > +\n> > +\t\tinfoMapHandle = iter->second;\n> > +\t} else {\n> > +\t\tinfoMapHandle = 0;\n> > +\t}\n> > +\n> > +\tsize_t entriesSize = list.size() * sizeof(struct ipa_control_value_entry);\n> > +\tsize_t valuesSize = 0;\n> > +\tfor (const auto &ctrl : list)\n> > +\t\tvaluesSize += binarySize(ctrl.second);\n> > +\n> > +\t/* Prepare the packet header. */\n> > +\tstruct ipa_controls_header hdr;\n> > +\thdr.version = IPA_CONTROLS_FORMAT_VERSION;\n> > +\thdr.handle = infoMapHandle;\n> > +\thdr.entries = list.size();\n> > +\thdr.size = sizeof(hdr) + entriesSize + valuesSize;\n> > +\thdr.data_offset = sizeof(hdr) + entriesSize;\n> > +\n> > +\tbuffer.write(&hdr);\n> > +\n> > +\tByteStreamBuffer entries = buffer.carveOut(entriesSize);\n> > +\tByteStreamBuffer values = buffer.carveOut(valuesSize);\n> > +\n> > +\t/* Serialize all entries. */\n> > +\tfor (const auto &ctrl : list) {\n> > +\t\tunsigned int id = ctrl.first;\n> > +\t\tconst ControlValue &value = ctrl.second;\n> > +\n> > +\t\tstruct ipa_control_value_entry entry;\n> > +\t\tentry.id = id;\n> > +\t\tentry.count = 1;\n> > +\t\tentry.type = value.type();\n> > +\t\tentry.offset = values.offset();\n> > +\t\tentries.write(&entry);\n> > +\n> > +\t\tstore(value, values);\n> \n> I would love to see something like\n>                 values.write(value)\n> \n> What would you think to move the store() (and load?) function to the\n> ByteStreamBuffer. It's only used here, and making it aware of\n> ControlValue and ControlRanges would not be a blasphemy... It would\n> remove the need to \"store()\" and \"load()\" data here, which basically\n> resolves to inspecting the ControlValue/Range type... If we split them\n> then the ControlSerializer would only deal with containers and\n> ByteStreamBuffer with actual values. Does it make sense ?\n\nI don't like it that much, as I think that splitting memory handling in\nByteStreamBuffer and serialization in ControlSerializer is a nice\nlogical split. I especially like the fact that all the serialization\ncode is contained in ControlSerializer.\n\n> > +\t}\n> > +\n> > +\tif (buffer.overflow())\n> > +\t\treturn -ENOSPC;\n> \n> Can't we check the ByteStreamBuffer::write return value in the loop\n> and break earlier ?\n\nWe could, but the whole point of overflow handling in ByteStreamBuffer\nis to allow a single check at the end :-) Let's not forget that this\nisn't supposed to happen ever, so a single check is also more efficient.\n\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +template<>\n> > +ControlValue ControlSerializer::load<ControlValue>(ControlType type,\n> > +\t\t\t\t\t\t   ByteStreamBuffer &b)\n> > +{\n> > +\tswitch (type) {\n> > +\tcase ControlTypeBool: {\n> > +\t\tbool value;\n> > +\t\tb.read(&value);\n> > +\t\treturn ControlValue(value);\n> > +\t}\n> > +\n> > +\tcase ControlTypeInteger32: {\n> > +\t\tint32_t value;\n> > +\t\tb.read(&value);\n> > +\t\treturn ControlValue(value);\n> > +\t}\n> > +\n> > +\tcase ControlTypeInteger64: {\n> > +\t\tint64_t value;\n> > +\t\tb.read(&value);\n> > +\t\treturn ControlValue(value);\n> > +\t}\n> > +\n> > +\tdefault:\n> > +\t\treturn ControlValue();\n> > +\t}\n> > +}\n> > +\n> > +template<>\n> > +ControlRange ControlSerializer::load<ControlRange>(ControlType type,\n> > +\t\t\t\t\t\t   ByteStreamBuffer &b)\n> > +{\n> > +\tControlValue min = load<ControlValue>(type, b);\n> > +\tControlValue max = load<ControlValue>(type, b);\n> > +\n> > +\treturn ControlRange(min, max);\n> > +}\n> > +\n> > +/**\n> > + * \\fn template<typename T> T ControlSerializer::deserialize(ByteStreamBuffer &buffer)\n> > + * \\brief Deserialize an object from a binary buffer\n> > + * \\param[in] buffer The memory buffer that contains the object\n> > + *\n> > + * This method is only valid when specialized for ControlInfoMap or\n> > + * ControlList. Any other typename \\a T is not supported.\n> > + */\n> > +\n> > +/**\n> > + * \\brief Deserialize a ControlInfoMap from a binary buffer\n> > + * \\param[in] buffer The memory buffer that contains the serialized map\n> > + *\n> > + * Re-construct a ControlInfoMap from a binary \\a buffer containing data\n> > + * serialized using the serialize() method.\n> > + *\n> > + * \\return The deserialized ControlInfoMap\n> > + */\n> > +template<>\n> > +ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &buffer)\n> > +{\n> > +\tstruct ipa_controls_header hdr;\n> > +\tbuffer.read(&hdr);\n> > +\n> > +\tif (hdr.version != IPA_CONTROLS_FORMAT_VERSION) {\n> > +\t\tLOG(Serializer, Error)\n> > +\t\t\t<< \"Unsupported controls format version \"\n> > +\t\t\t<< hdr.version;\n> > +\t\treturn {};\n> > +\t}\n> > +\n> > +\tByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr));\n> > +\tByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset);\n> > +\n> > +\tif (buffer.overflow())\n> > +\t\treturn {};\n> \n> Maybe an error message would help ?\n\nIt's not supposed to happen ever, but it's a good idea to debug IPC\nissues. I'll add\n\n                LOG(Serializer, Error) << \"Serialized packet too small\";\n\n> > +\n> > +\tControlInfoMap::Map ctrls;\n> > +\n> > +\tfor (unsigned int i = 0; i < hdr.entries; ++i) {\n> > +\t\tstruct ipa_control_range_entry entry;\n> > +\t\tentries.read(&entry);\n> > +\n> > +\t\t/* Create and cache the individual ControlId. */\n> > +\t\tControlType type = static_cast<ControlType>(entry.type);\n> > +\t\tcontrolIds_.emplace_back(utils::make_unique<ControlId>(entry.id, \"\", type));\n> \n> I would note with a todo, we've lost the name\n\nGood point. I'll add\n\n                /**\n                 * \\todo Find a way to preserve the control name for debugging\n                 * purpose.\n                 */\n\n> > +\n> > +\t\tif (entry.offset != values.offset()) {\n> > +\t\t\tLOG(Serializer, Error)\n> > +\t\t\t\t<< \"Bad data, entry offset mismatch (entry \"\n> > +\t\t\t\t<< i << \")\";\n> > +\t\t\treturn {};\n> > +\t\t}\n> > +\n> > +\t\t/* Create and store the ControlRange. */\n> > +\t\tctrls.emplace(controlIds_.back().get(),\n> > +\t\t\t      load<ControlRange>(type, values));\n> > +\t}\n> > +\n> > +\t/*\n> > +\t * Create the ControlInfoMap in the cache, and store the map to handle\n> > +\t * association.\n> > +\t */\n> > +\tControlInfoMap &map = infoMaps_[hdr.handle] = std::move(ctrls);\n> > +\tinfoMapHandles_[&map] = hdr.handle;\n> > +\n> > +\treturn map;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Deserialize a ControlList from a binary buffer\n> > + * \\param[in] buffer The memory buffer that contains the serialized list\n> > + *\n> > + * Re-construct a ControlList from a binary \\a buffer containing data\n> > + * serialized using the serialize() method.\n> > + *\n> > + * \\return The deserialized ControlList\n> > + */\n> > +template<>\n> > +ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer)\n> > +{\n> > +\tstruct ipa_controls_header hdr;\n> > +\tbuffer.read(&hdr);\n> > +\n> > +\tif (hdr.version != IPA_CONTROLS_FORMAT_VERSION) {\n> > +\t\tLOG(Serializer, Error)\n> > +\t\t\t<< \"Unsupported controls format version \"\n> > +\t\t\t<< hdr.version;\n> > +\t\treturn {};\n> > +\t}\n> > +\n> > +\tByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr));\n> > +\tByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset);\n> > +\n> > +\tif (buffer.overflow())\n\nI'll add an error message here too.\n\n> > +\t\treturn {};\n> > +\n> > +\t/*\n> > +\t * Retrieve the ControlInfoMap associated with the ControlList based on\n> > +\t * its ID. The mapping between infoMap and ID is set up when serializing\n> > +\t * or deserializing ControlInfoMap. If no mapping is found (which is\n> > +\t * currently the case for ControlList related to libcamera controls),\n> > +\t * use the global control::control idmap.\n> > +\t */\n> > +\tconst ControlInfoMap *infoMap;\n> > +\tif (hdr.handle) {\n> > +\t\tauto iter = std::find_if(infoMapHandles_.begin(), infoMapHandles_.end(),\n> > +\t\t\t\t\t [&](decltype(infoMapHandles_)::value_type &entry) {\n> > +\t\t\t\t\t\t return entry.second == hdr.handle;\n> > +\t\t\t\t\t });\n> > +\t\tif (iter == infoMapHandles_.end()) {\n> > +\t\t\tLOG(Serializer, Error)\n> > +\t\t\t\t<< \"Can't deserialize ControlList: unknown ControlInfoMap\";\n> > +\t\t\treturn {};\n> > +\t\t}\n> > +\n> > +\t\tinfoMap = iter->first;\n> > +\t} else {\n> > +\t\tinfoMap = nullptr;\n> > +\t}\n> > +\n> > +\tControlList ctrls(infoMap ? infoMap->idmap() : controls::controls);\n> > +\n> > +\tfor (unsigned int i = 0; i < hdr.entries; ++i) {\n> > +\t\tstruct ipa_control_value_entry entry;\n> > +\t\tentries.read(&entry);\n> > +\n> > +\t\tif (entry.offset != values.offset()) {\n> > +\t\t\tLOG(Serializer, Error)\n> > +\t\t\t\t<< \"Bad data, entry offset mismatch (entry \"\n> > +\t\t\t\t<< i << \")\";\n> > +\t\t\treturn {};\n> > +\t\t}\n> > +\n> > +\t\tControlType type = static_cast<ControlType>(entry.type);\n> > +\t\tctrls.set(entry.id, load<ControlValue>(type, values));\n> > +\t}\n> > +\n> > +\treturn ctrls;\n> > +}\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/include/control_serializer.h b/src/libcamera/include/control_serializer.h\n> > new file mode 100644\n> > index 000000000000..bb3cb8e7b904\n> > --- /dev/null\n> > +++ b/src/libcamera/include/control_serializer.h\n> > @@ -0,0 +1,52 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * control_serializer.h - Control (de)serializer\n> > + */\n> > +#ifndef __LIBCAMERA_CONTROL_SERIALIZER_H__\n> > +#define __LIBCAMERA_CONTROL_SERIALIZER_H__\n> > +\n> > +#include <map>\n> > +#include <memory>\n> > +#include <vector>\n> > +\n> > +#include <libcamera/controls.h>\n> > +\n> > +namespace libcamera {\n> > +\n> > +class ByteStreamBuffer;\n> > +\n> > +class ControlSerializer\n> > +{\n> > +public:\n> > +\tvoid reset();\n> > +\n> > +\tstatic size_t binarySize(const ControlInfoMap &info);\n> > +\tstatic size_t binarySize(const ControlList &list);\n> > +\n> > +\tint serialize(const ControlInfoMap &info, ByteStreamBuffer &buffer);\n> > +\tint serialize(const ControlList &list, ByteStreamBuffer &buffer);\n> > +\n> > +\ttemplate<typename T>\n> > +\tT deserialize(ByteStreamBuffer &buffer);\n> > +\n> > +private:\n> > +\tstatic size_t binarySize(const ControlValue &value);\n> > +\tstatic size_t binarySize(const ControlRange &range);\n> > +\n> > +\tstatic void store(const ControlValue &value, ByteStreamBuffer &buffer);\n> > +\tstatic void store(const ControlRange &range, ByteStreamBuffer &buffer);\n> > +\n> > +\ttemplate<typename T>\n> > +\tT load(ControlType type, ByteStreamBuffer &b);\n> > +\n> > +\tunsigned int serial_;\n> \n> Is serial_ a good name ? Shouldn't it be somethinkg like nextHandle ?\n\nI think about it as a serial number. I think I actually got the idea\nfrom the serial file in the openssl certificate authority :-) Do you\nthink it's confusing, should I rename it ?\n\n> > +\tstd::vector<std::unique_ptr<ControlId>> controlIds_;\n> > +\tstd::map<unsigned int, ControlInfoMap> infoMaps_;\n> > +\tstd::map<const ControlInfoMap *, unsigned int> infoMapHandles_;\n> > +};\n> > +\n> > +} /* namespace libcamera */\n> > +\n> > +#endif /* __LIBCAMERA_CONTROL_SERIALIZER_H__ */\n> > diff --git a/src/libcamera/include/meson.build b/src/libcamera/include/meson.build\n> > index 1ff0198662cc..697294f4b09b 100644\n> > --- a/src/libcamera/include/meson.build\n> > +++ b/src/libcamera/include/meson.build\n> > @@ -2,6 +2,7 @@ libcamera_headers = files([\n> >      'byte_stream_buffer.h',\n> >      'camera_controls.h',\n> >      'camera_sensor.h',\n> > +    'control_serializer.h',\n> >      'control_validator.h',\n> >      'device_enumerator.h',\n> >      'device_enumerator_sysfs.h',\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index dab2d8ad2649..59cf582580c4 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -7,6 +7,7 @@ libcamera_sources = files([\n> >      'camera_manager.cpp',\n> >      'camera_sensor.cpp',\n> >      'controls.cpp',\n> > +    'control_serializer.cpp',\n> >      'control_validator.cpp',\n> >      'device_enumerator.cpp',\n> >      'device_enumerator_sysfs.cpp',","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A814D60C22\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 19 Nov 2019 03:44:07 +0100 (CET)","from pendragon.ideasonboard.com (unknown [210.160.217.68])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 182E2811;\n\tTue, 19 Nov 2019 03:43:40 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1574131447;\n\tbh=GFx4gTvZn4JT7LhIx59AVdB0fKGQ0K8AuDCbidMuzds=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=BcC7V6rGJ65TMnpXbfaeXlWsTLWnajYyBbPbYzPmhTZ58ebpuTqBwFHOlNclftEEa\n\tn5RBzncg7tE0SAg3HBHrUZKXPU1Q73eyTGjFDtIpxnxpWAMYN/4/Jlee3z5umBS4OB\n\t/AQGCqgclY2igGD1Uz+/l7jVYNrHNCvXRGejpVTM=","Date":"Tue, 19 Nov 2019 04:41:51 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191119024151.GA4950@pendragon.ideasonboard.com>","References":"<20191108205409.18845-1-laurent.pinchart@ideasonboard.com>\n\t<20191108205409.18845-17-laurent.pinchart@ideasonboard.com>\n\t<20191118235000.hkjgsl26s4zurbc6@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20191118235000.hkjgsl26s4zurbc6@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 16/24] libcamera: Add controls\n\tserializer","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Tue, 19 Nov 2019 02:44:07 -0000"}}]