[{"id":19527,"web_url":"https://patchwork.libcamera.org/comment/19527/","msgid":"<20210908011545.GC968527@pyrite.rasen.tech>","date":"2021-09-08T01:15:45","subject":"Re: [libcamera-devel] [PATCH v2 5/5] libcamera: control_serializer:\n\tSeparate the handles space","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Tue, Sep 07, 2021 at 01:10:38PM +0200, Jacopo Mondi wrote:\n> Two instances of the ControlSerializer class are in use at the IPC\n> boundaries, one in the Proxy class that serializes data from the\n> pipeline handler to the IPA, and one in the ProxyWorker which serializes\n> data in the opposite direction.\n> \n> Each instance operates autonomously, without any centralized point of\n> control, and each one assigns a numerical handle to each ControlInfoMap\n> it serializes. This creates a risk of potential collision on the handle\n> values, as both instances will use the same numerical space and\n> are not aware of what handles has been already used by the instance \"on\n> the other side\".\n> \n> To fix that, partition the handles numerical space by initializing the\n> control serializer with a seed (even or odd) and increment the handle\n> number by 2, to avoid any collision risk.\n> \n> While this is temporary and rather hacky solution, it solves an issue\n> with isolated IPA modules without too much complexity added.\n> \n> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> ---\n>  .../libcamera/internal/control_serializer.h   |  8 ++-\n>  src/libcamera/control_serializer.cpp          | 60 +++++++++++++++++--\n>  test/serialization/control_serialization.cpp  |  4 +-\n>  .../ipa_data_serializer_test.cpp              | 26 ++++----\n>  .../module_ipa_proxy.cpp.tmpl                 |  6 +-\n>  .../module_ipa_proxy.h.tmpl                   |  2 +-\n>  .../module_ipa_proxy_worker.cpp.tmpl          | 14 +++--\n>  .../libcamera_templates/proxy_functions.tmpl  |  4 +-\n>  8 files changed, 97 insertions(+), 27 deletions(-)\n> \n> diff --git a/include/libcamera/internal/control_serializer.h b/include/libcamera/internal/control_serializer.h\n> index 8a66be324138..f96c6f8443d1 100644\n> --- a/include/libcamera/internal/control_serializer.h\n> +++ b/include/libcamera/internal/control_serializer.h\n> @@ -20,7 +20,12 @@ class ByteStreamBuffer;\n>  class ControlSerializer\n>  {\n>  public:\n> -\tControlSerializer();\n> +\tenum class Seeds {\n> +\t\tEVEN = 0,\n> +\t\tODD\n> +\t};\n\nI was going to say we could add a MAX and then you can += MAX and\nsupport any number of seeds, but that's probably overkill for a clever\nhack for IPC that only has two sides.\n\nReviewed-by: Paul Elder <paul.elder@ideasonboard.com>\n\n> +\n> +\tControlSerializer(Seeds seed);\n>  \n>  \tvoid reset();\n>  \n> @@ -47,6 +52,7 @@ private:\n>  \tControlInfo loadControlInfo(ControlType type, ByteStreamBuffer &buffer);\n>  \n>  \tunsigned int serial_;\n> +\tenum Seeds seed_;\n>  \tstd::vector<std::unique_ptr<ControlId>> controlIds_;\n>  \tstd::vector<std::unique_ptr<ControlIdMap>> controlIdMaps_;\n>  \tstd::map<unsigned int, ControlInfoMap> infoMaps_;\n> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> index f1245ea620ab..a60e665af5ae 100644\n> --- a/src/libcamera/control_serializer.cpp\n> +++ b/src/libcamera/control_serializer.cpp\n> @@ -62,6 +62,14 @@ LOG_DEFINE_CATEGORY(Serializer)\n>   * corresponding ControlInfoMap handle in the binary data, and when\n>   * deserializing to retrieve the corresponding ControlInfoMap.\n>   *\n> + * As a ControlSerializer instance is used at both sides of the IPC boundary,\n> + * and the two instances operate without a shared point of control, there is a\n> + * potential risk of collision of the numerical handles assigned to each\n> + * serialized ControlInfoMap. For this reason the control serializer is\n> + * initialized with an even/odd seed and the handle is incremented by 2, so that\n> + * instances initialized with a different seed operates on a separate numerical\n> + * space, avoiding any collision risk.\n> + *\n>   * In order to perform those tasks, the serializer keeps an internal state that\n>   * needs to be properly populated. This mechanism requires the ControlInfoMap\n>   * corresponding to a ControlList to have been serialized or deserialized\n> @@ -77,9 +85,42 @@ LOG_DEFINE_CATEGORY(Serializer)\n>   * proceed with care to avoid stale references.\n>   */\n>  \n> -ControlSerializer::ControlSerializer()\n> -\t: serial_(0)\n> +/**\n> + * \\enum ControlSerializer::Seeds\n> + * \\brief Define the seeds to initialize the ControlSerializer handle numerical\n> + * space with\n> + *\n> + * \\var ControlSerializer::Seeds::EVEN\n> + * \\brief The control serializer uses even numerical handles\n> + *\n> + * \\var ControlSerializer::Seeds::ODD\n> + * \\brief The control serializer uses odd numerical handles\n> + */\n> +\n> +/**\n> + * \\brief Construct a new ControlSerializer and initialize its seed\n> + * \\param[in] seed The serializer's handle seed\n> + */\n> +ControlSerializer::ControlSerializer(Seeds seed)\n> +\t: seed_(seed)\n>  {\n> +\t/*\n> +\t * Initialize the handle numerical space using the seed value.\n> +\t *\n> +\t * Instances initialized with a different seed will use a different\n> +\t * numerical handle space, avoiding any collision risk when, in example,\n> +\t * two instances of the ControlSerializer class are used at the IPC\n> +\t * boundaries.\n> +\t *\n> +\t * Start from 1 as '0' is a special value used as handle place holder\n> +\t * when serializing lists that do not have a ControlInfoMap associated\n> +\t * (in example list of libcamera controls::controls).\n> +\t *\n> +\t * \\todo This is a temporary hack and should probably be better\n> +\t * engineered, but for the time being it avoid collisions on the handle\n> +\t * value when using IPC.\n> +\t */\n> +\tserial_ = seed_ == Seeds::EVEN ? 2 : 1;\n>  }\n>  \n>  /**\n> @@ -90,7 +131,7 @@ ControlSerializer::ControlSerializer()\n>   */\n>  void ControlSerializer::reset()\n>  {\n> -\tserial_ = 0;\n> +\tserial_ = seed_ == Seeds::EVEN ? 2 : 1;\n>  \n>  \tinfoMapHandles_.clear();\n>  \tinfoMaps_.clear();\n> @@ -200,10 +241,10 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,\n>  \telse\n>  \t\tidMapType = IPA_CONTROL_ID_MAP_V4L2;\n>  \n> -\t/* Prepare the packet header, assign a handle to the ControlInfoMap. */\n> +\t/* Prepare the packet header. */\n>  \tstruct ipa_controls_header hdr;\n>  \thdr.version = IPA_CONTROLS_FORMAT_VERSION;\n> -\thdr.handle = ++serial_;\n> +\thdr.handle = serial_;\n>  \thdr.entries = infoMap.size();\n>  \thdr.size = sizeof(hdr) + entriesSize + valuesSize;\n>  \thdr.data_offset = sizeof(hdr) + entriesSize;\n> @@ -211,6 +252,15 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,\n>  \n>  \tbuffer.write(&hdr);\n>  \n> +\t/*\n> +\t * Increment the handle for the ControlInfoMap by 2 keep the handles\n> +\t * numerical space partitioned between instances initialized with a\n> +\t * different seed.\n> +\t *\n> +\t * \\sa ControlSerializer::Seeds\n> +\t */\n> +\tserial_ += 2;\n> +\n>  \t/*\n>  \t * Serialize all entries.\n>  \t * \\todo Serialize the control name too\n> diff --git a/test/serialization/control_serialization.cpp b/test/serialization/control_serialization.cpp\n> index 5ac9c4ede5f9..89c8ba59de24 100644\n> --- a/test/serialization/control_serialization.cpp\n> +++ b/test/serialization/control_serialization.cpp\n> @@ -30,8 +30,8 @@ protected:\n>  \n>  \tint run() override\n>  \t{\n> -\t\tControlSerializer serializer;\n> -\t\tControlSerializer deserializer;\n> +\t\tControlSerializer serializer(ControlSerializer::Seeds::EVEN);\n> +\t\tControlSerializer deserializer(ControlSerializer::Seeds::ODD);\n>  \n>  \t\tstd::vector<uint8_t> infoData;\n>  \t\tstd::vector<uint8_t> listData;\n> diff --git a/test/serialization/ipa_data_serializer_test.cpp b/test/serialization/ipa_data_serializer_test.cpp\n> index eca19a6642e1..a1ce8e3767bc 100644\n> --- a/test/serialization/ipa_data_serializer_test.cpp\n> +++ b/test/serialization/ipa_data_serializer_test.cpp\n> @@ -10,6 +10,7 @@\n>  #include <fcntl.h>\n>  #include <iostream>\n>  #include <limits>\n> +#include <memory>\n>  #include <stdlib.h>\n>  #include <sys/stat.h>\n>  #include <sys/types.h>\n> @@ -162,23 +163,24 @@ private:\n>  \n>  \tint testControls()\n>  \t{\n> -\t\tControlSerializer cs;\n> +\t\tstd::unique_ptr<ControlSerializer> cs =\n> +\t\t\tstd::make_unique<ControlSerializer>(ControlSerializer::Seeds::EVEN);\n>  \n>  \t\tconst ControlInfoMap &infoMap = camera_->controls();\n>  \t\tControlList list = generateControlList(infoMap);\n>  \n>  \t\tstd::vector<uint8_t> infoMapBuf;\n>  \t\tstd::tie(infoMapBuf, std::ignore) =\n> -\t\t\tIPADataSerializer<ControlInfoMap>::serialize(infoMap, &cs);\n> +\t\t\tIPADataSerializer<ControlInfoMap>::serialize(infoMap, cs.get());\n>  \n>  \t\tstd::vector<uint8_t> listBuf;\n>  \t\tstd::tie(listBuf, std::ignore) =\n> -\t\t\tIPADataSerializer<ControlList>::serialize(list, &cs);\n> +\t\t\tIPADataSerializer<ControlList>::serialize(list, cs.get());\n>  \n>  \t\tconst ControlInfoMap infoMapOut =\n> -\t\t\tIPADataSerializer<ControlInfoMap>::deserialize(infoMapBuf, &cs);\n> +\t\t\tIPADataSerializer<ControlInfoMap>::deserialize(infoMapBuf, cs.get());\n>  \n> -\t\tControlList listOut = IPADataSerializer<ControlList>::deserialize(listBuf, &cs);\n> +\t\tControlList listOut = IPADataSerializer<ControlList>::deserialize(listBuf, cs.get());\n>  \n>  \t\tif (!SerializationTest::equals(infoMap, infoMapOut)) {\n>  \t\t\tcerr << \"Deserialized map doesn't match original\" << endl;\n> @@ -195,7 +197,8 @@ private:\n>  \n>  \tint testVector()\n>  \t{\n> -\t\tControlSerializer cs;\n> +\t\tstd::unique_ptr<ControlSerializer> cs =\n> +\t\t\tstd::make_unique<ControlSerializer>(ControlSerializer::Seeds::EVEN);\n>  \n>  \t\t/*\n>  \t\t * We don't test FileDescriptor serdes because it dup()s, so we\n> @@ -257,7 +260,7 @@ private:\n>  \t\tif (testVectorSerdes(vecString) != TestPass)\n>  \t\t\treturn TestFail;\n>  \n> -\t\tif (testVectorSerdes(vecControlInfoMap, &cs) != TestPass)\n> +\t\tif (testVectorSerdes(vecControlInfoMap, cs.get()) != TestPass)\n>  \t\t\treturn TestFail;\n>  \n>  \t\treturn TestPass;\n> @@ -265,7 +268,8 @@ private:\n>  \n>  \tint testMap()\n>  \t{\n> -\t\tControlSerializer cs;\n> +\t\tstd::unique_ptr<ControlSerializer> cs =\n> +\t\t\tstd::make_unique<ControlSerializer>(ControlSerializer::Seeds::EVEN);\n>  \n>  \t\t/*\n>  \t\t * Realistically, only string and integral keys.\n> @@ -302,13 +306,13 @@ private:\n>  \t\tif (testMapSerdes(mapStrStr) != TestPass)\n>  \t\t\treturn TestFail;\n>  \n> -\t\tif (testMapSerdes(mapUintCIM, &cs) != TestPass)\n> +\t\tif (testMapSerdes(mapUintCIM, cs.get()) != TestPass)\n>  \t\t\treturn TestFail;\n>  \n> -\t\tif (testMapSerdes(mapIntCIM,  &cs) != TestPass)\n> +\t\tif (testMapSerdes(mapIntCIM, cs.get()) != TestPass)\n>  \t\t\treturn TestFail;\n>  \n> -\t\tif (testMapSerdes(mapStrCIM,  &cs) != TestPass)\n> +\t\tif (testMapSerdes(mapStrCIM, cs.get()) != TestPass)\n>  \t\t\treturn TestFail;\n>  \n>  \t\tif (testMapSerdes(mapUintBVec) != TestPass)\n> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> index aab1fffbcaf0..7e28c2ed129a 100644\n> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> @@ -52,6 +52,8 @@ namespace {{ns}} {\n>  \t\t<< \"initializing {{module_name}} proxy: loading IPA from \"\n>  \t\t<< ipam->path();\n>  \n> +\tcontrolSerializer_ = new ControlSerializer(ControlSerializer::Seeds::EVEN);\n> +\n>  \tif (isolate_) {\n>  \t\tconst std::string proxyWorkerPath = resolvePath(\"{{module_name}}_ipa_proxy\");\n>  \t\tif (proxyWorkerPath.empty()) {\n> @@ -95,6 +97,8 @@ namespace {{ns}} {\n>  \n>  {{proxy_name}}::~{{proxy_name}}()\n>  {\n> +\tdelete controlSerializer_;\n> +\n>  \tif (isolate_) {\n>  \t\tIPCMessage::Header header =\n>  \t\t\t{ static_cast<uint32_t>({{cmd_enum_name}}::Exit), seq_++ };\n> @@ -185,7 +189,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)\n>  {{proxy_funcs.func_sig(proxy_name, method, \"IPC\")}}\n>  {\n>  {%- if method.mojom_name == \"configure\" %}\n> -\tcontrolSerializer_.reset();\n> +\tcontrolSerializer_->reset();\n>  {%- endif %}\n>  {%- set has_output = true if method|method_param_outputs|length > 0 or method|method_return_value != \"void\" %}\n>  {%- set cmd = cmd_enum_name + \"::\" + method.mojom_name|cap %}\n> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> index 1979e68ff74d..fdbf25418963 100644\n> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> @@ -118,7 +118,7 @@ private:\n>  \n>  \tstd::unique_ptr<IPCPipeUnixSocket> ipc_;\n>  \n> -\tControlSerializer controlSerializer_;\n> +\tControlSerializer *controlSerializer_;\n>  \n>  {# \\todo Move this to IPCPipe #}\n>  \tuint32_t seq_;\n> 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\n> index c5e51532db53..5829059b8e93 100644\n> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n> @@ -53,9 +53,15 @@ class {{proxy_worker_name}}\n>  {\n>  public:\n>  \t{{proxy_worker_name}}()\n> -\t\t: ipa_(nullptr), exit_(false) {}\n> +\t\t: ipa_(nullptr), exit_(false)\n> +\t{\n> +\t\tcontrolSerializer_ = new ControlSerializer(ControlSerializer::Seeds::ODD);\n> +\t}\n>  \n> -\t~{{proxy_worker_name}}() {}\n> +\t~{{proxy_worker_name}}()\n> +\t{\n> +\t\tdelete controlSerializer_;\n> +\t}\n>  \n>  \tvoid readyRead()\n>  \t{\n> @@ -81,7 +87,7 @@ public:\n>  \t\tcase {{cmd_enum_name}}::{{method.mojom_name|cap}}: {\n>  \n>  {%- if method.mojom_name == \"configure\" %}\n> -\t\t\tcontrolSerializer_.reset();\n> +\t\t\tcontrolSerializer_->reset();\n>  {%- endif %}\n>  \t\t{{proxy_funcs.deserialize_call(method|method_param_inputs, '_ipcMessage.data()', '_ipcMessage.fds()', false, true)|indent(8, true)}}\n>  {% for param in method|method_param_outputs %}\n> @@ -180,7 +186,7 @@ private:\n>  \t{{interface_name}} *ipa_;\n>  \tIPCUnixSocket socket_;\n>  \n> -\tControlSerializer controlSerializer_;\n> +\tControlSerializer *controlSerializer_;\n>  \n>  \tbool exit_;\n>  };\n> diff --git a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl\n> index ebcd2aaaafae..d78e5c53566f 100644\n> --- a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl\n> +++ b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl\n> @@ -60,7 +60,7 @@\n>  \tstd::tie({{param.mojom_name}}Buf, std::ignore) =\n>  {%- endif %}\n>  \t\tIPADataSerializer<{{param|name}}>::serialize({{param.mojom_name}}\n> -{{- \", &controlSerializer_\" if param|needs_control_serializer -}}\n> +{{- \", controlSerializer_\" if param|needs_control_serializer -}}\n>  );\n>  {%- endfor %}\n>  \n> @@ -119,7 +119,7 @@\n>  {%- endif -%}\n>  {{- \",\" if param|needs_control_serializer}}\n>  {%- if param|needs_control_serializer %}\n> -\t&controlSerializer_\n> +\tcontrolSerializer_\n>  {%- endif -%}\n>  );\n>  {%- endmacro -%}\n> -- \n> 2.32.0\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id EF0F4BDC71\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed,  8 Sep 2021 01:15:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6D8846916E;\n\tWed,  8 Sep 2021 03:15:55 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9A64760137\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed,  8 Sep 2021 03:15:53 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0040F317;\n\tWed,  8 Sep 2021 03:15:51 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"W2K4ZV5v\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631063753;\n\tbh=dM2DNkWqRqPh+19OQ8I3p+uFAaLK2r0DZjPrxoMjqts=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=W2K4ZV5vsN+X+OzGCvUO32MCvhMlc9xtacbHP52Wh9dGiQw3UY4CbazuhImjA+Fha\n\tg7gfqfKnULwtV6sWIgwh28W+Zw+bxfgT4Af3rPnS9GhQznXrCA+qIJaZ49tlOy2ERR\n\tFXTR4cF60cB1uJnCd3er/XjWvJab0Ni5JXgDJs1A=","Date":"Wed, 8 Sep 2021 10:15:45 +0900","From":"paul.elder@ideasonboard.com","To":"Jacopo Mondi <jacopo@jmondi.org>","Message-ID":"<20210908011545.GC968527@pyrite.rasen.tech>","References":"<20210907111038.739104-1-jacopo@jmondi.org>\n\t<20210907111038.739104-6-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20210907111038.739104-6-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v2 5/5] libcamera: control_serializer:\n\tSeparate the handles space","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":19725,"web_url":"https://patchwork.libcamera.org/comment/19725/","msgid":"<YUmvDhA1UJfRtgTk@pendragon.ideasonboard.com>","date":"2021-09-21T10:08:14","subject":"Re: [libcamera-devel] [PATCH v2 5/5] libcamera: control_serializer:\n\tSeparate the handles space","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Tue, Sep 07, 2021 at 01:10:38PM +0200, Jacopo Mondi wrote:\n> Two instances of the ControlSerializer class are in use at the IPC\n> boundaries, one in the Proxy class that serializes data from the\n> pipeline handler to the IPA, and one in the ProxyWorker which serializes\n> data in the opposite direction.\n> \n> Each instance operates autonomously, without any centralized point of\n> control, and each one assigns a numerical handle to each ControlInfoMap\n> it serializes. This creates a risk of potential collision on the handle\n> values, as both instances will use the same numerical space and\n> are not aware of what handles has been already used by the instance \"on\n> the other side\".\n> \n> To fix that, partition the handles numerical space by initializing the\n> control serializer with a seed (even or odd) and increment the handle\n> number by 2, to avoid any collision risk.\n> \n> While this is temporary and rather hacky solution, it solves an issue\n> with isolated IPA modules without too much complexity added.\n\nI think it's a rather clever hack, in the noble sense of the term.\n\n> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> ---\n>  .../libcamera/internal/control_serializer.h   |  8 ++-\n>  src/libcamera/control_serializer.cpp          | 60 +++++++++++++++++--\n>  test/serialization/control_serialization.cpp  |  4 +-\n>  .../ipa_data_serializer_test.cpp              | 26 ++++----\n>  .../module_ipa_proxy.cpp.tmpl                 |  6 +-\n>  .../module_ipa_proxy.h.tmpl                   |  2 +-\n>  .../module_ipa_proxy_worker.cpp.tmpl          | 14 +++--\n>  .../libcamera_templates/proxy_functions.tmpl  |  4 +-\n>  8 files changed, 97 insertions(+), 27 deletions(-)\n> \n> diff --git a/include/libcamera/internal/control_serializer.h b/include/libcamera/internal/control_serializer.h\n> index 8a66be324138..f96c6f8443d1 100644\n> --- a/include/libcamera/internal/control_serializer.h\n> +++ b/include/libcamera/internal/control_serializer.h\n> @@ -20,7 +20,12 @@ class ByteStreamBuffer;\n>  class ControlSerializer\n>  {\n>  public:\n> -\tControlSerializer();\n> +\tenum class Seeds {\n> +\t\tEVEN = 0,\n> +\t\tODD\n\nWe use CamelCase for enumerators, so these should be Even and Odd.\n\n> +\t};\n> +\n> +\tControlSerializer(Seeds seed);\n>  \n>  \tvoid reset();\n>  \n> @@ -47,6 +52,7 @@ private:\n>  \tControlInfo loadControlInfo(ControlType type, ByteStreamBuffer &buffer);\n>  \n>  \tunsigned int serial_;\n> +\tenum Seeds seed_;\n\nNo need for the enum keyword.\n\n>  \tstd::vector<std::unique_ptr<ControlId>> controlIds_;\n>  \tstd::vector<std::unique_ptr<ControlIdMap>> controlIdMaps_;\n>  \tstd::map<unsigned int, ControlInfoMap> infoMaps_;\n> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> index f1245ea620ab..a60e665af5ae 100644\n> --- a/src/libcamera/control_serializer.cpp\n> +++ b/src/libcamera/control_serializer.cpp\n> @@ -62,6 +62,14 @@ LOG_DEFINE_CATEGORY(Serializer)\n>   * corresponding ControlInfoMap handle in the binary data, and when\n>   * deserializing to retrieve the corresponding ControlInfoMap.\n>   *\n> + * As a ControlSerializer instance is used at both sides of the IPC boundary,\n\nMaybe \"a different ControlSerializer instance is used\" or \"independent\nControlSerializer instances are used\" ?\n\ns/at both/on both/\n\n> + * and the two instances operate without a shared point of control, there is a\n> + * potential risk of collision of the numerical handles assigned to each\n> + * serialized ControlInfoMap. For this reason the control serializer is\n> + * initialized with an even/odd seed and the handle is incremented by 2, so that\n> + * instances initialized with a different seed operates on a separate numerical\n\ns/operates/operate/\n\n> + * space, avoiding any collision risk.\n> + *\n>   * In order to perform those tasks, the serializer keeps an internal state that\n>   * needs to be properly populated. This mechanism requires the ControlInfoMap\n>   * corresponding to a ControlList to have been serialized or deserialized\n> @@ -77,9 +85,42 @@ LOG_DEFINE_CATEGORY(Serializer)\n>   * proceed with care to avoid stale references.\n>   */\n>  \n> -ControlSerializer::ControlSerializer()\n> -\t: serial_(0)\n> +/**\n> + * \\enum ControlSerializer::Seeds\n> + * \\brief Define the seeds to initialize the ControlSerializer handle numerical\n> + * space with\n> + *\n> + * \\var ControlSerializer::Seeds::EVEN\n> + * \\brief The control serializer uses even numerical handles\n> + *\n> + * \\var ControlSerializer::Seeds::ODD\n> + * \\brief The control serializer uses odd numerical handles\n> + */\n\nThere's nothing but the implementation telling which seed should be used\non which side. I think it would be clearer from an API point of view to\nreplace\n\n\tenum class Seeds {\n\t\tEVEN,\n\t\tODD,\n\t};\n\nwith\n\n\tenum class Side {\n\t\tProxy,\n\t\tWorker,\n\t};\n\nSure, it harcodes serializer usage for the proxy and the worker, but the\nAPI isn't generic anyway as we only support two sides, and we have no\nother expected use case. There could also be a better term than \"Side\".\n\n> +\n> +/**\n> + * \\brief Construct a new ControlSerializer and initialize its seed\n> + * \\param[in] seed The serializer's handle seed\n> + */\n> +ControlSerializer::ControlSerializer(Seeds seed)\n> +\t: seed_(seed)\n>  {\n> +\t/*\n> +\t * Initialize the handle numerical space using the seed value.\n> +\t *\n> +\t * Instances initialized with a different seed will use a different\n> +\t * numerical handle space, avoiding any collision risk when, in example,\n> +\t * two instances of the ControlSerializer class are used at the IPC\n> +\t * boundaries.\n> +\t *\n> +\t * Start from 1 as '0' is a special value used as handle place holder\n> +\t * when serializing lists that do not have a ControlInfoMap associated\n> +\t * (in example list of libcamera controls::controls).\n> +\t *\n> +\t * \\todo This is a temporary hack and should probably be better\n> +\t * engineered, but for the time being it avoid collisions on the handle\n> +\t * value when using IPC.\n> +\t */\n> +\tserial_ = seed_ == Seeds::EVEN ? 2 : 1;\n>  }\n>  \n>  /**\n> @@ -90,7 +131,7 @@ ControlSerializer::ControlSerializer()\n>   */\n>  void ControlSerializer::reset()\n>  {\n> -\tserial_ = 0;\n> +\tserial_ = seed_ == Seeds::EVEN ? 2 : 1;\n\nHow about replacing enum Seeds with an unsigned int seed_, so that we\ncould write\n\n\tserial_ = seed_;\n\nhere and in the constructor ? We could also name the field serialSeed_\nto make it clearer.\n\n>  \n>  \tinfoMapHandles_.clear();\n>  \tinfoMaps_.clear();\n> @@ -200,10 +241,10 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,\n>  \telse\n>  \t\tidMapType = IPA_CONTROL_ID_MAP_V4L2;\n>  \n> -\t/* Prepare the packet header, assign a handle to the ControlInfoMap. */\n> +\t/* Prepare the packet header. */\n>  \tstruct ipa_controls_header hdr;\n>  \thdr.version = IPA_CONTROLS_FORMAT_VERSION;\n> -\thdr.handle = ++serial_;\n> +\thdr.handle = serial_;\n>  \thdr.entries = infoMap.size();\n>  \thdr.size = sizeof(hdr) + entriesSize + valuesSize;\n>  \thdr.data_offset = sizeof(hdr) + entriesSize;\n> @@ -211,6 +252,15 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,\n>  \n>  \tbuffer.write(&hdr);\n>  \n> +\t/*\n> +\t * Increment the handle for the ControlInfoMap by 2 keep the handles\n\nDid you mean \"by 2 to keep\" or \"by 2 keeps\" ?\n\n> +\t * numerical space partitioned between instances initialized with a\n> +\t * different seed.\n> +\t *\n> +\t * \\sa ControlSerializer::Seeds\n> +\t */\n> +\tserial_ += 2;\n> +\n>  \t/*\n>  \t * Serialize all entries.\n>  \t * \\todo Serialize the control name too\n> diff --git a/test/serialization/control_serialization.cpp b/test/serialization/control_serialization.cpp\n> index 5ac9c4ede5f9..89c8ba59de24 100644\n> --- a/test/serialization/control_serialization.cpp\n> +++ b/test/serialization/control_serialization.cpp\n> @@ -30,8 +30,8 @@ protected:\n>  \n>  \tint run() override\n>  \t{\n> -\t\tControlSerializer serializer;\n> -\t\tControlSerializer deserializer;\n> +\t\tControlSerializer serializer(ControlSerializer::Seeds::EVEN);\n> +\t\tControlSerializer deserializer(ControlSerializer::Seeds::ODD);\n>  \n>  \t\tstd::vector<uint8_t> infoData;\n>  \t\tstd::vector<uint8_t> listData;\n> diff --git a/test/serialization/ipa_data_serializer_test.cpp b/test/serialization/ipa_data_serializer_test.cpp\n> index eca19a6642e1..a1ce8e3767bc 100644\n> --- a/test/serialization/ipa_data_serializer_test.cpp\n> +++ b/test/serialization/ipa_data_serializer_test.cpp\n> @@ -10,6 +10,7 @@\n>  #include <fcntl.h>\n>  #include <iostream>\n>  #include <limits>\n> +#include <memory>\n>  #include <stdlib.h>\n>  #include <sys/stat.h>\n>  #include <sys/types.h>\n> @@ -162,23 +163,24 @@ private:\n>  \n>  \tint testControls()\n>  \t{\n> -\t\tControlSerializer cs;\n\nHow about simply\n\n\t\tControlSerializer cs(ControlSerializer::Seeds::EVEN);\n\n? I think you could then drop most of the changes below.\n\n> +\t\tstd::unique_ptr<ControlSerializer> cs =\n> +\t\t\tstd::make_unique<ControlSerializer>(ControlSerializer::Seeds::EVEN);\n>  \n>  \t\tconst ControlInfoMap &infoMap = camera_->controls();\n>  \t\tControlList list = generateControlList(infoMap);\n>  \n>  \t\tstd::vector<uint8_t> infoMapBuf;\n>  \t\tstd::tie(infoMapBuf, std::ignore) =\n> -\t\t\tIPADataSerializer<ControlInfoMap>::serialize(infoMap, &cs);\n> +\t\t\tIPADataSerializer<ControlInfoMap>::serialize(infoMap, cs.get());\n>  \n>  \t\tstd::vector<uint8_t> listBuf;\n>  \t\tstd::tie(listBuf, std::ignore) =\n> -\t\t\tIPADataSerializer<ControlList>::serialize(list, &cs);\n> +\t\t\tIPADataSerializer<ControlList>::serialize(list, cs.get());\n>  \n>  \t\tconst ControlInfoMap infoMapOut =\n> -\t\t\tIPADataSerializer<ControlInfoMap>::deserialize(infoMapBuf, &cs);\n> +\t\t\tIPADataSerializer<ControlInfoMap>::deserialize(infoMapBuf, cs.get());\n>  \n> -\t\tControlList listOut = IPADataSerializer<ControlList>::deserialize(listBuf, &cs);\n> +\t\tControlList listOut = IPADataSerializer<ControlList>::deserialize(listBuf, cs.get());\n>  \n>  \t\tif (!SerializationTest::equals(infoMap, infoMapOut)) {\n>  \t\t\tcerr << \"Deserialized map doesn't match original\" << endl;\n> @@ -195,7 +197,8 @@ private:\n>  \n>  \tint testVector()\n>  \t{\n> -\t\tControlSerializer cs;\n> +\t\tstd::unique_ptr<ControlSerializer> cs =\n> +\t\t\tstd::make_unique<ControlSerializer>(ControlSerializer::Seeds::EVEN);\n>  \n>  \t\t/*\n>  \t\t * We don't test FileDescriptor serdes because it dup()s, so we\n> @@ -257,7 +260,7 @@ private:\n>  \t\tif (testVectorSerdes(vecString) != TestPass)\n>  \t\t\treturn TestFail;\n>  \n> -\t\tif (testVectorSerdes(vecControlInfoMap, &cs) != TestPass)\n> +\t\tif (testVectorSerdes(vecControlInfoMap, cs.get()) != TestPass)\n>  \t\t\treturn TestFail;\n>  \n>  \t\treturn TestPass;\n> @@ -265,7 +268,8 @@ private:\n>  \n>  \tint testMap()\n>  \t{\n> -\t\tControlSerializer cs;\n> +\t\tstd::unique_ptr<ControlSerializer> cs =\n> +\t\t\tstd::make_unique<ControlSerializer>(ControlSerializer::Seeds::EVEN);\n>  \n>  \t\t/*\n>  \t\t * Realistically, only string and integral keys.\n> @@ -302,13 +306,13 @@ private:\n>  \t\tif (testMapSerdes(mapStrStr) != TestPass)\n>  \t\t\treturn TestFail;\n>  \n> -\t\tif (testMapSerdes(mapUintCIM, &cs) != TestPass)\n> +\t\tif (testMapSerdes(mapUintCIM, cs.get()) != TestPass)\n>  \t\t\treturn TestFail;\n>  \n> -\t\tif (testMapSerdes(mapIntCIM,  &cs) != TestPass)\n> +\t\tif (testMapSerdes(mapIntCIM, cs.get()) != TestPass)\n>  \t\t\treturn TestFail;\n>  \n> -\t\tif (testMapSerdes(mapStrCIM,  &cs) != TestPass)\n> +\t\tif (testMapSerdes(mapStrCIM, cs.get()) != TestPass)\n>  \t\t\treturn TestFail;\n>  \n>  \t\tif (testMapSerdes(mapUintBVec) != TestPass)\n> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> index aab1fffbcaf0..7e28c2ed129a 100644\n> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> @@ -52,6 +52,8 @@ namespace {{ns}} {\n>  \t\t<< \"initializing {{module_name}} proxy: loading IPA from \"\n>  \t\t<< ipam->path();\n>  \n> +\tcontrolSerializer_ = new ControlSerializer(ControlSerializer::Seeds::EVEN);\n\nYou can write\n\n{{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate)\n\t: IPAProxy(ipam), isolate_(isolate),\n\t  controlSerializer_(ControlSerializer::Seeds::EVEN), seq_(0)\n\nand avoid dynamic allocation of controlSerializer_. If you still prefer\nto allocate it dynamically, I would use a unique pointer to avoid the\nmanual delete.\n\nSame comments for the worker side.\n\n> +\n>  \tif (isolate_) {\n>  \t\tconst std::string proxyWorkerPath = resolvePath(\"{{module_name}}_ipa_proxy\");\n>  \t\tif (proxyWorkerPath.empty()) {\n> @@ -95,6 +97,8 @@ namespace {{ns}} {\n>  \n>  {{proxy_name}}::~{{proxy_name}}()\n>  {\n> +\tdelete controlSerializer_;\n> +\n>  \tif (isolate_) {\n>  \t\tIPCMessage::Header header =\n>  \t\t\t{ static_cast<uint32_t>({{cmd_enum_name}}::Exit), seq_++ };\n> @@ -185,7 +189,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)\n>  {{proxy_funcs.func_sig(proxy_name, method, \"IPC\")}}\n>  {\n>  {%- if method.mojom_name == \"configure\" %}\n> -\tcontrolSerializer_.reset();\n> +\tcontrolSerializer_->reset();\n>  {%- endif %}\n>  {%- set has_output = true if method|method_param_outputs|length > 0 or method|method_return_value != \"void\" %}\n>  {%- set cmd = cmd_enum_name + \"::\" + method.mojom_name|cap %}\n> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> index 1979e68ff74d..fdbf25418963 100644\n> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> @@ -118,7 +118,7 @@ private:\n>  \n>  \tstd::unique_ptr<IPCPipeUnixSocket> ipc_;\n>  \n> -\tControlSerializer controlSerializer_;\n> +\tControlSerializer *controlSerializer_;\n>  \n>  {# \\todo Move this to IPCPipe #}\n>  \tuint32_t seq_;\n> 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\n> index c5e51532db53..5829059b8e93 100644\n> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n> @@ -53,9 +53,15 @@ class {{proxy_worker_name}}\n>  {\n>  public:\n>  \t{{proxy_worker_name}}()\n> -\t\t: ipa_(nullptr), exit_(false) {}\n> +\t\t: ipa_(nullptr), exit_(false)\n> +\t{\n> +\t\tcontrolSerializer_ = new ControlSerializer(ControlSerializer::Seeds::ODD);\n> +\t}\n>  \n> -\t~{{proxy_worker_name}}() {}\n> +\t~{{proxy_worker_name}}()\n> +\t{\n> +\t\tdelete controlSerializer_;\n> +\t}\n>  \n>  \tvoid readyRead()\n>  \t{\n> @@ -81,7 +87,7 @@ public:\n>  \t\tcase {{cmd_enum_name}}::{{method.mojom_name|cap}}: {\n>  \n>  {%- if method.mojom_name == \"configure\" %}\n> -\t\t\tcontrolSerializer_.reset();\n> +\t\t\tcontrolSerializer_->reset();\n>  {%- endif %}\n>  \t\t{{proxy_funcs.deserialize_call(method|method_param_inputs, '_ipcMessage.data()', '_ipcMessage.fds()', false, true)|indent(8, true)}}\n>  {% for param in method|method_param_outputs %}\n> @@ -180,7 +186,7 @@ private:\n>  \t{{interface_name}} *ipa_;\n>  \tIPCUnixSocket socket_;\n>  \n> -\tControlSerializer controlSerializer_;\n> +\tControlSerializer *controlSerializer_;\n>  \n>  \tbool exit_;\n>  };\n> diff --git a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl\n> index ebcd2aaaafae..d78e5c53566f 100644\n> --- a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl\n> +++ b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl\n> @@ -60,7 +60,7 @@\n>  \tstd::tie({{param.mojom_name}}Buf, std::ignore) =\n>  {%- endif %}\n>  \t\tIPADataSerializer<{{param|name}}>::serialize({{param.mojom_name}}\n> -{{- \", &controlSerializer_\" if param|needs_control_serializer -}}\n> +{{- \", controlSerializer_\" if param|needs_control_serializer -}}\n>  );\n>  {%- endfor %}\n>  \n> @@ -119,7 +119,7 @@\n>  {%- endif -%}\n>  {{- \",\" if param|needs_control_serializer}}\n>  {%- if param|needs_control_serializer %}\n> -\t&controlSerializer_\n> +\tcontrolSerializer_\n>  {%- endif -%}\n>  );\n>  {%- endmacro -%}","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 82851BF01C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 21 Sep 2021 10:08:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 168906918A;\n\tTue, 21 Sep 2021 12:08:47 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0868860247\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 21 Sep 2021 12:08:46 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6E3A02BA;\n\tTue, 21 Sep 2021 12:08:45 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"gvWE97Ui\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632218925;\n\tbh=g68p/2iF4QOnhdNG45arEPqC3SCy3mqcrOOgfPGrBRA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=gvWE97UiZVGw+rP5yUj8LjnBf5MN22DLSTDSBZ1CLr7gTADqGU58eEYmO3GCS2fZ7\n\tca5E22RpVGSxuSRFUjGgImkpQf51SWKZwUjq7MQi5We6ps8Qj1VfRGfuaTciWvZE1j\n\tGpZdgnSPfAeiHhcO3QCW/wzPYbciHRlPSVspfkJw=","Date":"Tue, 21 Sep 2021 13:08:14 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Message-ID":"<YUmvDhA1UJfRtgTk@pendragon.ideasonboard.com>","References":"<20210907111038.739104-1-jacopo@jmondi.org>\n\t<20210907111038.739104-6-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210907111038.739104-6-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v2 5/5] libcamera: control_serializer:\n\tSeparate the handles space","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]