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 + }; + + ControlSerializer(Seeds seed); void reset(); @@ -47,6 +52,7 @@ private: ControlInfo loadControlInfo(ControlType type, ByteStreamBuffer &buffer); unsigned int serial_; + enum Seeds seed_; std::vector> controlIds_; std::vector> controlIdMaps_; std::map 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 infoData; std::vector 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 #include #include +#include #include #include #include @@ -162,23 +163,24 @@ private: int testControls() { - ControlSerializer cs; + std::unique_ptr cs = + std::make_unique(ControlSerializer::Seeds::EVEN); const ControlInfoMap &infoMap = camera_->controls(); ControlList list = generateControlList(infoMap); std::vector infoMapBuf; std::tie(infoMapBuf, std::ignore) = - IPADataSerializer::serialize(infoMap, &cs); + IPADataSerializer::serialize(infoMap, cs.get()); std::vector listBuf; std::tie(listBuf, std::ignore) = - IPADataSerializer::serialize(list, &cs); + IPADataSerializer::serialize(list, cs.get()); const ControlInfoMap infoMapOut = - IPADataSerializer::deserialize(infoMapBuf, &cs); + IPADataSerializer::deserialize(infoMapBuf, cs.get()); - ControlList listOut = IPADataSerializer::deserialize(listBuf, &cs); + ControlList listOut = IPADataSerializer::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 cs = + std::make_unique(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 cs = + std::make_unique(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({{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 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 -%}