[libcamera-devel] [PATCH v2 5/5] libcamera: control_serializer: Separate the handles space
paul.elder at ideasonboard.com
paul.elder at ideasonboard.com
Wed Sep 8 03:15:45 CEST 2021
Hi Jacopo,
On Tue, Sep 07, 2021 at 01:10:38PM +0200, Jacopo Mondi wrote:
> Two instances of the ControlSerializer class are in use at the IPC
> boundaries, one in the Proxy class that serializes data from the
> pipeline handler to the IPA, and one in the ProxyWorker which serializes
> data in the opposite direction.
>
> Each instance operates autonomously, without any centralized point of
> control, and each one assigns a numerical handle to each ControlInfoMap
> it serializes. This creates a risk of potential collision on the handle
> values, as both instances will use the same numerical space and
> are not aware of what handles has been already used by the instance "on
> the other side".
>
> To fix that, partition the handles numerical space by initializing the
> control serializer with a seed (even or odd) and increment the handle
> number by 2, to avoid any collision risk.
>
> While this is temporary and rather hacky solution, it solves an issue
> with isolated IPA modules without too much complexity added.
>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> .../libcamera/internal/control_serializer.h | 8 ++-
> src/libcamera/control_serializer.cpp | 60 +++++++++++++++++--
> test/serialization/control_serialization.cpp | 4 +-
> .../ipa_data_serializer_test.cpp | 26 ++++----
> .../module_ipa_proxy.cpp.tmpl | 6 +-
> .../module_ipa_proxy.h.tmpl | 2 +-
> .../module_ipa_proxy_worker.cpp.tmpl | 14 +++--
> .../libcamera_templates/proxy_functions.tmpl | 4 +-
> 8 files changed, 97 insertions(+), 27 deletions(-)
>
> diff --git a/include/libcamera/internal/control_serializer.h b/include/libcamera/internal/control_serializer.h
> index 8a66be324138..f96c6f8443d1 100644
> --- a/include/libcamera/internal/control_serializer.h
> +++ b/include/libcamera/internal/control_serializer.h
> @@ -20,7 +20,12 @@ class ByteStreamBuffer;
> class ControlSerializer
> {
> public:
> - ControlSerializer();
> + enum class Seeds {
> + EVEN = 0,
> + ODD
> + };
I was going to say we could add a MAX and then you can += MAX and
support any number of seeds, but that's probably overkill for a clever
hack for IPC that only has two sides.
Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> +
> + ControlSerializer(Seeds seed);
>
> void reset();
>
> @@ -47,6 +52,7 @@ private:
> ControlInfo loadControlInfo(ControlType type, ByteStreamBuffer &buffer);
>
> unsigned int serial_;
> + enum Seeds seed_;
> std::vector<std::unique_ptr<ControlId>> controlIds_;
> std::vector<std::unique_ptr<ControlIdMap>> controlIdMaps_;
> std::map<unsigned int, ControlInfoMap> infoMaps_;
> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp
> index f1245ea620ab..a60e665af5ae 100644
> --- a/src/libcamera/control_serializer.cpp
> +++ b/src/libcamera/control_serializer.cpp
> @@ -62,6 +62,14 @@ LOG_DEFINE_CATEGORY(Serializer)
> * corresponding ControlInfoMap handle in the binary data, and when
> * deserializing to retrieve the corresponding ControlInfoMap.
> *
> + * As a ControlSerializer instance is used at both sides of the IPC boundary,
> + * and the two instances operate without a shared point of control, there is a
> + * potential risk of collision of the numerical handles assigned to each
> + * serialized ControlInfoMap. For this reason the control serializer is
> + * initialized with an even/odd seed and the handle is incremented by 2, so that
> + * instances initialized with a different seed operates on a separate numerical
> + * space, avoiding any collision risk.
> + *
> * In order to perform those tasks, the serializer keeps an internal state that
> * needs to be properly populated. This mechanism requires the ControlInfoMap
> * corresponding to a ControlList to have been serialized or deserialized
> @@ -77,9 +85,42 @@ LOG_DEFINE_CATEGORY(Serializer)
> * proceed with care to avoid stale references.
> */
>
> -ControlSerializer::ControlSerializer()
> - : serial_(0)
> +/**
> + * \enum ControlSerializer::Seeds
> + * \brief Define the seeds to initialize the ControlSerializer handle numerical
> + * space with
> + *
> + * \var ControlSerializer::Seeds::EVEN
> + * \brief The control serializer uses even numerical handles
> + *
> + * \var ControlSerializer::Seeds::ODD
> + * \brief The control serializer uses odd numerical handles
> + */
> +
> +/**
> + * \brief Construct a new ControlSerializer and initialize its seed
> + * \param[in] seed The serializer's handle seed
> + */
> +ControlSerializer::ControlSerializer(Seeds seed)
> + : seed_(seed)
> {
> + /*
> + * Initialize the handle numerical space using the seed value.
> + *
> + * Instances initialized with a different seed will use a different
> + * numerical handle space, avoiding any collision risk when, in example,
> + * two instances of the ControlSerializer class are used at the IPC
> + * boundaries.
> + *
> + * Start from 1 as '0' is a special value used as handle place holder
> + * when serializing lists that do not have a ControlInfoMap associated
> + * (in example list of libcamera controls::controls).
> + *
> + * \todo This is a temporary hack and should probably be better
> + * engineered, but for the time being it avoid collisions on the handle
> + * value when using IPC.
> + */
> + serial_ = seed_ == Seeds::EVEN ? 2 : 1;
> }
>
> /**
> @@ -90,7 +131,7 @@ ControlSerializer::ControlSerializer()
> */
> void ControlSerializer::reset()
> {
> - serial_ = 0;
> + serial_ = seed_ == Seeds::EVEN ? 2 : 1;
>
> infoMapHandles_.clear();
> infoMaps_.clear();
> @@ -200,10 +241,10 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,
> else
> idMapType = IPA_CONTROL_ID_MAP_V4L2;
>
> - /* Prepare the packet header, assign a handle to the ControlInfoMap. */
> + /* Prepare the packet header. */
> struct ipa_controls_header hdr;
> hdr.version = IPA_CONTROLS_FORMAT_VERSION;
> - hdr.handle = ++serial_;
> + hdr.handle = serial_;
> hdr.entries = infoMap.size();
> hdr.size = sizeof(hdr) + entriesSize + valuesSize;
> hdr.data_offset = sizeof(hdr) + entriesSize;
> @@ -211,6 +252,15 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,
>
> buffer.write(&hdr);
>
> + /*
> + * Increment the handle for the ControlInfoMap by 2 keep the handles
> + * numerical space partitioned between instances initialized with a
> + * different seed.
> + *
> + * \sa ControlSerializer::Seeds
> + */
> + serial_ += 2;
> +
> /*
> * Serialize all entries.
> * \todo Serialize the control name too
> diff --git a/test/serialization/control_serialization.cpp b/test/serialization/control_serialization.cpp
> index 5ac9c4ede5f9..89c8ba59de24 100644
> --- a/test/serialization/control_serialization.cpp
> +++ b/test/serialization/control_serialization.cpp
> @@ -30,8 +30,8 @@ protected:
>
> int run() override
> {
> - ControlSerializer serializer;
> - ControlSerializer deserializer;
> + ControlSerializer serializer(ControlSerializer::Seeds::EVEN);
> + ControlSerializer deserializer(ControlSerializer::Seeds::ODD);
>
> std::vector<uint8_t> infoData;
> std::vector<uint8_t> listData;
> diff --git a/test/serialization/ipa_data_serializer_test.cpp b/test/serialization/ipa_data_serializer_test.cpp
> index eca19a6642e1..a1ce8e3767bc 100644
> --- a/test/serialization/ipa_data_serializer_test.cpp
> +++ b/test/serialization/ipa_data_serializer_test.cpp
> @@ -10,6 +10,7 @@
> #include <fcntl.h>
> #include <iostream>
> #include <limits>
> +#include <memory>
> #include <stdlib.h>
> #include <sys/stat.h>
> #include <sys/types.h>
> @@ -162,23 +163,24 @@ private:
>
> int testControls()
> {
> - ControlSerializer cs;
> + std::unique_ptr<ControlSerializer> cs =
> + std::make_unique<ControlSerializer>(ControlSerializer::Seeds::EVEN);
>
> const ControlInfoMap &infoMap = camera_->controls();
> ControlList list = generateControlList(infoMap);
>
> std::vector<uint8_t> infoMapBuf;
> std::tie(infoMapBuf, std::ignore) =
> - IPADataSerializer<ControlInfoMap>::serialize(infoMap, &cs);
> + IPADataSerializer<ControlInfoMap>::serialize(infoMap, cs.get());
>
> std::vector<uint8_t> listBuf;
> std::tie(listBuf, std::ignore) =
> - IPADataSerializer<ControlList>::serialize(list, &cs);
> + IPADataSerializer<ControlList>::serialize(list, cs.get());
>
> const ControlInfoMap infoMapOut =
> - IPADataSerializer<ControlInfoMap>::deserialize(infoMapBuf, &cs);
> + IPADataSerializer<ControlInfoMap>::deserialize(infoMapBuf, cs.get());
>
> - ControlList listOut = IPADataSerializer<ControlList>::deserialize(listBuf, &cs);
> + ControlList listOut = IPADataSerializer<ControlList>::deserialize(listBuf, cs.get());
>
> if (!SerializationTest::equals(infoMap, infoMapOut)) {
> cerr << "Deserialized map doesn't match original" << endl;
> @@ -195,7 +197,8 @@ private:
>
> int testVector()
> {
> - ControlSerializer cs;
> + std::unique_ptr<ControlSerializer> cs =
> + std::make_unique<ControlSerializer>(ControlSerializer::Seeds::EVEN);
>
> /*
> * We don't test FileDescriptor serdes because it dup()s, so we
> @@ -257,7 +260,7 @@ private:
> if (testVectorSerdes(vecString) != TestPass)
> return TestFail;
>
> - if (testVectorSerdes(vecControlInfoMap, &cs) != TestPass)
> + if (testVectorSerdes(vecControlInfoMap, cs.get()) != TestPass)
> return TestFail;
>
> return TestPass;
> @@ -265,7 +268,8 @@ private:
>
> int testMap()
> {
> - ControlSerializer cs;
> + std::unique_ptr<ControlSerializer> cs =
> + std::make_unique<ControlSerializer>(ControlSerializer::Seeds::EVEN);
>
> /*
> * Realistically, only string and integral keys.
> @@ -302,13 +306,13 @@ private:
> if (testMapSerdes(mapStrStr) != TestPass)
> return TestFail;
>
> - if (testMapSerdes(mapUintCIM, &cs) != TestPass)
> + if (testMapSerdes(mapUintCIM, cs.get()) != TestPass)
> return TestFail;
>
> - if (testMapSerdes(mapIntCIM, &cs) != TestPass)
> + if (testMapSerdes(mapIntCIM, cs.get()) != TestPass)
> return TestFail;
>
> - if (testMapSerdes(mapStrCIM, &cs) != TestPass)
> + if (testMapSerdes(mapStrCIM, cs.get()) != TestPass)
> return TestFail;
>
> if (testMapSerdes(mapUintBVec) != TestPass)
> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> index aab1fffbcaf0..7e28c2ed129a 100644
> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> @@ -52,6 +52,8 @@ namespace {{ns}} {
> << "initializing {{module_name}} proxy: loading IPA from "
> << ipam->path();
>
> + controlSerializer_ = new ControlSerializer(ControlSerializer::Seeds::EVEN);
> +
> if (isolate_) {
> const std::string proxyWorkerPath = resolvePath("{{module_name}}_ipa_proxy");
> if (proxyWorkerPath.empty()) {
> @@ -95,6 +97,8 @@ namespace {{ns}} {
>
> {{proxy_name}}::~{{proxy_name}}()
> {
> + delete controlSerializer_;
> +
> if (isolate_) {
> IPCMessage::Header header =
> { static_cast<uint32_t>({{cmd_enum_name}}::Exit), seq_++ };
> @@ -185,7 +189,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)
> {{proxy_funcs.func_sig(proxy_name, method, "IPC")}}
> {
> {%- if method.mojom_name == "configure" %}
> - controlSerializer_.reset();
> + controlSerializer_->reset();
> {%- endif %}
> {%- set has_output = true if method|method_param_outputs|length > 0 or method|method_return_value != "void" %}
> {%- set cmd = cmd_enum_name + "::" + method.mojom_name|cap %}
> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
> index 1979e68ff74d..fdbf25418963 100644
> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
> @@ -118,7 +118,7 @@ private:
>
> std::unique_ptr<IPCPipeUnixSocket> ipc_;
>
> - ControlSerializer controlSerializer_;
> + ControlSerializer *controlSerializer_;
>
> {# \todo Move this to IPCPipe #}
> uint32_t seq_;
> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
> index c5e51532db53..5829059b8e93 100644
> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
> @@ -53,9 +53,15 @@ class {{proxy_worker_name}}
> {
> public:
> {{proxy_worker_name}}()
> - : ipa_(nullptr), exit_(false) {}
> + : ipa_(nullptr), exit_(false)
> + {
> + controlSerializer_ = new ControlSerializer(ControlSerializer::Seeds::ODD);
> + }
>
> - ~{{proxy_worker_name}}() {}
> + ~{{proxy_worker_name}}()
> + {
> + delete controlSerializer_;
> + }
>
> void readyRead()
> {
> @@ -81,7 +87,7 @@ public:
> case {{cmd_enum_name}}::{{method.mojom_name|cap}}: {
>
> {%- if method.mojom_name == "configure" %}
> - controlSerializer_.reset();
> + controlSerializer_->reset();
> {%- endif %}
> {{proxy_funcs.deserialize_call(method|method_param_inputs, '_ipcMessage.data()', '_ipcMessage.fds()', false, true)|indent(8, true)}}
> {% for param in method|method_param_outputs %}
> @@ -180,7 +186,7 @@ private:
> {{interface_name}} *ipa_;
> IPCUnixSocket socket_;
>
> - ControlSerializer controlSerializer_;
> + ControlSerializer *controlSerializer_;
>
> bool exit_;
> };
> diff --git a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> index ebcd2aaaafae..d78e5c53566f 100644
> --- a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> +++ b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> @@ -60,7 +60,7 @@
> std::tie({{param.mojom_name}}Buf, std::ignore) =
> {%- endif %}
> IPADataSerializer<{{param|name}}>::serialize({{param.mojom_name}}
> -{{- ", &controlSerializer_" if param|needs_control_serializer -}}
> +{{- ", controlSerializer_" if param|needs_control_serializer -}}
> );
> {%- endfor %}
>
> @@ -119,7 +119,7 @@
> {%- endif -%}
> {{- "," if param|needs_control_serializer}}
> {%- if param|needs_control_serializer %}
> - &controlSerializer_
> + controlSerializer_
> {%- endif -%}
> );
> {%- endmacro -%}
> --
> 2.32.0
>
More information about the libcamera-devel
mailing list