[libcamera-devel,v3,10/38] libcamera: Add IPADataSerializer

9918 diff mbox series

Add an IPADataSerializer which implments de/serialization of built-in
(PODs, vector, map, string) and libcamera data structures. This is
intended to be used by the proxy and the proxy worker in the IPC layer.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

---
Changes in v3:
- reimplement append/readUInt with memcpy (intead of bit shifting)
- change DECLARE_INTEGRAL_SERIALIZER with DECLARE_POD_SERIALIZER
  - use this for int64_t, bool, float, and double
- fix comment style

Changes in v2:
- added serializers for all integer types, bool, and string
- use string serializer for IPASettings serializer
- add documentation
- add serializer for const ControlList
---
 .../libcamera/internal/ipa_data_serializer.h  | 1014 +++++++++++++++++
 src/libcamera/ipa_data_serializer.cpp         |  154 +++
 src/libcamera/meson.build                     |    1 +
 3 files changed, 1169 insertions(+)
 create mode 100644 include/libcamera/internal/ipa_data_serializer.h
 create mode 100644 src/libcamera/ipa_data_serializer.cpp

Hi Paul,

On Fri, Oct 02, 2020 at 11:31:26PM +0900, Paul Elder wrote:
> Add an IPADataSerializer which implments de/serialization of built-in
> (PODs, vector, map, string) and libcamera data structures. This is
> intended to be used by the proxy and the proxy worker in the IPC layer.
>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
>
> ---
> Changes in v3:
> - reimplement append/readUInt with memcpy (intead of bit shifting)
> - change DECLARE_INTEGRAL_SERIALIZER with DECLARE_POD_SERIALIZER
>   - use this for int64_t, bool, float, and double
> - fix comment style
>
> Changes in v2:
> - added serializers for all integer types, bool, and string
> - use string serializer for IPASettings serializer
> - add documentation
> - add serializer for const ControlList
> ---
>  .../libcamera/internal/ipa_data_serializer.h  | 1014 +++++++++++++++++
>  src/libcamera/ipa_data_serializer.cpp         |  154 +++
>  src/libcamera/meson.build                     |    1 +
>  3 files changed, 1169 insertions(+)
>  create mode 100644 include/libcamera/internal/ipa_data_serializer.h
>  create mode 100644 src/libcamera/ipa_data_serializer.cpp
>
> diff --git a/include/libcamera/internal/ipa_data_serializer.h b/include/libcamera/internal/ipa_data_serializer.h
> new file mode 100644
> index 00000000..9cd35c4c
> --- /dev/null
> +++ b/include/libcamera/internal/ipa_data_serializer.h
> @@ -0,0 +1,1014 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * ipa_data_serializer.h - Image Processing Algorithm data serializer
> + */
> +#ifndef __LIBCAMERA_INTERNAL_IPA_DATA_SERIALIZER_H__
> +#define __LIBCAMERA_INTERNAL_IPA_DATA_SERIALIZER_H__
> +
> +#include <deque>
> +#include <iostream>
> +#include <string.h>
> +#include <tuple>
> +#include <vector>
> +
> +#include <libcamera/buffer.h>
> +#include <libcamera/control_ids.h>
> +#include <libcamera/geometry.h>
> +#include <libcamera/ipa/ipa_interface.h>
> +
> +#include "libcamera/internal/byte_stream_buffer.h"
> +#include "libcamera/internal/camera_sensor.h"
> +#include "libcamera/internal/control_serializer.h"
> +#include "libcamera/internal/log.h"
> +
> +namespace libcamera {
> +
> +LOG_DECLARE_CATEGORY(IPADataSerializer)
> +

should these be wrapped in an anonymous namespace ? They are used by
the generated proxyes, but the header is included. I tried it locally
and complier seems to digest it

> +template<typename T>
> +void appendUInt(std::vector<uint8_t> &vec, T val)
> +{
> +	size_t byteWidth = sizeof(val);
> +	std::vector<uint8_t> v(byteWidth);
> +	memcpy(&v[0], &val, byteWidth);
> +
> +	vec.insert(vec.end(), v.begin(), v.end());
> +}
> +
> +template<typename T>
> +T readUInt(std::vector<uint8_t> &vec, size_t pos)
> +{
> +	T ret = 0;
> +	size_t byteWidth = sizeof(ret);
> +	if (pos + byteWidth > vec.size())
> +		return ret;
> +
> +	memcpy(&ret, &vec.data()[pos], byteWidth);

Just &vec[pos] should be enough

> +	return ret;
> +}
> +
> +template<typename T>
> +T readUInt(std::vector<uint8_t>::iterator it)
> +{
> +	T ret = 0;
> +	size_t byteWidth = sizeof(ret);
> +
> +	std::vector<uint8_t> v(it, it + byteWidth);
> +	memcpy(&ret, v.data(), byteWidth);
> +	return ret;
> +}

These new versions looks much better!

> +
> +template<typename T>
> +class IPADataSerializer
> +{
> +#ifdef __DOXYGEN__
> +public:
> +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> +	serialize(T data, ControlSerializer *cs);
> +
> +	static T deserialize(std::vector<uint8_t> &data,
> +			     ControlSerializer *cs);
> +	static T deserialize(std::vector<uint8_t>::iterator it1,
> +			     std::vector<uint8_t>::iterator it2,
> +			     ControlSerializer *cs);
> +

Intentional empty line ?

> +	static T deserialize(std::vector<uint8_t> &data,
> +			     std::vector<int32_t> &fds,
> +			     ControlSerializer *cs);
> +	static T deserialize(std::vector<uint8_t>::iterator data_it1,
> +			     std::vector<uint8_t>::iterator data_it2,
> +			     std::vector<int32_t>::iterator fds_it1,
> +			     std::vector<int32_t>::iterator fds_it2,
> +			     ControlSerializer *cs);

Aren't these better called data_start, data_end and fds_start, fds_end
?

> +#endif /* __DOXYGEN__ */

Do you need this ifdef __DOXYGEN__ ? you want these parsed, don't you ?
It's usually #ifndef __DOXYGEN__ to exclude from documentation parsing
some parts of the code, like you're done below

> +};
> +
> +#ifndef __DOXYGEN__
> +
> +template<typename V>
> +class IPADataSerializer<std::vector<V>>
> +{
> +public:
> +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> +	serialize(const std::vector<V> &data, ControlSerializer *cs = nullptr)
> +	{
> +		std::vector<uint8_t> data_vec;

We can reserve data.size() * sizeof(V) in the vector

> +		std::vector<int32_t> fds_vec;
> +
> +		/* Serialize the length. */
> +		uint32_t vec_len = data.size();
> +		appendUInt<uint32_t>(data_vec, vec_len);
> +
> +		/* Serialize the members. */
> +		for (auto it = data.begin(); it != data.end(); ++it) {

                for (auto const &it : data) ?

> +			std::vector<uint8_t> dvec;
> +			std::vector<int32_t> fvec;
> +
> +			std::tie(dvec, fvec) =
> +				IPADataSerializer<V>::serialize(*it, cs);
> +
> +			appendUInt<uint32_t>(data_vec, dvec.size());

Why is the size (in bytes) of each serialized entry recorded in the
serialized data buffer ? Aren't all the members of the same size ?

> +			appendUInt<uint32_t>(data_vec, fvec.size());

Same question, more relevant as FileDescriptors are known to be
uin32_t

> +
> +			data_vec.insert(data_vec.end(), dvec.begin(), dvec.end());
> +			fds_vec.insert(fds_vec.end(), fvec.begin(), fvec.end());
> +		}
> +
> +		return {data_vec, fds_vec};
> +	}
> +
> +	static std::vector<V> deserialize(std::vector<uint8_t> &data, ControlSerializer *cs = nullptr)
> +	{
> +		return IPADataSerializer<std::vector<V>>::deserialize(data.begin(), data.end(), cs);

question: do you need to fully qualify the deserializer() function
call ? I tried removing "IPADataSerializer<std::vector<V>>::" and
compiler is still happy

> +	}
> +
> +	static std::vector<V> deserialize(std::vector<uint8_t>::iterator it1,
> +					  std::vector<uint8_t>::iterator it2,
> +					  ControlSerializer *cs = nullptr)
> +	{
> +		std::vector<int32_t> fds;
> +		return IPADataSerializer<std::vector<V>>::deserialize(it1, it2,
> +								      fds.begin(), fds.end(),
> +								      cs);
> +	}
> +
> +	static std::vector<V> deserialize(std::vector<uint8_t> &data, std::vector<int32_t> &fds,
> +					  ControlSerializer *cs = nullptr)
> +	{
> +		return IPADataSerializer<std::vector<V>>::deserialize(data.begin(), data.end(),
> +								      fds.begin(), fds.end(),
> +								      cs);
> +	}
> +
> +	static std::vector<V> deserialize(std::vector<uint8_t>::iterator data_it1,
> +					  [[maybe_unused]] std::vector<uint8_t>::iterator data_it2,
> +					  std::vector<int32_t>::iterator fds_it1,
> +					  [[maybe_unused]] std::vector<int32_t>::iterator fds_it2,
> +					  ControlSerializer *cs = nullptr)
> +	{
> +		uint32_t vec_len = readUInt<uint32_t>(data_it1);
> +		std::vector<V> ret(vec_len);
> +
> +		std::vector<uint8_t>::iterator data_it = data_it1 + 4;
> +		std::vector<int32_t>::iterator fd_it = fds_it1;
> +		for (uint32_t i = 0; i < vec_len; i++) {
> +			uint32_t sizeof_data = readUInt<uint32_t>(data_it);

Same questions as above, do we need to insert the entry size in the
serialized buffer, or can it just be assumed to be sizeof(V) ?

> +			uint32_t sizeof_fds  = readUInt<uint32_t>(data_it + 4);

And this one to be 4 bytes ?

> +
> +			ret[i] = IPADataSerializer<V>::deserialize(data_it + 8,
> +								   data_it + 8 + sizeof_data,
> +								   fd_it,
> +								   fd_it + sizeof_fds,
> +								   cs);
> +
> +			data_it += 8 + sizeof_data;
> +			fd_it += sizeof_fds;
> +		}
> +
> +		return ret;
> +	}
> +};
> +
> +template<typename K, typename V>
> +class IPADataSerializer<std::map<K, V>>
> +{
> +public:
> +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> +	serialize(const std::map<K, V> &data, ControlSerializer *cs = nullptr)
> +	{
> +		std::vector<uint8_t> data_vec;
> +		std::vector<int32_t> fds_vec;
> +
> +		/* Serialize the length. */
> +		uint32_t map_len = data.size();
> +		appendUInt<uint32_t>(data_vec, map_len);
> +
> +		/* Serialize the members. */
> +		for (auto it = data.begin(); it != data.end(); ++it) {

Same comment: a range loop might be nicer
                for (auto const &it : data)

> +			std::vector<uint8_t> dvec;
> +			std::vector<int32_t> fvec;
> +
> +			std::tie(dvec, fvec) =
> +				IPADataSerializer<K>::serialize(it->first, cs);
> +
> +			appendUInt<uint32_t>(data_vec, dvec.size());
> +			appendUInt<uint32_t>(data_vec, fvec.size());
> +
> +			data_vec.insert(data_vec.end(), dvec.begin(), dvec.end());
> +			fds_vec.insert(fds_vec.end(), fvec.begin(), fvec.end());
> +
> +			std::tie(dvec, fvec) =
> +				IPADataSerializer<V>::serialize(it->second, cs);
> +
> +			appendUInt<uint32_t>(data_vec, dvec.size());
> +			appendUInt<uint32_t>(data_vec, fvec.size());
> +
> +			data_vec.insert(data_vec.end(), dvec.begin(), dvec.end());
> +			fds_vec.insert(fds_vec.end(), fvec.begin(), fvec.end());
> +		}
> +
> +		return {data_vec, fds_vec};
> +	}
> +
> +	static std::map<K, V> deserialize(std::vector<uint8_t> &data, ControlSerializer *cs = nullptr)
> +	{
> +		return IPADataSerializer<std::map<K, V>>::deserialize(data.begin(), data.end(), cs);
> +	}
> +
> +	static std::map<K, V> deserialize(std::vector<uint8_t>::iterator it1,
> +					  std::vector<uint8_t>::iterator it2,
> +					  ControlSerializer *cs = nullptr)
> +	{
> +		std::vector<int32_t> fds;
> +		return IPADataSerializer<std::map<K, V>>::deserialize(it1, it2,
> +								      fds.begin(), fds.end(),
> +								      cs);
> +	}
> +
> +	static std::map<K, V> deserialize(std::vector<uint8_t> &data, std::vector<int32_t> &fds,
> +					  ControlSerializer *cs = nullptr)
> +	{
> +		return IPADataSerializer<std::map<K, V>>::deserialize(data.begin(), data.end(),
> +								      fds.begin(), fds.end(),
> +								      cs);
> +	}

Same comments on the fully qualified path

> +
> +	static std::map<K, V> deserialize(std::vector<uint8_t>::iterator data_it1,
> +					  [[maybe_unused]] std::vector<uint8_t>::iterator data_it2,
> +					  std::vector<int32_t>::iterator fds_it1,
> +					  [[maybe_unused]] std::vector<int32_t>::iterator fds_it2,
> +					  ControlSerializer *cs = nullptr)
> +	{
> +		std::map<K, V> ret;
> +
> +		uint32_t map_len = readUInt<uint32_t>(data_it1);
> +
> +		std::vector<uint8_t>::iterator data_it = data_it1 + 4;
> +		std::vector<int32_t>::iterator fd_it = fds_it1;
> +		for (uint32_t i = 0; i < map_len; i++) {
> +			uint32_t sizeof_data = readUInt<uint32_t>(data_it);
> +			uint32_t sizeof_fds  = readUInt<uint32_t>(data_it + 4);
> +
> +			K key = IPADataSerializer<K>::deserialize(data_it + 8,
> +								  data_it + 8 + sizeof_data,
> +								  fd_it,
> +								  fd_it + sizeof_fds,
> +								  cs);
> +
> +			data_it += 8 + sizeof_data;
> +			fd_it += sizeof_fds;
> +			sizeof_data = readUInt<uint32_t>(data_it);
> +			sizeof_fds  = readUInt<uint32_t>(data_it + 4);
> +
> +			const V value = IPADataSerializer<V>::deserialize(data_it + 8,
> +									  data_it + 8 + sizeof_data,
> +									  fd_it,
> +									  fd_it + sizeof_fds,
> +									  cs);
> +			ret.insert({key, value});
> +
> +			data_it += 8 + sizeof_data;
> +			fd_it += sizeof_fds;
> +		}
> +
> +		return ret;
> +	}
> +};
> +
> +#define DECLARE_POD_SERIALIZER(type)					\
> +template<>								\
> +class IPADataSerializer<type>						\
> +{									\
> +public:									\
> +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>	\
> +	serialize(const type data,					\
> +		  [[maybe_unused]] ControlSerializer *cs = nullptr)	\
> +	{								\
> +		std::vector<uint8_t> data_vec;				\

Do you think pre-allocating sizeof(type) in the vector might help ?

> +		appendUInt<type>(data_vec, data);			\
> +									\
> +		return {data_vec, {}};					\
> +	}								\
> +									\
> +	static type deserialize(std::vector<uint8_t> &data,		\
> +				[[maybe_unused]] ControlSerializer *cs = nullptr)\
> +	{								\
> +		return IPADataSerializer<type>::deserialize(data.begin(),\
> +							    data.end());\
> +	}								\
> +									\
> +	static type deserialize(std::vector<uint8_t>::iterator it1,	\
> +				[[maybe_unused]] std::vector<uint8_t>::iterator it2,\
> +				[[maybe_unused]] ControlSerializer *cs = nullptr)\
> +	{								\
> +		return readUInt<type>(it1);				\
> +	}								\
> +									\
> +	static type deserialize(std::vector<uint8_t> &data,		\
> +				[[maybe_unused]] std::vector<int32_t> &fds,\
> +				[[maybe_unused]] ControlSerializer *cs = nullptr)\
> +	{								\
> +		return IPADataSerializer<type>::deserialize(data.begin(),\
> +							    data.end());\
> +	}								\
> +									\
> +	static type deserialize(std::vector<uint8_t>::iterator data_it1,\
> +				std::vector<uint8_t>::iterator data_it2,\
> +				[[maybe_unused]] std::vector<int32_t>::iterator fds_it1,\
> +				[[maybe_unused]] std::vector<int32_t>::iterator fds_it2,\
> +				[[maybe_unused]] ControlSerializer *cs = nullptr)\
> +	{								\
> +		return IPADataSerializer<type>::deserialize(data_it1,	\
> +							    data_it2);	\
> +	}								\
> +};
> +
> +DECLARE_POD_SERIALIZER(bool)
> +DECLARE_POD_SERIALIZER(uint8_t)
> +DECLARE_POD_SERIALIZER(uint16_t)
> +DECLARE_POD_SERIALIZER(uint32_t)
> +DECLARE_POD_SERIALIZER(uint64_t)
> +DECLARE_POD_SERIALIZER(int8_t)
> +DECLARE_POD_SERIALIZER(int16_t)
> +DECLARE_POD_SERIALIZER(int32_t)
> +DECLARE_POD_SERIALIZER(int64_t)

The I wonder if keeping here and in the documentation the term 'uint'
is correct and it shouldn't just be replaced with 'int'

> +DECLARE_POD_SERIALIZER(float)
> +DECLARE_POD_SERIALIZER(double)

Or even POD

> +
> +template<>
> +class IPADataSerializer<std::string>
> +{
> +public:
> +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> +	serialize(const std::string &data, [[maybe_unused]] ControlSerializer *cs = nullptr)
> +	{
> +		std::vector<uint8_t> data_vec(data.begin(), data.end());
> +
> +		return {data_vec, {}};
> +	}
> +
> +	static std::string deserialize(std::vector<uint8_t> &data,
> +				       [[maybe_unused]] ControlSerializer *cs = nullptr)
> +	{
> +		return IPADataSerializer<std::string>::deserialize(data.begin(), data.end());
> +	}
> +
> +	static std::string deserialize(std::vector<uint8_t>::iterator it1,
> +				       std::vector<uint8_t>::iterator it2,
> +				       [[maybe_unused]] ControlSerializer *cs = nullptr)
> +	{
> +		std::string str(it1, it2);
> +
> +		return str;

Can you just
                return {it1, it2};
?

> +	}
> +
> +	static std::string deserialize(std::vector<uint8_t> &data,
> +				       [[maybe_unused]] std::vector<int32_t> &fds,
> +				       [[maybe_unused]] ControlSerializer *cs = nullptr)
> +	{
> +		return IPADataSerializer<std::string>::deserialize(data.begin(), data.end());

Maybe
                return {data.being(), data.end()};

would save a function call ?
Same for other overloaded methods (and possibly not only in the
<string> specialization).


> +	}
> +
> +	static std::string deserialize(std::vector<uint8_t>::iterator data_it1,
> +				       std::vector<uint8_t>::iterator data_it2,
> +				       [[maybe_unused]] std::vector<int32_t>::iterator fds_it1,
> +				       [[maybe_unused]] std::vector<int32_t>::iterator fds_it2,
> +				       [[maybe_unused]] ControlSerializer *cs = nullptr)
> +	{
> +		return IPADataSerializer<std::string>::deserialize(data_it1, data_it2);
> +	}
> +};
> +
> +template<>
> +class IPADataSerializer<FileDescriptor>
> +{
> +public:
> +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> +	serialize(const FileDescriptor &data, [[maybe_unused]] ControlSerializer *cs = nullptr)
> +	{
> +		std::vector<uint8_t> data_vec = { data.isValid() };
> +		std::vector<int32_t> fd_vec;
> +		if (data.isValid())
> +			fd_vec.push_back(data.fd());
> +
> +		return {data_vec, fd_vec};
> +	}
> +
> +	static FileDescriptor deserialize(std::vector<uint8_t> &data, std::vector<int32_t> &fds,
> +					  [[maybe_unused]] ControlSerializer *cs = nullptr)
> +	{
> +		return IPADataSerializer<FileDescriptor>::deserialize(data.begin(), data.end(),
> +								      fds.begin(), fds.end());
> +	}
> +
> +	static FileDescriptor deserialize(std::vector<uint8_t>::iterator data_it1,
> +					  std::vector<uint8_t>::iterator data_it2,
> +					  std::vector<int32_t>::iterator fds_it1,
> +					  std::vector<int32_t>::iterator fds_it2,
> +					  [[maybe_unused]] ControlSerializer *cs = nullptr)
> +	{
> +		if (std::distance(data_it1, data_it2) < 1)
> +			LOG(IPADataSerializer, Fatal)
> +				<< "Invalid data to deserialize FileDescriptor";
> +
> +		bool valid = *data_it1;

in C this would be
                bool valid = !!*data_it1;
not sure about C++ and not even sure if the !! is just a paranoid
kernel convention, as I don't think even C requires that

> +
> +		if (valid && std::distance(fds_it1, fds_it2) < 1)

		if (valid && std::distance(fds_it1, fds_it2) < 1) {
			LOG(IPADataSerializer, Fatal)
				<< "Invalid fds to deserialize FileDescriptor";
                        return {}
                }

                return FileDescriptor(*fds_it1);

Or is returning {} when std::distance() < 1 wrong ? I don't think so,
as even if it is "valid" there's not fds serialized.


> +			LOG(IPADataSerializer, Fatal)
> +				<< "Invalid fds to deserialize FileDescriptor";
> +
> +		return valid ? FileDescriptor(*fds_it1) : FileDescriptor();
> +	}
> +};
> +
> +template<>
> +class IPADataSerializer<IPASettings>
> +{
> +public:
> +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> +	serialize(const IPASettings &data, [[maybe_unused]] ControlSerializer *cs = nullptr)
> +	{
> +		return IPADataSerializer<std::string>::serialize(data.configurationFile);
> +	}
> +
> +	static IPASettings deserialize(std::vector<uint8_t> &data,
> +				       [[maybe_unused]] ControlSerializer *cs = nullptr)
> +	{
> +		return IPADataSerializer<IPASettings>::deserialize(data.begin(), data.end());
> +	}
> +
> +	static IPASettings deserialize(std::vector<uint8_t>::iterator it1,
> +				       std::vector<uint8_t>::iterator it2,
> +				       [[maybe_unused]] ControlSerializer *cs = nullptr)
> +	{
> +		IPASettings ret;
> +		ret.configurationFile = IPADataSerializer<std::string>::deserialize(it1, it2);
> +
> +		return ret;

                return { IPADataSerializer<std::string>::deserialize(it1, it2) };

?

> +	}
> +
> +	static IPASettings deserialize(std::vector<uint8_t> &data,
> +				       [[maybe_unused]] std::vector<int32_t> &fds,
> +				       [[maybe_unused]] ControlSerializer *cs = nullptr)
> +	{
> +		return IPADataSerializer<IPASettings>::deserialize(data.begin(), data.end());

                return { IPADataSerializer<std::string>::deserialize(data.begin(), data.end() };

?

> +	}
> +
> +	static IPASettings deserialize(std::vector<uint8_t>::iterator data_it1,
> +				       std::vector<uint8_t>::iterator data_it2,
> +				       [[maybe_unused]] std::vector<int32_t>::iterator fds_it1,
> +				       [[maybe_unused]] std::vector<int32_t>::iterator fds_it2,
> +				       [[maybe_unused]] ControlSerializer *cs = nullptr)
> +	{
> +		return IPADataSerializer<IPASettings>::deserialize(data_it1, data_it2);

same

> +	}
> +};
> +
> +template<>
> +class IPADataSerializer<CameraSensorInfo>
> +{
> +public:
> +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> +	serialize(const CameraSensorInfo &data, [[maybe_unused]] ControlSerializer *cs = nullptr)
> +	{
> +		std::vector<uint8_t> data_vec;

Reserving sizeof(CameraSensorInfo) might help

I just noticed and I wonder why all this code uses this_naming_style
in place of the libcamera standard namingStyle

> +
> +		uint32_t str_len = data.model.size();
> +		appendUInt<uint32_t>(data_vec, str_len);
> +
> +		data_vec.insert(data_vec.end(), data.model.begin(), data.model.end());

Shouldn't this be serialized using the IPADataSerializer<std::string>
if it's not needed, do we need the specialization then ?

> +
> +		appendUInt<uint32_t>(data_vec, data.bitsPerPixel);
> +
> +		appendUInt<uint32_t>(data_vec, data.activeAreaSize.width);
> +		appendUInt<uint32_t>(data_vec, data.activeAreaSize.height);
> +
> +		appendUInt<uint32_t>(data_vec, static_cast<uint32_t>(data.analogCrop.x));
> +		appendUInt<uint32_t>(data_vec, static_cast<uint32_t>(data.analogCrop.y));
> +		appendUInt<uint32_t>(data_vec, data.analogCrop.width);
> +		appendUInt<uint32_t>(data_vec, data.analogCrop.height);
> +
> +		appendUInt<uint32_t>(data_vec, data.outputSize.width);
> +		appendUInt<uint32_t>(data_vec, data.outputSize.height);
> +
> +		appendUInt<uint64_t>(data_vec, data.pixelRate);
> +
> +		appendUInt<uint32_t>(data_vec, data.lineLength);
> +
> +		return {data_vec, {}};
> +	}
> +
> +	static CameraSensorInfo deserialize(std::vector<uint8_t> &data,
> +					    [[maybe_unused]] ControlSerializer *cs = nullptr)
> +	{
> +		return IPADataSerializer<CameraSensorInfo>::deserialize(data.begin(), data.end());
> +	}
> +
> +	static CameraSensorInfo deserialize(std::vector<uint8_t>::iterator it1,
> +					    [[maybe_unused]] std::vector<uint8_t>::iterator it2,
> +					    [[maybe_unused]] ControlSerializer *cs = nullptr)
> +	{
> +		CameraSensorInfo ret;
                CameraSensorInfo ret{};

Also for other statically allocated fields that are returned by other
specializations.

> +
> +		uint32_t str_len = readUInt<uint32_t>(it1);
> +		std::string str(it1 + 4, it1 + 4 + str_len);
> +		ret.model = str;
> +
> +		std::vector<uint8_t>::iterator it = it1 + 4 + str_len;
> +
> +		ret.bitsPerPixel = readUInt<uint32_t>(it);
> +
> +		ret.activeAreaSize.width = readUInt<uint32_t>(it + 4);
> +		ret.activeAreaSize.height = readUInt<uint32_t>(it + 8);
> +
> +		ret.analogCrop.x = static_cast<int32_t>(readUInt<uint32_t>(it + 12));
> +		ret.analogCrop.y = static_cast<int32_t>(readUInt<uint32_t>(it + 16));
> +		ret.analogCrop.width = readUInt<uint32_t>(it + 20);
> +		ret.analogCrop.height = readUInt<uint32_t>(it + 24);
> +
> +		ret.outputSize.width = readUInt<uint32_t>(it + 28);
> +		ret.outputSize.height = readUInt<uint32_t>(it + 32);
> +
> +		ret.pixelRate = readUInt<uint64_t>(it + 36);
> +
> +		ret.lineLength = readUInt<uint64_t>(it + 44);

lineLength is a 32 bits integer

> +
> +		return ret;
> +	}
> +
> +	static CameraSensorInfo deserialize(std::vector<uint8_t> &data,
> +					    [[maybe_unused]] std::vector<int32_t> &fds,
> +					    [[maybe_unused]] ControlSerializer *cs = nullptr)
> +	{
> +		return IPADataSerializer<CameraSensorInfo>::deserialize(data.begin(), data.end());
> +	}
> +
> +	static CameraSensorInfo deserialize(std::vector<uint8_t>::iterator data_it1,
> +					    std::vector<uint8_t>::iterator data_it2,
> +					    [[maybe_unused]] std::vector<int32_t>::iterator fds_it1,
> +					    [[maybe_unused]] std::vector<int32_t>::iterator fds_it2,
> +					    [[maybe_unused]] ControlSerializer *cs = nullptr)
> +	{
> +		return IPADataSerializer<CameraSensorInfo>::deserialize(data_it1, data_it2);
> +	}
> +};
> +
> +template<>
> +class IPADataSerializer<IPAStream>
> +{
> +public:
> +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> +	serialize(const IPAStream &data, [[maybe_unused]] ControlSerializer *cs = nullptr)
> +	{
> +		std::vector<uint8_t> data_vec;
> +
> +		appendUInt<uint32_t>(data_vec, data.pixelFormat);
> +
> +		appendUInt<uint32_t>(data_vec, data.size.width);
> +		appendUInt<uint32_t>(data_vec, data.size.height);
> +
> +		return {data_vec, {}};
> +	}
> +
> +	static IPAStream deserialize(std::vector<uint8_t> &data,
> +				     [[maybe_unused]] ControlSerializer *cs = nullptr)
> +	{
> +		return IPADataSerializer<IPAStream>::deserialize(data.begin(), data.end());
> +	}
> +
> +	static IPAStream deserialize(std::vector<uint8_t>::iterator it1,
> +				     [[maybe_unused]] std::vector<uint8_t>::iterator it2,
> +				     [[maybe_unused]] ControlSerializer *cs = nullptr)
> +	{
> +		IPAStream ret;
> +
> +		ret.pixelFormat = readUInt<uint32_t>(it1);
> +
> +		ret.size.width = readUInt<uint32_t>(it1 + 4);
> +		ret.size.height = readUInt<uint32_t>(it1 + 8);
> +
> +		return ret;
> +	}
> +
> +	static IPAStream deserialize(std::vector<uint8_t> &data,
> +				     [[maybe_unused]] std::vector<int32_t> &fds,
> +				     [[maybe_unused]] ControlSerializer *cs = nullptr)
> +	{
> +		return IPADataSerializer<IPAStream>::deserialize(data.begin(), data.end());
> +	}
> +
> +	static IPAStream deserialize(std::vector<uint8_t>::iterator data_it1,
> +				     std::vector<uint8_t>::iterator data_it2,
> +				     [[maybe_unused]] std::vector<int32_t>::iterator fds_it1,
> +				     [[maybe_unused]] std::vector<int32_t>::iterator fds_it2,
> +				     [[maybe_unused]] ControlSerializer *cs = nullptr)
> +	{
> +		return IPADataSerializer<IPAStream>::deserialize(data_it1, data_it2);
> +	}
> +};
> +
> +template<>
> +class IPADataSerializer<const ControlList>
> +{
> +public:
> +	/* map arg will be generated, since it's per-pipeline anyway. */
> +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> +	serialize(const ControlList &data, const ControlInfoMap &map,
> +		  ControlSerializer *cs)
> +	{
> +		if (!cs)
> +			LOG(IPADataSerializer, Fatal)
> +				<< "ControlSerializer not provided for serialization of ControlList";
> +
> +		size_t size = cs->binarySize(map);
> +		std::vector<uint8_t> infoData(size);
> +		ByteStreamBuffer buffer(infoData.data(), infoData.size());
> +		int ret = cs->serialize(map, buffer);
> +
> +		if (ret < 0 || buffer.overflow()) {
> +			LOG(IPADataSerializer, Error) << "Failed to serialize ControlList's ControlInfoMap";
> +			return {{}, {}};
> +		}
> +
> +		size = cs->binarySize(data);
> +		std::vector<uint8_t> listData(size);
> +		buffer = ByteStreamBuffer(listData.data(), listData.size());
> +		ret = cs->serialize(data, buffer);
> +
> +		if (ret < 0 || buffer.overflow()) {
> +			LOG(IPADataSerializer, Error) << "Failed to serialize ControlList";
> +			return {{}, {}};
> +		}
> +
> +		std::vector<uint8_t> data_vec;

You know the sizes and can reserve ?

> +		appendUInt<uint32_t>(data_vec, infoData.size());
> +		appendUInt<uint32_t>(data_vec, listData.size());
> +		data_vec.insert(data_vec.end(), infoData.begin(), infoData.end());
> +		data_vec.insert(data_vec.end(), listData.begin(), listData.end());
> +
> +		return {data_vec, {}};
> +	}
> +
> +	static const ControlList deserialize(std::vector<uint8_t> &data, ControlSerializer *cs)
> +	{
> +		return IPADataSerializer<const ControlList>::deserialize(data.begin(), data.end(), cs);
> +	}
> +
> +	static const ControlList deserialize(std::vector<uint8_t>::iterator it1,
> +				       [[maybe_unused]] std::vector<uint8_t>::iterator it2,
> +				       ControlSerializer *cs)
> +	{
> +		if (!cs)
> +			LOG(IPADataSerializer, Fatal)
> +				<< "ControlSerializer not provided for deserialization of ControlList";
> +
> +		uint32_t infoDataSize = readUInt<uint32_t>(it1);
> +		uint32_t listDataSize = readUInt<uint32_t>(it1 + 4);
> +
> +		std::vector<uint8_t>::iterator it = it1 + 8;
> +
> +		std::vector<uint8_t> infoData(it, it + infoDataSize);
> +		std::vector<uint8_t> listData(it + infoDataSize, it + infoDataSize + listDataSize);
> +
> +		ByteStreamBuffer buffer(const_cast<const uint8_t *>(infoData.data()), infoData.size());
> +		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();
> +		}
> +
> +		buffer = ByteStreamBuffer(const_cast<const uint8_t *>(listData.data()), listData.size());
> +		ControlList list = cs->deserialize<ControlList>(buffer);
> +		if (buffer.overflow())
> +			LOG(IPADataSerializer, Error) << "Failed to deserialize ControlList: buffer overflow";
> +		if (list.empty())
> +			LOG(IPADataSerializer, Error) << "Failed to deserialize ControlList: empty list";

Should we return {} in these cases or is it fine ?

> +
> +		return list;
> +	}
> +
> +	static const ControlList deserialize(std::vector<uint8_t> &data,
> +				       [[maybe_unused]] std::vector<int32_t> &fds,
> +				       ControlSerializer *cs)
> +	{
> +		return IPADataSerializer<const ControlList>::deserialize(data.begin(), data.end(), cs);
> +	}
> +
> +	static const ControlList deserialize(std::vector<uint8_t>::iterator data_it1,
> +				       std::vector<uint8_t>::iterator data_it2,
> +				       [[maybe_unused]] std::vector<int32_t>::iterator fds_it1,
> +				       [[maybe_unused]] std::vector<int32_t>::iterator fds_it2,
> +				       ControlSerializer *cs)
> +	{
> +		return IPADataSerializer<const ControlList>::deserialize(data_it1, data_it2, cs);
> +	}
> +};
> +
> +template<>
> +class IPADataSerializer<ControlList>
> +{
> +public:
> +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> +	serialize(const ControlList &data, const ControlInfoMap &map,
> +		  ControlSerializer *cs)
> +	{
> +		const ControlList &list_const = const_cast<const ControlList &>(data);
> +
> +		std::vector<uint8_t> data_vec;
> +		std::tie(data_vec, std::ignore) =
> +			IPADataSerializer<const ControlList>::serialize(list_const, map, cs);
> +
> +		return {data_vec, {}};
> +	}
> +
> +	static ControlList deserialize(std::vector<uint8_t> &data,
> +				       ControlSerializer *cs)
> +	{
> +		ControlList ret;
> +		const ControlList &list = ret;
> +		const_cast<ControlList &>(list) =
> +			IPADataSerializer<const ControlList>::deserialize(data, cs);
> +
> +		return ret;
> +	}
> +
> +	static ControlList deserialize(std::vector<uint8_t>::iterator it1,
> +				       [[maybe_unused]] std::vector<uint8_t>::iterator it2,
> +				       ControlSerializer *cs)
> +	{
> +		ControlList ret;
> +		const ControlList &list = ret;
> +		const_cast<ControlList &>(list) =
> +			IPADataSerializer<const ControlList>::deserialize(it1, it2, cs);

Why not
                const ControlList list =
			IPADataSerializer<const ControlList>::deserialize(it1, it2, cs);
                return const_cast<ControlList>(list)

> +
> +		return ret;
> +	}
> +
> +	static ControlList deserialize(std::vector<uint8_t> &data,
> +				       [[maybe_unused]] std::vector<int32_t> &fds,
> +				       ControlSerializer *cs)
> +	{
> +		ControlList ret;
> +		const ControlList &list = ret;
> +		const_cast<ControlList &>(list) =
> +			IPADataSerializer<const ControlList>::deserialize(data, fds, cs);
> +
> +		return ret;

same question

> +	}
> +
> +	static ControlList deserialize(std::vector<uint8_t>::iterator data_it1,
> +				       std::vector<uint8_t>::iterator data_it2,
> +				       [[maybe_unused]] std::vector<int32_t>::iterator fds_it1,
> +				       [[maybe_unused]] std::vector<int32_t>::iterator fds_it2,
> +				       ControlSerializer *cs)
> +	{
> +		ControlList ret;
> +		const ControlList &list = ret;
> +		const_cast<ControlList &>(list) =
> +			IPADataSerializer<const ControlList>::deserialize(data_it1, data_it2,
> +									  fds_it1, fds_it2, cs);

same question

> +
> +		return ret;
> +	}
> +};
> +
> +template<>
> +class IPADataSerializer<const ControlInfoMap>
> +{
> +public:
> +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> +	serialize(const ControlInfoMap &map, ControlSerializer *cs)
> +	{
> +		if (!cs)
> +			LOG(IPADataSerializer, Fatal)
> +				<< "ControlSerializer not provided for serialization of ControlInfoMap";
> +
> +		size_t size = cs->binarySize(map);
> +		std::vector<uint8_t> infoData(size);
> +		ByteStreamBuffer buffer(infoData.data(), infoData.size());
> +		int ret = cs->serialize(map, buffer);
> +
> +		if (ret < 0 || buffer.overflow())
> +			return {{}, {}};
> +
> +		std::vector<uint8_t> data_vec;
> +		appendUInt<uint32_t>(data_vec, infoData.size());
> +		data_vec.insert(data_vec.end(), infoData.begin(), infoData.end());
> +
> +		return {data_vec, {}};
> +	}
> +
> +	static const ControlInfoMap deserialize(std::vector<uint8_t> &data,
> +						ControlSerializer *cs)
> +	{
> +		return IPADataSerializer<const ControlInfoMap>::deserialize(data.begin(), data.end(), cs);
> +	}
> +
> +	static const ControlInfoMap deserialize(std::vector<uint8_t>::iterator it1,
> +						[[maybe_unused]] std::vector<uint8_t>::iterator it2,
> +						ControlSerializer *cs)
> +	{
> +		if (!cs)
> +			LOG(IPADataSerializer, Fatal)
> +				<< "ControlSerializer not provided for deserialization of ControlInfoMap";
> +
> +		uint32_t infoDataSize = readUInt<uint32_t>(it1);
> +
> +		std::vector<uint8_t>::iterator it = it1 + 4;
> +
> +		std::vector<uint8_t> infoData(it, it + infoDataSize);
> +
> +		ByteStreamBuffer buffer(const_cast<const uint8_t *>(infoData.data()), infoData.size());
> +		const ControlInfoMap map = cs->deserialize<ControlInfoMap>(buffer);
> +
> +		return map;
> +	}
> +
> +	static const ControlInfoMap deserialize(std::vector<uint8_t> &data,
> +						[[maybe_unused]] std::vector<int32_t> &fds,
> +						ControlSerializer *cs)
> +	{
> +		return IPADataSerializer<const ControlInfoMap>::deserialize(data.begin(), data.end(), cs);
> +	}
> +
> +	static const ControlInfoMap deserialize(std::vector<uint8_t>::iterator data_it1,
> +					  std::vector<uint8_t>::iterator data_it2,
> +					  [[maybe_unused]] std::vector<int32_t>::iterator fds_it1,
> +					  [[maybe_unused]] std::vector<int32_t>::iterator fds_it2,
> +					  ControlSerializer *cs)
> +	{
> +		return IPADataSerializer<const ControlInfoMap>::deserialize(data_it1, data_it2, cs);
> +	}
> +};
> +
> +template<>
> +class IPADataSerializer<ControlInfoMap>
> +{
> +public:
> +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> +	serialize(const ControlInfoMap &map, [[maybe_unused]] ControlSerializer *cs)
> +	{
> +		const ControlInfoMap &map_const = const_cast<const ControlInfoMap &>(map);
> +
> +		std::vector<uint8_t> data_vec;
> +		std::tie(data_vec, std::ignore) =
> +			IPADataSerializer<const ControlInfoMap>::serialize(map_const, cs);
> +
> +		return {data_vec, {}};
> +	}
> +
> +	static ControlInfoMap deserialize(std::vector<uint8_t> &data,
> +					  ControlSerializer *cs)
> +	{
> +		ControlInfoMap ret;
> +		const ControlInfoMap &map = ret;
> +		const_cast<ControlInfoMap &>(map) =
> +			IPADataSerializer<const ControlInfoMap>::deserialize(data, cs);
> +
> +		return ret;
> +	}
> +
> +	static ControlInfoMap deserialize(std::vector<uint8_t>::iterator it1,
> +						[[maybe_unused]] std::vector<uint8_t>::iterator it2,
> +						ControlSerializer *cs)
> +	{
> +		ControlInfoMap ret;
> +		const ControlInfoMap &map = ret;
> +		const_cast<ControlInfoMap &>(map) =
> +			IPADataSerializer<const ControlInfoMap>::deserialize(it1, it2, cs);
> +
> +		return ret;
> +	}
> +
> +	static ControlInfoMap deserialize(std::vector<uint8_t> &data,
> +						[[maybe_unused]] std::vector<int32_t> &fds,
> +						ControlSerializer *cs)
> +	{
> +		ControlInfoMap ret;
> +		const ControlInfoMap &map = ret;
> +		const_cast<ControlInfoMap &>(map) =
> +			IPADataSerializer<const ControlInfoMap>::deserialize(data, fds, cs);
> +
> +		return ret;
> +	}
> +
> +	static ControlInfoMap deserialize(std::vector<uint8_t>::iterator data_it1,
> +					  std::vector<uint8_t>::iterator data_it2,
> +					  [[maybe_unused]] std::vector<int32_t>::iterator fds_it1,
> +					  [[maybe_unused]] std::vector<int32_t>::iterator fds_it2,
> +					  ControlSerializer *cs)
> +	{
> +		ControlInfoMap ret;
> +		const ControlInfoMap &map = ret;
> +		const_cast<ControlInfoMap &>(map) =
> +			IPADataSerializer<const ControlInfoMap>::deserialize(data_it1, data_it2,
> +									     fds_it1, fds_it2, cs);

Very similar comment as per the ControlList implementation

> +
> +		return ret;
> +	}
> +};
> +
> +template<>
> +class IPADataSerializer<FrameBuffer::Plane>
> +{
> +public:
> +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> +	serialize(const FrameBuffer::Plane &data, [[maybe_unused]] ControlSerializer *cs = nullptr)
> +	{
> +		std::vector<uint8_t> data_vec;
> +		std::vector<int32_t> fds_vec;
> +
> +		std::vector<uint8_t> fdBuf;
> +		std::vector<int32_t> fdFds;
> +		std::tie(fdBuf, fdFds) =
> +			IPADataSerializer<FileDescriptor>::serialize(data.fd);
> +		data_vec.insert(data_vec.end(), fdBuf.begin(), fdBuf.end());
> +		fds_vec.insert(fds_vec.end(), fdFds.begin(), fdFds.end());
> +
> +		appendUInt<uint32_t>(data_vec, data.length);
> +
> +		return {data_vec, fds_vec};
> +	}
> +
> +	static FrameBuffer::Plane deserialize(std::vector<uint8_t> &data,
> +					      std::vector<int32_t> &fds,
> +					      ControlSerializer *cs = nullptr)
> +	{
> +		return IPADataSerializer<FrameBuffer::Plane>::deserialize(data.begin(), data.end(),
> +									  fds.begin(), fds.end(),
> +									  cs);
> +	}
> +
> +	static FrameBuffer::Plane deserialize(std::vector<uint8_t>::iterator data_it1,
> +					      [[maybe_unused]] std::vector<uint8_t>::iterator data_it2,
> +					      std::vector<int32_t>::iterator fds_it1,
> +					      [[maybe_unused]] std::vector<int32_t>::iterator fds_it2,
> +					      [[maybe_unused]] ControlSerializer *cs = nullptr)
> +	{
> +		FrameBuffer::Plane ret;
> +
> +		ret.fd = IPADataSerializer<FileDescriptor>::deserialize(data_it1, data_it1 + 1,
> +									fds_it1, fds_it1 + 1);
> +		ret.length = readUInt<uint32_t>(data_it1 + 1);
> +
> +		return ret;
> +	}
> +};
> +
> +
> +template<>
> +class IPADataSerializer<IPABuffer>
> +{
> +public:
> +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> +	serialize(const IPABuffer &data, ControlSerializer *cs = nullptr)
> +	{
> +		std::vector<uint8_t> data_vec;
> +
> +		appendUInt<uint32_t>(data_vec, data.id);
> +
> +		std::vector<uint8_t> planes_data_vec;
> +		std::vector<int32_t> planes_fds_vec;

Calculating in advance the occupation might help but it's not trivial

> +		std::tie(planes_data_vec, planes_fds_vec) =
> +			IPADataSerializer<std::vector<FrameBuffer::Plane>>::serialize(data.planes, cs);
> +
> +		data_vec.insert(data_vec.end(), planes_data_vec.begin(), planes_data_vec.end());
> +
> +		return {data_vec, planes_fds_vec};
> +	}
> +
> +	static IPABuffer deserialize(std::vector<uint8_t> &data,
> +				     std::vector<int32_t> &fds,
> +				     ControlSerializer *cs = nullptr)
> +	{
> +		return IPADataSerializer<IPABuffer>::deserialize(data.begin(), data.end(),
> +								 fds.begin(), fds.end(), cs);
> +	}
> +
> +	static IPABuffer deserialize(std::vector<uint8_t>::iterator data_it1,
> +				     std::vector<uint8_t>::iterator data_it2,
> +				     std::vector<int32_t>::iterator fds_it1,
> +				     std::vector<int32_t>::iterator fds_it2,
> +				     ControlSerializer *cs = nullptr)
> +	{
> +		IPABuffer ret;
> +
> +		ret.id = readUInt<uint32_t>(data_it1);
> +
> +		ret.planes =
> +			IPADataSerializer<std::vector<FrameBuffer::Plane>>::deserialize(
> +				data_it1 + 4, data_it2, fds_it1, fds_it2, cs);
> +
> +		return ret;
> +	}
> +};
> +
> +#endif /* __DOXYGEN__ */
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_INTERNAL_IPA_DATA_SERIALIZER_H__ */
> diff --git a/src/libcamera/ipa_data_serializer.cpp b/src/libcamera/ipa_data_serializer.cpp
> new file mode 100644
> index 00000000..5029cdf6
> --- /dev/null
> +++ b/src/libcamera/ipa_data_serializer.cpp
> @@ -0,0 +1,154 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * ipa_data_serializer.cpp - Image Processing Algorithm data serializer
> + */
> +
> +#include "libcamera/internal/ipa_data_serializer.h"
> +
> +#include "libcamera/internal/log.h"
> +
> +/**
> + * \file ipa_data_serializer.h
> + * \brief IPA Data Serializer
> + */
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(IPADataSerializer)
> +
> +/**
> + * \class IPADataSerializer
> + * \brief IPA Data Serializer
> + *
> + * Static template class that provides functions for serializing and
> + * deserializing IPA data.
> + */
> +
> +/**
> + * \fn template<typename T> void appendUInt(std::vector<uint8_t> &vec, T val)
> + * \brief Append uint to end of byte vector, in little-endian order
> + * \tparam T Type of uint to append
> + * \param[in] vec Byte vector to append to
> + * \param[in] val Value of uint to append

As said, this works for uint, int, float and double now

> + */
> +
> +/**
> + * \fn template<typename T> T readUInt(std::vector<uint8_t> &vec, size_t pos)
> + * \brief Read uint from byte vector, in little-endian order
> + * \tparam T Type of uint to read
> + * \param[in] vec Byte vector to read from
> + * \param[in] pos Index in vec to start reading from
> + *

No need for an empty line if no additional description is provided

> + * \return The uint read from \a vec, or 0 if reading goes past end of \a vec

I wonder if we should not pass a T * as an output parameter and return
an error code. Returning 0 makes it impossible to detect failure, as 0
is a valid value

> + */
> +
> +/**
> + * \fn template<typename T> T readUInt(std::vector<uint8_t>::iterator it)
> + * \brief Read uint from byte vector, in little-endian order
> + * \tparam T Type of uint to read
> + * \param[in] it Iterator of byte vector to read from
> + *

No need to empty line

> + * \return The uint read from \a vec
> + */
> +
> +/**
> + * \fn template<typename T> IPADataSerializer<T>::serialize(
> + * 	T data,
> + * 	ControlSerializer *cs = nullptr)
> + * \brief Serialize an object into byte vector and fd vector
> + * \tparam T Type of object to serialize
> + * \param[in] data Object to serialize
> + * \param[in] cs ControlSerializer
> + *
> + * \a cs is only necessary if the object type \a T or its members contain
> + * ControlList or ControlInfoMap.
> + *
> + * \return Tuple of byte vector and fd vector, that is the serialized form
> + * of \a data

Maybe later, but I think for each template specialization, we want to
document the serialization format, in example that for vectors, the
length is the first 4 bytes and so on (even more for custom libcamera
data types)

> + */
> +
> +/**
> + * \fn template<typename T> IPADataSerializer<T>::deserialize(
> + * 	std::vector<uint8_t> &data,
> + * 	ControlSerializer *cs = nullptr)
> + * \brief Deserialize byte vector into an object
> + * \tparam T Type of object to deserialize to
> + * \param[in] data 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 FileDescriptor.
> + *
> + * \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>::iterator it1,
> + * 	std::vector<uint8_t>::iterator it2,
> + * 	ControlSerializer *cs = nullptr)
> + * \brief Deserialize byte vector into an object
> + * \tparam T Type of object to deserialize to
> + * \param[in] it1 Begin iterator of byte vector to deserialize from
> + * \param[in] it2 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 FileDescriptor.
> + *
> + * \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> &data,
> + * 	std::vector<int32_t> &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 FileDescriptor.
> + *
> + * \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>::iterator data_it1,
> + * 	std::vector<uint8_t>::iterator data_it2,
> + * 	std::vector<int32_t>::iterator fds_it1,
> + * 	std::vector<int32_t>::iterator fds_it2,
> + * 	ControlSerializer *cs = nullptr)
> + * \brief Deserialize byte vector and fd vector into an object
> + * \tparam T Type of object to deserialize to
> + * \param[in] data_it1 Begin iterator of byte vector to deserialize from
> + * \param[in] data_it2 End iterator of byte vector to deserialize from
> + * \param[in] fds_it1 Begin iterator of fd vector to deserialize from
> + * \param[in] fds_it2 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 FileDescriptor.
> + *
> + * \a cs is only necessary if the object type \a T or its members contain
> + * ControlList or ControlInfoMap.
> + *
> + * \return The deserialized object
> + */
> +

Puff.. pant.. a lot of code to review work here! Nice one Paul!

> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 07711b5f..190d7490 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -24,6 +24,7 @@ libcamera_sources = files([
>      'geometry.cpp',
>      'ipa_context_wrapper.cpp',
>      'ipa_controls.cpp',
> +    'ipa_data_serializer.cpp',
>      'ipa_interface.cpp',
>      'ipa_manager.cpp',
>      'ipa_module.cpp',
> --
> 2.27.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Jacopo,

Thank you for the thorough review!

On Mon, Oct 26, 2020 at 05:07:52PM +0100, Jacopo Mondi wrote:
> Hi Paul,
> 
> On Fri, Oct 02, 2020 at 11:31:26PM +0900, Paul Elder wrote:
> > Add an IPADataSerializer which implments de/serialization of built-in
> > (PODs, vector, map, string) and libcamera data structures. This is
> > intended to be used by the proxy and the proxy worker in the IPC layer.
> >
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> >
> > ---
> > Changes in v3:
> > - reimplement append/readUInt with memcpy (intead of bit shifting)
> > - change DECLARE_INTEGRAL_SERIALIZER with DECLARE_POD_SERIALIZER
> >   - use this for int64_t, bool, float, and double
> > - fix comment style
> >
> > Changes in v2:
> > - added serializers for all integer types, bool, and string
> > - use string serializer for IPASettings serializer
> > - add documentation
> > - add serializer for const ControlList
> > ---
> >  .../libcamera/internal/ipa_data_serializer.h  | 1014 +++++++++++++++++
> >  src/libcamera/ipa_data_serializer.cpp         |  154 +++
> >  src/libcamera/meson.build                     |    1 +
> >  3 files changed, 1169 insertions(+)
> >  create mode 100644 include/libcamera/internal/ipa_data_serializer.h
> >  create mode 100644 src/libcamera/ipa_data_serializer.cpp
> >
> > diff --git a/include/libcamera/internal/ipa_data_serializer.h b/include/libcamera/internal/ipa_data_serializer.h
> > new file mode 100644
> > index 00000000..9cd35c4c
> > --- /dev/null
> > +++ b/include/libcamera/internal/ipa_data_serializer.h
> > @@ -0,0 +1,1014 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2020, Google Inc.
> > + *
> > + * ipa_data_serializer.h - Image Processing Algorithm data serializer
> > + */
> > +#ifndef __LIBCAMERA_INTERNAL_IPA_DATA_SERIALIZER_H__
> > +#define __LIBCAMERA_INTERNAL_IPA_DATA_SERIALIZER_H__
> > +
> > +#include <deque>
> > +#include <iostream>
> > +#include <string.h>
> > +#include <tuple>
> > +#include <vector>
> > +
> > +#include <libcamera/buffer.h>
> > +#include <libcamera/control_ids.h>
> > +#include <libcamera/geometry.h>
> > +#include <libcamera/ipa/ipa_interface.h>
> > +
> > +#include "libcamera/internal/byte_stream_buffer.h"
> > +#include "libcamera/internal/camera_sensor.h"
> > +#include "libcamera/internal/control_serializer.h"
> > +#include "libcamera/internal/log.h"
> > +
> > +namespace libcamera {
> > +
> > +LOG_DECLARE_CATEGORY(IPADataSerializer)
> > +
> 
> should these be wrapped in an anonymous namespace ? They are used by
> the generated proxyes, but the header is included. I tried it locally
> and complier seems to digest it

Ah yes, probably.

> > +template<typename T>
> > +void appendUInt(std::vector<uint8_t> &vec, T val)
> > +{
> > +	size_t byteWidth = sizeof(val);
> > +	std::vector<uint8_t> v(byteWidth);
> > +	memcpy(&v[0], &val, byteWidth);
> > +
> > +	vec.insert(vec.end(), v.begin(), v.end());
> > +}
> > +
> > +template<typename T>
> > +T readUInt(std::vector<uint8_t> &vec, size_t pos)
> > +{
> > +	T ret = 0;
> > +	size_t byteWidth = sizeof(ret);
> > +	if (pos + byteWidth > vec.size())
> > +		return ret;
> > +
> > +	memcpy(&ret, &vec.data()[pos], byteWidth);
> 
> Just &vec[pos] should be enough

It's fine even for memcpying?

> > +	return ret;
> > +}
> > +
> > +template<typename T>
> > +T readUInt(std::vector<uint8_t>::iterator it)
> > +{
> > +	T ret = 0;
> > +	size_t byteWidth = sizeof(ret);
> > +
> > +	std::vector<uint8_t> v(it, it + byteWidth);
> > +	memcpy(&ret, v.data(), byteWidth);
> > +	return ret;
> > +}
> 
> These new versions looks much better!

\o/

Ah, so later on you're saying that I should rename these, since they're
not limited to uints?

> > +
> > +template<typename T>
> > +class IPADataSerializer
> > +{
> > +#ifdef __DOXYGEN__
> > +public:
> > +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> > +	serialize(T data, ControlSerializer *cs);
> > +
> > +	static T deserialize(std::vector<uint8_t> &data,
> > +			     ControlSerializer *cs);
> > +	static T deserialize(std::vector<uint8_t>::iterator it1,
> > +			     std::vector<uint8_t>::iterator it2,
> > +			     ControlSerializer *cs);
> > +
> 
> Intentional empty line ?

Yes, the first two are data only, and the bottom two are data+fd.

> > +	static T deserialize(std::vector<uint8_t> &data,
> > +			     std::vector<int32_t> &fds,
> > +			     ControlSerializer *cs);
> > +	static T deserialize(std::vector<uint8_t>::iterator data_it1,
> > +			     std::vector<uint8_t>::iterator data_it2,
> > +			     std::vector<int32_t>::iterator fds_it1,
> > +			     std::vector<int32_t>::iterator fds_it2,
> > +			     ControlSerializer *cs);
> 
> Aren't these better called data_start, data_end and fds_start, fds_end
> ?

Yeah, that's better.

> > +#endif /* __DOXYGEN__ */
> 
> Do you need this ifdef __DOXYGEN__ ? you want these parsed, don't you ?
> It's usually #ifndef __DOXYGEN__ to exclude from documentation parsing
> some parts of the code, like you're done below

I do need this ifdef. I want doxygen to document just the functions in
the base serializer, since it's the same interface for all serializers,
but I don't want the base serializer template to be compiled.

> > +};
> > +
> > +#ifndef __DOXYGEN__
> > +
> > +template<typename V>
> > +class IPADataSerializer<std::vector<V>>
> > +{
> > +public:
> > +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> > +	serialize(const std::vector<V> &data, ControlSerializer *cs = nullptr)
> > +	{
> > +		std::vector<uint8_t> data_vec;
> 
> We can reserve data.size() * sizeof(V) in the vector

I'm not sure we can do that...

> > +		std::vector<int32_t> fds_vec;
> > +
> > +		/* Serialize the length. */
> > +		uint32_t vec_len = data.size();
> > +		appendUInt<uint32_t>(data_vec, vec_len);
> > +
> > +		/* Serialize the members. */
> > +		for (auto it = data.begin(); it != data.end(); ++it) {
> 
>                 for (auto const &it : data) ?

ack

> > +			std::vector<uint8_t> dvec;
> > +			std::vector<int32_t> fvec;
> > +
> > +			std::tie(dvec, fvec) =
> > +				IPADataSerializer<V>::serialize(*it, cs);
> > +
> > +			appendUInt<uint32_t>(data_vec, dvec.size());
> 
> Why is the size (in bytes) of each serialized entry recorded in the
> serialized data buffer ? Aren't all the members of the same size ?

...because the members of the vector aren't necessarily the same size.
The vector could contains variable-size objects, like other vectors, or
maps, or strings, etc, so we need to tag the size of every element.

> > +			appendUInt<uint32_t>(data_vec, fvec.size());
> 
> Same question, more relevant as FileDescriptors are known to be
> uin32_t

Same reason, the elements of the vector could contain a variable number
of FileDescriptors.

> > +
> > +			data_vec.insert(data_vec.end(), dvec.begin(), dvec.end());
> > +			fds_vec.insert(fds_vec.end(), fvec.begin(), fvec.end());
> > +		}
> > +
> > +		return {data_vec, fds_vec};
> > +	}
> > +
> > +	static std::vector<V> deserialize(std::vector<uint8_t> &data, ControlSerializer *cs = nullptr)
> > +	{
> > +		return IPADataSerializer<std::vector<V>>::deserialize(data.begin(), data.end(), cs);
> 
> question: do you need to fully qualify the deserializer() function
> call ? I tried removing "IPADataSerializer<std::vector<V>>::" and
> compiler is still happy

Oh, maybe not. I remember I was having some confusion about which
template function gets called without the full qualification.

> > +	}
> > +
> > +	static std::vector<V> deserialize(std::vector<uint8_t>::iterator it1,
> > +					  std::vector<uint8_t>::iterator it2,
> > +					  ControlSerializer *cs = nullptr)
> > +	{
> > +		std::vector<int32_t> fds;
> > +		return IPADataSerializer<std::vector<V>>::deserialize(it1, it2,
> > +								      fds.begin(), fds.end(),
> > +								      cs);
> > +	}
> > +
> > +	static std::vector<V> deserialize(std::vector<uint8_t> &data, std::vector<int32_t> &fds,
> > +					  ControlSerializer *cs = nullptr)
> > +	{
> > +		return IPADataSerializer<std::vector<V>>::deserialize(data.begin(), data.end(),
> > +								      fds.begin(), fds.end(),
> > +								      cs);
> > +	}
> > +
> > +	static std::vector<V> deserialize(std::vector<uint8_t>::iterator data_it1,
> > +					  [[maybe_unused]] std::vector<uint8_t>::iterator data_it2,
> > +					  std::vector<int32_t>::iterator fds_it1,
> > +					  [[maybe_unused]] std::vector<int32_t>::iterator fds_it2,
> > +					  ControlSerializer *cs = nullptr)
> > +	{
> > +		uint32_t vec_len = readUInt<uint32_t>(data_it1);
> > +		std::vector<V> ret(vec_len);
> > +
> > +		std::vector<uint8_t>::iterator data_it = data_it1 + 4;
> > +		std::vector<int32_t>::iterator fd_it = fds_it1;
> > +		for (uint32_t i = 0; i < vec_len; i++) {
> > +			uint32_t sizeof_data = readUInt<uint32_t>(data_it);
> 
> Same questions as above, do we need to insert the entry size in the
> serialized buffer, or can it just be assumed to be sizeof(V) ?

Same answer as above, sizeof(V) might not be constant.

> > +			uint32_t sizeof_fds  = readUInt<uint32_t>(data_it + 4);
> 
> And this one to be 4 bytes ?
> 
> > +
> > +			ret[i] = IPADataSerializer<V>::deserialize(data_it + 8,
> > +								   data_it + 8 + sizeof_data,
> > +								   fd_it,
> > +								   fd_it + sizeof_fds,
> > +								   cs);
> > +
> > +			data_it += 8 + sizeof_data;
> > +			fd_it += sizeof_fds;
> > +		}
> > +
> > +		return ret;
> > +	}
> > +};
> > +
> > +template<typename K, typename V>
> > +class IPADataSerializer<std::map<K, V>>
> > +{
> > +public:
> > +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> > +	serialize(const std::map<K, V> &data, ControlSerializer *cs = nullptr)
> > +	{
> > +		std::vector<uint8_t> data_vec;
> > +		std::vector<int32_t> fds_vec;
> > +
> > +		/* Serialize the length. */
> > +		uint32_t map_len = data.size();
> > +		appendUInt<uint32_t>(data_vec, map_len);
> > +
> > +		/* Serialize the members. */
> > +		for (auto it = data.begin(); it != data.end(); ++it) {
> 
> Same comment: a range loop might be nicer
>                 for (auto const &it : data)

ack

> > +			std::vector<uint8_t> dvec;
> > +			std::vector<int32_t> fvec;
> > +
> > +			std::tie(dvec, fvec) =
> > +				IPADataSerializer<K>::serialize(it->first, cs);
> > +
> > +			appendUInt<uint32_t>(data_vec, dvec.size());
> > +			appendUInt<uint32_t>(data_vec, fvec.size());
> > +
> > +			data_vec.insert(data_vec.end(), dvec.begin(), dvec.end());
> > +			fds_vec.insert(fds_vec.end(), fvec.begin(), fvec.end());
> > +
> > +			std::tie(dvec, fvec) =
> > +				IPADataSerializer<V>::serialize(it->second, cs);
> > +
> > +			appendUInt<uint32_t>(data_vec, dvec.size());
> > +			appendUInt<uint32_t>(data_vec, fvec.size());
> > +
> > +			data_vec.insert(data_vec.end(), dvec.begin(), dvec.end());
> > +			fds_vec.insert(fds_vec.end(), fvec.begin(), fvec.end());
> > +		}
> > +
> > +		return {data_vec, fds_vec};
> > +	}
> > +
> > +	static std::map<K, V> deserialize(std::vector<uint8_t> &data, ControlSerializer *cs = nullptr)
> > +	{
> > +		return IPADataSerializer<std::map<K, V>>::deserialize(data.begin(), data.end(), cs);
> > +	}
> > +
> > +	static std::map<K, V> deserialize(std::vector<uint8_t>::iterator it1,
> > +					  std::vector<uint8_t>::iterator it2,
> > +					  ControlSerializer *cs = nullptr)
> > +	{
> > +		std::vector<int32_t> fds;
> > +		return IPADataSerializer<std::map<K, V>>::deserialize(it1, it2,
> > +								      fds.begin(), fds.end(),
> > +								      cs);
> > +	}
> > +
> > +	static std::map<K, V> deserialize(std::vector<uint8_t> &data, std::vector<int32_t> &fds,
> > +					  ControlSerializer *cs = nullptr)
> > +	{
> > +		return IPADataSerializer<std::map<K, V>>::deserialize(data.begin(), data.end(),
> > +								      fds.begin(), fds.end(),
> > +								      cs);
> > +	}
> 
> Same comments on the fully qualified path

ack

> > +
> > +	static std::map<K, V> deserialize(std::vector<uint8_t>::iterator data_it1,
> > +					  [[maybe_unused]] std::vector<uint8_t>::iterator data_it2,
> > +					  std::vector<int32_t>::iterator fds_it1,
> > +					  [[maybe_unused]] std::vector<int32_t>::iterator fds_it2,
> > +					  ControlSerializer *cs = nullptr)
> > +	{
> > +		std::map<K, V> ret;
> > +
> > +		uint32_t map_len = readUInt<uint32_t>(data_it1);
> > +
> > +		std::vector<uint8_t>::iterator data_it = data_it1 + 4;
> > +		std::vector<int32_t>::iterator fd_it = fds_it1;
> > +		for (uint32_t i = 0; i < map_len; i++) {
> > +			uint32_t sizeof_data = readUInt<uint32_t>(data_it);
> > +			uint32_t sizeof_fds  = readUInt<uint32_t>(data_it + 4);
> > +
> > +			K key = IPADataSerializer<K>::deserialize(data_it + 8,
> > +								  data_it + 8 + sizeof_data,
> > +								  fd_it,
> > +								  fd_it + sizeof_fds,
> > +								  cs);
> > +
> > +			data_it += 8 + sizeof_data;
> > +			fd_it += sizeof_fds;
> > +			sizeof_data = readUInt<uint32_t>(data_it);
> > +			sizeof_fds  = readUInt<uint32_t>(data_it + 4);
> > +
> > +			const V value = IPADataSerializer<V>::deserialize(data_it + 8,
> > +									  data_it + 8 + sizeof_data,
> > +									  fd_it,
> > +									  fd_it + sizeof_fds,
> > +									  cs);
> > +			ret.insert({key, value});
> > +
> > +			data_it += 8 + sizeof_data;
> > +			fd_it += sizeof_fds;
> > +		}
> > +
> > +		return ret;
> > +	}
> > +};
> > +
> > +#define DECLARE_POD_SERIALIZER(type)					\
> > +template<>								\
> > +class IPADataSerializer<type>						\
> > +{									\
> > +public:									\
> > +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>	\
> > +	serialize(const type data,					\
> > +		  [[maybe_unused]] ControlSerializer *cs = nullptr)	\
> > +	{								\
> > +		std::vector<uint8_t> data_vec;				\
> 
> Do you think pre-allocating sizeof(type) in the vector might help ?

tbh, yeah.

> > +		appendUInt<type>(data_vec, data);			\

Oh, then this needs a writeUInt cousin.

> > +									\
> > +		return {data_vec, {}};					\
> > +	}								\
> > +									\
> > +	static type deserialize(std::vector<uint8_t> &data,		\
> > +				[[maybe_unused]] ControlSerializer *cs = nullptr)\
> > +	{								\
> > +		return IPADataSerializer<type>::deserialize(data.begin(),\
> > +							    data.end());\
> > +	}								\
> > +									\
> > +	static type deserialize(std::vector<uint8_t>::iterator it1,	\
> > +				[[maybe_unused]] std::vector<uint8_t>::iterator it2,\
> > +				[[maybe_unused]] ControlSerializer *cs = nullptr)\
> > +	{								\
> > +		return readUInt<type>(it1);				\
> > +	}								\
> > +									\
> > +	static type deserialize(std::vector<uint8_t> &data,		\
> > +				[[maybe_unused]] std::vector<int32_t> &fds,\
> > +				[[maybe_unused]] ControlSerializer *cs = nullptr)\
> > +	{								\
> > +		return IPADataSerializer<type>::deserialize(data.begin(),\
> > +							    data.end());\
> > +	}								\
> > +									\
> > +	static type deserialize(std::vector<uint8_t>::iterator data_it1,\
> > +				std::vector<uint8_t>::iterator data_it2,\
> > +				[[maybe_unused]] std::vector<int32_t>::iterator fds_it1,\
> > +				[[maybe_unused]] std::vector<int32_t>::iterator fds_it2,\
> > +				[[maybe_unused]] ControlSerializer *cs = nullptr)\
> > +	{								\
> > +		return IPADataSerializer<type>::deserialize(data_it1,	\
> > +							    data_it2);	\
> > +	}								\
> > +};
> > +
> > +DECLARE_POD_SERIALIZER(bool)
> > +DECLARE_POD_SERIALIZER(uint8_t)
> > +DECLARE_POD_SERIALIZER(uint16_t)
> > +DECLARE_POD_SERIALIZER(uint32_t)
> > +DECLARE_POD_SERIALIZER(uint64_t)
> > +DECLARE_POD_SERIALIZER(int8_t)
> > +DECLARE_POD_SERIALIZER(int16_t)
> > +DECLARE_POD_SERIALIZER(int32_t)
> > +DECLARE_POD_SERIALIZER(int64_t)
> 
> The I wonder if keeping here and in the documentation the term 'uint'
> is correct and it shouldn't just be replaced with 'int'

I'm guessing you're referring to {append,read}UInt()?

> > +DECLARE_POD_SERIALIZER(float)
> > +DECLARE_POD_SERIALIZER(double)
> 
> Or even POD

I'm not sure what you're referring to here :/

> > +
> > +template<>
> > +class IPADataSerializer<std::string>
> > +{
> > +public:
> > +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> > +	serialize(const std::string &data, [[maybe_unused]] ControlSerializer *cs = nullptr)
> > +	{
> > +		std::vector<uint8_t> data_vec(data.begin(), data.end());
> > +
> > +		return {data_vec, {}};
> > +	}
> > +
> > +	static std::string deserialize(std::vector<uint8_t> &data,
> > +				       [[maybe_unused]] ControlSerializer *cs = nullptr)
> > +	{
> > +		return IPADataSerializer<std::string>::deserialize(data.begin(), data.end());
> > +	}
> > +
> > +	static std::string deserialize(std::vector<uint8_t>::iterator it1,
> > +				       std::vector<uint8_t>::iterator it2,
> > +				       [[maybe_unused]] ControlSerializer *cs = nullptr)
> > +	{
> > +		std::string str(it1, it2);
> > +
> > +		return str;
> 
> Can you just
>                 return {it1, it2};
> ?

Ah, yes. I had it (and everything below) implemented in this manner
because I wanted to algorithmize the serdes code generation, so I was
practicing it on myself here. I guess it's about time for optimizations;
thank you for the pointers.

> > +	}
> > +
> > +	static std::string deserialize(std::vector<uint8_t> &data,
> > +				       [[maybe_unused]] std::vector<int32_t> &fds,
> > +				       [[maybe_unused]] ControlSerializer *cs = nullptr)
> > +	{
> > +		return IPADataSerializer<std::string>::deserialize(data.begin(), data.end());
> 
> Maybe
>                 return {data.being(), data.end()};
> 
> would save a function call ?
> Same for other overloaded methods (and possibly not only in the
> <string> specialization).
> 

ack

> > +	}
> > +
> > +	static std::string deserialize(std::vector<uint8_t>::iterator data_it1,
> > +				       std::vector<uint8_t>::iterator data_it2,
> > +				       [[maybe_unused]] std::vector<int32_t>::iterator fds_it1,
> > +				       [[maybe_unused]] std::vector<int32_t>::iterator fds_it2,
> > +				       [[maybe_unused]] ControlSerializer *cs = nullptr)
> > +	{
> > +		return IPADataSerializer<std::string>::deserialize(data_it1, data_it2);
> > +	}
> > +};
> > +
> > +template<>
> > +class IPADataSerializer<FileDescriptor>
> > +{
> > +public:
> > +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> > +	serialize(const FileDescriptor &data, [[maybe_unused]] ControlSerializer *cs = nullptr)
> > +	{
> > +		std::vector<uint8_t> data_vec = { data.isValid() };
> > +		std::vector<int32_t> fd_vec;
> > +		if (data.isValid())
> > +			fd_vec.push_back(data.fd());
> > +
> > +		return {data_vec, fd_vec};
> > +	}
> > +
> > +	static FileDescriptor deserialize(std::vector<uint8_t> &data, std::vector<int32_t> &fds,
> > +					  [[maybe_unused]] ControlSerializer *cs = nullptr)
> > +	{
> > +		return IPADataSerializer<FileDescriptor>::deserialize(data.begin(), data.end(),
> > +								      fds.begin(), fds.end());
> > +	}
> > +
> > +	static FileDescriptor deserialize(std::vector<uint8_t>::iterator data_it1,
> > +					  std::vector<uint8_t>::iterator data_it2,
> > +					  std::vector<int32_t>::iterator fds_it1,
> > +					  std::vector<int32_t>::iterator fds_it2,
> > +					  [[maybe_unused]] ControlSerializer *cs = nullptr)
> > +	{
> > +		if (std::distance(data_it1, data_it2) < 1)
> > +			LOG(IPADataSerializer, Fatal)
> > +				<< "Invalid data to deserialize FileDescriptor";
> > +
> > +		bool valid = *data_it1;
> 
> in C this would be
>                 bool valid = !!*data_it1;
> not sure about C++ and not even sure if the !! is just a paranoid
> kernel convention, as I don't think even C requires that

Oh... I'm not sure. It passed my de/serialization tests.

> > +
> > +		if (valid && std::distance(fds_it1, fds_it2) < 1)
> 
> 		if (valid && std::distance(fds_it1, fds_it2) < 1) {
> 			LOG(IPADataSerializer, Fatal)
> 				<< "Invalid fds to deserialize FileDescriptor";
>                         return {}
>                 }
> 
>                 return FileDescriptor(*fds_it1);
> 
> Or is returning {} when std::distance() < 1 wrong ? I don't think so,
> as even if it is "valid" there's not fds serialized.

If std::distance() < 1 then *fds_it1 will segfault. So if valid and
std::distance() < 1, then there's a big problem -> fatal. If not valid,
then std::distance() doesn't matter, and we return a new uninitialized
FileDescriptor, but we can't construct it using *fds_it1, since the
iterator shouldn't be valid.

It's perfectly fine to send an invalid FileDescriptor, for example if
the FileDescriptor field isn't used, or uninitialized. The problem with
sending it directly is that sendmsg() EINVALs if we try to send -1, so
we need this valid flag in the data vector.

> 
> > +			LOG(IPADataSerializer, Fatal)
> > +				<< "Invalid fds to deserialize FileDescriptor";
> > +
> > +		return valid ? FileDescriptor(*fds_it1) : FileDescriptor();
> > +	}
> > +};
> > +
> > +template<>
> > +class IPADataSerializer<IPASettings>
> > +{
> > +public:
> > +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> > +	serialize(const IPASettings &data, [[maybe_unused]] ControlSerializer *cs = nullptr)
> > +	{
> > +		return IPADataSerializer<std::string>::serialize(data.configurationFile);
> > +	}
> > +
> > +	static IPASettings deserialize(std::vector<uint8_t> &data,
> > +				       [[maybe_unused]] ControlSerializer *cs = nullptr)
> > +	{
> > +		return IPADataSerializer<IPASettings>::deserialize(data.begin(), data.end());
> > +	}
> > +
> > +	static IPASettings deserialize(std::vector<uint8_t>::iterator it1,
> > +				       std::vector<uint8_t>::iterator it2,
> > +				       [[maybe_unused]] ControlSerializer *cs = nullptr)
> > +	{
> > +		IPASettings ret;
> > +		ret.configurationFile = IPADataSerializer<std::string>::deserialize(it1, it2);
> > +
> > +		return ret;
> 
>                 return { IPADataSerializer<std::string>::deserialize(it1, it2) };
> 
> ?

Ah yes, much needed optimization.

> > +	}
> > +
> > +	static IPASettings deserialize(std::vector<uint8_t> &data,
> > +				       [[maybe_unused]] std::vector<int32_t> &fds,
> > +				       [[maybe_unused]] ControlSerializer *cs = nullptr)
> > +	{
> > +		return IPADataSerializer<IPASettings>::deserialize(data.begin(), data.end());
> 
>                 return { IPADataSerializer<std::string>::deserialize(data.begin(), data.end() };
> 
> ?

ack

> > +	}
> > +
> > +	static IPASettings deserialize(std::vector<uint8_t>::iterator data_it1,
> > +				       std::vector<uint8_t>::iterator data_it2,
> > +				       [[maybe_unused]] std::vector<int32_t>::iterator fds_it1,
> > +				       [[maybe_unused]] std::vector<int32_t>::iterator fds_it2,
> > +				       [[maybe_unused]] ControlSerializer *cs = nullptr)
> > +	{
> > +		return IPADataSerializer<IPASettings>::deserialize(data_it1, data_it2);
> 
> same

ack

> > +	}
> > +};
> > +
> > +template<>
> > +class IPADataSerializer<CameraSensorInfo>
> > +{
> > +public:
> > +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> > +	serialize(const CameraSensorInfo &data, [[maybe_unused]] ControlSerializer *cs = nullptr)
> > +	{
> > +		std::vector<uint8_t> data_vec;
> 
> Reserving sizeof(CameraSensorInfo) might help

It's got a string in it though, so we don't know sizeof(CameraSensorInfo).

> I just noticed and I wonder why all this code uses this_naming_style
> in place of the libcamera standard namingStyle

Ah yes, I should probably fix that...

> > +
> > +		uint32_t str_len = data.model.size();
> > +		appendUInt<uint32_t>(data_vec, str_len);
> > +
> > +		data_vec.insert(data_vec.end(), data.model.begin(), data.model.end());
> 
> Shouldn't this be serialized using the IPADataSerializer<std::string>
> if it's not needed, do we need the specialization then ?

It should be :)

> > +
> > +		appendUInt<uint32_t>(data_vec, data.bitsPerPixel);
> > +
> > +		appendUInt<uint32_t>(data_vec, data.activeAreaSize.width);
> > +		appendUInt<uint32_t>(data_vec, data.activeAreaSize.height);
> > +
> > +		appendUInt<uint32_t>(data_vec, static_cast<uint32_t>(data.analogCrop.x));
> > +		appendUInt<uint32_t>(data_vec, static_cast<uint32_t>(data.analogCrop.y));
> > +		appendUInt<uint32_t>(data_vec, data.analogCrop.width);
> > +		appendUInt<uint32_t>(data_vec, data.analogCrop.height);
> > +
> > +		appendUInt<uint32_t>(data_vec, data.outputSize.width);
> > +		appendUInt<uint32_t>(data_vec, data.outputSize.height);
> > +
> > +		appendUInt<uint64_t>(data_vec, data.pixelRate);
> > +
> > +		appendUInt<uint32_t>(data_vec, data.lineLength);
> > +
> > +		return {data_vec, {}};
> > +	}
> > +
> > +	static CameraSensorInfo deserialize(std::vector<uint8_t> &data,
> > +					    [[maybe_unused]] ControlSerializer *cs = nullptr)
> > +	{
> > +		return IPADataSerializer<CameraSensorInfo>::deserialize(data.begin(), data.end());
> > +	}
> > +
> > +	static CameraSensorInfo deserialize(std::vector<uint8_t>::iterator it1,
> > +					    [[maybe_unused]] std::vector<uint8_t>::iterator it2,
> > +					    [[maybe_unused]] ControlSerializer *cs = nullptr)
> > +	{
> > +		CameraSensorInfo ret;
>                 CameraSensorInfo ret{};
> 
> Also for other statically allocated fields that are returned by other
> specializations.

Do we need to, since all the fields are populated below?

> > +
> > +		uint32_t str_len = readUInt<uint32_t>(it1);
> > +		std::string str(it1 + 4, it1 + 4 + str_len);
> > +		ret.model = str;
> > +
> > +		std::vector<uint8_t>::iterator it = it1 + 4 + str_len;
> > +
> > +		ret.bitsPerPixel = readUInt<uint32_t>(it);
> > +
> > +		ret.activeAreaSize.width = readUInt<uint32_t>(it + 4);
> > +		ret.activeAreaSize.height = readUInt<uint32_t>(it + 8);
> > +
> > +		ret.analogCrop.x = static_cast<int32_t>(readUInt<uint32_t>(it + 12));
> > +		ret.analogCrop.y = static_cast<int32_t>(readUInt<uint32_t>(it + 16));
> > +		ret.analogCrop.width = readUInt<uint32_t>(it + 20);
> > +		ret.analogCrop.height = readUInt<uint32_t>(it + 24);
> > +
> > +		ret.outputSize.width = readUInt<uint32_t>(it + 28);
> > +		ret.outputSize.height = readUInt<uint32_t>(it + 32);
> > +
> > +		ret.pixelRate = readUInt<uint64_t>(it + 36);
> > +
> > +		ret.lineLength = readUInt<uint64_t>(it + 44);
> 
> lineLength is a 32 bits integer

Ah, yes.

> > +
> > +		return ret;
> > +	}
> > +
> > +	static CameraSensorInfo deserialize(std::vector<uint8_t> &data,
> > +					    [[maybe_unused]] std::vector<int32_t> &fds,
> > +					    [[maybe_unused]] ControlSerializer *cs = nullptr)
> > +	{
> > +		return IPADataSerializer<CameraSensorInfo>::deserialize(data.begin(), data.end());
> > +	}
> > +
> > +	static CameraSensorInfo deserialize(std::vector<uint8_t>::iterator data_it1,
> > +					    std::vector<uint8_t>::iterator data_it2,
> > +					    [[maybe_unused]] std::vector<int32_t>::iterator fds_it1,
> > +					    [[maybe_unused]] std::vector<int32_t>::iterator fds_it2,
> > +					    [[maybe_unused]] ControlSerializer *cs = nullptr)
> > +	{
> > +		return IPADataSerializer<CameraSensorInfo>::deserialize(data_it1, data_it2);
> > +	}
> > +};
> > +
> > +template<>
> > +class IPADataSerializer<IPAStream>
> > +{
> > +public:
> > +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> > +	serialize(const IPAStream &data, [[maybe_unused]] ControlSerializer *cs = nullptr)
> > +	{
> > +		std::vector<uint8_t> data_vec;
> > +
> > +		appendUInt<uint32_t>(data_vec, data.pixelFormat);
> > +
> > +		appendUInt<uint32_t>(data_vec, data.size.width);
> > +		appendUInt<uint32_t>(data_vec, data.size.height);
> > +
> > +		return {data_vec, {}};
> > +	}
> > +
> > +	static IPAStream deserialize(std::vector<uint8_t> &data,
> > +				     [[maybe_unused]] ControlSerializer *cs = nullptr)
> > +	{
> > +		return IPADataSerializer<IPAStream>::deserialize(data.begin(), data.end());
> > +	}
> > +
> > +	static IPAStream deserialize(std::vector<uint8_t>::iterator it1,
> > +				     [[maybe_unused]] std::vector<uint8_t>::iterator it2,
> > +				     [[maybe_unused]] ControlSerializer *cs = nullptr)
> > +	{
> > +		IPAStream ret;
> > +
> > +		ret.pixelFormat = readUInt<uint32_t>(it1);
> > +
> > +		ret.size.width = readUInt<uint32_t>(it1 + 4);
> > +		ret.size.height = readUInt<uint32_t>(it1 + 8);
> > +
> > +		return ret;
> > +	}
> > +
> > +	static IPAStream deserialize(std::vector<uint8_t> &data,
> > +				     [[maybe_unused]] std::vector<int32_t> &fds,
> > +				     [[maybe_unused]] ControlSerializer *cs = nullptr)
> > +	{
> > +		return IPADataSerializer<IPAStream>::deserialize(data.begin(), data.end());
> > +	}
> > +
> > +	static IPAStream deserialize(std::vector<uint8_t>::iterator data_it1,
> > +				     std::vector<uint8_t>::iterator data_it2,
> > +				     [[maybe_unused]] std::vector<int32_t>::iterator fds_it1,
> > +				     [[maybe_unused]] std::vector<int32_t>::iterator fds_it2,
> > +				     [[maybe_unused]] ControlSerializer *cs = nullptr)
> > +	{
> > +		return IPADataSerializer<IPAStream>::deserialize(data_it1, data_it2);
> > +	}
> > +};
> > +
> > +template<>
> > +class IPADataSerializer<const ControlList>
> > +{
> > +public:
> > +	/* map arg will be generated, since it's per-pipeline anyway. */
> > +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> > +	serialize(const ControlList &data, const ControlInfoMap &map,
> > +		  ControlSerializer *cs)
> > +	{
> > +		if (!cs)
> > +			LOG(IPADataSerializer, Fatal)
> > +				<< "ControlSerializer not provided for serialization of ControlList";
> > +
> > +		size_t size = cs->binarySize(map);
> > +		std::vector<uint8_t> infoData(size);
> > +		ByteStreamBuffer buffer(infoData.data(), infoData.size());
> > +		int ret = cs->serialize(map, buffer);
> > +
> > +		if (ret < 0 || buffer.overflow()) {
> > +			LOG(IPADataSerializer, Error) << "Failed to serialize ControlList's ControlInfoMap";
> > +			return {{}, {}};
> > +		}
> > +
> > +		size = cs->binarySize(data);
> > +		std::vector<uint8_t> listData(size);
> > +		buffer = ByteStreamBuffer(listData.data(), listData.size());
> > +		ret = cs->serialize(data, buffer);
> > +
> > +		if (ret < 0 || buffer.overflow()) {
> > +			LOG(IPADataSerializer, Error) << "Failed to serialize ControlList";
> > +			return {{}, {}};
> > +		}
> > +
> > +		std::vector<uint8_t> data_vec;
> 
> You know the sizes and can reserve ?

At this point. yes.

> > +		appendUInt<uint32_t>(data_vec, infoData.size());
> > +		appendUInt<uint32_t>(data_vec, listData.size());
> > +		data_vec.insert(data_vec.end(), infoData.begin(), infoData.end());
> > +		data_vec.insert(data_vec.end(), listData.begin(), listData.end());
> > +
> > +		return {data_vec, {}};
> > +	}
> > +
> > +	static const ControlList deserialize(std::vector<uint8_t> &data, ControlSerializer *cs)
> > +	{
> > +		return IPADataSerializer<const ControlList>::deserialize(data.begin(), data.end(), cs);
> > +	}
> > +
> > +	static const ControlList deserialize(std::vector<uint8_t>::iterator it1,
> > +				       [[maybe_unused]] std::vector<uint8_t>::iterator it2,
> > +				       ControlSerializer *cs)
> > +	{
> > +		if (!cs)
> > +			LOG(IPADataSerializer, Fatal)
> > +				<< "ControlSerializer not provided for deserialization of ControlList";
> > +
> > +		uint32_t infoDataSize = readUInt<uint32_t>(it1);
> > +		uint32_t listDataSize = readUInt<uint32_t>(it1 + 4);
> > +
> > +		std::vector<uint8_t>::iterator it = it1 + 8;
> > +
> > +		std::vector<uint8_t> infoData(it, it + infoDataSize);
> > +		std::vector<uint8_t> listData(it + infoDataSize, it + infoDataSize + listDataSize);
> > +
> > +		ByteStreamBuffer buffer(const_cast<const uint8_t *>(infoData.data()), infoData.size());
> > +		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();
> > +		}
> > +
> > +		buffer = ByteStreamBuffer(const_cast<const uint8_t *>(listData.data()), listData.size());
> > +		ControlList list = cs->deserialize<ControlList>(buffer);
> > +		if (buffer.overflow())
> > +			LOG(IPADataSerializer, Error) << "Failed to deserialize ControlList: buffer overflow";
> > +		if (list.empty())
> > +			LOG(IPADataSerializer, Error) << "Failed to deserialize ControlList: empty list";
> 
> Should we return {} in these cases or is it fine ?

ControlSerializer::deserialize() returns {} in these cases anyway, so it
should be fine.

> > +
> > +		return list;
> > +	}
> > +
> > +	static const ControlList deserialize(std::vector<uint8_t> &data,
> > +				       [[maybe_unused]] std::vector<int32_t> &fds,
> > +				       ControlSerializer *cs)
> > +	{
> > +		return IPADataSerializer<const ControlList>::deserialize(data.begin(), data.end(), cs);
> > +	}
> > +
> > +	static const ControlList deserialize(std::vector<uint8_t>::iterator data_it1,
> > +				       std::vector<uint8_t>::iterator data_it2,
> > +				       [[maybe_unused]] std::vector<int32_t>::iterator fds_it1,
> > +				       [[maybe_unused]] std::vector<int32_t>::iterator fds_it2,
> > +				       ControlSerializer *cs)
> > +	{
> > +		return IPADataSerializer<const ControlList>::deserialize(data_it1, data_it2, cs);
> > +	}
> > +};
> > +
> > +template<>
> > +class IPADataSerializer<ControlList>
> > +{
> > +public:
> > +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> > +	serialize(const ControlList &data, const ControlInfoMap &map,
> > +		  ControlSerializer *cs)
> > +	{
> > +		const ControlList &list_const = const_cast<const ControlList &>(data);
> > +
> > +		std::vector<uint8_t> data_vec;
> > +		std::tie(data_vec, std::ignore) =
> > +			IPADataSerializer<const ControlList>::serialize(list_const, map, cs);
> > +
> > +		return {data_vec, {}};
> > +	}
> > +
> > +	static ControlList deserialize(std::vector<uint8_t> &data,
> > +				       ControlSerializer *cs)
> > +	{
> > +		ControlList ret;
> > +		const ControlList &list = ret;
> > +		const_cast<ControlList &>(list) =
> > +			IPADataSerializer<const ControlList>::deserialize(data, cs);
> > +
> > +		return ret;
> > +	}
> > +
> > +	static ControlList deserialize(std::vector<uint8_t>::iterator it1,
> > +				       [[maybe_unused]] std::vector<uint8_t>::iterator it2,
> > +				       ControlSerializer *cs)
> > +	{
> > +		ControlList ret;
> > +		const ControlList &list = ret;
> > +		const_cast<ControlList &>(list) =
> > +			IPADataSerializer<const ControlList>::deserialize(it1, it2, cs);
> 
> Why not
>                 const ControlList list =
> 			IPADataSerializer<const ControlList>::deserialize(it1, it2, cs);
>                 return const_cast<ControlList>(list)

My understanding is that to remove a const with const_cast, the original
variable must not be const.

> > +
> > +		return ret;
> > +	}
> > +
> > +	static ControlList deserialize(std::vector<uint8_t> &data,
> > +				       [[maybe_unused]] std::vector<int32_t> &fds,
> > +				       ControlSerializer *cs)
> > +	{
> > +		ControlList ret;
> > +		const ControlList &list = ret;
> > +		const_cast<ControlList &>(list) =
> > +			IPADataSerializer<const ControlList>::deserialize(data, fds, cs);
> > +
> > +		return ret;
> 
> same question
> 
> > +	}
> > +
> > +	static ControlList deserialize(std::vector<uint8_t>::iterator data_it1,
> > +				       std::vector<uint8_t>::iterator data_it2,
> > +				       [[maybe_unused]] std::vector<int32_t>::iterator fds_it1,
> > +				       [[maybe_unused]] std::vector<int32_t>::iterator fds_it2,
> > +				       ControlSerializer *cs)
> > +	{
> > +		ControlList ret;
> > +		const ControlList &list = ret;
> > +		const_cast<ControlList &>(list) =
> > +			IPADataSerializer<const ControlList>::deserialize(data_it1, data_it2,
> > +									  fds_it1, fds_it2, cs);
> 
> same question
> 
> > +
> > +		return ret;
> > +	}
> > +};
> > +
> > +template<>
> > +class IPADataSerializer<const ControlInfoMap>
> > +{
> > +public:
> > +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> > +	serialize(const ControlInfoMap &map, ControlSerializer *cs)
> > +	{
> > +		if (!cs)
> > +			LOG(IPADataSerializer, Fatal)
> > +				<< "ControlSerializer not provided for serialization of ControlInfoMap";
> > +
> > +		size_t size = cs->binarySize(map);
> > +		std::vector<uint8_t> infoData(size);
> > +		ByteStreamBuffer buffer(infoData.data(), infoData.size());
> > +		int ret = cs->serialize(map, buffer);
> > +
> > +		if (ret < 0 || buffer.overflow())
> > +			return {{}, {}};
> > +
> > +		std::vector<uint8_t> data_vec;
> > +		appendUInt<uint32_t>(data_vec, infoData.size());
> > +		data_vec.insert(data_vec.end(), infoData.begin(), infoData.end());
> > +
> > +		return {data_vec, {}};
> > +	}
> > +
> > +	static const ControlInfoMap deserialize(std::vector<uint8_t> &data,
> > +						ControlSerializer *cs)
> > +	{
> > +		return IPADataSerializer<const ControlInfoMap>::deserialize(data.begin(), data.end(), cs);
> > +	}
> > +
> > +	static const ControlInfoMap deserialize(std::vector<uint8_t>::iterator it1,
> > +						[[maybe_unused]] std::vector<uint8_t>::iterator it2,
> > +						ControlSerializer *cs)
> > +	{
> > +		if (!cs)
> > +			LOG(IPADataSerializer, Fatal)
> > +				<< "ControlSerializer not provided for deserialization of ControlInfoMap";
> > +
> > +		uint32_t infoDataSize = readUInt<uint32_t>(it1);
> > +
> > +		std::vector<uint8_t>::iterator it = it1 + 4;
> > +
> > +		std::vector<uint8_t> infoData(it, it + infoDataSize);
> > +
> > +		ByteStreamBuffer buffer(const_cast<const uint8_t *>(infoData.data()), infoData.size());
> > +		const ControlInfoMap map = cs->deserialize<ControlInfoMap>(buffer);
> > +
> > +		return map;
> > +	}
> > +
> > +	static const ControlInfoMap deserialize(std::vector<uint8_t> &data,
> > +						[[maybe_unused]] std::vector<int32_t> &fds,
> > +						ControlSerializer *cs)
> > +	{
> > +		return IPADataSerializer<const ControlInfoMap>::deserialize(data.begin(), data.end(), cs);
> > +	}
> > +
> > +	static const ControlInfoMap deserialize(std::vector<uint8_t>::iterator data_it1,
> > +					  std::vector<uint8_t>::iterator data_it2,
> > +					  [[maybe_unused]] std::vector<int32_t>::iterator fds_it1,
> > +					  [[maybe_unused]] std::vector<int32_t>::iterator fds_it2,
> > +					  ControlSerializer *cs)
> > +	{
> > +		return IPADataSerializer<const ControlInfoMap>::deserialize(data_it1, data_it2, cs);
> > +	}
> > +};
> > +
> > +template<>
> > +class IPADataSerializer<ControlInfoMap>
> > +{
> > +public:
> > +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> > +	serialize(const ControlInfoMap &map, [[maybe_unused]] ControlSerializer *cs)
> > +	{
> > +		const ControlInfoMap &map_const = const_cast<const ControlInfoMap &>(map);
> > +
> > +		std::vector<uint8_t> data_vec;
> > +		std::tie(data_vec, std::ignore) =
> > +			IPADataSerializer<const ControlInfoMap>::serialize(map_const, cs);
> > +
> > +		return {data_vec, {}};
> > +	}
> > +
> > +	static ControlInfoMap deserialize(std::vector<uint8_t> &data,
> > +					  ControlSerializer *cs)
> > +	{
> > +		ControlInfoMap ret;
> > +		const ControlInfoMap &map = ret;
> > +		const_cast<ControlInfoMap &>(map) =
> > +			IPADataSerializer<const ControlInfoMap>::deserialize(data, cs);
> > +
> > +		return ret;
> > +	}
> > +
> > +	static ControlInfoMap deserialize(std::vector<uint8_t>::iterator it1,
> > +						[[maybe_unused]] std::vector<uint8_t>::iterator it2,
> > +						ControlSerializer *cs)
> > +	{
> > +		ControlInfoMap ret;
> > +		const ControlInfoMap &map = ret;
> > +		const_cast<ControlInfoMap &>(map) =
> > +			IPADataSerializer<const ControlInfoMap>::deserialize(it1, it2, cs);
> > +
> > +		return ret;
> > +	}
> > +
> > +	static ControlInfoMap deserialize(std::vector<uint8_t> &data,
> > +						[[maybe_unused]] std::vector<int32_t> &fds,
> > +						ControlSerializer *cs)
> > +	{
> > +		ControlInfoMap ret;
> > +		const ControlInfoMap &map = ret;
> > +		const_cast<ControlInfoMap &>(map) =
> > +			IPADataSerializer<const ControlInfoMap>::deserialize(data, fds, cs);
> > +
> > +		return ret;
> > +	}
> > +
> > +	static ControlInfoMap deserialize(std::vector<uint8_t>::iterator data_it1,
> > +					  std::vector<uint8_t>::iterator data_it2,
> > +					  [[maybe_unused]] std::vector<int32_t>::iterator fds_it1,
> > +					  [[maybe_unused]] std::vector<int32_t>::iterator fds_it2,
> > +					  ControlSerializer *cs)
> > +	{
> > +		ControlInfoMap ret;
> > +		const ControlInfoMap &map = ret;
> > +		const_cast<ControlInfoMap &>(map) =
> > +			IPADataSerializer<const ControlInfoMap>::deserialize(data_it1, data_it2,
> > +									     fds_it1, fds_it2, cs);
> 
> Very similar comment as per the ControlList implementation
> 
> > +
> > +		return ret;
> > +	}
> > +};
> > +
> > +template<>
> > +class IPADataSerializer<FrameBuffer::Plane>
> > +{
> > +public:
> > +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> > +	serialize(const FrameBuffer::Plane &data, [[maybe_unused]] ControlSerializer *cs = nullptr)
> > +	{
> > +		std::vector<uint8_t> data_vec;
> > +		std::vector<int32_t> fds_vec;
> > +
> > +		std::vector<uint8_t> fdBuf;
> > +		std::vector<int32_t> fdFds;
> > +		std::tie(fdBuf, fdFds) =
> > +			IPADataSerializer<FileDescriptor>::serialize(data.fd);
> > +		data_vec.insert(data_vec.end(), fdBuf.begin(), fdBuf.end());
> > +		fds_vec.insert(fds_vec.end(), fdFds.begin(), fdFds.end());
> > +
> > +		appendUInt<uint32_t>(data_vec, data.length);
> > +
> > +		return {data_vec, fds_vec};
> > +	}
> > +
> > +	static FrameBuffer::Plane deserialize(std::vector<uint8_t> &data,
> > +					      std::vector<int32_t> &fds,
> > +					      ControlSerializer *cs = nullptr)
> > +	{
> > +		return IPADataSerializer<FrameBuffer::Plane>::deserialize(data.begin(), data.end(),
> > +									  fds.begin(), fds.end(),
> > +									  cs);
> > +	}
> > +
> > +	static FrameBuffer::Plane deserialize(std::vector<uint8_t>::iterator data_it1,
> > +					      [[maybe_unused]] std::vector<uint8_t>::iterator data_it2,
> > +					      std::vector<int32_t>::iterator fds_it1,
> > +					      [[maybe_unused]] std::vector<int32_t>::iterator fds_it2,
> > +					      [[maybe_unused]] ControlSerializer *cs = nullptr)
> > +	{
> > +		FrameBuffer::Plane ret;
> > +
> > +		ret.fd = IPADataSerializer<FileDescriptor>::deserialize(data_it1, data_it1 + 1,
> > +									fds_it1, fds_it1 + 1);
> > +		ret.length = readUInt<uint32_t>(data_it1 + 1);
> > +
> > +		return ret;
> > +	}
> > +};
> > +
> > +
> > +template<>
> > +class IPADataSerializer<IPABuffer>
> > +{
> > +public:
> > +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> > +	serialize(const IPABuffer &data, ControlSerializer *cs = nullptr)
> > +	{
> > +		std::vector<uint8_t> data_vec;
> > +
> > +		appendUInt<uint32_t>(data_vec, data.id);
> > +
> > +		std::vector<uint8_t> planes_data_vec;
> > +		std::vector<int32_t> planes_fds_vec;
> 
> Calculating in advance the occupation might help but it's not trivial

Yeah, that's what IPADataSerializer is for :)

> > +		std::tie(planes_data_vec, planes_fds_vec) =
> > +			IPADataSerializer<std::vector<FrameBuffer::Plane>>::serialize(data.planes, cs);
> > +
> > +		data_vec.insert(data_vec.end(), planes_data_vec.begin(), planes_data_vec.end());
> > +
> > +		return {data_vec, planes_fds_vec};
> > +	}
> > +
> > +	static IPABuffer deserialize(std::vector<uint8_t> &data,
> > +				     std::vector<int32_t> &fds,
> > +				     ControlSerializer *cs = nullptr)
> > +	{
> > +		return IPADataSerializer<IPABuffer>::deserialize(data.begin(), data.end(),
> > +								 fds.begin(), fds.end(), cs);
> > +	}
> > +
> > +	static IPABuffer deserialize(std::vector<uint8_t>::iterator data_it1,
> > +				     std::vector<uint8_t>::iterator data_it2,
> > +				     std::vector<int32_t>::iterator fds_it1,
> > +				     std::vector<int32_t>::iterator fds_it2,
> > +				     ControlSerializer *cs = nullptr)
> > +	{
> > +		IPABuffer ret;
> > +
> > +		ret.id = readUInt<uint32_t>(data_it1);
> > +
> > +		ret.planes =
> > +			IPADataSerializer<std::vector<FrameBuffer::Plane>>::deserialize(
> > +				data_it1 + 4, data_it2, fds_it1, fds_it2, cs);
> > +
> > +		return ret;
> > +	}
> > +};
> > +
> > +#endif /* __DOXYGEN__ */
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_INTERNAL_IPA_DATA_SERIALIZER_H__ */
> > diff --git a/src/libcamera/ipa_data_serializer.cpp b/src/libcamera/ipa_data_serializer.cpp
> > new file mode 100644
> > index 00000000..5029cdf6
> > --- /dev/null
> > +++ b/src/libcamera/ipa_data_serializer.cpp
> > @@ -0,0 +1,154 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2020, Google Inc.
> > + *
> > + * ipa_data_serializer.cpp - Image Processing Algorithm data serializer
> > + */
> > +
> > +#include "libcamera/internal/ipa_data_serializer.h"
> > +
> > +#include "libcamera/internal/log.h"
> > +
> > +/**
> > + * \file ipa_data_serializer.h
> > + * \brief IPA Data Serializer
> > + */
> > +
> > +namespace libcamera {
> > +
> > +LOG_DEFINE_CATEGORY(IPADataSerializer)
> > +
> > +/**
> > + * \class IPADataSerializer
> > + * \brief IPA Data Serializer
> > + *
> > + * Static template class that provides functions for serializing and
> > + * deserializing IPA data.
> > + */
> > +
> > +/**
> > + * \fn template<typename T> void appendUInt(std::vector<uint8_t> &vec, T val)
> > + * \brief Append uint to end of byte vector, in little-endian order
> > + * \tparam T Type of uint to append
> > + * \param[in] vec Byte vector to append to
> > + * \param[in] val Value of uint to append
> 
> As said, this works for uint, int, float and double now

Ah okay, I'll rename this then.

> > + */
> > +
> > +/**
> > + * \fn template<typename T> T readUInt(std::vector<uint8_t> &vec, size_t pos)
> > + * \brief Read uint from byte vector, in little-endian order
> > + * \tparam T Type of uint to read
> > + * \param[in] vec Byte vector to read from
> > + * \param[in] pos Index in vec to start reading from
> > + *
> 
> No need for an empty line if no additional description is provided

ack

> > + * \return The uint read from \a vec, or 0 if reading goes past end of \a vec
> 
> I wonder if we should not pass a T * as an output parameter and return
> an error code. Returning 0 makes it impossible to detect failure, as 0
> is a valid value

I was actually wondering about that since when I wrote it in the first
place. This might be a good solution.

> > + */
> > +
> > +/**
> > + * \fn template<typename T> T readUInt(std::vector<uint8_t>::iterator it)
> > + * \brief Read uint from byte vector, in little-endian order
> > + * \tparam T Type of uint to read
> > + * \param[in] it Iterator of byte vector to read from
> > + *
> 
> No need to empty line

ack

> > + * \return The uint read from \a vec
> > + */
> > +
> > +/**
> > + * \fn template<typename T> IPADataSerializer<T>::serialize(
> > + * 	T data,
> > + * 	ControlSerializer *cs = nullptr)
> > + * \brief Serialize an object into byte vector and fd vector
> > + * \tparam T Type of object to serialize
> > + * \param[in] data Object to serialize
> > + * \param[in] cs ControlSerializer
> > + *
> > + * \a cs is only necessary if the object type \a T or its members contain
> > + * ControlList or ControlInfoMap.
> > + *
> > + * \return Tuple of byte vector and fd vector, that is the serialized form
> > + * of \a data
> 
> Maybe later, but I think for each template specialization, we want to
> document the serialization format, in example that for vectors, the
> length is the first 4 bytes and so on (even more for custom libcamera
> data types)

Good idea.

> > + */
> > +
> > +/**
> > + * \fn template<typename T> IPADataSerializer<T>::deserialize(
> > + * 	std::vector<uint8_t> &data,
> > + * 	ControlSerializer *cs = nullptr)
> > + * \brief Deserialize byte vector into an object
> > + * \tparam T Type of object to deserialize to
> > + * \param[in] data 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 FileDescriptor.
> > + *
> > + * \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>::iterator it1,
> > + * 	std::vector<uint8_t>::iterator it2,
> > + * 	ControlSerializer *cs = nullptr)
> > + * \brief Deserialize byte vector into an object
> > + * \tparam T Type of object to deserialize to
> > + * \param[in] it1 Begin iterator of byte vector to deserialize from
> > + * \param[in] it2 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 FileDescriptor.
> > + *
> > + * \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> &data,
> > + * 	std::vector<int32_t> &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 FileDescriptor.
> > + *
> > + * \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>::iterator data_it1,
> > + * 	std::vector<uint8_t>::iterator data_it2,
> > + * 	std::vector<int32_t>::iterator fds_it1,
> > + * 	std::vector<int32_t>::iterator fds_it2,
> > + * 	ControlSerializer *cs = nullptr)
> > + * \brief Deserialize byte vector and fd vector into an object
> > + * \tparam T Type of object to deserialize to
> > + * \param[in] data_it1 Begin iterator of byte vector to deserialize from
> > + * \param[in] data_it2 End iterator of byte vector to deserialize from
> > + * \param[in] fds_it1 Begin iterator of fd vector to deserialize from
> > + * \param[in] fds_it2 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 FileDescriptor.
> > + *
> > + * \a cs is only necessary if the object type \a T or its members contain
> > + * ControlList or ControlInfoMap.
> > + *
> > + * \return The deserialized object
> > + */
> > +
> 
> Puff.. pant.. a lot of code to review work here! Nice one Paul!

Thank you for the reviewing the whole thing :)


Paul

> > +} /* namespace libcamera */
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index 07711b5f..190d7490 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -24,6 +24,7 @@ libcamera_sources = files([
> >      'geometry.cpp',
> >      'ipa_context_wrapper.cpp',
> >      'ipa_controls.cpp',
> > +    'ipa_data_serializer.cpp',
> >      'ipa_interface.cpp',
> >      'ipa_manager.cpp',
> >      'ipa_module.cpp',
> > --
> > 2.27.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel

Hi Paul,

On Tue, Oct 27, 2020 at 07:14:03PM +0900, paul.elder@ideasonboard.com wrote:
> Hi Jacopo,
>
> Thank you for the thorough review!
>

Thank you for the enormous work

> On Mon, Oct 26, 2020 at 05:07:52PM +0100, Jacopo Mondi wrote:
> > Hi Paul,
> >
> > On Fri, Oct 02, 2020 at 11:31:26PM +0900, Paul Elder wrote:
> > > Add an IPADataSerializer which implments de/serialization of built-in
> > > (PODs, vector, map, string) and libcamera data structures. This is
> > > intended to be used by the proxy and the proxy worker in the IPC layer.
> > >
> > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > >
> > > ---
> > > Changes in v3:
> > > - reimplement append/readUInt with memcpy (intead of bit shifting)
> > > - change DECLARE_INTEGRAL_SERIALIZER with DECLARE_POD_SERIALIZER
> > >   - use this for int64_t, bool, float, and double
> > > - fix comment style
> > >
> > > Changes in v2:
> > > - added serializers for all integer types, bool, and string
> > > - use string serializer for IPASettings serializer
> > > - add documentation
> > > - add serializer for const ControlList
> > > ---
> > >  .../libcamera/internal/ipa_data_serializer.h  | 1014 +++++++++++++++++
> > >  src/libcamera/ipa_data_serializer.cpp         |  154 +++
> > >  src/libcamera/meson.build                     |    1 +
> > >  3 files changed, 1169 insertions(+)
> > >  create mode 100644 include/libcamera/internal/ipa_data_serializer.h
> > >  create mode 100644 src/libcamera/ipa_data_serializer.cpp
> > >
> > > diff --git a/include/libcamera/internal/ipa_data_serializer.h b/include/libcamera/internal/ipa_data_serializer.h
> > > new file mode 100644
> > > index 00000000..9cd35c4c
> > > --- /dev/null
> > > +++ b/include/libcamera/internal/ipa_data_serializer.h
> > > @@ -0,0 +1,1014 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2020, Google Inc.
> > > + *
> > > + * ipa_data_serializer.h - Image Processing Algorithm data serializer
> > > + */
> > > +#ifndef __LIBCAMERA_INTERNAL_IPA_DATA_SERIALIZER_H__
> > > +#define __LIBCAMERA_INTERNAL_IPA_DATA_SERIALIZER_H__
> > > +
> > > +#include <deque>
> > > +#include <iostream>
> > > +#include <string.h>
> > > +#include <tuple>
> > > +#include <vector>
> > > +
> > > +#include <libcamera/buffer.h>
> > > +#include <libcamera/control_ids.h>
> > > +#include <libcamera/geometry.h>
> > > +#include <libcamera/ipa/ipa_interface.h>
> > > +
> > > +#include "libcamera/internal/byte_stream_buffer.h"
> > > +#include "libcamera/internal/camera_sensor.h"
> > > +#include "libcamera/internal/control_serializer.h"
> > > +#include "libcamera/internal/log.h"
> > > +
> > > +namespace libcamera {
> > > +
> > > +LOG_DECLARE_CATEGORY(IPADataSerializer)
> > > +
> >
> > should these be wrapped in an anonymous namespace ? They are used by
> > the generated proxyes, but the header is included. I tried it locally
> > and complier seems to digest it
>
> Ah yes, probably.
>
> > > +template<typename T>
> > > +void appendUInt(std::vector<uint8_t> &vec, T val)
> > > +{
> > > +	size_t byteWidth = sizeof(val);
> > > +	std::vector<uint8_t> v(byteWidth);
> > > +	memcpy(&v[0], &val, byteWidth);
> > > +
> > > +	vec.insert(vec.end(), v.begin(), v.end());
> > > +}
> > > +
> > > +template<typename T>
> > > +T readUInt(std::vector<uint8_t> &vec, size_t pos)
> > > +{
> > > +	T ret = 0;
> > > +	size_t byteWidth = sizeof(ret);
> > > +	if (pos + byteWidth > vec.size())
> > > +		return ret;
> > > +
> > > +	memcpy(&ret, &vec.data()[pos], byteWidth);
> >
> > Just &vec[pos] should be enough
>
> It's fine even for memcpying?
>

std::vector::operator[] returns a T&, so taking its address should be
equivalent to &std::vector::data()[pos]

> > > +	return ret;
> > > +}
> > > +
> > > +template<typename T>
> > > +T readUInt(std::vector<uint8_t>::iterator it)
> > > +{
> > > +	T ret = 0;
> > > +	size_t byteWidth = sizeof(ret);
> > > +
> > > +	std::vector<uint8_t> v(it, it + byteWidth);
> > > +	memcpy(&ret, v.data(), byteWidth);
> > > +	return ret;
> > > +}
> >
> > These new versions looks much better!
>
> \o/
>
> Ah, so later on you're saying that I should rename these, since they're
> not limited to uints?
>

Yes, I think removing UInt from the names might be useful.
As said below this apply to most POD types, so appendPOD() and
readPOD() might be more appropriate. But even just 'append()' and
'read()' might be enough and nicer to read (assuming they're wrapped
in an anonymous namespace). Ah wait, 'read()' -might- already be used :)

We could live with that, as the read syscall shall be prefixed with ::

This, in example, works with g++ 10.2.0 and clang++ 10.0.1

#include <unistd.h>
#include <iostream>

namespace {
void read(char b)
{
	std::cout << b << std::endl;
}
};

int main()
{
	char a;

	::read(1, &a, 1);
	read(a);

	return 0;
}


I'm not sure we want to go there though.

Otherwise, name the namespace that wraps readUInt() ? You can use
'detail' as the namespace name as it's done for Span<> ?

> > > +
> > > +template<typename T>
> > > +class IPADataSerializer
> > > +{
> > > +#ifdef __DOXYGEN__
> > > +public:
> > > +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> > > +	serialize(T data, ControlSerializer *cs);
> > > +
> > > +	static T deserialize(std::vector<uint8_t> &data,
> > > +			     ControlSerializer *cs);
> > > +	static T deserialize(std::vector<uint8_t>::iterator it1,
> > > +			     std::vector<uint8_t>::iterator it2,
> > > +			     ControlSerializer *cs);
> > > +
> >
> > Intentional empty line ?
>
> Yes, the first two are data only, and the bottom two are data+fd.
>
> > > +	static T deserialize(std::vector<uint8_t> &data,
> > > +			     std::vector<int32_t> &fds,
> > > +			     ControlSerializer *cs);
> > > +	static T deserialize(std::vector<uint8_t>::iterator data_it1,
> > > +			     std::vector<uint8_t>::iterator data_it2,
> > > +			     std::vector<int32_t>::iterator fds_it1,
> > > +			     std::vector<int32_t>::iterator fds_it2,
> > > +			     ControlSerializer *cs);
> >
> > Aren't these better called data_start, data_end and fds_start, fds_end
> > ?
>
> Yeah, that's better.
>
> > > +#endif /* __DOXYGEN__ */
> >
> > Do you need this ifdef __DOXYGEN__ ? you want these parsed, don't you ?
> > It's usually #ifndef __DOXYGEN__ to exclude from documentation parsing
> > some parts of the code, like you're done below
>
> I do need this ifdef. I want doxygen to document just the functions in
> the base serializer, since it's the same interface for all serializers,

But won't doxygen parse it anyway. I tried removing the #ifdef and I
have documentation generated for IPADataSerializer::serialize() and
friends generated.

> but I don't want the base serializer template to be compiled.

That's ok. And that's why you need the below #ifndef
Or didn't I get what you mean with 'base serializer' ?

>
> > > +};
> > > +
> > > +#ifndef __DOXYGEN__
> > > +
> > > +template<typename V>
> > > +class IPADataSerializer<std::vector<V>>
> > > +{
> > > +public:
> > > +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> > > +	serialize(const std::vector<V> &data, ControlSerializer *cs = nullptr)
> > > +	{
> > > +		std::vector<uint8_t> data_vec;
> >
> > We can reserve data.size() * sizeof(V) in the vector
>
> I'm not sure we can do that...
>
> > > +		std::vector<int32_t> fds_vec;
> > > +
> > > +		/* Serialize the length. */
> > > +		uint32_t vec_len = data.size();
> > > +		appendUInt<uint32_t>(data_vec, vec_len);
> > > +
> > > +		/* Serialize the members. */
> > > +		for (auto it = data.begin(); it != data.end(); ++it) {
> >
> >                 for (auto const &it : data) ?
>
> ack
>
> > > +			std::vector<uint8_t> dvec;
> > > +			std::vector<int32_t> fvec;
> > > +
> > > +			std::tie(dvec, fvec) =
> > > +				IPADataSerializer<V>::serialize(*it, cs);
> > > +
> > > +			appendUInt<uint32_t>(data_vec, dvec.size());
> >
> > Why is the size (in bytes) of each serialized entry recorded in the
> > serialized data buffer ? Aren't all the members of the same size ?
>
> ...because the members of the vector aren't necessarily the same size.
> The vector could contains variable-size objects, like other vectors, or
> maps, or strings, etc, so we need to tag the size of every element.

The fact you've pointed out that, in example, CameraSensor contains a
string of variable length makes me agree with you and the fact we have
to prefix the actual data with the expected size in the serialized
format. The same for pre-reserving the vectors, sizeof() won't surely
help with !POD data.

>
> > > +			appendUInt<uint32_t>(data_vec, fvec.size());
> >

[snip]

> > > +#define DECLARE_POD_SERIALIZER(type)					\
> > > +template<>								\
> > > +class IPADataSerializer<type>						\
> > > +{									\
> > > +public:									\
> > > +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>	\
> > > +	serialize(const type data,					\
> > > +		  [[maybe_unused]] ControlSerializer *cs = nullptr)	\
> > > +	{								\
> > > +		std::vector<uint8_t> data_vec;				\
> >
> > Do you think pre-allocating sizeof(type) in the vector might help ?
>
> tbh, yeah.
>
> > > +		appendUInt<type>(data_vec, data);			\
>
> Oh, then this needs a writeUInt cousin.
>

Why so ?

> > > +									\
> > > +		return {data_vec, {}};					\
> > > +	}								\
> > > +									\
> > > +	static type deserialize(std::vector<uint8_t> &data,		\
> > > +				[[maybe_unused]] ControlSerializer *cs = nullptr)\
> > > +	{								\
> > > +		return IPADataSerializer<type>::deserialize(data.begin(),\
> > > +							    data.end());\
> > > +	}								\
> > > +									\
> > > +	static type deserialize(std::vector<uint8_t>::iterator it1,	\
> > > +				[[maybe_unused]] std::vector<uint8_t>::iterator it2,\
> > > +				[[maybe_unused]] ControlSerializer *cs = nullptr)\
> > > +	{								\
> > > +		return readUInt<type>(it1);				\
> > > +	}								\
> > > +									\
> > > +	static type deserialize(std::vector<uint8_t> &data,		\
> > > +				[[maybe_unused]] std::vector<int32_t> &fds,\
> > > +				[[maybe_unused]] ControlSerializer *cs = nullptr)\
> > > +	{								\
> > > +		return IPADataSerializer<type>::deserialize(data.begin(),\
> > > +							    data.end());\
> > > +	}								\
> > > +									\
> > > +	static type deserialize(std::vector<uint8_t>::iterator data_it1,\
> > > +				std::vector<uint8_t>::iterator data_it2,\
> > > +				[[maybe_unused]] std::vector<int32_t>::iterator fds_it1,\
> > > +				[[maybe_unused]] std::vector<int32_t>::iterator fds_it2,\
> > > +				[[maybe_unused]] ControlSerializer *cs = nullptr)\
> > > +	{								\
> > > +		return IPADataSerializer<type>::deserialize(data_it1,	\
> > > +							    data_it2);	\
> > > +	}								\
> > > +};
> > > +
> > > +DECLARE_POD_SERIALIZER(bool)
> > > +DECLARE_POD_SERIALIZER(uint8_t)
> > > +DECLARE_POD_SERIALIZER(uint16_t)
> > > +DECLARE_POD_SERIALIZER(uint32_t)
> > > +DECLARE_POD_SERIALIZER(uint64_t)
> > > +DECLARE_POD_SERIALIZER(int8_t)
> > > +DECLARE_POD_SERIALIZER(int16_t)
> > > +DECLARE_POD_SERIALIZER(int32_t)
> > > +DECLARE_POD_SERIALIZER(int64_t)
> >
> > The I wonder if keeping here and in the documentation the term 'uint'
> > is correct and it shouldn't just be replaced with 'int'
>
> I'm guessing you're referring to {append,read}UInt()?
>

Yes

> > > +DECLARE_POD_SERIALIZER(float)
> > > +DECLARE_POD_SERIALIZER(double)
> >
> > Or even POD
>
> I'm not sure what you're referring to here :/
>

Still on the name, see above for the readPOD() appendPOD() discussion
(these two are awful names tbh)

> > > +
> > > +template<>
> > > +class IPADataSerializer<std::string>
> > > +{
> > > +public:
> > > +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> > > +	serialize(const std::string &data, [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > +	{
> > > +		std::vector<uint8_t> data_vec(data.begin(), data.end());
> > > +
> > > +		return {data_vec, {}};
> > > +	}
> > > +
> > > +	static std::string deserialize(std::vector<uint8_t> &data,
> > > +				       [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > +	{
> > > +		return IPADataSerializer<std::string>::deserialize(data.begin(), data.end());
> > > +	}
> > > +
> > > +	static std::string deserialize(std::vector<uint8_t>::iterator it1,
> > > +				       std::vector<uint8_t>::iterator it2,
> > > +				       [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > +	{
> > > +		std::string str(it1, it2);
> > > +
> > > +		return str;
> >
> > Can you just
> >                 return {it1, it2};
> > ?
>
> Ah, yes. I had it (and everything below) implemented in this manner
> because I wanted to algorithmize the serdes code generation, so I was
> practicing it on myself here. I guess it's about time for optimizations;
> thank you for the pointers.
>
> > > +	}
> > > +
> > > +	static std::string deserialize(std::vector<uint8_t> &data,
> > > +				       [[maybe_unused]] std::vector<int32_t> &fds,
> > > +				       [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > +	{
> > > +		return IPADataSerializer<std::string>::deserialize(data.begin(), data.end());
> >
> > Maybe
> >                 return {data.being(), data.end()};
> >
> > would save a function call ?
> > Same for other overloaded methods (and possibly not only in the
> > <string> specialization).
> >
>
> ack
>
> > > +	}
> > > +
> > > +	static std::string deserialize(std::vector<uint8_t>::iterator data_it1,
> > > +				       std::vector<uint8_t>::iterator data_it2,
> > > +				       [[maybe_unused]] std::vector<int32_t>::iterator fds_it1,
> > > +				       [[maybe_unused]] std::vector<int32_t>::iterator fds_it2,
> > > +				       [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > +	{
> > > +		return IPADataSerializer<std::string>::deserialize(data_it1, data_it2);
> > > +	}
> > > +};
> > > +
> > > +template<>
> > > +class IPADataSerializer<FileDescriptor>
> > > +{
> > > +public:
> > > +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> > > +	serialize(const FileDescriptor &data, [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > +	{
> > > +		std::vector<uint8_t> data_vec = { data.isValid() };
> > > +		std::vector<int32_t> fd_vec;
> > > +		if (data.isValid())
> > > +			fd_vec.push_back(data.fd());
> > > +
> > > +		return {data_vec, fd_vec};
> > > +	}
> > > +
> > > +	static FileDescriptor deserialize(std::vector<uint8_t> &data, std::vector<int32_t> &fds,
> > > +					  [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > +	{
> > > +		return IPADataSerializer<FileDescriptor>::deserialize(data.begin(), data.end(),
> > > +								      fds.begin(), fds.end());
> > > +	}
> > > +
> > > +	static FileDescriptor deserialize(std::vector<uint8_t>::iterator data_it1,
> > > +					  std::vector<uint8_t>::iterator data_it2,
> > > +					  std::vector<int32_t>::iterator fds_it1,
> > > +					  std::vector<int32_t>::iterator fds_it2,
> > > +					  [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > +	{
> > > +		if (std::distance(data_it1, data_it2) < 1)
> > > +			LOG(IPADataSerializer, Fatal)
> > > +				<< "Invalid data to deserialize FileDescriptor";
> > > +
> > > +		bool valid = *data_it1;
> >
> > in C this would be
> >                 bool valid = !!*data_it1;
> > not sure about C++ and not even sure if the !! is just a paranoid
> > kernel convention, as I don't think even C requires that
>
> Oh... I'm not sure. It passed my de/serialization tests.
>
> > > +
> > > +		if (valid && std::distance(fds_it1, fds_it2) < 1)
> >
> > 		if (valid && std::distance(fds_it1, fds_it2) < 1) {
> > 			LOG(IPADataSerializer, Fatal)
> > 				<< "Invalid fds to deserialize FileDescriptor";
> >                         return {}
> >                 }
> >
> >                 return FileDescriptor(*fds_it1);
> >
> > Or is returning {} when std::distance() < 1 wrong ? I don't think so,
> > as even if it is "valid" there's not fds serialized.
>
> If std::distance() < 1 then *fds_it1 will segfault. So if valid and
> std::distance() < 1, then there's a big problem -> fatal. If not valid,
> then std::distance() doesn't matter, and we return a new uninitialized
> FileDescriptor, but we can't construct it using *fds_it1, since the
> iterator shouldn't be valid.
>

I've missed 'Fatal' in the LOG() call. Sorry about this.

> It's perfectly fine to send an invalid FileDescriptor, for example if
> the FileDescriptor field isn't used, or uninitialized. The problem with
> sending it directly is that sendmsg() EINVALs if we try to send -1, so
> we need this valid flag in the data vector.
>
> >
> > > +			LOG(IPADataSerializer, Fatal)
> > > +				<< "Invalid fds to deserialize FileDescriptor";
> > > +
> > > +		return valid ? FileDescriptor(*fds_it1) : FileDescriptor();
> > > +	}
> > > +};
> > > +
> > > +template<>
> > > +class IPADataSerializer<IPASettings>
> > > +{
> > > +public:
> > > +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> > > +	serialize(const IPASettings &data, [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > +	{
> > > +		return IPADataSerializer<std::string>::serialize(data.configurationFile);
> > > +	}
> > > +
> > > +	static IPASettings deserialize(std::vector<uint8_t> &data,
> > > +				       [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > +	{
> > > +		return IPADataSerializer<IPASettings>::deserialize(data.begin(), data.end());
> > > +	}
> > > +
> > > +	static IPASettings deserialize(std::vector<uint8_t>::iterator it1,
> > > +				       std::vector<uint8_t>::iterator it2,
> > > +				       [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > +	{
> > > +		IPASettings ret;
> > > +		ret.configurationFile = IPADataSerializer<std::string>::deserialize(it1, it2);
> > > +
> > > +		return ret;
> >
> >                 return { IPADataSerializer<std::string>::deserialize(it1, it2) };
> >
> > ?
>
> Ah yes, much needed optimization.
>
> > > +	}
> > > +
> > > +	static IPASettings deserialize(std::vector<uint8_t> &data,
> > > +				       [[maybe_unused]] std::vector<int32_t> &fds,
> > > +				       [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > +	{
> > > +		return IPADataSerializer<IPASettings>::deserialize(data.begin(), data.end());
> >
> >                 return { IPADataSerializer<std::string>::deserialize(data.begin(), data.end() };
> >
> > ?
>
> ack
>
> > > +	}
> > > +
> > > +	static IPASettings deserialize(std::vector<uint8_t>::iterator data_it1,
> > > +				       std::vector<uint8_t>::iterator data_it2,
> > > +				       [[maybe_unused]] std::vector<int32_t>::iterator fds_it1,
> > > +				       [[maybe_unused]] std::vector<int32_t>::iterator fds_it2,
> > > +				       [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > +	{
> > > +		return IPADataSerializer<IPASettings>::deserialize(data_it1, data_it2);
> >
> > same
>
> ack
>
> > > +	}
> > > +};
> > > +
> > > +template<>
> > > +class IPADataSerializer<CameraSensorInfo>
> > > +{
> > > +public:
> > > +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> > > +	serialize(const CameraSensorInfo &data, [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > +	{
> > > +		std::vector<uint8_t> data_vec;
> >
> > Reserving sizeof(CameraSensorInfo) might help
>
> It's got a string in it though, so we don't know sizeof(CameraSensorInfo).
>

It could be calculated though. Nevermind

> > I just noticed and I wonder why all this code uses this_naming_style
> > in place of the libcamera standard namingStyle
>
> Ah yes, I should probably fix that...
>

Thanks

> > > +
> > > +		uint32_t str_len = data.model.size();
> > > +		appendUInt<uint32_t>(data_vec, str_len);
> > > +
> > > +		data_vec.insert(data_vec.end(), data.model.begin(), data.model.end());
> >
> > Shouldn't this be serialized using the IPADataSerializer<std::string>
> > if it's not needed, do we need the specialization then ?
>
> It should be :)
>
> > > +
> > > +		appendUInt<uint32_t>(data_vec, data.bitsPerPixel);
> > > +
> > > +		appendUInt<uint32_t>(data_vec, data.activeAreaSize.width);
> > > +		appendUInt<uint32_t>(data_vec, data.activeAreaSize.height);
> > > +
> > > +		appendUInt<uint32_t>(data_vec, static_cast<uint32_t>(data.analogCrop.x));
> > > +		appendUInt<uint32_t>(data_vec, static_cast<uint32_t>(data.analogCrop.y));
> > > +		appendUInt<uint32_t>(data_vec, data.analogCrop.width);
> > > +		appendUInt<uint32_t>(data_vec, data.analogCrop.height);
> > > +
> > > +		appendUInt<uint32_t>(data_vec, data.outputSize.width);
> > > +		appendUInt<uint32_t>(data_vec, data.outputSize.height);
> > > +
> > > +		appendUInt<uint64_t>(data_vec, data.pixelRate);
> > > +
> > > +		appendUInt<uint32_t>(data_vec, data.lineLength);
> > > +
> > > +		return {data_vec, {}};
> > > +	}
> > > +
> > > +	static CameraSensorInfo deserialize(std::vector<uint8_t> &data,
> > > +					    [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > +	{
> > > +		return IPADataSerializer<CameraSensorInfo>::deserialize(data.begin(), data.end());
> > > +	}
> > > +
> > > +	static CameraSensorInfo deserialize(std::vector<uint8_t>::iterator it1,
> > > +					    [[maybe_unused]] std::vector<uint8_t>::iterator it2,
> > > +					    [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > +	{
> > > +		CameraSensorInfo ret;
> >                 CameraSensorInfo ret{};
> >
> > Also for other statically allocated fields that are returned by other
> > specializations.
>
> Do we need to, since all the fields are populated below?
>

Technically not, and for all the supported types you fill in all
fields, you're right. But to be honest I won't mind a
0-initialization anyway, mostly because if we happen to add support
for new types, and if they won't be fully-filled, we keep the good
practice of {} and we're safe. A bit paranoid maybe :)

> > > +
> > > +		uint32_t str_len = readUInt<uint32_t>(it1);
> > > +		std::string str(it1 + 4, it1 + 4 + str_len);
> > > +		ret.model = str;
> > > +
> > > +		std::vector<uint8_t>::iterator it = it1 + 4 + str_len;
> > > +
> > > +		ret.bitsPerPixel = readUInt<uint32_t>(it);
> > > +
> > > +		ret.activeAreaSize.width = readUInt<uint32_t>(it + 4);
> > > +		ret.activeAreaSize.height = readUInt<uint32_t>(it + 8);
> > > +
> > > +		ret.analogCrop.x = static_cast<int32_t>(readUInt<uint32_t>(it + 12));
> > > +		ret.analogCrop.y = static_cast<int32_t>(readUInt<uint32_t>(it + 16));
> > > +		ret.analogCrop.width = readUInt<uint32_t>(it + 20);
> > > +		ret.analogCrop.height = readUInt<uint32_t>(it + 24);
> > > +
> > > +		ret.outputSize.width = readUInt<uint32_t>(it + 28);
> > > +		ret.outputSize.height = readUInt<uint32_t>(it + 32);
> > > +
> > > +		ret.pixelRate = readUInt<uint64_t>(it + 36);
> > > +
> > > +		ret.lineLength = readUInt<uint64_t>(it + 44);
> >
> > lineLength is a 32 bits integer
>
> Ah, yes.
>
> > > +
> > > +		return ret;
> > > +	}
> > > +
> > > +	static CameraSensorInfo deserialize(std::vector<uint8_t> &data,
> > > +					    [[maybe_unused]] std::vector<int32_t> &fds,
> > > +					    [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > +	{
> > > +		return IPADataSerializer<CameraSensorInfo>::deserialize(data.begin(), data.end());
> > > +	}
> > > +
> > > +	static CameraSensorInfo deserialize(std::vector<uint8_t>::iterator data_it1,
> > > +					    std::vector<uint8_t>::iterator data_it2,
> > > +					    [[maybe_unused]] std::vector<int32_t>::iterator fds_it1,
> > > +					    [[maybe_unused]] std::vector<int32_t>::iterator fds_it2,
> > > +					    [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > +	{
> > > +		return IPADataSerializer<CameraSensorInfo>::deserialize(data_it1, data_it2);
> > > +	}
> > > +};
> > > +
> > > +template<>
> > > +class IPADataSerializer<IPAStream>
> > > +{
> > > +public:
> > > +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> > > +	serialize(const IPAStream &data, [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > +	{
> > > +		std::vector<uint8_t> data_vec;
> > > +
> > > +		appendUInt<uint32_t>(data_vec, data.pixelFormat);
> > > +
> > > +		appendUInt<uint32_t>(data_vec, data.size.width);
> > > +		appendUInt<uint32_t>(data_vec, data.size.height);
> > > +
> > > +		return {data_vec, {}};
> > > +	}
> > > +
> > > +	static IPAStream deserialize(std::vector<uint8_t> &data,
> > > +				     [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > +	{
> > > +		return IPADataSerializer<IPAStream>::deserialize(data.begin(), data.end());
> > > +	}
> > > +
> > > +	static IPAStream deserialize(std::vector<uint8_t>::iterator it1,
> > > +				     [[maybe_unused]] std::vector<uint8_t>::iterator it2,
> > > +				     [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > +	{
> > > +		IPAStream ret;
> > > +
> > > +		ret.pixelFormat = readUInt<uint32_t>(it1);
> > > +
> > > +		ret.size.width = readUInt<uint32_t>(it1 + 4);
> > > +		ret.size.height = readUInt<uint32_t>(it1 + 8);
> > > +
> > > +		return ret;
> > > +	}
> > > +
> > > +	static IPAStream deserialize(std::vector<uint8_t> &data,
> > > +				     [[maybe_unused]] std::vector<int32_t> &fds,
> > > +				     [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > +	{
> > > +		return IPADataSerializer<IPAStream>::deserialize(data.begin(), data.end());
> > > +	}
> > > +
> > > +	static IPAStream deserialize(std::vector<uint8_t>::iterator data_it1,
> > > +				     std::vector<uint8_t>::iterator data_it2,
> > > +				     [[maybe_unused]] std::vector<int32_t>::iterator fds_it1,
> > > +				     [[maybe_unused]] std::vector<int32_t>::iterator fds_it2,
> > > +				     [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > +	{
> > > +		return IPADataSerializer<IPAStream>::deserialize(data_it1, data_it2);
> > > +	}
> > > +};
> > > +
> > > +template<>
> > > +class IPADataSerializer<const ControlList>
> > > +{
> > > +public:
> > > +	/* map arg will be generated, since it's per-pipeline anyway. */
> > > +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> > > +	serialize(const ControlList &data, const ControlInfoMap &map,
> > > +		  ControlSerializer *cs)
> > > +	{
> > > +		if (!cs)
> > > +			LOG(IPADataSerializer, Fatal)
> > > +				<< "ControlSerializer not provided for serialization of ControlList";
> > > +
> > > +		size_t size = cs->binarySize(map);
> > > +		std::vector<uint8_t> infoData(size);
> > > +		ByteStreamBuffer buffer(infoData.data(), infoData.size());
> > > +		int ret = cs->serialize(map, buffer);
> > > +
> > > +		if (ret < 0 || buffer.overflow()) {
> > > +			LOG(IPADataSerializer, Error) << "Failed to serialize ControlList's ControlInfoMap";
> > > +			return {{}, {}};
> > > +		}
> > > +
> > > +		size = cs->binarySize(data);
> > > +		std::vector<uint8_t> listData(size);
> > > +		buffer = ByteStreamBuffer(listData.data(), listData.size());
> > > +		ret = cs->serialize(data, buffer);
> > > +
> > > +		if (ret < 0 || buffer.overflow()) {
> > > +			LOG(IPADataSerializer, Error) << "Failed to serialize ControlList";
> > > +			return {{}, {}};
> > > +		}
> > > +
> > > +		std::vector<uint8_t> data_vec;
> >
> > You know the sizes and can reserve ?
>
> At this point. yes.
>
> > > +		appendUInt<uint32_t>(data_vec, infoData.size());
> > > +		appendUInt<uint32_t>(data_vec, listData.size());
> > > +		data_vec.insert(data_vec.end(), infoData.begin(), infoData.end());
> > > +		data_vec.insert(data_vec.end(), listData.begin(), listData.end());
> > > +
> > > +		return {data_vec, {}};
> > > +	}
> > > +
> > > +	static const ControlList deserialize(std::vector<uint8_t> &data, ControlSerializer *cs)
> > > +	{
> > > +		return IPADataSerializer<const ControlList>::deserialize(data.begin(), data.end(), cs);
> > > +	}
> > > +
> > > +	static const ControlList deserialize(std::vector<uint8_t>::iterator it1,
> > > +				       [[maybe_unused]] std::vector<uint8_t>::iterator it2,
> > > +				       ControlSerializer *cs)
> > > +	{
> > > +		if (!cs)
> > > +			LOG(IPADataSerializer, Fatal)
> > > +				<< "ControlSerializer not provided for deserialization of ControlList";
> > > +
> > > +		uint32_t infoDataSize = readUInt<uint32_t>(it1);
> > > +		uint32_t listDataSize = readUInt<uint32_t>(it1 + 4);
> > > +
> > > +		std::vector<uint8_t>::iterator it = it1 + 8;
> > > +
> > > +		std::vector<uint8_t> infoData(it, it + infoDataSize);
> > > +		std::vector<uint8_t> listData(it + infoDataSize, it + infoDataSize + listDataSize);
> > > +
> > > +		ByteStreamBuffer buffer(const_cast<const uint8_t *>(infoData.data()), infoData.size());
> > > +		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();
> > > +		}
> > > +
> > > +		buffer = ByteStreamBuffer(const_cast<const uint8_t *>(listData.data()), listData.size());
> > > +		ControlList list = cs->deserialize<ControlList>(buffer);
> > > +		if (buffer.overflow())
> > > +			LOG(IPADataSerializer, Error) << "Failed to deserialize ControlList: buffer overflow";
> > > +		if (list.empty())
> > > +			LOG(IPADataSerializer, Error) << "Failed to deserialize ControlList: empty list";
> >
> > Should we return {} in these cases or is it fine ?
>
> ControlSerializer::deserialize() returns {} in these cases anyway, so it
> should be fine.
>
> > > +
> > > +		return list;
> > > +	}
> > > +
> > > +	static const ControlList deserialize(std::vector<uint8_t> &data,
> > > +				       [[maybe_unused]] std::vector<int32_t> &fds,
> > > +				       ControlSerializer *cs)
> > > +	{
> > > +		return IPADataSerializer<const ControlList>::deserialize(data.begin(), data.end(), cs);
> > > +	}
> > > +
> > > +	static const ControlList deserialize(std::vector<uint8_t>::iterator data_it1,
> > > +				       std::vector<uint8_t>::iterator data_it2,
> > > +				       [[maybe_unused]] std::vector<int32_t>::iterator fds_it1,
> > > +				       [[maybe_unused]] std::vector<int32_t>::iterator fds_it2,
> > > +				       ControlSerializer *cs)
> > > +	{
> > > +		return IPADataSerializer<const ControlList>::deserialize(data_it1, data_it2, cs);
> > > +	}
> > > +};
> > > +
> > > +template<>
> > > +class IPADataSerializer<ControlList>
> > > +{
> > > +public:
> > > +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> > > +	serialize(const ControlList &data, const ControlInfoMap &map,
> > > +		  ControlSerializer *cs)
> > > +	{
> > > +		const ControlList &list_const = const_cast<const ControlList &>(data);
> > > +
> > > +		std::vector<uint8_t> data_vec;
> > > +		std::tie(data_vec, std::ignore) =
> > > +			IPADataSerializer<const ControlList>::serialize(list_const, map, cs);
> > > +
> > > +		return {data_vec, {}};
> > > +	}
> > > +
> > > +	static ControlList deserialize(std::vector<uint8_t> &data,
> > > +				       ControlSerializer *cs)
> > > +	{
> > > +		ControlList ret;
> > > +		const ControlList &list = ret;
> > > +		const_cast<ControlList &>(list) =
> > > +			IPADataSerializer<const ControlList>::deserialize(data, cs);
> > > +
> > > +		return ret;
> > > +	}
> > > +
> > > +	static ControlList deserialize(std::vector<uint8_t>::iterator it1,
> > > +				       [[maybe_unused]] std::vector<uint8_t>::iterator it2,
> > > +				       ControlSerializer *cs)
> > > +	{
> > > +		ControlList ret;
> > > +		const ControlList &list = ret;
> > > +		const_cast<ControlList &>(list) =
> > > +			IPADataSerializer<const ControlList>::deserialize(it1, it2, cs);
> >
> > Why not
> >                 const ControlList list =
> > 			IPADataSerializer<const ControlList>::deserialize(it1, it2, cs);
> >                 return const_cast<ControlList>(list)
>
> My understanding is that to remove a const with const_cast, the original
> variable must not be const.

You're right, this won't work, cast should happen on a pointer or
reference.

This works with gcc and clang:

		const ControlList &list =
			IPADataSerializer<const ControlList>::deserialize(data, cs);

		return const_cast<ControlList &>(list);

As the function returns by value it should be ok.
Tests pass as well, but I'm not sure if this case is tested though.

>
> > > +
> > > +		return ret;
> > > +	}
> > > +
> > > + */

[snip]

> > > +
> > > +/**
> > > + * \fn template<typename T> T readUInt(std::vector<uint8_t> &vec, size_t pos)
> > > + * \brief Read uint from byte vector, in little-endian order
> > > + * \tparam T Type of uint to read
> > > + * \param[in] vec Byte vector to read from
> > > + * \param[in] pos Index in vec to start reading from
> > > + *
> >
> > No need for an empty line if no additional description is provided
>
> ack
>
> > > + * \return The uint read from \a vec, or 0 if reading goes past end of \a vec
> >
> > I wonder if we should not pass a T * as an output parameter and return
> > an error code. Returning 0 makes it impossible to detect failure, as 0
> > is a valid value
>
> I was actually wondering about that since when I wrote it in the first
> place. This might be a good solution.

Up to you. Do you check for the return value or do you need to do so ?

>
> > > + */
> > > +
> > > +/**
> > > + * \fn template<typename T> T readUInt(std::vector<uint8_t>::iterator it)
> > > + * \brief Read uint from byte vector, in little-endian order
> > > + * \tparam T Type of uint to read
> > > + * \param[in] it Iterator of byte vector to read from
> > > + *
> >
> > No need to empty line
>
> ack
>
> > > + * \return The uint read from \a vec
> > > + */
> > > +
> > > +/**
> > > + * \fn template<typename T> IPADataSerializer<T>::serialize(
> > > + * 	T data,
> > > + * 	ControlSerializer *cs = nullptr)
> > > + * \brief Serialize an object into byte vector and fd vector
> > > + * \tparam T Type of object to serialize
> > > + * \param[in] data Object to serialize
> > > + * \param[in] cs ControlSerializer
> > > + *
> > > + * \a cs is only necessary if the object type \a T or its members contain
> > > + * ControlList or ControlInfoMap.
> > > + *
> > > + * \return Tuple of byte vector and fd vector, that is the serialized form
> > > + * of \a data
> >
> > Maybe later, but I think for each template specialization, we want to
> > document the serialization format, in example that for vectors, the
> > length is the first 4 bytes and so on (even more for custom libcamera
> > data types)
>
> Good idea.

Although it won't be parsed by doxygen and will remain in-code only.
But knowing how each container will be serialized might help
validating it.

Actually, how does it work for closed source IPAs ? Before this series
the pipeline handler and the IPA were in charge of
serializing/deserializing with custom code, as the close source blob,
possibly written in C, wouldn't be able to link against libcamera and
use its helpers (better, they might chose not to, as from a compliance
pov it's fine linking a blob against L-GPL code). Is it still possible
? In that case, documenting how things gets serialized might be
even more important.

Thanks
  j

Hi Jacopo,

On Tue, Oct 27, 2020 at 12:30:19PM +0100, Jacopo Mondi wrote:
> Hi Paul,
> 
> On Tue, Oct 27, 2020 at 07:14:03PM +0900, paul.elder@ideasonboard.com wrote:
> > Hi Jacopo,
> >
> > Thank you for the thorough review!
> >
> 
> Thank you for the enormous work
> 
> > On Mon, Oct 26, 2020 at 05:07:52PM +0100, Jacopo Mondi wrote:
> > > Hi Paul,
> > >
> > > On Fri, Oct 02, 2020 at 11:31:26PM +0900, Paul Elder wrote:
> > > > Add an IPADataSerializer which implments de/serialization of built-in
> > > > (PODs, vector, map, string) and libcamera data structures. This is
> > > > intended to be used by the proxy and the proxy worker in the IPC layer.
> > > >
> > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > >
> > > > ---
> > > > Changes in v3:
> > > > - reimplement append/readUInt with memcpy (intead of bit shifting)
> > > > - change DECLARE_INTEGRAL_SERIALIZER with DECLARE_POD_SERIALIZER
> > > >   - use this for int64_t, bool, float, and double
> > > > - fix comment style
> > > >
> > > > Changes in v2:
> > > > - added serializers for all integer types, bool, and string
> > > > - use string serializer for IPASettings serializer
> > > > - add documentation
> > > > - add serializer for const ControlList
> > > > ---
> > > >  .../libcamera/internal/ipa_data_serializer.h  | 1014 +++++++++++++++++
> > > >  src/libcamera/ipa_data_serializer.cpp         |  154 +++
> > > >  src/libcamera/meson.build                     |    1 +
> > > >  3 files changed, 1169 insertions(+)
> > > >  create mode 100644 include/libcamera/internal/ipa_data_serializer.h
> > > >  create mode 100644 src/libcamera/ipa_data_serializer.cpp
> > > >
> > > > diff --git a/include/libcamera/internal/ipa_data_serializer.h b/include/libcamera/internal/ipa_data_serializer.h
> > > > new file mode 100644
> > > > index 00000000..9cd35c4c
> > > > --- /dev/null
> > > > +++ b/include/libcamera/internal/ipa_data_serializer.h
> > > > @@ -0,0 +1,1014 @@
> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > +/*
> > > > + * Copyright (C) 2020, Google Inc.
> > > > + *
> > > > + * ipa_data_serializer.h - Image Processing Algorithm data serializer
> > > > + */
> > > > +#ifndef __LIBCAMERA_INTERNAL_IPA_DATA_SERIALIZER_H__
> > > > +#define __LIBCAMERA_INTERNAL_IPA_DATA_SERIALIZER_H__
> > > > +
> > > > +#include <deque>
> > > > +#include <iostream>
> > > > +#include <string.h>
> > > > +#include <tuple>
> > > > +#include <vector>
> > > > +
> > > > +#include <libcamera/buffer.h>
> > > > +#include <libcamera/control_ids.h>
> > > > +#include <libcamera/geometry.h>
> > > > +#include <libcamera/ipa/ipa_interface.h>
> > > > +
> > > > +#include "libcamera/internal/byte_stream_buffer.h"
> > > > +#include "libcamera/internal/camera_sensor.h"
> > > > +#include "libcamera/internal/control_serializer.h"
> > > > +#include "libcamera/internal/log.h"
> > > > +
> > > > +namespace libcamera {
> > > > +
> > > > +LOG_DECLARE_CATEGORY(IPADataSerializer)
> > > > +
> > >
> > > should these be wrapped in an anonymous namespace ? They are used by
> > > the generated proxyes, but the header is included. I tried it locally
> > > and complier seems to digest it
> >
> > Ah yes, probably.
> >
> > > > +template<typename T>
> > > > +void appendUInt(std::vector<uint8_t> &vec, T val)
> > > > +{
> > > > +	size_t byteWidth = sizeof(val);
> > > > +	std::vector<uint8_t> v(byteWidth);
> > > > +	memcpy(&v[0], &val, byteWidth);
> > > > +
> > > > +	vec.insert(vec.end(), v.begin(), v.end());
> > > > +}
> > > > +
> > > > +template<typename T>
> > > > +T readUInt(std::vector<uint8_t> &vec, size_t pos)
> > > > +{
> > > > +	T ret = 0;
> > > > +	size_t byteWidth = sizeof(ret);
> > > > +	if (pos + byteWidth > vec.size())
> > > > +		return ret;
> > > > +
> > > > +	memcpy(&ret, &vec.data()[pos], byteWidth);
> > >
> > > Just &vec[pos] should be enough
> >
> > It's fine even for memcpying?
> >
> 
> std::vector::operator[] returns a T&, so taking its address should be
> equivalent to &std::vector::data()[pos]

Ah, I see.

> > > > +	return ret;
> > > > +}
> > > > +
> > > > +template<typename T>
> > > > +T readUInt(std::vector<uint8_t>::iterator it)
> > > > +{
> > > > +	T ret = 0;
> > > > +	size_t byteWidth = sizeof(ret);
> > > > +
> > > > +	std::vector<uint8_t> v(it, it + byteWidth);
> > > > +	memcpy(&ret, v.data(), byteWidth);
> > > > +	return ret;
> > > > +}
> > >
> > > These new versions looks much better!
> >
> > \o/
> >
> > Ah, so later on you're saying that I should rename these, since they're
> > not limited to uints?
> >
> 
> Yes, I think removing UInt from the names might be useful.
> As said below this apply to most POD types, so appendPOD() and
> readPOD() might be more appropriate. But even just 'append()' and
> 'read()' might be enough and nicer to read (assuming they're wrapped
> in an anonymous namespace). Ah wait, 'read()' -might- already be used :)
> 
> We could live with that, as the read syscall shall be prefixed with ::
> 
> This, in example, works with g++ 10.2.0 and clang++ 10.0.1
> 
> #include <unistd.h>
> #include <iostream>
> 
> namespace {
> void read(char b)
> {
> 	std::cout << b << std::endl;
> }
> };
> 
> int main()
> {
> 	char a;
> 
> 	::read(1, &a, 1);
> 	read(a);
> 
> 	return 0;
> }
> 
> 
> I'm not sure we want to go there though.
> 
> Otherwise, name the namespace that wraps readUInt() ? You can use
> 'detail' as the namespace name as it's done for Span<> ?

Eh... I'll probably just go with {read,append}POD().

> > > > +
> > > > +template<typename T>
> > > > +class IPADataSerializer
> > > > +{
> > > > +#ifdef __DOXYGEN__
> > > > +public:
> > > > +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> > > > +	serialize(T data, ControlSerializer *cs);
> > > > +
> > > > +	static T deserialize(std::vector<uint8_t> &data,
> > > > +			     ControlSerializer *cs);
> > > > +	static T deserialize(std::vector<uint8_t>::iterator it1,
> > > > +			     std::vector<uint8_t>::iterator it2,
> > > > +			     ControlSerializer *cs);
> > > > +
> > >
> > > Intentional empty line ?
> >
> > Yes, the first two are data only, and the bottom two are data+fd.
> >
> > > > +	static T deserialize(std::vector<uint8_t> &data,
> > > > +			     std::vector<int32_t> &fds,
> > > > +			     ControlSerializer *cs);
> > > > +	static T deserialize(std::vector<uint8_t>::iterator data_it1,
> > > > +			     std::vector<uint8_t>::iterator data_it2,
> > > > +			     std::vector<int32_t>::iterator fds_it1,
> > > > +			     std::vector<int32_t>::iterator fds_it2,
> > > > +			     ControlSerializer *cs);
> > >
> > > Aren't these better called data_start, data_end and fds_start, fds_end
> > > ?
> >
> > Yeah, that's better.
> >
> > > > +#endif /* __DOXYGEN__ */
> > >
> > > Do you need this ifdef __DOXYGEN__ ? you want these parsed, don't you ?
> > > It's usually #ifndef __DOXYGEN__ to exclude from documentation parsing
> > > some parts of the code, like you're done below
> >
> > I do need this ifdef. I want doxygen to document just the functions in
> > the base serializer, since it's the same interface for all serializers,
> 
> But won't doxygen parse it anyway. I tried removing the #ifdef and I
> have documentation generated for IPADataSerializer::serialize() and
> friends generated.
> 
> > but I don't want the base serializer template to be compiled.
> 
> That's ok. And that's why you need the below #ifndef
> Or didn't I get what you mean with 'base serializer' ?

No, I mean I don't want the unspecialized serializer template to be
compiled. It's there only for generating documentation.

> >
> > > > +};
> > > > +
> > > > +#ifndef __DOXYGEN__
> > > > +
> > > > +template<typename V>
> > > > +class IPADataSerializer<std::vector<V>>
> > > > +{
> > > > +public:
> > > > +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> > > > +	serialize(const std::vector<V> &data, ControlSerializer *cs = nullptr)
> > > > +	{
> > > > +		std::vector<uint8_t> data_vec;
> > >
> > > We can reserve data.size() * sizeof(V) in the vector
> >
> > I'm not sure we can do that...
> >
> > > > +		std::vector<int32_t> fds_vec;
> > > > +
> > > > +		/* Serialize the length. */
> > > > +		uint32_t vec_len = data.size();
> > > > +		appendUInt<uint32_t>(data_vec, vec_len);
> > > > +
> > > > +		/* Serialize the members. */
> > > > +		for (auto it = data.begin(); it != data.end(); ++it) {
> > >
> > >                 for (auto const &it : data) ?
> >
> > ack
> >
> > > > +			std::vector<uint8_t> dvec;
> > > > +			std::vector<int32_t> fvec;
> > > > +
> > > > +			std::tie(dvec, fvec) =
> > > > +				IPADataSerializer<V>::serialize(*it, cs);
> > > > +
> > > > +			appendUInt<uint32_t>(data_vec, dvec.size());
> > >
> > > Why is the size (in bytes) of each serialized entry recorded in the
> > > serialized data buffer ? Aren't all the members of the same size ?
> >
> > ...because the members of the vector aren't necessarily the same size.
> > The vector could contains variable-size objects, like other vectors, or
> > maps, or strings, etc, so we need to tag the size of every element.
> 
> The fact you've pointed out that, in example, CameraSensor contains a
> string of variable length makes me agree with you and the fact we have
> to prefix the actual data with the expected size in the serialized
> format. The same for pre-reserving the vectors, sizeof() won't surely
> help with !POD data.
> 
> >
> > > > +			appendUInt<uint32_t>(data_vec, fvec.size());
> > >
> 
> [snip]
> 
> > > > +#define DECLARE_POD_SERIALIZER(type)					\
> > > > +template<>								\
> > > > +class IPADataSerializer<type>						\
> > > > +{									\
> > > > +public:									\
> > > > +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>	\
> > > > +	serialize(const type data,					\
> > > > +		  [[maybe_unused]] ControlSerializer *cs = nullptr)	\
> > > > +	{								\
> > > > +		std::vector<uint8_t> data_vec;				\
> > >
> > > Do you think pre-allocating sizeof(type) in the vector might help ?
> >
> > tbh, yeah.
> >
> > > > +		appendUInt<type>(data_vec, data);			\
> >
> > Oh, then this needs a writeUInt cousin.
> >
> 
> Why so ?

Oh nvm, I thought that reserve would put empty space.

> > > > +									\
> > > > +		return {data_vec, {}};					\
> > > > +	}								\
> > > > +									\
> > > > +	static type deserialize(std::vector<uint8_t> &data,		\
> > > > +				[[maybe_unused]] ControlSerializer *cs = nullptr)\
> > > > +	{								\
> > > > +		return IPADataSerializer<type>::deserialize(data.begin(),\
> > > > +							    data.end());\
> > > > +	}								\
> > > > +									\
> > > > +	static type deserialize(std::vector<uint8_t>::iterator it1,	\
> > > > +				[[maybe_unused]] std::vector<uint8_t>::iterator it2,\
> > > > +				[[maybe_unused]] ControlSerializer *cs = nullptr)\
> > > > +	{								\
> > > > +		return readUInt<type>(it1);				\
> > > > +	}								\
> > > > +									\
> > > > +	static type deserialize(std::vector<uint8_t> &data,		\
> > > > +				[[maybe_unused]] std::vector<int32_t> &fds,\
> > > > +				[[maybe_unused]] ControlSerializer *cs = nullptr)\
> > > > +	{								\
> > > > +		return IPADataSerializer<type>::deserialize(data.begin(),\
> > > > +							    data.end());\
> > > > +	}								\
> > > > +									\
> > > > +	static type deserialize(std::vector<uint8_t>::iterator data_it1,\
> > > > +				std::vector<uint8_t>::iterator data_it2,\
> > > > +				[[maybe_unused]] std::vector<int32_t>::iterator fds_it1,\
> > > > +				[[maybe_unused]] std::vector<int32_t>::iterator fds_it2,\
> > > > +				[[maybe_unused]] ControlSerializer *cs = nullptr)\
> > > > +	{								\
> > > > +		return IPADataSerializer<type>::deserialize(data_it1,	\
> > > > +							    data_it2);	\
> > > > +	}								\
> > > > +};
> > > > +
> > > > +DECLARE_POD_SERIALIZER(bool)
> > > > +DECLARE_POD_SERIALIZER(uint8_t)
> > > > +DECLARE_POD_SERIALIZER(uint16_t)
> > > > +DECLARE_POD_SERIALIZER(uint32_t)
> > > > +DECLARE_POD_SERIALIZER(uint64_t)
> > > > +DECLARE_POD_SERIALIZER(int8_t)
> > > > +DECLARE_POD_SERIALIZER(int16_t)
> > > > +DECLARE_POD_SERIALIZER(int32_t)
> > > > +DECLARE_POD_SERIALIZER(int64_t)
> > >
> > > The I wonder if keeping here and in the documentation the term 'uint'
> > > is correct and it shouldn't just be replaced with 'int'
> >
> > I'm guessing you're referring to {append,read}UInt()?
> >
> 
> Yes
> 
> > > > +DECLARE_POD_SERIALIZER(float)
> > > > +DECLARE_POD_SERIALIZER(double)
> > >
> > > Or even POD
> >
> > I'm not sure what you're referring to here :/
> >
> 
> Still on the name, see above for the readPOD() appendPOD() discussion
> (these two are awful names tbh)

I think they're good enough :p

> > > > +
> > > > +template<>
> > > > +class IPADataSerializer<std::string>
> > > > +{
> > > > +public:
> > > > +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> > > > +	serialize(const std::string &data, [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > > +	{
> > > > +		std::vector<uint8_t> data_vec(data.begin(), data.end());
> > > > +
> > > > +		return {data_vec, {}};
> > > > +	}
> > > > +
> > > > +	static std::string deserialize(std::vector<uint8_t> &data,
> > > > +				       [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > > +	{
> > > > +		return IPADataSerializer<std::string>::deserialize(data.begin(), data.end());
> > > > +	}
> > > > +
> > > > +	static std::string deserialize(std::vector<uint8_t>::iterator it1,
> > > > +				       std::vector<uint8_t>::iterator it2,
> > > > +				       [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > > +	{
> > > > +		std::string str(it1, it2);
> > > > +
> > > > +		return str;
> > >
> > > Can you just
> > >                 return {it1, it2};
> > > ?
> >
> > Ah, yes. I had it (and everything below) implemented in this manner
> > because I wanted to algorithmize the serdes code generation, so I was
> > practicing it on myself here. I guess it's about time for optimizations;
> > thank you for the pointers.
> >
> > > > +	}
> > > > +
> > > > +	static std::string deserialize(std::vector<uint8_t> &data,
> > > > +				       [[maybe_unused]] std::vector<int32_t> &fds,
> > > > +				       [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > > +	{
> > > > +		return IPADataSerializer<std::string>::deserialize(data.begin(), data.end());
> > >
> > > Maybe
> > >                 return {data.being(), data.end()};
> > >
> > > would save a function call ?
> > > Same for other overloaded methods (and possibly not only in the
> > > <string> specialization).
> > >
> >
> > ack
> >
> > > > +	}
> > > > +
> > > > +	static std::string deserialize(std::vector<uint8_t>::iterator data_it1,
> > > > +				       std::vector<uint8_t>::iterator data_it2,
> > > > +				       [[maybe_unused]] std::vector<int32_t>::iterator fds_it1,
> > > > +				       [[maybe_unused]] std::vector<int32_t>::iterator fds_it2,
> > > > +				       [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > > +	{
> > > > +		return IPADataSerializer<std::string>::deserialize(data_it1, data_it2);
> > > > +	}
> > > > +};
> > > > +
> > > > +template<>
> > > > +class IPADataSerializer<FileDescriptor>
> > > > +{
> > > > +public:
> > > > +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> > > > +	serialize(const FileDescriptor &data, [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > > +	{
> > > > +		std::vector<uint8_t> data_vec = { data.isValid() };
> > > > +		std::vector<int32_t> fd_vec;
> > > > +		if (data.isValid())
> > > > +			fd_vec.push_back(data.fd());
> > > > +
> > > > +		return {data_vec, fd_vec};
> > > > +	}
> > > > +
> > > > +	static FileDescriptor deserialize(std::vector<uint8_t> &data, std::vector<int32_t> &fds,
> > > > +					  [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > > +	{
> > > > +		return IPADataSerializer<FileDescriptor>::deserialize(data.begin(), data.end(),
> > > > +								      fds.begin(), fds.end());
> > > > +	}
> > > > +
> > > > +	static FileDescriptor deserialize(std::vector<uint8_t>::iterator data_it1,
> > > > +					  std::vector<uint8_t>::iterator data_it2,
> > > > +					  std::vector<int32_t>::iterator fds_it1,
> > > > +					  std::vector<int32_t>::iterator fds_it2,
> > > > +					  [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > > +	{
> > > > +		if (std::distance(data_it1, data_it2) < 1)
> > > > +			LOG(IPADataSerializer, Fatal)
> > > > +				<< "Invalid data to deserialize FileDescriptor";
> > > > +
> > > > +		bool valid = *data_it1;
> > >
> > > in C this would be
> > >                 bool valid = !!*data_it1;
> > > not sure about C++ and not even sure if the !! is just a paranoid
> > > kernel convention, as I don't think even C requires that
> >
> > Oh... I'm not sure. It passed my de/serialization tests.
> >
> > > > +
> > > > +		if (valid && std::distance(fds_it1, fds_it2) < 1)
> > >
> > > 		if (valid && std::distance(fds_it1, fds_it2) < 1) {
> > > 			LOG(IPADataSerializer, Fatal)
> > > 				<< "Invalid fds to deserialize FileDescriptor";
> > >                         return {}
> > >                 }
> > >
> > >                 return FileDescriptor(*fds_it1);
> > >
> > > Or is returning {} when std::distance() < 1 wrong ? I don't think so,
> > > as even if it is "valid" there's not fds serialized.
> >
> > If std::distance() < 1 then *fds_it1 will segfault. So if valid and
> > std::distance() < 1, then there's a big problem -> fatal. If not valid,
> > then std::distance() doesn't matter, and we return a new uninitialized
> > FileDescriptor, but we can't construct it using *fds_it1, since the
> > iterator shouldn't be valid.
> >
> 
> I've missed 'Fatal' in the LOG() call. Sorry about this.
> 
> > It's perfectly fine to send an invalid FileDescriptor, for example if
> > the FileDescriptor field isn't used, or uninitialized. The problem with
> > sending it directly is that sendmsg() EINVALs if we try to send -1, so
> > we need this valid flag in the data vector.
> >
> > >
> > > > +			LOG(IPADataSerializer, Fatal)
> > > > +				<< "Invalid fds to deserialize FileDescriptor";
> > > > +
> > > > +		return valid ? FileDescriptor(*fds_it1) : FileDescriptor();
> > > > +	}
> > > > +};
> > > > +
> > > > +template<>
> > > > +class IPADataSerializer<IPASettings>
> > > > +{
> > > > +public:
> > > > +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> > > > +	serialize(const IPASettings &data, [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > > +	{
> > > > +		return IPADataSerializer<std::string>::serialize(data.configurationFile);
> > > > +	}
> > > > +
> > > > +	static IPASettings deserialize(std::vector<uint8_t> &data,
> > > > +				       [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > > +	{
> > > > +		return IPADataSerializer<IPASettings>::deserialize(data.begin(), data.end());
> > > > +	}
> > > > +
> > > > +	static IPASettings deserialize(std::vector<uint8_t>::iterator it1,
> > > > +				       std::vector<uint8_t>::iterator it2,
> > > > +				       [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > > +	{
> > > > +		IPASettings ret;
> > > > +		ret.configurationFile = IPADataSerializer<std::string>::deserialize(it1, it2);
> > > > +
> > > > +		return ret;
> > >
> > >                 return { IPADataSerializer<std::string>::deserialize(it1, it2) };
> > >
> > > ?
> >
> > Ah yes, much needed optimization.
> >
> > > > +	}
> > > > +
> > > > +	static IPASettings deserialize(std::vector<uint8_t> &data,
> > > > +				       [[maybe_unused]] std::vector<int32_t> &fds,
> > > > +				       [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > > +	{
> > > > +		return IPADataSerializer<IPASettings>::deserialize(data.begin(), data.end());
> > >
> > >                 return { IPADataSerializer<std::string>::deserialize(data.begin(), data.end() };
> > >
> > > ?
> >
> > ack
> >
> > > > +	}
> > > > +
> > > > +	static IPASettings deserialize(std::vector<uint8_t>::iterator data_it1,
> > > > +				       std::vector<uint8_t>::iterator data_it2,
> > > > +				       [[maybe_unused]] std::vector<int32_t>::iterator fds_it1,
> > > > +				       [[maybe_unused]] std::vector<int32_t>::iterator fds_it2,
> > > > +				       [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > > +	{
> > > > +		return IPADataSerializer<IPASettings>::deserialize(data_it1, data_it2);
> > >
> > > same
> >
> > ack
> >
> > > > +	}
> > > > +};
> > > > +
> > > > +template<>
> > > > +class IPADataSerializer<CameraSensorInfo>
> > > > +{
> > > > +public:
> > > > +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> > > > +	serialize(const CameraSensorInfo &data, [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > > +	{
> > > > +		std::vector<uint8_t> data_vec;
> > >
> > > Reserving sizeof(CameraSensorInfo) might help
> >
> > It's got a string in it though, so we don't know sizeof(CameraSensorInfo).
> >
> 
> It could be calculated though. Nevermind
> 
> > > I just noticed and I wonder why all this code uses this_naming_style
> > > in place of the libcamera standard namingStyle
> >
> > Ah yes, I should probably fix that...
> >
> 
> Thanks
> 
> > > > +
> > > > +		uint32_t str_len = data.model.size();
> > > > +		appendUInt<uint32_t>(data_vec, str_len);
> > > > +
> > > > +		data_vec.insert(data_vec.end(), data.model.begin(), data.model.end());
> > >
> > > Shouldn't this be serialized using the IPADataSerializer<std::string>
> > > if it's not needed, do we need the specialization then ?
> >
> > It should be :)
> >
> > > > +
> > > > +		appendUInt<uint32_t>(data_vec, data.bitsPerPixel);
> > > > +
> > > > +		appendUInt<uint32_t>(data_vec, data.activeAreaSize.width);
> > > > +		appendUInt<uint32_t>(data_vec, data.activeAreaSize.height);
> > > > +
> > > > +		appendUInt<uint32_t>(data_vec, static_cast<uint32_t>(data.analogCrop.x));
> > > > +		appendUInt<uint32_t>(data_vec, static_cast<uint32_t>(data.analogCrop.y));
> > > > +		appendUInt<uint32_t>(data_vec, data.analogCrop.width);
> > > > +		appendUInt<uint32_t>(data_vec, data.analogCrop.height);
> > > > +
> > > > +		appendUInt<uint32_t>(data_vec, data.outputSize.width);
> > > > +		appendUInt<uint32_t>(data_vec, data.outputSize.height);
> > > > +
> > > > +		appendUInt<uint64_t>(data_vec, data.pixelRate);
> > > > +
> > > > +		appendUInt<uint32_t>(data_vec, data.lineLength);
> > > > +
> > > > +		return {data_vec, {}};
> > > > +	}
> > > > +
> > > > +	static CameraSensorInfo deserialize(std::vector<uint8_t> &data,
> > > > +					    [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > > +	{
> > > > +		return IPADataSerializer<CameraSensorInfo>::deserialize(data.begin(), data.end());
> > > > +	}
> > > > +
> > > > +	static CameraSensorInfo deserialize(std::vector<uint8_t>::iterator it1,
> > > > +					    [[maybe_unused]] std::vector<uint8_t>::iterator it2,
> > > > +					    [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > > +	{
> > > > +		CameraSensorInfo ret;
> > >                 CameraSensorInfo ret{};
> > >
> > > Also for other statically allocated fields that are returned by other
> > > specializations.
> >
> > Do we need to, since all the fields are populated below?
> >
> 
> Technically not, and for all the supported types you fill in all
> fields, you're right. But to be honest I won't mind a
> 0-initialization anyway, mostly because if we happen to add support
> for new types, and if they won't be fully-filled, we keep the good
> practice of {} and we're safe. A bit paranoid maybe :)

Hm maybe better paranoid than segfault?

If we add new fields, the de/serializer will be updated to serialize
from the field, and deserialize to the field. All the fields will be
written to, and they'll be zero-initialized by readPOD().

Which reminds me... if readPOD() can return -1 and error then we'll
have to check every single readPOD()... that's going to make serdes code
huge...

Maybe that's why I just had it return zero, so that we can just continue
instead of fataling. I think I'll just print an error and continue,
instead of silently returning zero.

> > > > +
> > > > +		uint32_t str_len = readUInt<uint32_t>(it1);
> > > > +		std::string str(it1 + 4, it1 + 4 + str_len);
> > > > +		ret.model = str;
> > > > +
> > > > +		std::vector<uint8_t>::iterator it = it1 + 4 + str_len;
> > > > +
> > > > +		ret.bitsPerPixel = readUInt<uint32_t>(it);
> > > > +
> > > > +		ret.activeAreaSize.width = readUInt<uint32_t>(it + 4);
> > > > +		ret.activeAreaSize.height = readUInt<uint32_t>(it + 8);
> > > > +
> > > > +		ret.analogCrop.x = static_cast<int32_t>(readUInt<uint32_t>(it + 12));
> > > > +		ret.analogCrop.y = static_cast<int32_t>(readUInt<uint32_t>(it + 16));
> > > > +		ret.analogCrop.width = readUInt<uint32_t>(it + 20);
> > > > +		ret.analogCrop.height = readUInt<uint32_t>(it + 24);
> > > > +
> > > > +		ret.outputSize.width = readUInt<uint32_t>(it + 28);
> > > > +		ret.outputSize.height = readUInt<uint32_t>(it + 32);
> > > > +
> > > > +		ret.pixelRate = readUInt<uint64_t>(it + 36);
> > > > +
> > > > +		ret.lineLength = readUInt<uint64_t>(it + 44);
> > >
> > > lineLength is a 32 bits integer
> >
> > Ah, yes.
> >
> > > > +
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	static CameraSensorInfo deserialize(std::vector<uint8_t> &data,
> > > > +					    [[maybe_unused]] std::vector<int32_t> &fds,
> > > > +					    [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > > +	{
> > > > +		return IPADataSerializer<CameraSensorInfo>::deserialize(data.begin(), data.end());
> > > > +	}
> > > > +
> > > > +	static CameraSensorInfo deserialize(std::vector<uint8_t>::iterator data_it1,
> > > > +					    std::vector<uint8_t>::iterator data_it2,
> > > > +					    [[maybe_unused]] std::vector<int32_t>::iterator fds_it1,
> > > > +					    [[maybe_unused]] std::vector<int32_t>::iterator fds_it2,
> > > > +					    [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > > +	{
> > > > +		return IPADataSerializer<CameraSensorInfo>::deserialize(data_it1, data_it2);
> > > > +	}
> > > > +};
> > > > +
> > > > +template<>
> > > > +class IPADataSerializer<IPAStream>
> > > > +{
> > > > +public:
> > > > +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> > > > +	serialize(const IPAStream &data, [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > > +	{
> > > > +		std::vector<uint8_t> data_vec;
> > > > +
> > > > +		appendUInt<uint32_t>(data_vec, data.pixelFormat);
> > > > +
> > > > +		appendUInt<uint32_t>(data_vec, data.size.width);
> > > > +		appendUInt<uint32_t>(data_vec, data.size.height);
> > > > +
> > > > +		return {data_vec, {}};
> > > > +	}
> > > > +
> > > > +	static IPAStream deserialize(std::vector<uint8_t> &data,
> > > > +				     [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > > +	{
> > > > +		return IPADataSerializer<IPAStream>::deserialize(data.begin(), data.end());
> > > > +	}
> > > > +
> > > > +	static IPAStream deserialize(std::vector<uint8_t>::iterator it1,
> > > > +				     [[maybe_unused]] std::vector<uint8_t>::iterator it2,
> > > > +				     [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > > +	{
> > > > +		IPAStream ret;
> > > > +
> > > > +		ret.pixelFormat = readUInt<uint32_t>(it1);
> > > > +
> > > > +		ret.size.width = readUInt<uint32_t>(it1 + 4);
> > > > +		ret.size.height = readUInt<uint32_t>(it1 + 8);
> > > > +
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	static IPAStream deserialize(std::vector<uint8_t> &data,
> > > > +				     [[maybe_unused]] std::vector<int32_t> &fds,
> > > > +				     [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > > +	{
> > > > +		return IPADataSerializer<IPAStream>::deserialize(data.begin(), data.end());
> > > > +	}
> > > > +
> > > > +	static IPAStream deserialize(std::vector<uint8_t>::iterator data_it1,
> > > > +				     std::vector<uint8_t>::iterator data_it2,
> > > > +				     [[maybe_unused]] std::vector<int32_t>::iterator fds_it1,
> > > > +				     [[maybe_unused]] std::vector<int32_t>::iterator fds_it2,
> > > > +				     [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > > +	{
> > > > +		return IPADataSerializer<IPAStream>::deserialize(data_it1, data_it2);
> > > > +	}
> > > > +};
> > > > +
> > > > +template<>
> > > > +class IPADataSerializer<const ControlList>
> > > > +{
> > > > +public:
> > > > +	/* map arg will be generated, since it's per-pipeline anyway. */
> > > > +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> > > > +	serialize(const ControlList &data, const ControlInfoMap &map,
> > > > +		  ControlSerializer *cs)
> > > > +	{
> > > > +		if (!cs)
> > > > +			LOG(IPADataSerializer, Fatal)
> > > > +				<< "ControlSerializer not provided for serialization of ControlList";
> > > > +
> > > > +		size_t size = cs->binarySize(map);
> > > > +		std::vector<uint8_t> infoData(size);
> > > > +		ByteStreamBuffer buffer(infoData.data(), infoData.size());
> > > > +		int ret = cs->serialize(map, buffer);
> > > > +
> > > > +		if (ret < 0 || buffer.overflow()) {
> > > > +			LOG(IPADataSerializer, Error) << "Failed to serialize ControlList's ControlInfoMap";
> > > > +			return {{}, {}};
> > > > +		}
> > > > +
> > > > +		size = cs->binarySize(data);
> > > > +		std::vector<uint8_t> listData(size);
> > > > +		buffer = ByteStreamBuffer(listData.data(), listData.size());
> > > > +		ret = cs->serialize(data, buffer);
> > > > +
> > > > +		if (ret < 0 || buffer.overflow()) {
> > > > +			LOG(IPADataSerializer, Error) << "Failed to serialize ControlList";
> > > > +			return {{}, {}};
> > > > +		}
> > > > +
> > > > +		std::vector<uint8_t> data_vec;
> > >
> > > You know the sizes and can reserve ?
> >
> > At this point. yes.
> >
> > > > +		appendUInt<uint32_t>(data_vec, infoData.size());
> > > > +		appendUInt<uint32_t>(data_vec, listData.size());
> > > > +		data_vec.insert(data_vec.end(), infoData.begin(), infoData.end());
> > > > +		data_vec.insert(data_vec.end(), listData.begin(), listData.end());
> > > > +
> > > > +		return {data_vec, {}};
> > > > +	}
> > > > +
> > > > +	static const ControlList deserialize(std::vector<uint8_t> &data, ControlSerializer *cs)
> > > > +	{
> > > > +		return IPADataSerializer<const ControlList>::deserialize(data.begin(), data.end(), cs);
> > > > +	}
> > > > +
> > > > +	static const ControlList deserialize(std::vector<uint8_t>::iterator it1,
> > > > +				       [[maybe_unused]] std::vector<uint8_t>::iterator it2,
> > > > +				       ControlSerializer *cs)
> > > > +	{
> > > > +		if (!cs)
> > > > +			LOG(IPADataSerializer, Fatal)
> > > > +				<< "ControlSerializer not provided for deserialization of ControlList";
> > > > +
> > > > +		uint32_t infoDataSize = readUInt<uint32_t>(it1);
> > > > +		uint32_t listDataSize = readUInt<uint32_t>(it1 + 4);
> > > > +
> > > > +		std::vector<uint8_t>::iterator it = it1 + 8;
> > > > +
> > > > +		std::vector<uint8_t> infoData(it, it + infoDataSize);
> > > > +		std::vector<uint8_t> listData(it + infoDataSize, it + infoDataSize + listDataSize);
> > > > +
> > > > +		ByteStreamBuffer buffer(const_cast<const uint8_t *>(infoData.data()), infoData.size());
> > > > +		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();
> > > > +		}
> > > > +
> > > > +		buffer = ByteStreamBuffer(const_cast<const uint8_t *>(listData.data()), listData.size());
> > > > +		ControlList list = cs->deserialize<ControlList>(buffer);
> > > > +		if (buffer.overflow())
> > > > +			LOG(IPADataSerializer, Error) << "Failed to deserialize ControlList: buffer overflow";
> > > > +		if (list.empty())
> > > > +			LOG(IPADataSerializer, Error) << "Failed to deserialize ControlList: empty list";
> > >
> > > Should we return {} in these cases or is it fine ?
> >
> > ControlSerializer::deserialize() returns {} in these cases anyway, so it
> > should be fine.
> >
> > > > +
> > > > +		return list;
> > > > +	}
> > > > +
> > > > +	static const ControlList deserialize(std::vector<uint8_t> &data,
> > > > +				       [[maybe_unused]] std::vector<int32_t> &fds,
> > > > +				       ControlSerializer *cs)
> > > > +	{
> > > > +		return IPADataSerializer<const ControlList>::deserialize(data.begin(), data.end(), cs);
> > > > +	}
> > > > +
> > > > +	static const ControlList deserialize(std::vector<uint8_t>::iterator data_it1,
> > > > +				       std::vector<uint8_t>::iterator data_it2,
> > > > +				       [[maybe_unused]] std::vector<int32_t>::iterator fds_it1,
> > > > +				       [[maybe_unused]] std::vector<int32_t>::iterator fds_it2,
> > > > +				       ControlSerializer *cs)
> > > > +	{
> > > > +		return IPADataSerializer<const ControlList>::deserialize(data_it1, data_it2, cs);
> > > > +	}
> > > > +};
> > > > +
> > > > +template<>
> > > > +class IPADataSerializer<ControlList>
> > > > +{
> > > > +public:
> > > > +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> > > > +	serialize(const ControlList &data, const ControlInfoMap &map,
> > > > +		  ControlSerializer *cs)
> > > > +	{
> > > > +		const ControlList &list_const = const_cast<const ControlList &>(data);
> > > > +
> > > > +		std::vector<uint8_t> data_vec;
> > > > +		std::tie(data_vec, std::ignore) =
> > > > +			IPADataSerializer<const ControlList>::serialize(list_const, map, cs);
> > > > +
> > > > +		return {data_vec, {}};
> > > > +	}
> > > > +
> > > > +	static ControlList deserialize(std::vector<uint8_t> &data,
> > > > +				       ControlSerializer *cs)
> > > > +	{
> > > > +		ControlList ret;
> > > > +		const ControlList &list = ret;
> > > > +		const_cast<ControlList &>(list) =
> > > > +			IPADataSerializer<const ControlList>::deserialize(data, cs);
> > > > +
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	static ControlList deserialize(std::vector<uint8_t>::iterator it1,
> > > > +				       [[maybe_unused]] std::vector<uint8_t>::iterator it2,
> > > > +				       ControlSerializer *cs)
> > > > +	{
> > > > +		ControlList ret;
> > > > +		const ControlList &list = ret;
> > > > +		const_cast<ControlList &>(list) =
> > > > +			IPADataSerializer<const ControlList>::deserialize(it1, it2, cs);
> > >
> > > Why not
> > >                 const ControlList list =
> > > 			IPADataSerializer<const ControlList>::deserialize(it1, it2, cs);
> > >                 return const_cast<ControlList>(list)
> >
> > My understanding is that to remove a const with const_cast, the original
> > variable must not be const.
> 
> You're right, this won't work, cast should happen on a pointer or
> reference.
> 
> This works with gcc and clang:
> 
> 		const ControlList &list =
> 			IPADataSerializer<const ControlList>::deserialize(data, cs);
> 
> 		return const_cast<ControlList &>(list);
> 
> As the function returns by value it should be ok.
> Tests pass as well, but I'm not sure if this case is tested though.
> 
> >
> > > > +
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > + */
> 
> [snip]
> 
> > > > +
> > > > +/**
> > > > + * \fn template<typename T> T readUInt(std::vector<uint8_t> &vec, size_t pos)
> > > > + * \brief Read uint from byte vector, in little-endian order
> > > > + * \tparam T Type of uint to read
> > > > + * \param[in] vec Byte vector to read from
> > > > + * \param[in] pos Index in vec to start reading from
> > > > + *
> > >
> > > No need for an empty line if no additional description is provided
> >
> > ack
> >
> > > > + * \return The uint read from \a vec, or 0 if reading goes past end of \a vec
> > >
> > > I wonder if we should not pass a T * as an output parameter and return
> > > an error code. Returning 0 makes it impossible to detect failure, as 0
> > > is a valid value
> >
> > I was actually wondering about that since when I wrote it in the first
> > place. This might be a good solution.
> 
> Up to you. Do you check for the return value or do you need to do so ?

Now that I think about it, I don't think we should return error. I think
just logging error and return default value and continuing is fine.

Although maybe it would be better to use the default value instead of
zero, if a default value is specified.

Ah, that's why I used default constructor, rather than zero constructor
in the deserializer! Well, that's the case for the generated
de/serializer at least. ...which still doesn't work because this will
return zero and it'll get written...

But also, if this errors, it means something is *really* wrong with
deserialization, as in, we didn't receive enough data to deserialize the
expected object. So maybe we should be fataling instead of
continuing...?

Not sure which way to go...

> >
> > > > + */
> > > > +
> > > > +/**
> > > > + * \fn template<typename T> T readUInt(std::vector<uint8_t>::iterator it)
> > > > + * \brief Read uint from byte vector, in little-endian order
> > > > + * \tparam T Type of uint to read
> > > > + * \param[in] it Iterator of byte vector to read from
> > > > + *
> > >
> > > No need to empty line
> >
> > ack
> >
> > > > + * \return The uint read from \a vec
> > > > + */
> > > > +
> > > > +/**
> > > > + * \fn template<typename T> IPADataSerializer<T>::serialize(
> > > > + * 	T data,
> > > > + * 	ControlSerializer *cs = nullptr)
> > > > + * \brief Serialize an object into byte vector and fd vector
> > > > + * \tparam T Type of object to serialize
> > > > + * \param[in] data Object to serialize
> > > > + * \param[in] cs ControlSerializer
> > > > + *
> > > > + * \a cs is only necessary if the object type \a T or its members contain
> > > > + * ControlList or ControlInfoMap.
> > > > + *
> > > > + * \return Tuple of byte vector and fd vector, that is the serialized form
> > > > + * of \a data
> > >
> > > Maybe later, but I think for each template specialization, we want to
> > > document the serialization format, in example that for vectors, the
> > > length is the first 4 bytes and so on (even more for custom libcamera
> > > data types)
> >
> > Good idea.
> 
> Although it won't be parsed by doxygen and will remain in-code only.
> But knowing how each container will be serialized might help
> validating it.

Yeah.

> Actually, how does it work for closed source IPAs ? Before this series
> the pipeline handler and the IPA were in charge of
> serializing/deserializing with custom code, as the close source blob,
> possibly written in C, wouldn't be able to link against libcamera and
> use its helpers (better, they might chose not to, as from a compliance
> pov it's fine linking a blob against L-GPL code). Is it still possible
> ? In that case, documenting how things gets serialized might be
> even more important.

Now we have the custom IPA interface, and the header that's generated
from it. This header and the public libcamera headers contain the
definitions of all the objects that would be passed between the pipeline
handler and the IPA. The proxy worker links to the IPA, which exposes
ipaCreate() (extern C) which returns an IPA{pipeline_name}Interface (C++).
So this way, the proxy worker can call the IPA functions in the IPA
directly, while passing C++ objects directly!

As for the other direction, since Signal is in a public header, simply
including that will allow closed-source IPAs to emit Signals to the
proxy worker... right?


Thanks,

Paul