[RFC PATCH v1 8/8] utils: codegen: ipc: Simplify deserialization
Paul Elder
paul.elder at ideasonboard.com
Fri May 16 19:30:40 CEST 2025
Quoting Barnabás Pőcze (2025-05-15 14:00:12)
> First, introduce the `SeriReader` type, which is a collection
> of bytes and file descriptors, with the appropriate methods to
> consume the first couples bytes / file descriptors.
>
> Then a new method is added to `IPCMessage` that returns an appropriately
> constructed `SeriReader` for parsing the message contents.
>
> Then three of the four `deserialize()` overloads are removed, the
> remaining one is converted to have a single `SeriReader` and an
> optional `ControlSerializer` as arguments.
>
> The remaining `deserialize()` function is also changed to return an
> `std::optional` to be able to report deserialization failure.
>
> There is also a more fundamental change in the serialization: previously,
> the number of bytes taken up by an item has been written before the serialized
> bytes (and conditionally the number of file descriptors) when the item is
> serialized as part of a struct, array, map, function parameter list. This
> is changed: the number of bytes and file descriptors are *not* serialized
> into the final buffer. This affords some simplification of the serialization
> related code paths, but most importantly, it greatly simplifies and unifies
> how an object is (de)serialized because the deserialization of every object
> becomes completely self-contained.
>
> As a consequence of that, strings now include their lengths as part of the
> string serialization, and it is not left to an "upper" layer.
>
> Another consequence is that an "out parameter" of a remote function call
> must be deserialized if a later out parameter is needed, even if itself
> is not. This does not appear to be a great limitation since in all
> situations presently none of the out parameters are ignored.
>
> Finally, the code generation templates are adapted to the above changes.
> This allows the simplification of the deserialization templates as now
> calling `IPADataSerializer<T>::deserialize(reader, &controlSerializer_)`
> is appropriate for any type.
>
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=269
> Signed-off-by: Barnabás Pőcze <barnabas.pocze at ideasonboard.com>
> ---
> .../libcamera/internal/ipa_data_serializer.h | 228 +++--------
> include/libcamera/internal/ipc_pipe.h | 3 +
> include/libcamera/internal/serialization.h | 91 +++++
> src/libcamera/ipa_data_serializer.cpp | 356 ++++--------------
> src/libcamera/ipc_pipe.cpp | 5 +
> test/ipc/unixsocket_ipc.cpp | 18 +-
> .../generated_serializer_test.cpp | 17 +-
> .../ipa_data_serializer_test.cpp | 32 +-
> .../module_ipa_proxy.cpp.tmpl | 30 +-
> .../module_ipa_proxy.h.tmpl | 5 +-
> .../module_ipa_proxy_worker.cpp.tmpl | 5 +-
> .../libcamera_templates/proxy_functions.tmpl | 113 +-----
> .../libcamera_templates/serializer.tmpl | 238 +-----------
> 13 files changed, 316 insertions(+), 825 deletions(-)
> create mode 100644 include/libcamera/internal/serialization.h
>
> diff --git a/include/libcamera/internal/ipa_data_serializer.h b/include/libcamera/internal/ipa_data_serializer.h
> index 564f59e25..0d03729a1 100644
> --- a/include/libcamera/internal/ipa_data_serializer.h
> +++ b/include/libcamera/internal/ipa_data_serializer.h
> @@ -7,6 +7,7 @@
>
> #pragma once
>
> +#include <optional>
> #include <stdint.h>
> #include <string.h>
> #include <tuple>
> @@ -23,6 +24,7 @@
> #include <libcamera/ipa/ipa_interface.h>
>
> #include "libcamera/internal/control_serializer.h"
> +#include "libcamera/internal/serialization.h"
>
> namespace libcamera {
>
> @@ -39,26 +41,6 @@ void appendPOD(std::vector<uint8_t> &vec, T val)
> memcpy(&*(vec.end() - byteWidth), &val, byteWidth);
> }
>
> -template<typename T,
> - std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>
> -T readPOD(std::vector<uint8_t>::const_iterator it, size_t pos,
> - std::vector<uint8_t>::const_iterator end)
> -{
> - ASSERT(pos + it < end);
> -
> - T ret = 0;
> - memcpy(&ret, &(*(it + pos)), sizeof(ret));
> -
> - return ret;
> -}
> -
> -template<typename T,
> - std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>
> -T readPOD(std::vector<uint8_t> &vec, size_t pos)
> -{
> - return readPOD<T>(vec.cbegin(), pos, vec.end());
> -}
> -
> } /* namespace */
>
> template<typename T, typename = void>
> @@ -68,20 +50,8 @@ public:
> static std::tuple<std::vector<uint8_t>, std::vector<SharedFD>>
> serialize(const T &data, ControlSerializer *cs = nullptr);
>
> - static T deserialize(const std::vector<uint8_t> &data,
> - ControlSerializer *cs = nullptr);
> - static T deserialize(std::vector<uint8_t>::const_iterator dataBegin,
> - std::vector<uint8_t>::const_iterator dataEnd,
> - ControlSerializer *cs = nullptr);
> -
> - static T deserialize(const std::vector<uint8_t> &data,
> - const std::vector<SharedFD> &fds,
> - ControlSerializer *cs = nullptr);
> - static T deserialize(std::vector<uint8_t>::const_iterator dataBegin,
> - std::vector<uint8_t>::const_iterator dataEnd,
> - std::vector<SharedFD>::const_iterator fdsBegin,
> - std::vector<SharedFD>::const_iterator fdsEnd,
> - ControlSerializer *cs = nullptr);
> + [[nodiscard]] static std::optional<T>
> + deserialize(SeriReader &reader, ControlSerializer *cs = nullptr);
> };
>
> #ifndef __DOXYGEN__
> @@ -121,9 +91,6 @@ public:
> std::tie(dvec, fvec) =
> IPADataSerializer<V>::serialize(it, cs);
>
> - appendPOD<uint32_t>(dataVec, dvec.size());
> - appendPOD<uint32_t>(dataVec, fvec.size());
> -
> dataVec.insert(dataVec.end(), dvec.begin(), dvec.end());
> fdsVec.insert(fdsVec.end(), fvec.begin(), fvec.end());
> }
> @@ -131,52 +98,25 @@ public:
> return { dataVec, fdsVec };
> }
>
> - static std::vector<V> deserialize(std::vector<uint8_t> &data, ControlSerializer *cs = nullptr)
> + [[nodiscard]] static std::optional<std::vector<V>>
> + deserialize(SeriReader &reader, ControlSerializer *cs = nullptr)
> {
> - return deserialize(data.cbegin(), data.cend(), cs);
> - }
> + uint32_t vecLen;
> + if (!reader.read(vecLen))
> + return {};
>
> - static std::vector<V> deserialize(std::vector<uint8_t>::const_iterator dataBegin,
> - std::vector<uint8_t>::const_iterator dataEnd,
> - ControlSerializer *cs = nullptr)
> - {
> - std::vector<SharedFD> fds;
> - return deserialize(dataBegin, dataEnd, fds.cbegin(), fds.cend(), cs);
> - }
> + std::vector<V> ret;
> + ret.reserve(vecLen);
>
> - static std::vector<V> deserialize(std::vector<uint8_t> &data, std::vector<SharedFD> &fds,
> - ControlSerializer *cs = nullptr)
> - {
> - return deserialize(data.cbegin(), data.cend(), fds.cbegin(), fds.cend(), cs);
> - }
> -
> - static std::vector<V> deserialize(std::vector<uint8_t>::const_iterator dataBegin,
> - std::vector<uint8_t>::const_iterator dataEnd,
> - std::vector<SharedFD>::const_iterator fdsBegin,
> - [[maybe_unused]] std::vector<SharedFD>::const_iterator fdsEnd,
> - ControlSerializer *cs = nullptr)
> - {
> - uint32_t vecLen = readPOD<uint32_t>(dataBegin, 0, dataEnd);
> - std::vector<V> ret(vecLen);
> -
> - std::vector<uint8_t>::const_iterator dataIter = dataBegin + 4;
> - std::vector<SharedFD>::const_iterator fdIter = fdsBegin;
> for (uint32_t i = 0; i < vecLen; i++) {
> - uint32_t sizeofData = readPOD<uint32_t>(dataIter, 0, dataEnd);
> - uint32_t sizeofFds = readPOD<uint32_t>(dataIter, 4, dataEnd);
> - dataIter += 8;
> -
> - ret[i] = IPADataSerializer<V>::deserialize(dataIter,
> - dataIter + sizeofData,
> - fdIter,
> - fdIter + sizeofFds,
> - cs);
> -
> - dataIter += sizeofData;
> - fdIter += sizeofFds;
> + auto item = IPADataSerializer<V>::deserialize(reader, cs);
> + if (!item)
> + return {};
> +
> + ret.emplace_back(std::move(*item));
> }
>
> - return ret;
> + return std::move(ret);
> }
> };
>
> @@ -218,18 +158,12 @@ public:
> std::tie(dvec, fvec) =
> IPADataSerializer<K>::serialize(it.first, cs);
>
> - appendPOD<uint32_t>(dataVec, dvec.size());
> - appendPOD<uint32_t>(dataVec, fvec.size());
> -
> dataVec.insert(dataVec.end(), dvec.begin(), dvec.end());
> fdsVec.insert(fdsVec.end(), fvec.begin(), fvec.end());
>
> std::tie(dvec, fvec) =
> IPADataSerializer<V>::serialize(it.second, cs);
>
> - appendPOD<uint32_t>(dataVec, dvec.size());
> - appendPOD<uint32_t>(dataVec, fvec.size());
> -
> dataVec.insert(dataVec.end(), dvec.begin(), dvec.end());
> fdsVec.insert(fdsVec.end(), fvec.begin(), fvec.end());
> }
> @@ -237,63 +171,25 @@ public:
> return { dataVec, fdsVec };
> }
>
> - static std::map<K, V> deserialize(std::vector<uint8_t> &data, ControlSerializer *cs = nullptr)
> - {
> - return deserialize(data.cbegin(), data.cend(), cs);
> - }
> -
> - static std::map<K, V> deserialize(std::vector<uint8_t>::const_iterator dataBegin,
> - std::vector<uint8_t>::const_iterator dataEnd,
> - ControlSerializer *cs = nullptr)
> + [[nodiscard]] static std::optional<std::map<K, V>>
> + deserialize(SeriReader &reader, ControlSerializer *cs = nullptr)
> {
> - std::vector<SharedFD> fds;
> - return deserialize(dataBegin, dataEnd, fds.cbegin(), fds.cend(), cs);
> - }
> + uint32_t mapLen;
> + if (!reader.read(mapLen))
> + return {};
>
> - static std::map<K, V> deserialize(std::vector<uint8_t> &data, std::vector<SharedFD> &fds,
> - ControlSerializer *cs = nullptr)
> - {
> - return deserialize(data.cbegin(), data.cend(), fds.cbegin(), fds.cend(), cs);
> - }
> -
> - static std::map<K, V> deserialize(std::vector<uint8_t>::const_iterator dataBegin,
> - std::vector<uint8_t>::const_iterator dataEnd,
> - std::vector<SharedFD>::const_iterator fdsBegin,
> - [[maybe_unused]] std::vector<SharedFD>::const_iterator fdsEnd,
> - ControlSerializer *cs = nullptr)
> - {
> std::map<K, V> ret;
>
> - uint32_t mapLen = readPOD<uint32_t>(dataBegin, 0, dataEnd);
> -
> - std::vector<uint8_t>::const_iterator dataIter = dataBegin + 4;
> - std::vector<SharedFD>::const_iterator fdIter = fdsBegin;
> for (uint32_t i = 0; i < mapLen; i++) {
> - uint32_t sizeofData = readPOD<uint32_t>(dataIter, 0, dataEnd);
> - uint32_t sizeofFds = readPOD<uint32_t>(dataIter, 4, dataEnd);
> - dataIter += 8;
> -
> - K key = IPADataSerializer<K>::deserialize(dataIter,
> - dataIter + sizeofData,
> - fdIter,
> - fdIter + sizeofFds,
> - cs);
> -
> - dataIter += sizeofData;
> - fdIter += sizeofFds;
> - sizeofData = readPOD<uint32_t>(dataIter, 0, dataEnd);
> - sizeofFds = readPOD<uint32_t>(dataIter, 4, dataEnd);
> - dataIter += 8;
> -
> - const V value = IPADataSerializer<V>::deserialize(dataIter,
> - dataIter + sizeofData,
> - fdIter,
> - fdIter + sizeofFds,
> - cs);
> - ret.insert({ key, value });
> -
> - dataIter += sizeofData;
> - fdIter += sizeofFds;
> + auto key = IPADataSerializer<K>::deserialize(reader, cs);
> + if (!key)
> + return {};
> +
> + auto value = IPADataSerializer<V>::deserialize(reader, cs);
> + if (!value)
> + return {};
> +
> + ret.try_emplace(std::move(*key), std::move(*value));
I'm curious, what's the reason for using try_emplace() here?
> }
>
> return ret;
> @@ -314,33 +210,14 @@ public:
> return { dataVec, {} };
> }
>
> - static Flags<E> deserialize(std::vector<uint8_t> &data,
> - [[maybe_unused]] ControlSerializer *cs = nullptr)
> - {
> - return deserialize(data.cbegin(), data.cend());
> - }
> -
> - static Flags<E> deserialize(std::vector<uint8_t>::const_iterator dataBegin,
> - std::vector<uint8_t>::const_iterator dataEnd,
> - [[maybe_unused]] ControlSerializer *cs = nullptr)
> + [[nodiscard]] static std::optional<Flags<E>>
> + deserialize(SeriReader &reader, [[maybe_unused]] ControlSerializer *cs = nullptr)
> {
> - return Flags<E>{ static_cast<E>(readPOD<uint32_t>(dataBegin, 0, dataEnd)) };
> - }
> + uint32_t value;
> + if (!reader.read(value))
> + return {};
>
> - static Flags<E> deserialize(std::vector<uint8_t> &data,
> - [[maybe_unused]] std::vector<SharedFD> &fds,
> - [[maybe_unused]] ControlSerializer *cs = nullptr)
> - {
> - return deserialize(data.cbegin(), data.cend());
> - }
> -
> - static Flags<E> deserialize(std::vector<uint8_t>::const_iterator dataBegin,
> - std::vector<uint8_t>::const_iterator dataEnd,
> - [[maybe_unused]] std::vector<SharedFD>::const_iterator fdsBegin,
> - [[maybe_unused]] std::vector<SharedFD>::const_iterator fdsEnd,
> - [[maybe_unused]] ControlSerializer *cs = nullptr)
> - {
> - return deserialize(dataBegin, dataEnd);
> + return Flags<E>{ static_cast<E>(value) };
> }
> };
>
> @@ -360,33 +237,14 @@ public:
> return { dataVec, {} };
> }
>
> - static E deserialize(std::vector<uint8_t> &data,
> - [[maybe_unused]] ControlSerializer *cs = nullptr)
> + [[nodiscard]] static std::optional<E>
> + deserialize(SeriReader &reader, [[maybe_unused]] ControlSerializer *cs = nullptr)
> {
> - return deserialize(data.cbegin(), data.cend());
> - }
> + U value;
> + if (!reader.read(value))
> + return {};
>
> - static E deserialize(std::vector<uint8_t>::const_iterator dataBegin,
> - std::vector<uint8_t>::const_iterator dataEnd,
> - [[maybe_unused]] ControlSerializer *cs = nullptr)
> - {
> - return static_cast<E>(readPOD<U>(dataBegin, 0, dataEnd));
> - }
> -
> - static E deserialize(std::vector<uint8_t> &data,
> - [[maybe_unused]] std::vector<SharedFD> &fds,
> - [[maybe_unused]] ControlSerializer *cs = nullptr)
> - {
> - return deserialize(data.cbegin(), data.cend());
> - }
> -
> - static E deserialize(std::vector<uint8_t>::const_iterator dataBegin,
> - std::vector<uint8_t>::const_iterator dataEnd,
> - [[maybe_unused]] std::vector<SharedFD>::const_iterator fdsBegin,
> - [[maybe_unused]] std::vector<SharedFD>::const_iterator fdsEnd,
> - [[maybe_unused]] ControlSerializer *cs = nullptr)
> - {
> - return deserialize(dataBegin, dataEnd);
> + return static_cast<E>(value);
> }
> };
>
> diff --git a/include/libcamera/internal/ipc_pipe.h b/include/libcamera/internal/ipc_pipe.h
> index 418c4622f..2b6cde042 100644
> --- a/include/libcamera/internal/ipc_pipe.h
> +++ b/include/libcamera/internal/ipc_pipe.h
> @@ -14,6 +14,7 @@
> #include <libcamera/base/signal.h>
>
> #include "libcamera/internal/ipc_unixsocket.h"
> +#include "libcamera/internal/serialization.h"
>
> namespace libcamera {
>
> @@ -40,6 +41,8 @@ public:
> const std::vector<uint8_t> &data() const { return data_; }
> const std::vector<SharedFD> &fds() const { return fds_; }
>
> + SeriReader reader() const { return { data(), fds() }; }
> +
> private:
> Header header_;
>
> diff --git a/include/libcamera/internal/serialization.h b/include/libcamera/internal/serialization.h
> new file mode 100644
> index 000000000..daa7e5438
> --- /dev/null
> +++ b/include/libcamera/internal/serialization.h
> @@ -0,0 +1,91 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2025, Ideas On Board Oy
> + *
> + * Data (de)serialization helper structures
> + */
> +#pragma once
> +
> +#include <optional>
> +#include <string.h>
> +#include <tuple>
> +
> +#include <libcamera/base/shared_fd.h>
> +#include <libcamera/base/span.h>
> +
> +namespace libcamera {
> +
> +#ifndef __DOXYGEN__
> +class SeriReader
> +{
> +public:
> + SeriReader(Span<const std::byte> data, Span<const SharedFD> fds = {})
> + : data_(data),
> + fds_(fds)
> + {
> + }
> +
> + SeriReader(Span<const uint8_t> data, Span<const SharedFD> fds = {})
> + : data_(reinterpret_cast<const std::byte *>(data.data()),
> + reinterpret_cast<const std::byte *>(data.data() + data.size())),
> + fds_(fds)
> + {
> + }
> +
> + [[nodiscard]] const std::byte *consume(std::size_t s)
> + {
> + if (data_.size() < s)
> + return nullptr;
> +
> + const auto *p = data_.data();
> + data_ = data_.subspan(s);
> +
> + return p;
> + }
> +
> + template<typename... Ts>
> + [[nodiscard]] bool read(Ts &...xs)
> + {
> + static_assert((std::is_trivially_copyable_v<Ts> && ...));
> +
> + const auto *p = consume((sizeof(xs) + ...));
> + if (p)
> + ((memcpy(&xs, p, sizeof(xs)), p += sizeof(xs)), ...);
> +
> + return p;
> + }
That is so cool.
> +
> + template<typename... Ts>
> + [[nodiscard]] auto read()
> + {
> + std::tuple<Ts...> xs;
> + bool ok = std::apply([&](auto &...vs) {
> + return read(vs...);
> + }, xs);
> +
> + if constexpr (sizeof...(Ts) == 1)
> + return ok ? std::optional(std::get<0>(xs)) : std::nullopt;
> + else
> + return ok ? std::optional(xs) : std::nullopt;
This means that each std::nullopt is of different types no? afaict you only use
this function when sizeof...(Ts) == 1.
> + }
> +
> + [[nodiscard]] const SharedFD *nextFd()
> + {
> + if (fds_.empty())
> + return nullptr;
> +
> + const auto *p = fds_.data();
> + fds_ = fds_.subspan(1);
> +
> + return p;
> + }
> +
> + [[nodiscard]] bool empty() const { return data_.empty() && fds_.empty(); }
> +
> +private:
> + Span<const std::byte> data_;
> + Span<const SharedFD> fds_;
> +};
> +#endif
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/ipa_data_serializer.cpp b/src/libcamera/ipa_data_serializer.cpp
> index 0537f785b..979a1e0db 100644
> --- a/src/libcamera/ipa_data_serializer.cpp
> +++ b/src/libcamera/ipa_data_serializer.cpp
> @@ -50,42 +50,6 @@ namespace {
> * generated IPA proxies.
> */
>
> -/**
> - * \fn template<typename T> T readPOD(std::vector<uint8_t>::iterator it, size_t pos,
> - * std::vector<uint8_t>::iterator end)
> - * \brief Read POD from byte vector, in little-endian order
> - * \tparam T Type of POD to read
> - * \param[in] it Iterator of byte vector to read from
> - * \param[in] pos Index in byte vector to read from
> - * \param[in] end Iterator marking end of byte vector
> - *
> - * This function is meant to be used by the IPA data serializer, and the
> - * generated IPA proxies.
> - *
> - * If the \a pos plus the byte-width of the desired POD is past \a end, it is
> - * a fata error will occur, as it means there is insufficient data for
> - * deserialization, which should never happen.
> - *
> - * \return The POD read from \a it at index \a pos
> - */
> -
> -/**
> - * \fn template<typename T> T readPOD(std::vector<uint8_t> &vec, size_t pos)
> - * \brief Read POD from byte vector, in little-endian order
> - * \tparam T Type of POD to read
> - * \param[in] vec Byte vector to read from
> - * \param[in] pos Index in vec to start reading from
> - *
> - * This function is meant to be used by the IPA data serializer, and the
> - * generated IPA proxies.
> - *
> - * If the \a pos plus the byte-width of the desired POD is past the end of
> - * \a vec, a fatal error will occur, as it means there is insufficient data
> - * for deserialization, which should never happen.
> - *
> - * \return The POD read from \a vec at index \a pos
> - */
> -
> } /* namespace */
>
> /**
> @@ -106,80 +70,13 @@ namespace {
>
> /**
> * \fn template<typename T> IPADataSerializer<T>::deserialize(
> - * const std::vector<uint8_t> &data,
> + * SeriReader &reader,
> * ControlSerializer *cs = nullptr)
> - * \brief Deserialize byte vector into an object
> + * \brief Deserialize bytes and file descriptors vector into an object
> * \tparam T Type of object to deserialize to
> - * \param[in] data Byte vector to deserialize from
> + * \param[in] reader Source of bytes and file descriptors
> * \param[in] cs ControlSerializer
> *
> - * This version of deserialize() can be used if the object type \a T and its
> - * members don't have any SharedFD.
> - *
> - * \a cs is only necessary if the object type \a T or its members contain
> - * ControlList or ControlInfoMap.
> - *
> - * \return The deserialized object
> - */
> -
> -/**
> - * \fn template<typename T> IPADataSerializer<T>::deserialize(
> - * std::vector<uint8_t>::const_iterator dataBegin,
> - * std::vector<uint8_t>::const_iterator dataEnd,
> - * ControlSerializer *cs = nullptr)
> - * \brief Deserialize byte vector into an object
> - * \tparam T Type of object to deserialize to
> - * \param[in] dataBegin Begin iterator of byte vector to deserialize from
> - * \param[in] dataEnd End iterator of byte vector to deserialize from
> - * \param[in] cs ControlSerializer
> - *
> - * This version of deserialize() can be used if the object type \a T and its
> - * members don't have any SharedFD.
> - *
> - * \a cs is only necessary if the object type \a T or its members contain
> - * ControlList or ControlInfoMap.
> - *
> - * \return The deserialized object
> - */
> -
> -/**
> - * \fn template<typename T> IPADataSerializer<T>::deserialize(
> - * const std::vector<uint8_t> &data,
> - * const std::vector<SharedFD> &fds,
> - * ControlSerializer *cs = nullptr)
> - * \brief Deserialize byte vector and fd vector into an object
> - * \tparam T Type of object to deserialize to
> - * \param[in] data Byte vector to deserialize from
> - * \param[in] fds Fd vector to deserialize from
> - * \param[in] cs ControlSerializer
> - *
> - * This version of deserialize() (or the iterator version) must be used if
> - * the object type \a T or its members contain SharedFD.
> - *
> - * \a cs is only necessary if the object type \a T or its members contain
> - * ControlList or ControlInfoMap.
> - *
> - * \return The deserialized object
> - */
> -
> -/**
> - * \fn template<typename T> IPADataSerializer::deserialize(
> - * std::vector<uint8_t>::const_iterator dataBegin,
> - * std::vector<uint8_t>::const_iterator dataEnd,
> - * std::vector<SharedFD>::const_iterator fdsBegin,
> - * std::vector<SharedFD>::const_iterator fdsEnd,
> - * ControlSerializer *cs = nullptr)
> - * \brief Deserialize byte vector and fd vector into an object
> - * \tparam T Type of object to deserialize to
> - * \param[in] dataBegin Begin iterator of byte vector to deserialize from
> - * \param[in] dataEnd End iterator of byte vector to deserialize from
> - * \param[in] fdsBegin Begin iterator of fd vector to deserialize from
> - * \param[in] fdsEnd End iterator of fd vector to deserialize from
> - * \param[in] cs ControlSerializer
> - *
> - * This version of deserialize() (or the vector version) must be used if
> - * the object type \a T or its members contain SharedFD.
> - *
> * \a cs is only necessary if the object type \a T or its members contain
> * ControlList or ControlInfoMap.
> *
> @@ -202,37 +99,11 @@ IPADataSerializer<type>::serialize(const type &data, \
> } \
> \
> template<> \
> -type IPADataSerializer<type>::deserialize(std::vector<uint8_t>::const_iterator dataBegin, \
> - std::vector<uint8_t>::const_iterator dataEnd, \
> - [[maybe_unused]] ControlSerializer *cs) \
> +std::optional<type> IPADataSerializer<type>::deserialize(SeriReader &reader, \
> + [[maybe_unused]] ControlSerializer *cs) \
> { \
> - return readPOD<type>(dataBegin, 0, dataEnd); \
> + return reader.read<type>(); \
> } \
> - \
> -template<> \
> -type IPADataSerializer<type>::deserialize(const std::vector<uint8_t> &data, \
> - ControlSerializer *cs) \
> -{ \
> - return deserialize(data.cbegin(), data.end(), cs); \
> -} \
> - \
> -template<> \
> -type IPADataSerializer<type>::deserialize(const std::vector<uint8_t> &data, \
> - [[maybe_unused]] const std::vector<SharedFD> &fds, \
> - ControlSerializer *cs) \
> -{ \
> - return deserialize(data.cbegin(), data.end(), cs); \
> -} \
> - \
> -template<> \
> -type IPADataSerializer<type>::deserialize(std::vector<uint8_t>::const_iterator dataBegin, \
> - std::vector<uint8_t>::const_iterator dataEnd, \
> - [[maybe_unused]] std::vector<SharedFD>::const_iterator fdsBegin, \
> - [[maybe_unused]] std::vector<SharedFD>::const_iterator fdsEnd, \
> - ControlSerializer *cs) \
> -{ \
> - return deserialize(dataBegin, dataEnd, cs); \
> -}
>
> DEFINE_POD_SERIALIZER(bool)
> DEFINE_POD_SERIALIZER(uint8_t)
> @@ -256,44 +127,29 @@ std::tuple<std::vector<uint8_t>, std::vector<SharedFD>>
> IPADataSerializer<std::string>::serialize(const std::string &data,
> [[maybe_unused]] ControlSerializer *cs)
> {
> - return { { data.cbegin(), data.end() }, {} };
> -}
> + std::vector<uint8_t> dataVec;
>
> -template<>
> -std::string
> -IPADataSerializer<std::string>::deserialize(const std::vector<uint8_t> &data,
> - [[maybe_unused]] ControlSerializer *cs)
> -{
> - return { data.cbegin(), data.cend() };
> -}
> + ASSERT(data.size() <= std::numeric_limits<uint32_t>::max());
> + appendPOD<uint32_t>(dataVec, data.size());
> + dataVec.insert(dataVec.end(), data.c_str(), data.c_str() + data.size());
>
> -template<>
> -std::string
> -IPADataSerializer<std::string>::deserialize(std::vector<uint8_t>::const_iterator dataBegin,
> - std::vector<uint8_t>::const_iterator dataEnd,
> - [[maybe_unused]] ControlSerializer *cs)
> -{
> - return { dataBegin, dataEnd };
> + return { dataVec, {} };
> }
>
> template<>
> -std::string
> -IPADataSerializer<std::string>::deserialize(const std::vector<uint8_t> &data,
> - [[maybe_unused]] const std::vector<SharedFD> &fds,
> +std::optional<std::string>
> +IPADataSerializer<std::string>::deserialize(SeriReader &reader,
> [[maybe_unused]] ControlSerializer *cs)
> {
> - return { data.cbegin(), data.cend() };
> -}
> + uint32_t length;
> + if (!reader.read(length))
> + return {};
>
> -template<>
> -std::string
> -IPADataSerializer<std::string>::deserialize(std::vector<uint8_t>::const_iterator dataBegin,
> - std::vector<uint8_t>::const_iterator dataEnd,
> - [[maybe_unused]] std::vector<SharedFD>::const_iterator fdsBegin,
> - [[maybe_unused]] std::vector<SharedFD>::const_iterator fdsEnd,
> - [[maybe_unused]] ControlSerializer *cs)
> -{
> - return { dataBegin, dataEnd };
> + const auto *p = reader.consume(length);
> + if (!p)
> + return {};
> +
> + return { { reinterpret_cast<const char *>(p), std::size_t(length) } };
> }
>
> /*
> @@ -356,73 +212,43 @@ IPADataSerializer<ControlList>::serialize(const ControlList &data, ControlSerial
> }
>
> template<>
> -ControlList
> -IPADataSerializer<ControlList>::deserialize(std::vector<uint8_t>::const_iterator dataBegin,
> - std::vector<uint8_t>::const_iterator dataEnd,
> +std::optional<ControlList>
> +IPADataSerializer<ControlList>::deserialize(SeriReader &reader,
> ControlSerializer *cs)
> {
> if (!cs)
> LOG(IPADataSerializer, Fatal)
> << "ControlSerializer not provided for deserialization of ControlList";
>
> - if (std::distance(dataBegin, dataEnd) < 8)
> - return {};
> -
> - uint32_t infoDataSize = readPOD<uint32_t>(dataBegin, 0, dataEnd);
> - uint32_t listDataSize = readPOD<uint32_t>(dataBegin, 4, dataEnd);
> -
> - std::vector<uint8_t>::const_iterator it = dataBegin + 8;
> -
> - if (infoDataSize + listDataSize < infoDataSize ||
> - static_cast<uint32_t>(std::distance(it, dataEnd)) < infoDataSize + listDataSize)
> + uint32_t infoDataSize, listDataSize;
> + if (!reader.read(infoDataSize, listDataSize))
> return {};
>
> if (infoDataSize > 0) {
> - ByteStreamBuffer buffer(&*it, infoDataSize);
> + const auto *p = reader.consume(infoDataSize);
> + if (!p)
> + return {};
> +
> + ByteStreamBuffer buffer(reinterpret_cast<const uint8_t *>(p), infoDataSize);
That is cool.
> ControlInfoMap map = cs->deserialize<ControlInfoMap>(buffer);
> /* It's fine if map is empty. */
> if (buffer.overflow()) {
> LOG(IPADataSerializer, Error)
> << "Failed to deserialize ControlLists's ControlInfoMap: buffer overflow";
> - return ControlList();
> + return {};
> }
> }
>
> - it += infoDataSize;
> - ByteStreamBuffer buffer(&*it, listDataSize);
> + const auto *p = reader.consume(listDataSize);
> +
> + ByteStreamBuffer buffer(reinterpret_cast<const uint8_t *>(p), listDataSize);
> ControlList list = cs->deserialize<ControlList>(buffer);
> - if (buffer.overflow())
> + if (buffer.overflow()) {
> LOG(IPADataSerializer, Error) << "Failed to deserialize ControlList: buffer overflow";
> + return {};
> + }
>
> - return list;
> -}
> -
> -template<>
> -ControlList
> -IPADataSerializer<ControlList>::deserialize(const std::vector<uint8_t> &data,
> - ControlSerializer *cs)
> -{
> - return deserialize(data.cbegin(), data.end(), cs);
> -}
> -
> -template<>
> -ControlList
> -IPADataSerializer<ControlList>::deserialize(const std::vector<uint8_t> &data,
> - [[maybe_unused]] const std::vector<SharedFD> &fds,
> - ControlSerializer *cs)
> -{
> - return deserialize(data.cbegin(), data.end(), cs);
> -}
> -
> -template<>
> -ControlList
> -IPADataSerializer<ControlList>::deserialize(std::vector<uint8_t>::const_iterator dataBegin,
> - std::vector<uint8_t>::const_iterator dataEnd,
> - [[maybe_unused]] std::vector<SharedFD>::const_iterator fdsBegin,
> - [[maybe_unused]] std::vector<SharedFD>::const_iterator fdsEnd,
> - ControlSerializer *cs)
> -{
> - return deserialize(dataBegin, dataEnd, cs);
> + return std::move(list);
> }
>
> /*
> @@ -458,57 +284,28 @@ IPADataSerializer<ControlInfoMap>::serialize(const ControlInfoMap &map,
> }
>
> template<>
> -ControlInfoMap
> -IPADataSerializer<ControlInfoMap>::deserialize(std::vector<uint8_t>::const_iterator dataBegin,
> - std::vector<uint8_t>::const_iterator dataEnd,
> +std::optional<ControlInfoMap>
> +IPADataSerializer<ControlInfoMap>::deserialize(SeriReader &reader,
> ControlSerializer *cs)
> {
> if (!cs)
> LOG(IPADataSerializer, Fatal)
> << "ControlSerializer not provided for deserialization of ControlInfoMap";
>
> - if (std::distance(dataBegin, dataEnd) < 4)
> + uint32_t infoDataSize;
> + if (!reader.read(infoDataSize))
> return {};
>
> - uint32_t infoDataSize = readPOD<uint32_t>(dataBegin, 0, dataEnd);
> -
> - std::vector<uint8_t>::const_iterator it = dataBegin + 4;
> -
> - if (static_cast<uint32_t>(std::distance(it, dataEnd)) < infoDataSize)
> + const auto *p = reader.consume(infoDataSize);
> + if (!p)
> return {};
>
> - ByteStreamBuffer buffer(&*it, infoDataSize);
> + ByteStreamBuffer buffer(reinterpret_cast<const uint8_t *>(p), infoDataSize);
> ControlInfoMap map = cs->deserialize<ControlInfoMap>(buffer);
> + if (buffer.overflow())
> + return {};
>
> - return map;
> -}
> -
> -template<>
> -ControlInfoMap
> -IPADataSerializer<ControlInfoMap>::deserialize(const std::vector<uint8_t> &data,
> - ControlSerializer *cs)
> -{
> - return deserialize(data.cbegin(), data.end(), cs);
> -}
> -
> -template<>
> -ControlInfoMap
> -IPADataSerializer<ControlInfoMap>::deserialize(const std::vector<uint8_t> &data,
> - [[maybe_unused]] const std::vector<SharedFD> &fds,
> - ControlSerializer *cs)
> -{
> - return deserialize(data.cbegin(), data.end(), cs);
> -}
> -
> -template<>
> -ControlInfoMap
> -IPADataSerializer<ControlInfoMap>::deserialize(std::vector<uint8_t>::const_iterator dataBegin,
> - std::vector<uint8_t>::const_iterator dataEnd,
> - [[maybe_unused]] std::vector<SharedFD>::const_iterator fdsBegin,
> - [[maybe_unused]] std::vector<SharedFD>::const_iterator fdsEnd,
> - ControlSerializer *cs)
> -{
> - return deserialize(dataBegin, dataEnd, cs);
> + return std::move(map);
> }
>
> /*
> @@ -542,27 +339,23 @@ IPADataSerializer<SharedFD>::serialize(const SharedFD &data,
> }
>
> template<>
> -SharedFD IPADataSerializer<SharedFD>::deserialize([[maybe_unused]] std::vector<uint8_t>::const_iterator dataBegin,
> - [[maybe_unused]] std::vector<uint8_t>::const_iterator dataEnd,
> - std::vector<SharedFD>::const_iterator fdsBegin,
> - std::vector<SharedFD>::const_iterator fdsEnd,
> - [[maybe_unused]] ControlSerializer *cs)
> +std::optional<SharedFD>
> +IPADataSerializer<SharedFD>::deserialize(SeriReader &reader,
> + [[maybe_unused]] ControlSerializer *cs)
> {
> - ASSERT(std::distance(dataBegin, dataEnd) >= 4);
> + uint32_t valid;
>
> - uint32_t valid = readPOD<uint32_t>(dataBegin, 0, dataEnd);
> + if (!reader.read(valid))
> + return {};
>
> - ASSERT(!(valid && std::distance(fdsBegin, fdsEnd) < 1));
> + if (!valid)
> + return SharedFD{};
>
> - return valid ? *fdsBegin : SharedFD();
> -}
> + const auto *fd = reader.nextFd();
> + if (!fd)
> + return {};
>
> -template<>
> -SharedFD IPADataSerializer<SharedFD>::deserialize(const std::vector<uint8_t> &data,
> - const std::vector<SharedFD> &fds,
> - [[maybe_unused]] ControlSerializer *cs)
> -{
> - return deserialize(data.cbegin(), data.end(), fds.cbegin(), fds.end());
> + return *fd;
> }
>
> /*
> @@ -594,30 +387,19 @@ IPADataSerializer<FrameBuffer::Plane>::serialize(const FrameBuffer::Plane &data,
> }
>
> template<>
> -FrameBuffer::Plane
> -IPADataSerializer<FrameBuffer::Plane>::deserialize(std::vector<uint8_t>::const_iterator dataBegin,
> - std::vector<uint8_t>::const_iterator dataEnd,
> - std::vector<SharedFD>::const_iterator fdsBegin,
> - [[maybe_unused]] std::vector<SharedFD>::const_iterator fdsEnd,
> +std::optional<FrameBuffer::Plane>
> +IPADataSerializer<FrameBuffer::Plane>::deserialize(SeriReader &reader,
> [[maybe_unused]] ControlSerializer *cs)
> {
> - FrameBuffer::Plane ret;
> -
> - ret.fd = IPADataSerializer<SharedFD>::deserialize(dataBegin, dataBegin + 4,
> - fdsBegin, fdsBegin + 1);
> - ret.offset = readPOD<uint32_t>(dataBegin, 4, dataEnd);
> - ret.length = readPOD<uint32_t>(dataBegin, 8, dataEnd);
> + auto fd = IPADataSerializer<SharedFD>::deserialize(reader);
> + if (!fd)
> + return {};
>
> - return ret;
> -}
> + uint32_t offset, length;
> + if (!reader.read(offset, length))
> + return {};
>
> -template<>
> -FrameBuffer::Plane
> -IPADataSerializer<FrameBuffer::Plane>::deserialize(const std::vector<uint8_t> &data,
> - const std::vector<SharedFD> &fds,
> - ControlSerializer *cs)
> -{
> - return deserialize(data.cbegin(), data.end(), fds.cbegin(), fds.end(), cs);
> + return { { std::move(*fd), offset, length } };
> }
>
> #endif /* __DOXYGEN__ */
> diff --git a/src/libcamera/ipc_pipe.cpp b/src/libcamera/ipc_pipe.cpp
> index 548299d05..71d0c2cb4 100644
> --- a/src/libcamera/ipc_pipe.cpp
> +++ b/src/libcamera/ipc_pipe.cpp
> @@ -148,6 +148,11 @@ IPCUnixSocket::Payload IPCMessage::payload() const
> * \brief Returns a const reference to the vector containing file descriptors
> */
>
> +/**
> + * \fn IPCMessage::reader() const
> + * \brief Returns a `SeriReader` instance for parsing the message
> + */
> +
> /**
> * \class IPCPipe
> * \brief IPC message pipe for IPA isolation
> diff --git a/test/ipc/unixsocket_ipc.cpp b/test/ipc/unixsocket_ipc.cpp
> index df7d9c2b4..c0e174ba9 100644
> --- a/test/ipc/unixsocket_ipc.cpp
> +++ b/test/ipc/unixsocket_ipc.cpp
> @@ -102,7 +102,14 @@ private:
> }
>
> case CmdSetAsync: {
> - value_ = IPADataSerializer<int32_t>::deserialize(ipcMessage.data());
> + SeriReader reader = ipcMessage.reader();
> + auto value = IPADataSerializer<int32_t>::deserialize(reader);
> + if (!value) {
> + cerr << "Failed to deserialize value" << endl;
> + stop(-ENODATA);
> + } else {
> + value_ = *value;
> + }
> break;
> }
> }
> @@ -155,7 +162,14 @@ protected:
> return ret;
> }
>
> - return IPADataSerializer<int32_t>::deserialize(buf.data());
> + SeriReader reader = buf.reader();
> + auto value = IPADataSerializer<int32_t>::deserialize(reader);
> + if (!value) {
> + cerr << "Failed to deserialize value" << endl;
> + return -ENODATA;
> + }
> +
> + return *value;
> }
>
> int exit()
> diff --git a/test/serialization/generated_serializer/generated_serializer_test.cpp b/test/serialization/generated_serializer/generated_serializer_test.cpp
> index dd6968850..0640e741d 100644
> --- a/test/serialization/generated_serializer/generated_serializer_test.cpp
> +++ b/test/serialization/generated_serializer/generated_serializer_test.cpp
> @@ -43,7 +43,7 @@ if (struct1.field != struct2.field) { \
> }
>
>
> - ipa::test::TestStruct t, u;
> + ipa::test::TestStruct t;
>
> t.m = {
> { "a", "z" },
> @@ -71,8 +71,13 @@ if (struct1.field != struct2.field) { \
>
> std::tie(serialized, ignore) =
> IPADataSerializer<ipa::test::TestStruct>::serialize(t);
> + SeriReader reader(serialized);
>
> - u = IPADataSerializer<ipa::test::TestStruct>::deserialize(serialized);
> + auto optu = IPADataSerializer<ipa::test::TestStruct>::deserialize(reader);
> + if (!optu)
> + return TestFail;
> +
> + auto &u = *optu;
>
> if (!equals(t.m, u.m))
> return TestFail;
> @@ -91,12 +96,16 @@ if (struct1.field != struct2.field) { \
>
> /* Test vector of generated structs */
> std::vector<ipa::test::TestStruct> v = { t, u };
> - std::vector<ipa::test::TestStruct> w;
>
> std::tie(serialized, ignore) =
> IPADataSerializer<vector<ipa::test::TestStruct>>::serialize(v);
> + reader = SeriReader(serialized);
> +
> + auto optw = IPADataSerializer<vector<ipa::test::TestStruct>>::deserialize(reader);
> + if (!optw)
> + return TestFail;
>
> - w = IPADataSerializer<vector<ipa::test::TestStruct>>::deserialize(serialized);
> + auto &w = *optw;
>
> if (!equals(v[0].m, w[0].m) ||
> !equals(v[1].m, w[1].m))
> diff --git a/test/serialization/ipa_data_serializer_test.cpp b/test/serialization/ipa_data_serializer_test.cpp
> index afea93a6c..3873808a6 100644
> --- a/test/serialization/ipa_data_serializer_test.cpp
> +++ b/test/serialization/ipa_data_serializer_test.cpp
> @@ -52,7 +52,9 @@ int testPodSerdes(T in)
> std::vector<SharedFD> fds;
>
> std::tie(buf, fds) = IPADataSerializer<T>::serialize(in);
> - T out = IPADataSerializer<T>::deserialize(buf, fds);
> + SeriReader reader(buf, fds);
> +
> + auto out = IPADataSerializer<T>::deserialize(reader);
> if (in == out)
> return TestPass;
>
> @@ -71,7 +73,9 @@ int testVectorSerdes(const std::vector<T> &in,
> std::vector<SharedFD> fds;
>
> std::tie(buf, fds) = IPADataSerializer<std::vector<T>>::serialize(in, cs);
> - std::vector<T> out = IPADataSerializer<std::vector<T>>::deserialize(buf, fds, cs);
> + SeriReader reader(buf, fds);
> +
> + auto out = IPADataSerializer<std::vector<T>>::deserialize(reader, cs);
> if (in == out)
> return TestPass;
>
> @@ -91,7 +95,9 @@ int testMapSerdes(const std::map<K, V> &in,
> std::vector<SharedFD> fds;
>
> std::tie(buf, fds) = IPADataSerializer<std::map<K, V>>::serialize(in, cs);
> - std::map<K, V> out = IPADataSerializer<std::map<K, V>>::deserialize(buf, fds, cs);
> + SeriReader reader(buf, fds);
> +
> + auto out = IPADataSerializer<std::map<K, V>>::deserialize(reader, cs);
> if (in == out)
> return TestPass;
>
> @@ -171,17 +177,27 @@ private:
> std::tie(listBuf, std::ignore) =
> IPADataSerializer<ControlList>::serialize(list, &cs);
>
> - const ControlInfoMap infoMapOut =
> - IPADataSerializer<ControlInfoMap>::deserialize(infoMapBuf, &cs);
> + SeriReader listReader(listBuf);
> + SeriReader infoMapReader(infoMapBuf);
> +
> + auto infoMapOut = IPADataSerializer<ControlInfoMap>::deserialize(infoMapReader, &cs);
> + if (!infoMapOut) {
> + cerr << "`ControlInfoMap` cannot be deserialized" << endl;
> + return TestFail;
> + }
>
> - ControlList listOut = IPADataSerializer<ControlList>::deserialize(listBuf, &cs);
> + auto listOut = IPADataSerializer<ControlList>::deserialize(listReader, &cs);
> + if (!listOut) {
> + cerr << "`ControlList` cannot be deserialized" << endl;
> + return TestFail;
> + }
>
> - if (!SerializationTest::equals(infoMap, infoMapOut)) {
> + if (!SerializationTest::equals(infoMap, *infoMapOut)) {
> cerr << "Deserialized map doesn't match original" << endl;
> return TestFail;
> }
>
> - if (!SerializationTest::equals(list, listOut)) {
> + if (!SerializationTest::equals(list, *listOut)) {
> cerr << "Deserialized list doesn't match original" << endl;
> return TestFail;
> }
> diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> index 9a3aadbd2..843260b4b 100644
> --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> @@ -107,13 +107,13 @@ namespace {{ns}} {
> {% if interface_event.methods|length > 0 %}
> void {{proxy_name}}::recvMessage(const IPCMessage &data)
> {
> - size_t dataSize = data.data().size();
> + SeriReader reader = data.reader();
> {{cmd_event_enum_name}} _cmd = static_cast<{{cmd_event_enum_name}}>(data.header().cmd);
>
> switch (_cmd) {
> {%- for method in interface_event.methods %}
> case {{cmd_event_enum_name}}::{{method.mojom_name|cap}}: {
> - {{method.mojom_name}}IPC(data.data().cbegin(), dataSize, data.fds());
> + {{method.mojom_name}}IPC(reader);
> break;
> }
> {%- endfor %}
> @@ -211,18 +211,23 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)
> return;
> {%- endif %}
> }
> +{% if has_output %}
> + SeriReader _outputReader = _ipcOutputBuf.reader();
> +{% endif -%}
> {% if method|method_return_value != "void" %}
> - {{method|method_return_value}} _retValue = IPADataSerializer<{{method|method_return_value}}>::deserialize(_ipcOutputBuf.data(), 0);
> -
> -{{proxy_funcs.deserialize_call(method|method_param_outputs, '_ipcOutputBuf.data()', '_ipcOutputBuf.fds()', init_offset = method|method_return_value|byte_width|int)}}
> + auto _retValue = IPADataSerializer<{{method|method_return_value}}>::deserialize(_outputReader);
> + ASSERT(_retValue);
> +{% endif -%}
>
> - return _retValue;
> +{% if has_output %}
I think this should be {% if method|method_param_outputs|length > 0 %} because
has_output also covers `method|method_return_value != "void"`, so you'd be
deserializing the single _retValue twice.
Or maybe an ASSERT(_outputReader.empty()) might be valuable after
ASSERT(_retValue) above.
> + {{proxy_funcs.deserialize_call(method|method_param_outputs, '_outputReader')}}
> + ASSERT(_outputReader.empty());
> +{% endif -%}
>
> -{% elif method|method_param_outputs|length > 0 %}
> -{{proxy_funcs.deserialize_call(method|method_param_outputs, '_ipcOutputBuf.data()', '_ipcOutputBuf.fds()')}}
> +{% if method|method_return_value != "void" %}
> + return std::move(*_retValue);
> {% endif -%}
> }
> -
> {% endfor %}
>
> {% for method in interface_event.methods %}
> @@ -232,12 +237,9 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)
> {{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});
> }
>
> -void {{proxy_name}}::{{method.mojom_name}}IPC(
> - [[maybe_unused]] std::vector<uint8_t>::const_iterator data,
> - [[maybe_unused]] size_t dataSize,
> - [[maybe_unused]] const std::vector<SharedFD> &fds)
> +void {{proxy_name}}::{{method.mojom_name}}IPC([[maybe_unused]] SeriReader &reader)
> {
> -{{proxy_funcs.deserialize_call(method.parameters, 'data', 'fds', false, true, true, 'dataSize')}}
> + {{proxy_funcs.deserialize_call(method.parameters, 'reader', false, true)}}
I think this is one level too much indentation? Same for the other places you
add this call.
> {{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});
> }
> {% endfor %}
> diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
> index a0312a7c1..945a4ded9 100644
> --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
> +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
> @@ -53,10 +53,7 @@ private:
> {% endfor %}
> {% for method in interface_event.methods %}
> {{proxy_funcs.func_sig(proxy_name, method, "Thread", false)|indent(8, true)}};
> - void {{method.mojom_name}}IPC(
> - std::vector<uint8_t>::const_iterator data,
> - size_t dataSize,
> - const std::vector<SharedFD> &fds);
> + void {{method.mojom_name}}IPC(SeriReader &reader);
> {% endfor %}
>
> /* Helper class to invoke async functions in another thread. */
> diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
> index 1f990d3f9..de6378be0 100644
> --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
> +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
> @@ -71,6 +71,7 @@ public:
> }
>
> IPCMessage _ipcMessage(_message);
> + SeriReader _reader = _ipcMessage.reader();
>
> {{cmd_enum_name}} _cmd = static_cast<{{cmd_enum_name}}>(_ipcMessage.header().cmd);
>
> @@ -85,7 +86,9 @@ public:
> {%- if method.mojom_name == "configure" %}
> controlSerializer_.reset();
> {%- endif %}
> - {{proxy_funcs.deserialize_call(method|method_param_inputs, '_ipcMessage.data()', '_ipcMessage.fds()', false, true)|indent(16, true)}}
> + {{proxy_funcs.deserialize_call(method|method_param_inputs, '_reader', false, true)|indent(16, true)}}
> + ASSERT(_reader.empty());
> +
> {% for param in method|method_param_outputs %}
> {{param|name}} {{param.mojom_name}};
> {% endfor %}
> diff --git a/utils/codegen/ipc/generators/libcamera_templates/proxy_functions.tmpl b/utils/codegen/ipc/generators/libcamera_templates/proxy_functions.tmpl
> index 01e2567ca..b6835ca35 100644
> --- a/utils/codegen/ipc/generators/libcamera_templates/proxy_functions.tmpl
> +++ b/utils/codegen/ipc/generators/libcamera_templates/proxy_functions.tmpl
> @@ -64,15 +64,6 @@
> );
> {%- endfor %}
>
> -{%- if params|length > 1 %}
> -{%- for param in params %}
> - appendPOD<uint32_t>({{buf}}, {{param.mojom_name}}Buf.size());
> -{%- if param|has_fd %}
> - appendPOD<uint32_t>({{buf}}, {{param.mojom_name}}Fds.size());
> -{%- endif %}
> -{%- endfor %}
> -{%- endif %}
> -
> {%- for param in params %}
> {{buf}}.insert({{buf}}.end(), {{param.mojom_name}}Buf.begin(), {{param.mojom_name}}Buf.end());
> {%- endfor %}
> @@ -84,104 +75,28 @@
> {%- endfor %}
> {%- endmacro -%}
>
> -
> -{#
> - # \brief Deserialize a single object from data buffer and fd vector
> - #
> - # \param pointer If true, deserialize the object into a dereferenced pointer
> - # \param iter If true, treat \a buf as an iterator instead of a vector
> - # \param data_size Variable that holds the size of the vector referenced by \a buf
> - #
> - # Generate code to deserialize a single object, as specified in \a param,
> - # from \a buf data buffer and \a fds fd vector.
> - # This code is meant to be used by macro deserialize_call.
> - #}
> -{%- macro deserialize_param(param, pointer, loop, buf, fds, iter, data_size) -%}
> -{{"*" if pointer}}{{param.mojom_name}} =
> -IPADataSerializer<{{param|name_full}}>::deserialize(
> - {{buf}}{{- ".cbegin()" if not iter}} + {{param.mojom_name}}Start,
> -{%- if loop.last and not iter %}
> - {{buf}}.cend()
> -{%- elif not iter %}
> - {{buf}}.cbegin() + {{param.mojom_name}}Start + {{param.mojom_name}}BufSize
> -{%- elif iter and loop.length == 1 %}
> - {{buf}} + {{data_size}}
> -{%- else %}
> - {{buf}} + {{param.mojom_name}}Start + {{param.mojom_name}}BufSize
> -{%- endif -%}
> -{{- "," if param|has_fd}}
> -{%- if param|has_fd %}
> - {{fds}}.cbegin() + {{param.mojom_name}}FdStart,
> -{%- if loop.last %}
> - {{fds}}.cend()
> -{%- else %}
> - {{fds}}.cbegin() + {{param.mojom_name}}FdStart + {{param.mojom_name}}FdsSize
> -{%- endif -%}
> -{%- endif -%}
> -{{- "," if param|needs_control_serializer}}
> -{%- if param|needs_control_serializer %}
> - &controlSerializer_
> -{%- endif -%}
> -);
> -{%- endmacro -%}
> -
> -
> {#
> - # \brief Deserialize multiple objects from data buffer and fd vector
> + # \brief Deserialize multiple objects
> #
> # \param pointer If true, deserialize objects into pointers, and adds a null check.
> # \param declare If true, declare the objects in addition to deserialization.
> - # \param iter if true, treat \a buf as an iterator instead of a vector
> - # \param data_size Variable that holds the size of the vector referenced by \a buf
> - #
> - # Generate code to deserialize multiple objects, as specified in \a params
> - # (which are the parameters to some function), from \a buf data buffer and
> - # \a fds fd vector.
> - # This code is meant to be used by the proxy, for deserializing after IPC calls.
> #
> - # \todo Avoid intermediate vectors
> + # Generate code to deserialize multiple objects, as specified in \a params,
> + # from \a reader. This code is meant to be used by the proxy, for deserializing
> + # after IPC calls.
> #}
> -{%- macro deserialize_call(params, buf, fds, pointer = true, declare = false, iter = false, data_size = '', init_offset = 0) -%}
> -{% set ns = namespace(size_offset = init_offset) %}
> -{%- if params|length > 1 %}
> +{%- macro deserialize_call(params, reader, pointer = true, declare = false) -%}
> {%- for param in params %}
> - [[maybe_unused]] const size_t {{param.mojom_name}}BufSize = readPOD<uint32_t>({{buf}}, {{ns.size_offset}}
> -{%- if iter -%}
> -, {{buf}} + {{data_size}}
> -{%- endif -%}
> -);
> - {%- set ns.size_offset = ns.size_offset + 4 %}
> -{%- if param|has_fd %}
> - [[maybe_unused]] const size_t {{param.mojom_name}}FdsSize = readPOD<uint32_t>({{buf}}, {{ns.size_offset}}
> -{%- if iter -%}
> -, {{buf}} + {{data_size}}
> -{%- endif -%}
> -);
> - {%- set ns.size_offset = ns.size_offset + 4 %}
> -{%- endif %}
> -{%- endfor %}
> -{%- endif %}
> -{% for param in params %}
> -{%- if loop.first %}
> - const size_t {{param.mojom_name}}Start = {{ns.size_offset}};
> -{%- else %}
> - const size_t {{param.mojom_name}}Start = {{loop.previtem.mojom_name}}Start + {{loop.previtem.mojom_name}}BufSize;
> -{%- endif %}
> -{%- endfor %}
> -{% for param in params|with_fds %}
> -{%- if loop.first %}
> - const size_t {{param.mojom_name}}FdStart = 0;
> + auto param_{{param.mojom_name}} = IPADataSerializer<{{param|name_full}}>::deserialize({{reader}}, &controlSerializer_);
> + ASSERT(param_{{param.mojom_name}});
> +
> +{%- if pointer %}
> + if ({{param.mojom_name}})
> + *{{param.mojom_name}} = std::move(*param_{{param.mojom_name}});
> +{%- elif declare %}
> + {{param|name}} &{{param.mojom_name}} = *param_{{param.mojom_name}};
> {%- else %}
> - const size_t {{param.mojom_name}}FdStart = {{loop.previtem.mojom_name}}FdStart + {{loop.previtem.mojom_name}}FdsSize;
> + {{param.mojom_name}} = std::move(*param_{{param.mojom_name}});
> {%- endif %}
> -{%- endfor %}
> -{% for param in params %}
> - {%- if pointer %}
> - if ({{param.mojom_name}}) {
> -{{deserialize_param(param, pointer, loop, buf, fds, iter, data_size)|indent(16, True)}}
> - }
> - {%- else %}
> - {{param|name + " " if declare}}{{deserialize_param(param, pointer, loop, buf, fds, iter, data_size)|indent(8)}}
> - {%- endif %}
This is beautiful.
What an amazing patch. I'm excited for v2.
Thanks,
Paul
> {% endfor %}
> {%- endmacro -%}
> diff --git a/utils/codegen/ipc/generators/libcamera_templates/serializer.tmpl b/utils/codegen/ipc/generators/libcamera_templates/serializer.tmpl
> index e316dd88a..9e9dd0ca6 100644
> --- a/utils/codegen/ipc/generators/libcamera_templates/serializer.tmpl
> +++ b/utils/codegen/ipc/generators/libcamera_templates/serializer.tmpl
> @@ -2,22 +2,6 @@
> # SPDX-License-Identifier: LGPL-2.1-or-later
> # Copyright (C) 2020, Google Inc.
> -#}
> -{#
> - # \brief Verify that there is enough bytes to deserialize
> - #
> - # Generate code that verifies that \a size is not greater than \a dataSize.
> - # Otherwise log an error with \a name and \a typename.
> - #}
> -{%- macro check_data_size(size, dataSize, name, typename) %}
> - if ({{dataSize}} < {{size}}) {
> - LOG(IPADataSerializer, Error)
> - << "Failed to deserialize " << "{{name}}"
> - << ": not enough {{typename}}, expected "
> - << ({{size}}) << ", got " << ({{dataSize}});
> - return ret;
> - }
> -{%- endmacro %}
> -
>
> {#
> # \brief Serialize a field into return vector
> @@ -42,15 +26,10 @@
> retData.insert(retData.end(), {{field.mojom_name}}.begin(), {{field.mojom_name}}.end());
> retFds.insert(retFds.end(), {{field.mojom_name}}Fds.begin(), {{field.mojom_name}}Fds.end());
> {%- elif field|is_controls %}
> - if (data.{{field.mojom_name}}.size() > 0) {
> - std::vector<uint8_t> {{field.mojom_name}};
> - std::tie({{field.mojom_name}}, std::ignore) =
> - IPADataSerializer<{{field|name}}>::serialize(data.{{field.mojom_name}}, cs);
> - appendPOD<uint32_t>(retData, {{field.mojom_name}}.size());
> - retData.insert(retData.end(), {{field.mojom_name}}.begin(), {{field.mojom_name}}.end());
> - } else {
> - appendPOD<uint32_t>(retData, 0);
> - }
> + std::vector<uint8_t> {{field.mojom_name}};
> + std::tie({{field.mojom_name}}, std::ignore) =
> + IPADataSerializer<{{field|name}}>::serialize(data.{{field.mojom_name}}, cs);
> + retData.insert(retData.end(), {{field.mojom_name}}.begin(), {{field.mojom_name}}.end());
> {%- elif field|is_plain_struct or field|is_array or field|is_map or field|is_str %}
> std::vector<uint8_t> {{field.mojom_name}};
> {%- if field|has_fd %}
> @@ -65,10 +44,6 @@
> IPADataSerializer<{{field|name}}>::serialize(data.{{field.mojom_name}});
> {%- else %}
> IPADataSerializer<{{field|name_full}}>::serialize(data.{{field.mojom_name}}, cs);
> - {%- endif %}
> - appendPOD<uint32_t>(retData, {{field.mojom_name}}.size());
> - {%- if field|has_fd %}
> - appendPOD<uint32_t>(retData, {{field.mojom_name}}Fds.size());
> {%- endif %}
> retData.insert(retData.end(), {{field.mojom_name}}.begin(), {{field.mojom_name}}.end());
> {%- if field|has_fd %}
> @@ -79,89 +54,6 @@
> {%- endif %}
> {%- endmacro %}
>
> -
> -{#
> - # \brief Deserialize a field into return struct
> - #
> - # Generate code to deserialize \a field into object ret.
> - # This code is meant to be used by the IPADataSerializer specialization.
> - #}
> -{%- macro deserializer_field(field, loop) %}
> -{% if field|is_pod or field|is_enum %}
> - {%- set field_size = (field|bit_width|int / 8)|int %}
> - {{- check_data_size(field_size, 'dataSize', field.mojom_name, 'data')}}
> - ret.{{field.mojom_name}} = IPADataSerializer<{{field|name_full}}>::deserialize(m, m + {{field_size}});
> - {%- if not loop.last %}
> - m += {{field_size}};
> - dataSize -= {{field_size}};
> - {%- endif %}
> -{% elif field|is_fd %}
> - {%- set field_size = 4 %}
> - {{- check_data_size(field_size, 'dataSize', field.mojom_name, 'data')}}
> - ret.{{field.mojom_name}} = IPADataSerializer<{{field|name}}>::deserialize(m, m + {{field_size}}, n, n + 1, cs);
> - {%- if not loop.last %}
> - m += {{field_size}};
> - dataSize -= {{field_size}};
> - n += ret.{{field.mojom_name}}.isValid() ? 1 : 0;
> - fdsSize -= ret.{{field.mojom_name}}.isValid() ? 1 : 0;
> - {%- endif %}
> -{% elif field|is_controls %}
> - {%- set field_size = 4 %}
> - {{- check_data_size(field_size, 'dataSize', field.mojom_name + 'Size', 'data')}}
> - const size_t {{field.mojom_name}}Size = readPOD<uint32_t>(m, 0, dataEnd);
> - m += {{field_size}};
> - dataSize -= {{field_size}};
> - {%- set field_size = field.mojom_name + 'Size' -%}
> - {{- check_data_size(field_size, 'dataSize', field.mojom_name, 'data')}}
> - if ({{field.mojom_name}}Size > 0)
> - ret.{{field.mojom_name}} =
> - IPADataSerializer<{{field|name}}>::deserialize(m, m + {{field.mojom_name}}Size, cs);
> - {%- if not loop.last %}
> - m += {{field_size}};
> - dataSize -= {{field_size}};
> - {%- endif %}
> -{% elif field|is_plain_struct or field|is_array or field|is_map or field|is_str %}
> - {%- set field_size = 4 %}
> - {{- check_data_size(field_size, 'dataSize', field.mojom_name + 'Size', 'data')}}
> - const size_t {{field.mojom_name}}Size = readPOD<uint32_t>(m, 0, dataEnd);
> - m += {{field_size}};
> - dataSize -= {{field_size}};
> - {%- if field|has_fd %}
> - {%- set field_size = 4 %}
> - {{- check_data_size(field_size, 'dataSize', field.mojom_name + 'FdsSize', 'data')}}
> - const size_t {{field.mojom_name}}FdsSize = readPOD<uint32_t>(m, 0, dataEnd);
> - m += {{field_size}};
> - dataSize -= {{field_size}};
> - {{- check_data_size(field.mojom_name + 'FdsSize', 'fdsSize', field.mojom_name, 'fds')}}
> - {%- endif %}
> - {%- set field_size = field.mojom_name + 'Size' -%}
> - {{- check_data_size(field_size, 'dataSize', field.mojom_name, 'data')}}
> - ret.{{field.mojom_name}} =
> - {%- if field|is_str %}
> - IPADataSerializer<{{field|name}}>::deserialize(m, m + {{field.mojom_name}}Size);
> - {%- elif field|has_fd and (field|is_array or field|is_map) %}
> - IPADataSerializer<{{field|name}}>::deserialize(m, m + {{field.mojom_name}}Size, n, n + {{field.mojom_name}}FdsSize, cs);
> - {%- elif field|has_fd and (not (field|is_array or field|is_map)) %}
> - IPADataSerializer<{{field|name_full}}>::deserialize(m, m + {{field.mojom_name}}Size, n, n + {{field.mojom_name}}FdsSize, cs);
> - {%- elif (not field|has_fd) and (field|is_array or field|is_map) %}
> - IPADataSerializer<{{field|name}}>::deserialize(m, m + {{field.mojom_name}}Size, cs);
> - {%- else %}
> - IPADataSerializer<{{field|name_full}}>::deserialize(m, m + {{field.mojom_name}}Size, cs);
> - {%- endif %}
> - {%- if not loop.last %}
> - m += {{field_size}};
> - dataSize -= {{field_size}};
> - {%- if field|has_fd %}
> - n += {{field.mojom_name}}FdsSize;
> - fdsSize -= {{field.mojom_name}}FdsSize;
> - {%- endif %}
> - {%- endif %}
> -{% else %}
> - /* Unknown deserialization for {{field.mojom_name}}. */
> -{%- endif %}
> -{%- endmacro %}
> -
> -
> {#
> # \brief Serialize a struct
> #
> @@ -194,126 +86,30 @@
>
>
> {#
> - # \brief Deserialize a struct that has fds
> + # \brief Deserialize a struct
> #
> - # Generate code for IPADataSerializer specialization, for deserializing
> - # \a struct, in the case that \a struct has file descriptors.
> + # Generate code for IPADataSerializer specialization, for deserializing \a struct.
> #}
> -{%- macro deserializer_fd(struct) %}
> - static {{struct|name_full}}
> - deserialize(std::vector<uint8_t> &data,
> - std::vector<SharedFD> &fds,
> -{%- if struct|needs_control_serializer %}
> - ControlSerializer *cs)
> -{%- else %}
> - ControlSerializer *cs = nullptr)
> -{%- endif %}
> - {
> - return IPADataSerializer<{{struct|name_full}}>::deserialize(data.cbegin(), data.cend(), fds.cbegin(), fds.cend(), cs);
> - }
> -
> +{%- macro deserializer(struct) %}
> {# \todo Don't inline this function #}
> - static {{struct|name_full}}
> - deserialize(std::vector<uint8_t>::const_iterator dataBegin,
> - std::vector<uint8_t>::const_iterator dataEnd,
> - std::vector<SharedFD>::const_iterator fdsBegin,
> - std::vector<SharedFD>::const_iterator fdsEnd,
> + [[nodiscard]] static std::optional<{{struct|name_full}}>
> + deserialize(SeriReader &reader,
> {%- if struct|needs_control_serializer %}
> ControlSerializer *cs)
> {%- else %}
> [[maybe_unused]] ControlSerializer *cs = nullptr)
> {%- endif %}
> {
> - {{struct|name_full}} ret;
> - std::vector<uint8_t>::const_iterator m = dataBegin;
> - std::vector<SharedFD>::const_iterator n = fdsBegin;
> -
> - size_t dataSize = std::distance(dataBegin, dataEnd);
> - [[maybe_unused]] size_t fdsSize = std::distance(fdsBegin, fdsEnd);
> -{%- for field in struct.fields -%}
> -{{deserializer_field(field, loop)}}
> +{%- for field in struct.fields %}
> + auto {{field.mojom_name}} = IPADataSerializer<{{field|name_full}}>::deserialize(reader, cs);
> + if (!{{field.mojom_name}})
> + return {};
> {%- endfor %}
> - return ret;
> - }
> -{%- endmacro %}
> -
> -{#
> - # \brief Deserialize a struct that has fds, using non-fd
> - #
> - # Generate code for IPADataSerializer specialization, for deserializing
> - # \a struct, in the case that \a struct has no file descriptors but requires
> - # deserializers with file descriptors.
> - #}
> -{%- macro deserializer_fd_simple(struct) %}
> - static {{struct|name_full}}
> - deserialize(std::vector<uint8_t> &data,
> - [[maybe_unused]] std::vector<SharedFD> &fds,
> - ControlSerializer *cs = nullptr)
> - {
> - return IPADataSerializer<{{struct|name_full}}>::deserialize(data.cbegin(), data.cend(), cs);
> - }
> -
> - static {{struct|name_full}}
> - deserialize(std::vector<uint8_t>::const_iterator dataBegin,
> - std::vector<uint8_t>::const_iterator dataEnd,
> - [[maybe_unused]] std::vector<SharedFD>::const_iterator fdsBegin,
> - [[maybe_unused]] std::vector<SharedFD>::const_iterator fdsEnd,
> - ControlSerializer *cs = nullptr)
> - {
> - return IPADataSerializer<{{struct|name_full}}>::deserialize(dataBegin, dataEnd, cs);
> - }
> -{%- endmacro %}
> -
> -
> -{#
> - # \brief Deserialize a struct that has no fds
> - #
> - # Generate code for IPADataSerializer specialization, for deserializing
> - # \a struct, in the case that \a struct does not have file descriptors.
> - #}
> -{%- macro deserializer_no_fd(struct) %}
> - static {{struct|name_full}}
> - deserialize(std::vector<uint8_t> &data,
> -{%- if struct|needs_control_serializer %}
> - ControlSerializer *cs)
> -{%- else %}
> - ControlSerializer *cs = nullptr)
> -{%- endif %}
> - {
> - return IPADataSerializer<{{struct|name_full}}>::deserialize(data.cbegin(), data.cend(), cs);
> - }
>
> -{# \todo Don't inline this function #}
> - static {{struct|name_full}}
> - deserialize(std::vector<uint8_t>::const_iterator dataBegin,
> - std::vector<uint8_t>::const_iterator dataEnd,
> -{%- if struct|needs_control_serializer %}
> - ControlSerializer *cs)
> -{%- else %}
> - [[maybe_unused]] ControlSerializer *cs = nullptr)
> -{%- endif %}
> - {
> - {{struct|name_full}} ret;
> - std::vector<uint8_t>::const_iterator m = dataBegin;
> -
> - size_t dataSize = std::distance(dataBegin, dataEnd);
> -{%- for field in struct.fields -%}
> -{{deserializer_field(field, loop)}}
> + return { {
> +{%- for field in struct.fields %}
> + std::move(*{{field.mojom_name}}),
> {%- endfor %}
> - return ret;
> + } };
> }
> {%- endmacro %}
> -
> -{#
> - # \brief Deserialize a struct
> - #
> - # Generate code for IPADataSerializer specialization, for deserializing \a struct.
> - #}
> -{%- macro deserializer(struct) %}
> -{%- if struct|has_fd %}
> -{{deserializer_fd(struct)}}
> -{%- else %}
> -{{deserializer_no_fd(struct)}}
> -{{deserializer_fd_simple(struct)}}
> -{%- endif %}
> -{%- endmacro %}
> --
> 2.49.0
>
More information about the libcamera-devel
mailing list