diff --git a/include/libcamera/internal/control_serializer.h b/include/libcamera/internal/control_serializer.h index 8a66be324138..caeafa11bc54 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 Role { + Proxy, + Worker + }; + + ControlSerializer(Role role); void reset(); @@ -47,6 +52,7 @@ private: ControlInfo loadControlInfo(ControlType type, ByteStreamBuffer &buffer); unsigned int serial_; + unsigned int serialSeed_; 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..025222d77e4d 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 independent ControlSerializer instances are used on 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 a seed and the handle is incremented by 2, so that instances + * initialized with a different seed operate 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,45 @@ LOG_DEFINE_CATEGORY(Serializer) * proceed with care to avoid stale references. */ -ControlSerializer::ControlSerializer() - : serial_(0) +/** + * \enum ControlSerializer::Role + * \brief Define the role of the IPC component using the control serializer. + * + * The role of the component that creates the serializer is used to initialize + * the handles numerical space. + * + * \var ControlSerializer::Role::Proxy + * \brief The control serializer is used by the IPC Proxy classes + * + * \var ControlSerializer::Role::Worker + * \brief The control serializer is used by the IPC ProxyWorker classes + */ + +/** + * \brief Construct a new ControlSerializer + * \param[in] role The role of the IPC component using the serializer + */ +ControlSerializer::ControlSerializer(Role role) { + /* + * Initialize the handle numerical space using the role of the + * component that created the instance. + * + * Instances initialized for a different role 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 counting handles from '1' as '0' is a special value used as + * 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. + */ + serialSeed_ = role == Role::Proxy ? 1 : 2; + serial_ = serialSeed_; } /** @@ -90,7 +134,7 @@ ControlSerializer::ControlSerializer() */ void ControlSerializer::reset() { - serial_ = 0; + serial_ = serialSeed_; infoMapHandles_.clear(); infoMaps_.clear(); @@ -200,10 +244,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 +255,15 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap, buffer.write(&hdr); + /* + * Increment the handle for the ControlInfoMap by 2 to keep the handles + * numerical space partitioned between instances initialized for a + * different role. + * + * \sa ControlSerializer::Role + */ + 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..a507d98a152d 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::Role::Proxy); + ControlSerializer deserializer(ControlSerializer::Role::Worker); 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..5fcdcb8eae92 100644 --- a/test/serialization/ipa_data_serializer_test.cpp +++ b/test/serialization/ipa_data_serializer_test.cpp @@ -162,7 +162,7 @@ private: int testControls() { - ControlSerializer cs; + ControlSerializer cs(ControlSerializer::Role::Proxy); const ControlInfoMap &infoMap = camera_->controls(); ControlList list = generateControlList(infoMap); @@ -195,7 +195,7 @@ private: int testVector() { - ControlSerializer cs; + ControlSerializer cs(ControlSerializer::Role::Proxy); /* * We don't test FileDescriptor serdes because it dup()s, so we @@ -265,7 +265,7 @@ private: int testMap() { - ControlSerializer cs; + ControlSerializer cs(ControlSerializer::Role::Proxy); /* * Realistically, only string and integral keys. 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..d856339aa9ee 100644 --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl @@ -46,7 +46,8 @@ namespace {{ns}} { {%- endif %} {{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate) - : IPAProxy(ipam), isolate_(isolate), seq_(0) + : IPAProxy(ipam), isolate_(isolate), + controlSerializer_(ControlSerializer::Role::Proxy), seq_(0) { LOG(IPAProxy, Debug) << "initializing {{module_name}} proxy: loading IPA from " 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 890246a8849b..764e7a3af63a 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,7 +53,9 @@ class {{proxy_worker_name}} { public: {{proxy_worker_name}}() - : ipa_(nullptr), exit_(false) {} + : ipa_(nullptr), + controlSerializer_(ControlSerializer::Role::Worker), + exit_(false) {} ~{{proxy_worker_name}}() {}