[libcamera-devel] [PATCH v2 16/24] libcamera: Add controls serializer
Jacopo Mondi
jacopo at jmondi.org
Tue Nov 19 00:50:00 CET 2019
Hi Laurent,
On Fri, Nov 08, 2019 at 10:54:01PM +0200, Laurent Pinchart wrote:
> Add a new ControlSerializer helper to serialize and deserialize
> ControlInfoMap and ControlList instances. This will be used to implement
> the C IPA protocol and the communication with IPA through IPC.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> src/libcamera/control_serializer.cpp | 501 +++++++++++++++++++++
> src/libcamera/include/control_serializer.h | 52 +++
> src/libcamera/include/meson.build | 1 +
> src/libcamera/meson.build | 1 +
> 4 files changed, 555 insertions(+)
> create mode 100644 src/libcamera/control_serializer.cpp
> create mode 100644 src/libcamera/include/control_serializer.h
>
> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp
> new file mode 100644
> index 000000000000..5fe096128e49
> --- /dev/null
> +++ b/src/libcamera/control_serializer.cpp
> @@ -0,0 +1,501 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * control_serializer.cpp - Control (de)serializer
> + */
> +
> +#include "control_serializer.h"
> +
> +#include <algorithm>
> +#include <memory>
> +#include <vector>
> +
> +#include <ipa/ipa_controls.h>
> +#include <libcamera/control_ids.h>
> +#include <libcamera/controls.h>
> +
> +#include "byte_stream_buffer.h"
> +#include "log.h"
> +
> +/**
> + * \file control_serializer.h
> + * \brief Serialization and deserialization helpers for controls
> + */
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(Serializer)
> +
> +namespace {
> +
> +static constexpr size_t ControlValueSize[] = {
> + [ControlTypeNone] = 1,
> + [ControlTypeBool] = sizeof(bool),
> + [ControlTypeInteger32] = sizeof(int32_t),
> + [ControlTypeInteger64] = sizeof(int64_t),
> +};
> +
> +} /* namespace */
> +
> +/**
> + * \class ControlSerializer
> + * \brief Serializer and deserializer for control-related classes
> + *
> + * The control serializer is a helper to serialize and deserialize
> + * ControlInfoMap and ControlValue instances for the purpose of communication
> + * with IPA modules.
> + *
> + * Neither the ControlInfoMap nor the ControlList are self-contained data
> + * container. ControlInfoMap references an external ControlId in each of its
> + * entries, and ControlList references a ControlInfoMap for the purpose of
> + * validation. Serializing and deserializing those objects thus requires a
> + * context that maintains the associations between them. The control serializer
> + * fulfils this task.
> + *
> + * ControlInfoMap instances can be serialized on their own, but require
> + * ControlId instances to be provided at deserialization time. The serializer
> + * recreates those ControlId instances and stores them in an internal cache,
> + * from which the ControlInfoMap is populated.
> + *
> + * ControlList instances need to be associated with a ControlInfoMap when
> + * deserialized. To make this possible, the control lists are serialized with a
> + * handle to their ControlInfoMap, and the map is looked up from the handle at
s/from the handle/from the list of handles/ ? or just 'from the list'
> + * deserialization time. To make this possible, the serializer assigns a
> + * numerical handle to ControlInfoList instances when they are serialized, and
s/ControlInfoList/ControlInfoMap/
> + * stores the mapping between handle and ControlInfoList both when serializing
ditto
> + * (for the pipeline handler side) and deserializing (for the IPA side) them.
> + * This mapping is used when serializing a ControlList to include the
> + * corresponding ControlInfoMap handle in the binary data, and when
> + * deserializing to retrieve the corresponding ControlInfoMap.
That's very nice to know, but this feels more like a comment to the
implementation than part of the documentation..
> + *
> + * In order to perform those tasks, the serializer keeps an internal state that
> + * needs to be properly populated. This mechanism requires the ControlInfoMap
> + * corresponding to a ControlList to have been serialized or deserialized
> + * before the ControlList is serialized or deserialized. Failure to comply with
> + * that constraint results in serialization or deserialization failure of the
> + * ControlList.
> + *
> + * The serializer can be reset() to clear its internal state. This may be
> + * performed when reconfiguring an IPA to avoid constant growth of the internal
> + * state, especially if the contents of the ControlInfoMap instances change at
> + * that time. A reset of the serializer invalidates all ControlList and
> + * ControlInfoMap that have been previously deserialized. The caller shall thus
> + * proceed with care to avoid stale references.
> + */
> +
> +/**
> + * \brief Reset the serializer
> + *
> + * Reset the internal state of the serializer. This invalidates all the
> + * ControlList and ControlInfoMap that have been previously deserialized.
> + */
> +void ControlSerializer::reset()
> +{
> + serial_ = 0;
> +
> + infoMapHandles_.clear();
> + infoMaps_.clear();
> + controlIds_.clear();
> +}
> +
> +size_t ControlSerializer::binarySize(const ControlValue &value)
> +{
> + return ControlValueSize[value.type()];
> +}
> +
> +size_t ControlSerializer::binarySize(const ControlRange &range)
> +{
> + return binarySize(range.min()) + binarySize(range.max());
> +}
> +
> +/**
> + * \brief Retrieve the size in bytes required to serialize a ControlInfoMap
> + * \param[in] info The control info map
> + *
> + * Compute and return the size in bytes required to store the serialized
> + * ControlInfoMap.
> + *
> + * \return The size in bytes required to store the serialized ControlInfoMap
> + */
> +size_t ControlSerializer::binarySize(const ControlInfoMap &info)
> +{
> + size_t size = sizeof(struct ipa_controls_header)
> + + info.size() * sizeof(struct ipa_control_range_entry);
> +
> + for (const auto &ctrl : info)
> + size += binarySize(ctrl.second);
> +
> + return size;
> +}
> +
> +/**
> + * \brief Retrieve the size in bytes required to serialize a ControlList
> + * \param[in] list The control list
> + *
> + * Compute and return the size in bytes required to store the serialized
> + * ControlList.
> + *
> + * \return The size in bytes required to store the serialized ControlList
> + */
> +size_t ControlSerializer::binarySize(const ControlList &list)
> +{
> + size_t size = sizeof(struct ipa_controls_header)
> + + list.size() * sizeof(struct ipa_control_value_entry);
> +
> + for (const auto &ctrl : list)
> + size += binarySize(ctrl.second);
> +
> + return size;
> +}
> +
> +void ControlSerializer::store(const ControlValue &value,
> + ByteStreamBuffer &buffer)
> +{
> + switch (value.type()) {
> + case ControlTypeBool: {
> + bool data = value.get<bool>();
> + buffer.write(&data);
Actually it's quite nice how ByteStreamBuffer abstract away memory
handling. Now I'm debated...
> + break;
> + }
> +
> + case ControlTypeInteger32: {
> + int32_t data = value.get<int32_t>();
> + buffer.write(&data);
> + break;
> + }
> +
> + case ControlTypeInteger64: {
> + uint64_t data = value.get<int64_t>();
> + buffer.write(&data);
> + break;
> + }
> +
> + default:
> + break;
> + }
> +}
> +
> +void ControlSerializer::store(const ControlRange &range,
> + ByteStreamBuffer &buffer)
> +{
> + store(range.min(), buffer);
> + store(range.max(), buffer);
> +}
> +
> +/**
> + * \brief Serialize a ControlInfoMap in a buffer
> + * \param[in] info The control info map to serialize
> + * \param[in] buffer The memory buffer where to serialize the ControlInfoMap
> + *
> + * Serialize the \a info map into the \a buffer using the serialization format
> + * defined by the IPA context interface in ipa_controls.h.
> + *
> + * The serializer stores a reference to the \a info internally. The caller
> + * shall ensure that \a info stays valid until the serializer is reset().
> + *
> + * \return 0 on success, a negative error code otherwise
> + * \retval -ENOSPC Not enough space is available in the buffer
> + */
> +int ControlSerializer::serialize(const ControlInfoMap &info,
> + ByteStreamBuffer &buffer)
> +{
> + /* Compute entries and data required sizes. */
> + size_t entriesSize = info.size() * sizeof(struct ipa_control_range_entry);
> + size_t valuesSize = 0;
> + for (const auto &ctrl : info)
> + valuesSize += binarySize(ctrl.second);
> +
> + /* Prepare the packet header, assign a handle to the ControlInfoMap. */
> + struct ipa_controls_header hdr;
> + hdr.version = IPA_CONTROLS_FORMAT_VERSION;
> + hdr.handle = ++serial_;
> + hdr.entries = info.size();
> + hdr.size = sizeof(hdr) + entriesSize + valuesSize;
> + hdr.data_offset = sizeof(hdr) + entriesSize;
> +
> + buffer.write(&hdr);
> +
> + /*
> + * Serialize all entries.
> + * \todo Serialize the control name too
> + */
> + ByteStreamBuffer entries = buffer.carveOut(entriesSize);
> + ByteStreamBuffer values = buffer.carveOut(valuesSize);
> +
> + for (const auto &ctrl : info) {
> + const ControlId *id = ctrl.first;
> + const ControlRange &range = ctrl.second;
> +
> + struct ipa_control_range_entry entry;
> + entry.id = id->id();
> + entry.type = id->type();
> + entry.offset = values.offset();
> + entries.write(&entry);
> +
> + store(range, values);
> + }
> +
> + if (buffer.overflow())
> + return -ENOSPC;
> +
> + /*
> + * Store the map to handle association, to be used to serialize and
> + * deserialize control lists.
> + */
> + infoMapHandles_[&info] = hdr.handle;
> +
> + return 0;
> +}
> +
> +/**
> + * \brief Serialize a ControlList in a buffer
> + * \param[in] list The control list to serialize
> + * \param[in] buffer The memory buffer where to serialize the ControlList
> + *
> + * Serialize the \a list into the \a buffer using the serialization format
> + * defined by the IPA context interface in ipa_controls.h.
> + *
> + * \return 0 on success, a negative error code otherwise
> + * \retval -ENOSPC Not enough space is available in the buffer
> + */
> +int ControlSerializer::serialize(const ControlList &list,
> + ByteStreamBuffer &buffer)
> +{
> + /*
> + * Find the ControlInfoMap handle for the ControlList if it has one, or
> + * use 0 for ControlList without a ControlInfoMap.
> + */
> + unsigned int infoMapHandle;
> + if (list.infoMap()) {
> + auto iter = infoMapHandles_.find(list.infoMap());
> + if (iter == infoMapHandles_.end()) {
> + LOG(Serializer, Error)
> + << "Can't serialize ControlList: unknown ControlInfoMap";
> + return -ENOENT;
Document -ENOENT too ?
> + }
> +
> + infoMapHandle = iter->second;
> + } else {
> + infoMapHandle = 0;
> + }
> +
> + size_t entriesSize = list.size() * sizeof(struct ipa_control_value_entry);
> + size_t valuesSize = 0;
> + for (const auto &ctrl : list)
> + valuesSize += binarySize(ctrl.second);
> +
> + /* Prepare the packet header. */
> + struct ipa_controls_header hdr;
> + hdr.version = IPA_CONTROLS_FORMAT_VERSION;
> + hdr.handle = infoMapHandle;
> + hdr.entries = list.size();
> + hdr.size = sizeof(hdr) + entriesSize + valuesSize;
> + hdr.data_offset = sizeof(hdr) + entriesSize;
> +
> + buffer.write(&hdr);
> +
> + ByteStreamBuffer entries = buffer.carveOut(entriesSize);
> + ByteStreamBuffer values = buffer.carveOut(valuesSize);
> +
> + /* Serialize all entries. */
> + for (const auto &ctrl : list) {
> + unsigned int id = ctrl.first;
> + const ControlValue &value = ctrl.second;
> +
> + struct ipa_control_value_entry entry;
> + entry.id = id;
> + entry.count = 1;
> + entry.type = value.type();
> + entry.offset = values.offset();
> + entries.write(&entry);
> +
> + store(value, values);
I would love to see something like
values.write(value)
What would you think to move the store() (and load?) function to the
ByteStreamBuffer. It's only used here, and making it aware of
ControlValue and ControlRanges would not be a blasphemy... It would
remove the need to "store()" and "load()" data here, which basically
resolves to inspecting the ControlValue/Range type... If we split them
then the ControlSerializer would only deal with containers and
ByteStreamBuffer with actual values. Does it make sense ?
> + }
> +
> + if (buffer.overflow())
> + return -ENOSPC;
Can't we check the ByteStreamBuffer::write return value in the loop
and break earlier ?
> +
> + return 0;
> +}
> +
> +template<>
> +ControlValue ControlSerializer::load<ControlValue>(ControlType type,
> + ByteStreamBuffer &b)
> +{
> + switch (type) {
> + case ControlTypeBool: {
> + bool value;
> + b.read(&value);
> + return ControlValue(value);
> + }
> +
> + case ControlTypeInteger32: {
> + int32_t value;
> + b.read(&value);
> + return ControlValue(value);
> + }
> +
> + case ControlTypeInteger64: {
> + int64_t value;
> + b.read(&value);
> + return ControlValue(value);
> + }
> +
> + default:
> + return ControlValue();
> + }
> +}
> +
> +template<>
> +ControlRange ControlSerializer::load<ControlRange>(ControlType type,
> + ByteStreamBuffer &b)
> +{
> + ControlValue min = load<ControlValue>(type, b);
> + ControlValue max = load<ControlValue>(type, b);
> +
> + return ControlRange(min, max);
> +}
> +
> +/**
> + * \fn template<typename T> T ControlSerializer::deserialize(ByteStreamBuffer &buffer)
> + * \brief Deserialize an object from a binary buffer
> + * \param[in] buffer The memory buffer that contains the object
> + *
> + * This method is only valid when specialized for ControlInfoMap or
> + * ControlList. Any other typename \a T is not supported.
> + */
> +
> +/**
> + * \brief Deserialize a ControlInfoMap from a binary buffer
> + * \param[in] buffer The memory buffer that contains the serialized map
> + *
> + * Re-construct a ControlInfoMap from a binary \a buffer containing data
> + * serialized using the serialize() method.
> + *
> + * \return The deserialized ControlInfoMap
> + */
> +template<>
> +ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &buffer)
> +{
> + struct ipa_controls_header hdr;
> + buffer.read(&hdr);
> +
> + if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) {
> + LOG(Serializer, Error)
> + << "Unsupported controls format version "
> + << hdr.version;
> + return {};
> + }
> +
> + ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr));
> + ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset);
> +
> + if (buffer.overflow())
> + return {};
Maybe an error message would help ?
> +
> + ControlInfoMap::Map ctrls;
> +
> + for (unsigned int i = 0; i < hdr.entries; ++i) {
> + struct ipa_control_range_entry entry;
> + entries.read(&entry);
> +
> + /* Create and cache the individual ControlId. */
> + ControlType type = static_cast<ControlType>(entry.type);
> + controlIds_.emplace_back(utils::make_unique<ControlId>(entry.id, "", type));
I would note with a todo, we've lost the name
> +
> + if (entry.offset != values.offset()) {
> + LOG(Serializer, Error)
> + << "Bad data, entry offset mismatch (entry "
> + << i << ")";
> + return {};
> + }
> +
> + /* Create and store the ControlRange. */
> + ctrls.emplace(controlIds_.back().get(),
> + load<ControlRange>(type, values));
> + }
> +
> + /*
> + * Create the ControlInfoMap in the cache, and store the map to handle
> + * association.
> + */
> + ControlInfoMap &map = infoMaps_[hdr.handle] = std::move(ctrls);
> + infoMapHandles_[&map] = hdr.handle;
> +
> + return map;
> +}
> +
> +/**
> + * \brief Deserialize a ControlList from a binary buffer
> + * \param[in] buffer The memory buffer that contains the serialized list
> + *
> + * Re-construct a ControlList from a binary \a buffer containing data
> + * serialized using the serialize() method.
> + *
> + * \return The deserialized ControlList
> + */
> +template<>
> +ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer)
> +{
> + struct ipa_controls_header hdr;
> + buffer.read(&hdr);
> +
> + if (hdr.version != IPA_CONTROLS_FORMAT_VERSION) {
> + LOG(Serializer, Error)
> + << "Unsupported controls format version "
> + << hdr.version;
> + return {};
> + }
> +
> + ByteStreamBuffer entries = buffer.carveOut(hdr.data_offset - sizeof(hdr));
> + ByteStreamBuffer values = buffer.carveOut(hdr.size - hdr.data_offset);
> +
> + if (buffer.overflow())
> + return {};
> +
> + /*
> + * Retrieve the ControlInfoMap associated with the ControlList based on
> + * its ID. The mapping between infoMap and ID is set up when serializing
> + * or deserializing ControlInfoMap. If no mapping is found (which is
> + * currently the case for ControlList related to libcamera controls),
> + * use the global control::control idmap.
> + */
> + const ControlInfoMap *infoMap;
> + if (hdr.handle) {
> + auto iter = std::find_if(infoMapHandles_.begin(), infoMapHandles_.end(),
> + [&](decltype(infoMapHandles_)::value_type &entry) {
> + return entry.second == hdr.handle;
> + });
> + if (iter == infoMapHandles_.end()) {
> + LOG(Serializer, Error)
> + << "Can't deserialize ControlList: unknown ControlInfoMap";
> + return {};
> + }
> +
> + infoMap = iter->first;
> + } else {
> + infoMap = nullptr;
> + }
> +
> + ControlList ctrls(infoMap ? infoMap->idmap() : controls::controls);
> +
> + for (unsigned int i = 0; i < hdr.entries; ++i) {
> + struct ipa_control_value_entry entry;
> + entries.read(&entry);
> +
> + if (entry.offset != values.offset()) {
> + LOG(Serializer, Error)
> + << "Bad data, entry offset mismatch (entry "
> + << i << ")";
> + return {};
> + }
> +
> + ControlType type = static_cast<ControlType>(entry.type);
> + ctrls.set(entry.id, load<ControlValue>(type, values));
> + }
> +
> + return ctrls;
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/include/control_serializer.h b/src/libcamera/include/control_serializer.h
> new file mode 100644
> index 000000000000..bb3cb8e7b904
> --- /dev/null
> +++ b/src/libcamera/include/control_serializer.h
> @@ -0,0 +1,52 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * control_serializer.h - Control (de)serializer
> + */
> +#ifndef __LIBCAMERA_CONTROL_SERIALIZER_H__
> +#define __LIBCAMERA_CONTROL_SERIALIZER_H__
> +
> +#include <map>
> +#include <memory>
> +#include <vector>
> +
> +#include <libcamera/controls.h>
> +
> +namespace libcamera {
> +
> +class ByteStreamBuffer;
> +
> +class ControlSerializer
> +{
> +public:
> + void reset();
> +
> + static size_t binarySize(const ControlInfoMap &info);
> + static size_t binarySize(const ControlList &list);
> +
> + int serialize(const ControlInfoMap &info, ByteStreamBuffer &buffer);
> + int serialize(const ControlList &list, ByteStreamBuffer &buffer);
> +
> + template<typename T>
> + T deserialize(ByteStreamBuffer &buffer);
> +
> +private:
> + static size_t binarySize(const ControlValue &value);
> + static size_t binarySize(const ControlRange &range);
> +
> + static void store(const ControlValue &value, ByteStreamBuffer &buffer);
> + static void store(const ControlRange &range, ByteStreamBuffer &buffer);
> +
> + template<typename T>
> + T load(ControlType type, ByteStreamBuffer &b);
> +
> + unsigned int serial_;
Is serial_ a good name ? Shouldn't it be somethinkg like nextHandle ?
> + std::vector<std::unique_ptr<ControlId>> controlIds_;
> + std::map<unsigned int, ControlInfoMap> infoMaps_;
> + std::map<const ControlInfoMap *, unsigned int> infoMapHandles_;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_CONTROL_SERIALIZER_H__ */
> diff --git a/src/libcamera/include/meson.build b/src/libcamera/include/meson.build
> index 1ff0198662cc..697294f4b09b 100644
> --- a/src/libcamera/include/meson.build
> +++ b/src/libcamera/include/meson.build
> @@ -2,6 +2,7 @@ libcamera_headers = files([
> 'byte_stream_buffer.h',
> 'camera_controls.h',
> 'camera_sensor.h',
> + 'control_serializer.h',
> 'control_validator.h',
> 'device_enumerator.h',
> 'device_enumerator_sysfs.h',
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index dab2d8ad2649..59cf582580c4 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -7,6 +7,7 @@ libcamera_sources = files([
> 'camera_manager.cpp',
> 'camera_sensor.cpp',
> 'controls.cpp',
> + 'control_serializer.cpp',
> 'control_validator.cpp',
> 'device_enumerator.cpp',
> 'device_enumerator_sysfs.cpp',
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20191119/09c79e51/attachment.sig>
More information about the libcamera-devel
mailing list