[libcamera-devel] [PATCH v4 08/37] libcamera: Add IPADataSerializer
Jacopo Mondi
jacopo at jmondi.org
Wed Nov 18 11:47:34 CET 2020
Hi Paul,
On Wed, Nov 18, 2020 at 07:20:14PM +0900, paul.elder at ideasonboard.com wrote:
> Hi Jacopo,
>
> On Tue, Nov 17, 2020 at 03:48:05PM +0100, Jacopo Mondi wrote:
> > Hi Paul,
> >
> > On Fri, Nov 06, 2020 at 07:36:38PM +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 at ideasonboard.com>
> > >
> > > ---
> > > Changes in v4:
> > > - rename readUInt/appendUInt to readPOD/appendPOD
> > > - put them in anonymous namespace
> > > - and add length check
> > > - fatal if failure, because it means not enough data to deserialize,
> > > which is technically a segfault
> > > - use the new readPOD/appendPOD with length protections
> > > - expand on their docs correspondingly
> > > - change snake_case to camelCase
> > > - add optimizations to the hand-written de/serializers
> > > - reserve vector length where trivially possible
> > > - remove unnecessary IPADataSerializer<type>:: explicit calls (if
> > > they're calling a specialization from the same specialization)
> > >
> > > 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 | 1004 +++++++++++++++++
> > > src/libcamera/ipa_data_serializer.cpp | 178 +++
> > > src/libcamera/meson.build | 1 +
> > > 3 files changed, 1183 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..9a18f914
> > > --- /dev/null
> > > +++ b/include/libcamera/internal/ipa_data_serializer.h
> > > @@ -0,0 +1,1004 @@
> > > +/* 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)
> > > +
> > > +namespace {
> > > +
> > > +template<typename T>
> > > +void appendPOD(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 readPOD(std::vector<uint8_t> &vec, size_t pos)
> > > +{
> > > + T ret = 0;
> > > + size_t byteWidth = sizeof(ret);
> > > + if (pos + byteWidth > vec.size())
> > > + LOG(IPADataSerializer, Fatal)
> > > + << "Not enough data to deserialize POD";
> > > +
> > > + memcpy(&ret, &vec[pos], byteWidth);
> > > + return ret;
> > > +}
> >
> > Can this function just be:
> > return readPOD<T>(vec.begin(), pos, vec.end());
>
> Oh, it can. It has to be moved below the next one, though.
>
> > > +
> > > +template<typename T>
> > > +T readPOD(std::vector<uint8_t>::iterator it, size_t pos,
> > > + std::vector<uint8_t>::iterator end)
> > > +{
> > > + T ret = 0;
> > > + size_t byteWidth = sizeof(ret);
> > > +
> > Can you align to this style (empty line between the variable declarations
> > and the if()) in the two readPOD implementations ?
> >
> > > + if (pos + it >= end)
> > > + LOG(IPADataSerializer, Fatal)
> > > + << "Not enough data to deserialize POD";
> > > +
> > > + std::vector<uint8_t> v(it + pos, it + pos + byteWidth);
> > > + memcpy(&ret, v.data(), byteWidth);
> >
> > Can you
> > it += pos;
> > memcpy(&ret, &(*it), byteWidth);
> >
> > Instead of going through a vector ?
> >
> > The end result looks like:
> >
> > template<typename T>
> > T readPOD(std::vector<uint8_t>::iterator it, size_t pos,
> > std::vector<uint8_t>::iterator end)
> > {
> > if (pos + it >= end)
> > LOG(IPADataSerializer, Fatal)
> > << "Not enough data to deserialize POD";
> >
> > T ret = 0;
> > memcpy(&ret, &(*(it + pos)), sizeof(ret));
> >
> > return ret;
> > }
> >
> > template<typename T>
> > T readPOD(std::vector<uint8_t> &vec, size_t pos)
> > {
> > return readPOD<T>(vec.begin(), pos, vec.end());
> > }
>
> Yeah, it passes the tests!
>
> > (I would have liked to run tests, but serialization tests get skipped
> > and I've not investigated tbh)
>
> The IPA serialization test needs vimc.
>
TIL `modprobe vivid vimc` only loads vivid :/
Sorry about this
> > > + return ret;
> > > +}
> > > +
> > > +} /* namespace */
> > > +
> > > +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 dataBegin,
> > > + std::vector<uint8_t>::iterator dataEnd,
> > > + ControlSerializer *cs);
> > > +
> > > + static T deserialize(std::vector<uint8_t> &data,
> > > + std::vector<int32_t> &fds,
> > > + ControlSerializer *cs);
> > > + static T deserialize(std::vector<uint8_t>::iterator dataBegin,
> > > + std::vector<uint8_t>::iterator dataEnd,
> > > + std::vector<int32_t>::iterator fdsBegin,
> > > + std::vector<int32_t>::iterator fdsEnd,
> > > + ControlSerializer *cs);
> > > +#endif /* __DOXYGEN__ */
> >
> > I've already expressed my perplexity for this #ifdef, and I know you
> > have answered but I didn't get it :) you want this parsed by doxygen,
> > isn't it equal from removing the ifdef guard ?
>
> Oh huh, for some reason I was worried about template resolution
> problems. You're right, it works even without the ifdef guard.
>
Great, let's drop it!
> > > +};
> > > +
[snip]
> > > +
> > > +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> dataVec;
> > > +
> > > + uint32_t strLen = data.model.size();
> > > + appendPOD<uint32_t>(dataVec, strLen);
> > > +
> > > + dataVec.insert(dataVec.end(), data.model.begin(), data.model.end());
> >
> > Can this be done using the <string> IPADataSerializer specialization ?
>
> It can, but this is hand-written so it can be hand-optimized.
>
Yeah, we save a few calls indeed
> > > +
> > > + appendPOD<uint32_t>(dataVec, data.bitsPerPixel);
> > > +
> > > + appendPOD<uint32_t>(dataVec, data.activeAreaSize.width);
> > > + appendPOD<uint32_t>(dataVec, data.activeAreaSize.height);
> > > +
> > > + appendPOD<uint32_t>(dataVec, static_cast<uint32_t>(data.analogCrop.x));
> > > + appendPOD<uint32_t>(dataVec, static_cast<uint32_t>(data.analogCrop.y));
> > > + appendPOD<uint32_t>(dataVec, data.analogCrop.width);
> > > + appendPOD<uint32_t>(dataVec, data.analogCrop.height);
> >
> > Depending on how many users we could potentially have <Size> and
> > <Rectangle> serializer might be an helpful helper
>
> Oh yeah good idea, I'll provide Size and Rectangle as available structs
> in IPA interfaces.
>
> But here it'll probably still be hand-optimized :)
>
That's ok and you can do on-top not to delay this series even more
> > > +
> > > + appendPOD<uint32_t>(dataVec, data.outputSize.width);
> > > + appendPOD<uint32_t>(dataVec, data.outputSize.height);
[snip]
> >
> > Still wishing for the serialization format for each type to be
>
> I added it; it's in the next version :)
>
great!
Thank you!
> > described, but this version is very nice according to my tastes!
>
> \o/
>
> > Thanks for keep pushing and addressing the numerous comments!
> >
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>
>
> Thanks,
>
> Paul
>
> > > --
> > > 2.27.0
> > >
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel at lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list