[{"id":35400,"web_url":"https://patchwork.libcamera.org/comment/35400/","msgid":"<175516195566.3701817.15703185411370250000@neptunite.rasen.tech>","date":"2025-08-14T08:59:15","subject":"Re: [RFC PATCH v1] utils: codegen: ipc: Split proxy types","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Barnabás,\n\nThanks for the patch.\n\nQuoting Barnabás Pőcze (2025-08-05 01:52:26)\n> Even though there is an abstract class to represent the interface of an IPA,\n> the threaded and IPC versions are still multiplexed using the same type,\n> which uses a boolean to actually dispatch to the right function.\n> \n> Instead of doing that, split the classes into a \"local\" and \"remote\" variant,\n> and make `IPAManager` choose accordingly.\n\nWow, that is a cool upgrade!\n\n> \n> Signed-off-by: Barnabás Pőcze \n> ---\n> include/libcamera/internal/ipa_manager.h | 8 +-\n> .../module_ipa_proxy.cpp.tmpl | 149 +++++++++---------\n> .../module_ipa_proxy.h.tmpl | 56 +++++--\n> 3 files changed, 118 insertions(+), 95 deletions(-)\n> \n> diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h\n> index a0d448cf9..847a69066 100644\n> --- a/include/libcamera/internal/ipa_manager.h\n> +++ b/include/libcamera/internal/ipa_manager.h\n> @@ -42,7 +42,13 @@ public:\n> if (!m)\n> return nullptr;\n> \n> - std::unique_ptr proxy = std::make_unique(m, !self->isSignatureValid(m));\n> + auto proxy = [&]() -> std::unique_ptr {\n> + if (self->isSignatureValid(m))\n> + return std::make_unique(m);\n> + else\n> + return std::make_unique(m);\n> + } ();\n> +\n> if (!proxy->isValid()) {\n> LOG(IPAManager, Error) << \"Failed to load proxy\";\n> return nullptr;\n> diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> index beb646e2d..8792b3b9e 100644\n> --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> @@ -45,36 +45,13 @@ namespace {{ns}} {\n> {% endfor %}\n> {%- endif %}\n> \n> -{{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate)\n> - : IPAProxy(ipam), isolate_(isolate),\n> - controlSerializer_(ControlSerializer::Role::Proxy), seq_(0)\n> +{{proxy_name}}Local::{{proxy_name}}Local(IPAModule *ipam)\n> + : {{proxy_name}}(ipam)\n> {\n> LOG(IPAProxy, Debug)\n> << \"initializing {{module_name}} proxy: loading IPA from \"\n> << ipam->path();\n\nMight it be worth it to also print information about isolation? Same for the\nconstructor below.\n\n> \n> - if (isolate_) {\n> - const std::string proxyWorkerPath = resolvePath(\"{{module_name}}_ipa_proxy\");\n> - if (proxyWorkerPath.empty()) {\n> - LOG(IPAProxy, Error)\n> - << \"Failed to get proxy worker path\";\n> - return;\n> - }\n> -\n> - auto ipc = std::make_unique(ipam->path().c_str(),\n> - proxyWorkerPath.c_str());\n> - if (!ipc->isConnected()) {\n> - LOG(IPAProxy, Error) << \"Failed to create IPCPipe\";\n> - return;\n> - }\n> -\n> - ipc->recv.connect(this, &{{proxy_name}}::recvMessage);\n> -\n> - ipc_ = std::move(ipc);\n> - valid_ = true;\n> - return;\n> - }\n> -\n> if (!ipam->load())\n> return;\n> \n> @@ -89,59 +66,16 @@ namespace {{ns}} {\n> proxy_.setIPA(ipa_.get());\n> \n> {% for method in interface_event.methods %}\n> - ipa_->{{method.mojom_name}}.connect(this, &{{proxy_name}}::{{method.mojom_name}}Thread);\n> + ipa_->{{method.mojom_name}}.connect(this, &{{proxy_name}}Local::{{method.mojom_name}}Handler);\n> {%- endfor %}\n> \n> valid_ = true;\n> }\n> \n> -{{proxy_name}}::~{{proxy_name}}()\n> -{\n> - if (ipc_) {\n> - IPCMessage::Header header =\n> - { static_cast({{cmd_enum_name}}::Exit), seq_++ };\n> - IPCMessage msg(header);\n> - ipc_->sendAsync(msg);\n> - }\n> -}\n> -\n> -{% if interface_event.methods|length > 0 %}\n> -void {{proxy_name}}::recvMessage(const IPCMessage &data)\n> -{\n> - size_t dataSize = data.data().size();\n> - {{cmd_event_enum_name}} _cmd = static_cast<{{cmd_event_enum_name}}>(data.header().cmd);\n> -\n> - switch (_cmd) {\n> -{%- for method in interface_event.methods %}\n> - case {{cmd_event_enum_name}}::{{method.mojom_name|cap}}: {\n> - {{method.mojom_name}}IPC(data.data().cbegin(), dataSize, data.fds());\n> - break;\n> - }\n> -{%- endfor %}\n> - default:\n> - LOG(IPAProxy, Error) << \"Unknown command \" << static_cast(_cmd);\n> - }\n> -}\n> -{%- endif %}\n> +{{proxy_name}}Local::~{{proxy_name}}Local() = default;\n> \n> {% for method in interface_main.methods %}\n> -{{proxy_funcs.func_sig(proxy_name, method)}}\n> -{\n> - if (isolate_)\n> - return {{method.mojom_name}}IPC(\n> -{%- for param in method|method_param_names -%}\n> - {{param}}{{- \", \" if not loop.last}}\n> -{%- endfor -%}\n> -);\n> - else\n> - return {{method.mojom_name}}Thread(\n> -{%- for param in method|method_param_names -%}\n> - {{param}}{{- \", \" if not loop.last}}\n> -{%- endfor -%}\n> -);\n> -}\n> -\n> -{{proxy_funcs.func_sig(proxy_name, method, \"Thread\")}}\n> +{{proxy_funcs.func_sig(proxy_name + \"Local\", method)}}\n> {\n> {%- if method.mojom_name == \"stop\" %}\n> {{proxy_funcs.stop_thread_body()}}\n> @@ -181,8 +115,58 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)\n> );\n> {%- endif %}\n> }\n> +{% endfor %}\n> \n> -{{proxy_funcs.func_sig(proxy_name, method, \"IPC\")}}\n> +{% for method in interface_event.methods %}\n> +{{proxy_funcs.func_sig(proxy_name + \"Local\", method, \"Handler\")}}\n> +{\n> + ASSERT(state_ != ProxyStopped);\n> + {{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});\n> +}\n> +{% endfor %}\n> +\n> +/* ========================================================================== */\n> +\n> +{{proxy_name}}Remote::{{proxy_name}}Remote(IPAModule *ipam)\n> + : {{proxy_name}}(ipam),\n> + controlSerializer_(ControlSerializer::Role::Proxy), seq_(0)\n> +{\n> + LOG(IPAProxy, Debug)\n> + << \"initializing {{module_name}} proxy: loading IPA from \"\n> + << ipam->path();\n\nHere ^\n\n> +\n> + const std::string proxyWorkerPath = resolvePath(\"{{module_name}}_ipa_proxy\");\n> + if (proxyWorkerPath.empty()) {\n> + LOG(IPAProxy, Error)\n> + << \"Failed to get proxy worker path\";\n> + return;\n> + }\n> +\n> + auto ipc = std::make_unique(ipam->path().c_str(),\n> + proxyWorkerPath.c_str());\n> + if (!ipc->isConnected()) {\n> + LOG(IPAProxy, Error) << \"Failed to create IPCPipe\";\n> + return;\n> + }\n> +\n> + ipc->recv.connect(this, &{{proxy_name}}Remote::recvMessage);\n> +\n> + ipc_ = std::move(ipc);\n> + valid_ = true;\n> +}\n> +\n> +{{proxy_name}}Remote::~{{proxy_name}}Remote()\n> +{\n> + if (ipc_) {\n> + IPCMessage::Header header =\n> + { static_cast({{cmd_enum_name}}::Exit), seq_++ };\n> + IPCMessage msg(header);\n> + ipc_->sendAsync(msg);\n> + }\n> +}\n> +\n> +{% for method in interface_main.methods %}\n> +{{proxy_funcs.func_sig(proxy_name + \"Remote\", method)}}\n> {\n> {%- if method.mojom_name == \"configure\" %}\n> controlSerializer_.reset();\n> @@ -226,14 +210,25 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)\n> \n> {% endfor %}\n> \n> -{% for method in interface_event.methods %}\n> -{{proxy_funcs.func_sig(proxy_name, method, \"Thread\")}}\n> +void {{proxy_name}}Remote::recvMessage(const IPCMessage &data)\n> {\n> - ASSERT(state_ != ProxyStopped);\n> - {{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});\n> + size_t dataSize = data.data().size();\n> + {{cmd_event_enum_name}} _cmd = static_cast<{{cmd_event_enum_name}}>(data.header().cmd);\n> +\n> + switch (_cmd) {\n> +{%- for method in interface_event.methods %}\n> + case {{cmd_event_enum_name}}::{{method.mojom_name|cap}}: {\n> + {{method.mojom_name}}Handler(data.data().cbegin(), dataSize, data.fds());\n> + break;\n> + }\n> +{%- endfor %}\n> + default:\n> + LOG(IPAProxy, Error) << \"Unknown command \" << static_cast(_cmd);\n> + }\n> }\n> \n> -void {{proxy_name}}::{{method.mojom_name}}IPC(\n> +{% for method in interface_event.methods %}\n> +void {{proxy_name}}Remote::{{method.mojom_name}}Handler(\n> [[maybe_unused]] std::vector::const_iterator data,\n> [[maybe_unused]] size_t dataSize,\n> [[maybe_unused]] const std::vector &fds)\n> diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> index a0312a7c1..0c4ef985b 100644\n> --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> @@ -34,29 +34,32 @@ namespace {{ns}} {\n> {% endfor %}\n> {%- endif %}\n> \n> +class {{proxy_name}}Local;\n> +class {{proxy_name}}Remote;\n> +\n> class {{proxy_name}} : public IPAProxy, public {{interface_name}}, public Object\n> {\n> public:\n> - {{proxy_name}}(IPAModule *ipam, bool isolate);\n> - ~{{proxy_name}}();\n> + using Local = {{proxy_name}}Local;\n> + using Remote = {{proxy_name}}Remote;\n> \n> -{% for method in interface_main.methods %}\n> -{{proxy_funcs.func_sig(proxy_name, method, \"\", false, true)|indent(8, true)}};\n> -{% endfor %}\n> +protected:\n> + using IPAProxy::IPAProxy;\n> +};\n> \n> -private:\n> - void recvMessage(const IPCMessage &data);\n> +class {{proxy_name}}Local : public {{proxy_name}}\n> +{\n> +public:\n> + {{proxy_name}}Local(IPAModule *ipam);\n> + ~{{proxy_name}}Local();\n> \n> {% for method in interface_main.methods %}\n> -{{proxy_funcs.func_sig(proxy_name, method, \"Thread\", false)|indent(8, true)}};\n> -{{proxy_funcs.func_sig(proxy_name, method, \"IPC\", false)|indent(8, true)}};\n> +{{proxy_funcs.func_sig(proxy_name + \"Local\", method, \"\", false, true)|indent(8, true)}};\n> {% endfor %}\n> +\n> +private:\n> {% for method in interface_event.methods %}\n> -{{proxy_funcs.func_sig(proxy_name, method, \"Thread\", false)|indent(8, true)}};\n> - void {{method.mojom_name}}IPC(\n> - std::vector::const_iterator data,\n> - size_t dataSize,\n> - const std::vector &fds);\n> +{{proxy_funcs.func_sig(proxy_name + \"Local\", method, \"Handler\", false)|indent(8, true)}};\n> {% endfor %}\n> \n> /* Helper class to invoke async functions in another thread. */\n> @@ -79,12 +82,12 @@ private:\n> }\n> {% for method in interface_main.methods %}\n> {%- if method|is_async %}\n> - {{proxy_funcs.func_sig(proxy_name, method, \"\", false)|indent(16)}}\n> + {{proxy_funcs.func_sig(proxy_name + \"Local\", method, \"\", false)|indent(16)}}\n> {\n> ipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}});\n> }\n> {%- elif method.mojom_name == \"start\" %}\n> - {{proxy_funcs.func_sig(proxy_name, method, \"\", false)|indent(16)}}\n> + {{proxy_funcs.func_sig(proxy_name + \"Local\", method, \"\", false)|indent(16)}}\n> {\n> {%- if method|method_return_value != \"void\" %}\n> return ipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}});\n> @@ -104,8 +107,27 @@ private:\n> Thread thread_;\n> ThreadProxy proxy_;\n> std::unique_ptr<{{interface_name}}> ipa_;\n> +};\n> \n> - const bool isolate_;\n> +class {{proxy_name}}Remote : public {{proxy_name}}\n> +{\n> +public:\n> + {{proxy_name}}Remote(IPAModule *ipam);\n> + ~{{proxy_name}}Remote();\n> +\n> +{% for method in interface_main.methods %}\n> +{{proxy_funcs.func_sig(proxy_name + \"Local\", method, \"\", false, true)|indent(8, true)}};\n\ns/Local/Remote/ ?\n\n\nOther than that, looks good to me. This is exciting!\n\nPaul\n\n> +{% endfor %}\n> +\n> +private:\n> + void recvMessage(const IPCMessage &data);\n> +\n> +{% for method in interface_event.methods %}\n> + void {{method.mojom_name}}Handler(\n> + std::vector::const_iterator data,\n> + size_t dataSize,\n> + const std::vector &fds);\n> +{% endfor %}\n> \n> std::unique_ptr ipc_;\n> \n> -- \n> 2.50.1\n>","headers":{"Return-Path":"","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 F1BD9BDCC1\n\tfor ;\n\tThu, 14 Aug 2025 08:59:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1E96E69233;\n\tThu, 14 Aug 2025 10:59:26 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 32DF061436\n\tfor ;\n\tThu, 14 Aug 2025 10:59:24 +0200 (CEST)","from neptunite.rasen.tech (unknown\n\t[IPv6:2404:7a81:160:2100:d967:c4f9:b351:2e11])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 7E2E27E6;\n\tThu, 14 Aug 2025 10:58:29 +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=\"uFvhev4S\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1755161910;\n\tbh=nNxSQC/yAxOU9q4wm3zqYuEV7WbYT1Ip8+Omqrb63Qg=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=uFvhev4S/i/PRaoq6UoMF7WvLV0whxXcdUxOr7QtFNAhmm1hFvkE0mWFFwdHBVkaX\n\taKeqsaEmOi5qLqPazmw2UgT3c5JrUTSu178iLDH1yPb4ecHTKGlJr1z1QvTLzMSMZu\n\tO8jPiOdu81biwu22OxLjC6U89kMtHnPugGKuO354=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250804165226.63825-1-barnabas.pocze@ideasonboard.com>","References":"<20250804165226.63825-1-barnabas.pocze@ideasonboard.com>","Subject":"Re: [RFC PATCH v1] utils: codegen: ipc: Split proxy types","From":"Paul Elder ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 14 Aug 2025 17:59:15 +0900","Message-ID":"<175516195566.3701817.15703185411370250000@neptunite.rasen.tech>","User-Agent":"alot/0.0.0","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":35401,"web_url":"https://patchwork.libcamera.org/comment/35401/","msgid":"<0015f255-e725-42d7-aefb-1330b4940bc3@ideasonboard.com>","date":"2025-08-14T09:42:39","subject":"Re: [RFC PATCH v1] utils: codegen: ipc: Split proxy types","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n2025. 08. 14. 10:59 keltezéssel, Paul Elder írta:\n> Hi Barnabás,\n> \n> Thanks for the patch.\n> \n> Quoting Barnabás Pőcze (2025-08-05 01:52:26)\n>> Even though there is an abstract class to represent the interface of an IPA,\n>> the threaded and IPC versions are still multiplexed using the same type,\n>> which uses a boolean to actually dispatch to the right function.\n>>\n>> Instead of doing that, split the classes into a \"local\" and \"remote\" variant,\n>> and make `IPAManager` choose accordingly.\n> \n> Wow, that is a cool upgrade!\n> \n>>\n>> Signed-off-by: Barnabás Pőcze \n>> ---\n>> include/libcamera/internal/ipa_manager.h | 8 +-\n>> .../module_ipa_proxy.cpp.tmpl | 149 +++++++++---------\n>> .../module_ipa_proxy.h.tmpl | 56 +++++--\n>> 3 files changed, 118 insertions(+), 95 deletions(-)\n>>\n>> diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h\n>> index a0d448cf9..847a69066 100644\n>> --- a/include/libcamera/internal/ipa_manager.h\n>> +++ b/include/libcamera/internal/ipa_manager.h\n>> @@ -42,7 +42,13 @@ public:\n>> if (!m)\n>> return nullptr;\n>> \n>> - std::unique_ptr proxy = std::make_unique(m, !self->isSignatureValid(m));\n>> + auto proxy = [&]() -> std::unique_ptr {\n>> + if (self->isSignatureValid(m))\n>> + return std::make_unique(m);\n>> + else\n>> + return std::make_unique(m);\n>> + } ();\n>> +\n>> if (!proxy->isValid()) {\n>> LOG(IPAManager, Error) << \"Failed to load proxy\";\n>> return nullptr;\n>> diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n>> index beb646e2d..8792b3b9e 100644\n>> --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n>> +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n>> @@ -45,36 +45,13 @@ namespace {{ns}} {\n>> {% endfor %}\n>> {%- endif %}\n>> \n>> -{{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate)\n>> - : IPAProxy(ipam), isolate_(isolate),\n>> - controlSerializer_(ControlSerializer::Role::Proxy), seq_(0)\n>> +{{proxy_name}}Local::{{proxy_name}}Local(IPAModule *ipam)\n>> + : {{proxy_name}}(ipam)\n>> {\n>> LOG(IPAProxy, Debug)\n>> << \"initializing {{module_name}} proxy: loading IPA from \"\n>> << ipam->path();\n> \n> Might it be worth it to also print information about isolation? Same for the\n> constructor below.\n> \n>> \n>> - if (isolate_) {\n>> - const std::string proxyWorkerPath = resolvePath(\"{{module_name}}_ipa_proxy\");\n>> - if (proxyWorkerPath.empty()) {\n>> - LOG(IPAProxy, Error)\n>> - << \"Failed to get proxy worker path\";\n>> - return;\n>> - }\n>> -\n>> - auto ipc = std::make_unique(ipam->path().c_str(),\n>> - proxyWorkerPath.c_str());\n>> - if (!ipc->isConnected()) {\n>> - LOG(IPAProxy, Error) << \"Failed to create IPCPipe\";\n>> - return;\n>> - }\n>> -\n>> - ipc->recv.connect(this, &{{proxy_name}}::recvMessage);\n>> -\n>> - ipc_ = std::move(ipc);\n>> - valid_ = true;\n>> - return;\n>> - }\n>> -\n>> if (!ipam->load())\n>> return;\n>> \n>> @@ -89,59 +66,16 @@ namespace {{ns}} {\n>> proxy_.setIPA(ipa_.get());\n>> \n>> {% for method in interface_event.methods %}\n>> - ipa_->{{method.mojom_name}}.connect(this, &{{proxy_name}}::{{method.mojom_name}}Thread);\n>> + ipa_->{{method.mojom_name}}.connect(this, &{{proxy_name}}Local::{{method.mojom_name}}Handler);\n>> {%- endfor %}\n>> \n>> valid_ = true;\n>> }\n>> \n>> -{{proxy_name}}::~{{proxy_name}}()\n>> -{\n>> - if (ipc_) {\n>> - IPCMessage::Header header =\n>> - { static_cast({{cmd_enum_name}}::Exit), seq_++ };\n>> - IPCMessage msg(header);\n>> - ipc_->sendAsync(msg);\n>> - }\n>> -}\n>> -\n>> -{% if interface_event.methods|length > 0 %}\n>> -void {{proxy_name}}::recvMessage(const IPCMessage &data)\n>> -{\n>> - size_t dataSize = data.data().size();\n>> - {{cmd_event_enum_name}} _cmd = static_cast<{{cmd_event_enum_name}}>(data.header().cmd);\n>> -\n>> - switch (_cmd) {\n>> -{%- for method in interface_event.methods %}\n>> - case {{cmd_event_enum_name}}::{{method.mojom_name|cap}}: {\n>> - {{method.mojom_name}}IPC(data.data().cbegin(), dataSize, data.fds());\n>> - break;\n>> - }\n>> -{%- endfor %}\n>> - default:\n>> - LOG(IPAProxy, Error) << \"Unknown command \" << static_cast(_cmd);\n>> - }\n>> -}\n>> -{%- endif %}\n>> +{{proxy_name}}Local::~{{proxy_name}}Local() = default;\n>> \n>> {% for method in interface_main.methods %}\n>> -{{proxy_funcs.func_sig(proxy_name, method)}}\n>> -{\n>> - if (isolate_)\n>> - return {{method.mojom_name}}IPC(\n>> -{%- for param in method|method_param_names -%}\n>> - {{param}}{{- \", \" if not loop.last}}\n>> -{%- endfor -%}\n>> -);\n>> - else\n>> - return {{method.mojom_name}}Thread(\n>> -{%- for param in method|method_param_names -%}\n>> - {{param}}{{- \", \" if not loop.last}}\n>> -{%- endfor -%}\n>> -);\n>> -}\n>> -\n>> -{{proxy_funcs.func_sig(proxy_name, method, \"Thread\")}}\n>> +{{proxy_funcs.func_sig(proxy_name + \"Local\", method)}}\n>> {\n>> {%- if method.mojom_name == \"stop\" %}\n>> {{proxy_funcs.stop_thread_body()}}\n>> @@ -181,8 +115,58 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)\n>> );\n>> {%- endif %}\n>> }\n>> +{% endfor %}\n>> \n>> -{{proxy_funcs.func_sig(proxy_name, method, \"IPC\")}}\n>> +{% for method in interface_event.methods %}\n>> +{{proxy_funcs.func_sig(proxy_name + \"Local\", method, \"Handler\")}}\n>> +{\n>> + ASSERT(state_ != ProxyStopped);\n>> + {{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});\n>> +}\n>> +{% endfor %}\n>> +\n>> +/* ========================================================================== */\n>> +\n>> +{{proxy_name}}Remote::{{proxy_name}}Remote(IPAModule *ipam)\n>> + : {{proxy_name}}(ipam),\n>> + controlSerializer_(ControlSerializer::Role::Proxy), seq_(0)\n>> +{\n>> + LOG(IPAProxy, Debug)\n>> + << \"initializing {{module_name}} proxy: loading IPA from \"\n>> + << ipam->path();\n> \n> Here ^\n\nI'll add \"locally\" and \"remotely\" to the messages. Or should it be more concrete?\n\n\n> \n>> +\n>> + const std::string proxyWorkerPath = resolvePath(\"{{module_name}}_ipa_proxy\");\n>> + if (proxyWorkerPath.empty()) {\n>> + LOG(IPAProxy, Error)\n>> + << \"Failed to get proxy worker path\";\n>> + return;\n>> + }\n>> +\n>> + auto ipc = std::make_unique(ipam->path().c_str(),\n>> + proxyWorkerPath.c_str());\n>> + if (!ipc->isConnected()) {\n>> + LOG(IPAProxy, Error) << \"Failed to create IPCPipe\";\n>> + return;\n>> + }\n>> +\n>> + ipc->recv.connect(this, &{{proxy_name}}Remote::recvMessage);\n>> +\n>> + ipc_ = std::move(ipc);\n>> + valid_ = true;\n>> +}\n>> +\n>> +{{proxy_name}}Remote::~{{proxy_name}}Remote()\n>> +{\n>> + if (ipc_) {\n>> + IPCMessage::Header header =\n>> + { static_cast({{cmd_enum_name}}::Exit), seq_++ };\n>> + IPCMessage msg(header);\n>> + ipc_->sendAsync(msg);\n>> + }\n>> +}\n>> +\n>> +{% for method in interface_main.methods %}\n>> +{{proxy_funcs.func_sig(proxy_name + \"Remote\", method)}}\n>> {\n>> {%- if method.mojom_name == \"configure\" %}\n>> controlSerializer_.reset();\n>> @@ -226,14 +210,25 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)\n>> \n>> {% endfor %}\n>> \n>> -{% for method in interface_event.methods %}\n>> -{{proxy_funcs.func_sig(proxy_name, method, \"Thread\")}}\n>> +void {{proxy_name}}Remote::recvMessage(const IPCMessage &data)\n>> {\n>> - ASSERT(state_ != ProxyStopped);\n>> - {{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});\n>> + size_t dataSize = data.data().size();\n>> + {{cmd_event_enum_name}} _cmd = static_cast<{{cmd_event_enum_name}}>(data.header().cmd);\n>> +\n>> + switch (_cmd) {\n>> +{%- for method in interface_event.methods %}\n>> + case {{cmd_event_enum_name}}::{{method.mojom_name|cap}}: {\n>> + {{method.mojom_name}}Handler(data.data().cbegin(), dataSize, data.fds());\n>> + break;\n>> + }\n>> +{%- endfor %}\n>> + default:\n>> + LOG(IPAProxy, Error) << \"Unknown command \" << static_cast(_cmd);\n>> + }\n>> }\n>> \n>> -void {{proxy_name}}::{{method.mojom_name}}IPC(\n>> +{% for method in interface_event.methods %}\n>> +void {{proxy_name}}Remote::{{method.mojom_name}}Handler(\n>> [[maybe_unused]] std::vector::const_iterator data,\n>> [[maybe_unused]] size_t dataSize,\n>> [[maybe_unused]] const std::vector &fds)\n>> diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n>> index a0312a7c1..0c4ef985b 100644\n>> --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n>> +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n>> @@ -34,29 +34,32 @@ namespace {{ns}} {\n>> {% endfor %}\n>> {%- endif %}\n>> \n>> +class {{proxy_name}}Local;\n>> +class {{proxy_name}}Remote;\n>> +\n>> class {{proxy_name}} : public IPAProxy, public {{interface_name}}, public Object\n>> {\n>> public:\n>> - {{proxy_name}}(IPAModule *ipam, bool isolate);\n>> - ~{{proxy_name}}();\n>> + using Local = {{proxy_name}}Local;\n>> + using Remote = {{proxy_name}}Remote;\n>> \n>> -{% for method in interface_main.methods %}\n>> -{{proxy_funcs.func_sig(proxy_name, method, \"\", false, true)|indent(8, true)}};\n>> -{% endfor %}\n>> +protected:\n>> + using IPAProxy::IPAProxy;\n>> +};\n>> \n>> -private:\n>> - void recvMessage(const IPCMessage &data);\n>> +class {{proxy_name}}Local : public {{proxy_name}}\n>> +{\n>> +public:\n>> + {{proxy_name}}Local(IPAModule *ipam);\n>> + ~{{proxy_name}}Local();\n>> \n>> {% for method in interface_main.methods %}\n>> -{{proxy_funcs.func_sig(proxy_name, method, \"Thread\", false)|indent(8, true)}};\n>> -{{proxy_funcs.func_sig(proxy_name, method, \"IPC\", false)|indent(8, true)}};\n>> +{{proxy_funcs.func_sig(proxy_name + \"Local\", method, \"\", false, true)|indent(8, true)}};\n>> {% endfor %}\n>> +\n>> +private:\n>> {% for method in interface_event.methods %}\n>> -{{proxy_funcs.func_sig(proxy_name, method, \"Thread\", false)|indent(8, true)}};\n>> - void {{method.mojom_name}}IPC(\n>> - std::vector::const_iterator data,\n>> - size_t dataSize,\n>> - const std::vector &fds);\n>> +{{proxy_funcs.func_sig(proxy_name + \"Local\", method, \"Handler\", false)|indent(8, true)}};\n>> {% endfor %}\n>> \n>> /* Helper class to invoke async functions in another thread. */\n>> @@ -79,12 +82,12 @@ private:\n>> }\n>> {% for method in interface_main.methods %}\n>> {%- if method|is_async %}\n>> - {{proxy_funcs.func_sig(proxy_name, method, \"\", false)|indent(16)}}\n>> + {{proxy_funcs.func_sig(proxy_name + \"Local\", method, \"\", false)|indent(16)}}\n>> {\n>> ipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}});\n>> }\n>> {%- elif method.mojom_name == \"start\" %}\n>> - {{proxy_funcs.func_sig(proxy_name, method, \"\", false)|indent(16)}}\n>> + {{proxy_funcs.func_sig(proxy_name + \"Local\", method, \"\", false)|indent(16)}}\n>> {\n>> {%- if method|method_return_value != \"void\" %}\n>> return ipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}});\n>> @@ -104,8 +107,27 @@ private:\n>> Thread thread_;\n>> ThreadProxy proxy_;\n>> std::unique_ptr<{{interface_name}}> ipa_;\n>> +};\n>> \n>> - const bool isolate_;\n>> +class {{proxy_name}}Remote : public {{proxy_name}}\n>> +{\n>> +public:\n>> + {{proxy_name}}Remote(IPAModule *ipam);\n>> + ~{{proxy_name}}Remote();\n>> +\n>> +{% for method in interface_main.methods %}\n>> +{{proxy_funcs.func_sig(proxy_name + \"Local\", method, \"\", false, true)|indent(8, true)}};\n> \n> s/Local/Remote/ ?\n> \n> \n> Other than that, looks good to me. This is exciting!\n\nOops, fixed. I was wondering why it did not cause any issues... of course\nthe name of the class is not used due to the fourth argument being false.\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \n> Paul\n> \n>> +{% endfor %}\n>> +\n>> +private:\n>> + void recvMessage(const IPCMessage &data);\n>> +\n>> +{% for method in interface_event.methods %}\n>> + void {{method.mojom_name}}Handler(\n>> + std::vector::const_iterator data,\n>> + size_t dataSize,\n>> + const std::vector &fds);\n>> +{% endfor %}\n>> \n>> std::unique_ptr ipc_;\n>> \n>> -- \n>> 2.50.1\n>>","headers":{"Return-Path":"","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 2FD59BEFBE\n\tfor ;\n\tThu, 14 Aug 2025 09:42:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0EEFF69252;\n\tThu, 14 Aug 2025 11:42:45 +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 A010161436\n\tfor ;\n\tThu, 14 Aug 2025 11:42:43 +0200 (CEST)","from [192.168.33.15] (185.221.141.188.nat.pool.zt.hu\n\t[185.221.141.188])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 32FE983D;\n\tThu, 14 Aug 2025 11:41:49 +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=\"XsvDnUvI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1755164509;\n\tbh=G+TqfLEQ5sqtLQRuze9cITxNCx9Ic7Zl+j1ybp09t8o=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=XsvDnUvIVeL44EWVpgeko/nnMN3Feci4l94uZDfUfJrHjURbWJpUAuZVVqLVdKfve\n\tC37bDyx1ppDEESQQoNh5gVg/a/bM61dqXZL3lyfEVigCIfDeoWvuATtaWEHb/zxCgZ\n\t2H10GciKTkHpbZSaDfBUOl3+em1Gt6lETwaVPskw=","Message-ID":"<0015f255-e725-42d7-aefb-1330b4940bc3@ideasonboard.com>","Date":"Thu, 14 Aug 2025 11:42:39 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [RFC PATCH v1] utils: codegen: ipc: Split proxy types","To":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20250804165226.63825-1-barnabas.pocze@ideasonboard.com>\n\t<175516195566.3701817.15703185411370250000@neptunite.rasen.tech>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<175516195566.3701817.15703185411370250000@neptunite.rasen.tech>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":35419,"web_url":"https://patchwork.libcamera.org/comment/35419/","msgid":"<20250814222448.GD6201@pendragon.ideasonboard.com>","date":"2025-08-14T22:24:48","subject":"Re: [RFC PATCH v1] utils: codegen: ipc: Split proxy types","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Thu, Aug 14, 2025 at 11:42:39AM +0200, Barnabás Pőcze wrote:\n> 2025. 08. 14. 10:59 keltezéssel, Paul Elder írta:\n> > Quoting Barnabás Pőcze (2025-08-05 01:52:26)\n> >> Even though there is an abstract class to represent the interface of an IPA,\n> >> the threaded and IPC versions are still multiplexed using the same type,\n> >> which uses a boolean to actually dispatch to the right function.\n> >>\n> >> Instead of doing that, split the classes into a \"local\" and \"remote\" variant,\n> >> and make `IPAManager` choose accordingly.\n\n\"Local\" and \"Remote\" are not the clearest names. Would \"Threaded\" and\n\"Isolated\" be better ? I'm sure there are even better option.\n\n> > Wow, that is a cool upgrade!\n> > \n> >> Signed-off-by: Barnabás Pőcze \n> >> ---\n> >> include/libcamera/internal/ipa_manager.h | 8 +-\n> >> .../module_ipa_proxy.cpp.tmpl | 149 +++++++++---------\n> >> .../module_ipa_proxy.h.tmpl | 56 +++++--\n> >> 3 files changed, 118 insertions(+), 95 deletions(-)\n> >>\n> >> diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h\n> >> index a0d448cf9..847a69066 100644\n> >> --- a/include/libcamera/internal/ipa_manager.h\n> >> +++ b/include/libcamera/internal/ipa_manager.h\n> >> @@ -42,7 +42,13 @@ public:\n> >> if (!m)\n> >> return nullptr;\n> >> \n> >> - std::unique_ptr proxy = std::make_unique(m, !self->isSignatureValid(m));\n> >> + auto proxy = [&]() -> std::unique_ptr {\n> >> + if (self->isSignatureValid(m))\n> >> + return std::make_unique(m);\n> >> + else\n> >> + return std::make_unique(m);\n> >> + } ();\n\nIs this using a lambda just for the sake of it ? :-)\n\n\t\tstd::unique_ptrisSignatureValid(m)\n\t\t\t\t\t? std::make_unique(m)\n\t\t\t\t\t: std::make_unique(m);\n\n> >> +\n> >> if (!proxy->isValid()) {\n> >> LOG(IPAManager, Error) << \"Failed to load proxy\";\n> >> return nullptr;\n> >> diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> >> index beb646e2d..8792b3b9e 100644\n> >> --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> >> +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> >> @@ -45,36 +45,13 @@ namespace {{ns}} {\n> >> {% endfor %}\n> >> {%- endif %}\n> >> \n> >> -{{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate)\n> >> - : IPAProxy(ipam), isolate_(isolate),\n> >> - controlSerializer_(ControlSerializer::Role::Proxy), seq_(0)\n> >> +{{proxy_name}}Local::{{proxy_name}}Local(IPAModule *ipam)\n> >> + : {{proxy_name}}(ipam)\n> >> {\n> >> LOG(IPAProxy, Debug)\n> >> << \"initializing {{module_name}} proxy: loading IPA from \"\n> >> << ipam->path();\n> > \n> > Might it be worth it to also print information about isolation? Same for the\n> > constructor below.\n> > \n> >> \n> >> - if (isolate_) {\n> >> - const std::string proxyWorkerPath = resolvePath(\"{{module_name}}_ipa_proxy\");\n> >> - if (proxyWorkerPath.empty()) {\n> >> - LOG(IPAProxy, Error)\n> >> - << \"Failed to get proxy worker path\";\n> >> - return;\n> >> - }\n> >> -\n> >> - auto ipc = std::make_unique(ipam->path().c_str(),\n> >> - proxyWorkerPath.c_str());\n> >> - if (!ipc->isConnected()) {\n> >> - LOG(IPAProxy, Error) << \"Failed to create IPCPipe\";\n> >> - return;\n> >> - }\n> >> -\n> >> - ipc->recv.connect(this, &{{proxy_name}}::recvMessage);\n> >> -\n> >> - ipc_ = std::move(ipc);\n> >> - valid_ = true;\n> >> - return;\n> >> - }\n> >> -\n> >> if (!ipam->load())\n> >> return;\n> >> \n> >> @@ -89,59 +66,16 @@ namespace {{ns}} {\n> >> proxy_.setIPA(ipa_.get());\n> >> \n> >> {% for method in interface_event.methods %}\n> >> - ipa_->{{method.mojom_name}}.connect(this, &{{proxy_name}}::{{method.mojom_name}}Thread);\n> >> + ipa_->{{method.mojom_name}}.connect(this, &{{proxy_name}}Local::{{method.mojom_name}}Handler);\n> >> {%- endfor %}\n> >> \n> >> valid_ = true;\n> >> }\n> >> \n> >> -{{proxy_name}}::~{{proxy_name}}()\n> >> -{\n> >> - if (ipc_) {\n> >> - IPCMessage::Header header =\n> >> - { static_cast({{cmd_enum_name}}::Exit), seq_++ };\n> >> - IPCMessage msg(header);\n> >> - ipc_->sendAsync(msg);\n> >> - }\n> >> -}\n> >> -\n> >> -{% if interface_event.methods|length > 0 %}\n> >> -void {{proxy_name}}::recvMessage(const IPCMessage &data)\n> >> -{\n> >> - size_t dataSize = data.data().size();\n> >> - {{cmd_event_enum_name}} _cmd = static_cast<{{cmd_event_enum_name}}>(data.header().cmd);\n> >> -\n> >> - switch (_cmd) {\n> >> -{%- for method in interface_event.methods %}\n> >> - case {{cmd_event_enum_name}}::{{method.mojom_name|cap}}: {\n> >> - {{method.mojom_name}}IPC(data.data().cbegin(), dataSize, data.fds());\n> >> - break;\n> >> - }\n> >> -{%- endfor %}\n> >> - default:\n> >> - LOG(IPAProxy, Error) << \"Unknown command \" << static_cast(_cmd);\n> >> - }\n> >> -}\n> >> -{%- endif %}\n> >> +{{proxy_name}}Local::~{{proxy_name}}Local() = default;\n> >> \n> >> {% for method in interface_main.methods %}\n> >> -{{proxy_funcs.func_sig(proxy_name, method)}}\n> >> -{\n> >> - if (isolate_)\n> >> - return {{method.mojom_name}}IPC(\n> >> -{%- for param in method|method_param_names -%}\n> >> - {{param}}{{- \", \" if not loop.last}}\n> >> -{%- endfor -%}\n> >> -);\n> >> - else\n> >> - return {{method.mojom_name}}Thread(\n> >> -{%- for param in method|method_param_names -%}\n> >> - {{param}}{{- \", \" if not loop.last}}\n> >> -{%- endfor -%}\n> >> -);\n> >> -}\n> >> -\n> >> -{{proxy_funcs.func_sig(proxy_name, method, \"Thread\")}}\n> >> +{{proxy_funcs.func_sig(proxy_name + \"Local\", method)}}\n> >> {\n> >> {%- if method.mojom_name == \"stop\" %}\n> >> {{proxy_funcs.stop_thread_body()}}\n> >> @@ -181,8 +115,58 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)\n> >> );\n> >> {%- endif %}\n> >> }\n> >> +{% endfor %}\n> >> \n> >> -{{proxy_funcs.func_sig(proxy_name, method, \"IPC\")}}\n> >> +{% for method in interface_event.methods %}\n> >> +{{proxy_funcs.func_sig(proxy_name + \"Local\", method, \"Handler\")}}\n> >> +{\n> >> + ASSERT(state_ != ProxyStopped);\n> >> + {{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});\n> >> +}\n> >> +{% endfor %}\n> >> +\n> >> +/* ========================================================================== */\n> >> +\n> >> +{{proxy_name}}Remote::{{proxy_name}}Remote(IPAModule *ipam)\n> >> + : {{proxy_name}}(ipam),\n> >> + controlSerializer_(ControlSerializer::Role::Proxy), seq_(0)\n> >> +{\n> >> + LOG(IPAProxy, Debug)\n> >> + << \"initializing {{module_name}} proxy: loading IPA from \"\n> >> + << ipam->path();\n> > \n> > Here ^\n> \n> I'll add \"locally\" and \"remotely\" to the messages. Or should it be more concrete?\n> \n> >> +\n> >> + const std::string proxyWorkerPath = resolvePath(\"{{module_name}}_ipa_proxy\");\n> >> + if (proxyWorkerPath.empty()) {\n> >> + LOG(IPAProxy, Error)\n> >> + << \"Failed to get proxy worker path\";\n> >> + return;\n> >> + }\n> >> +\n> >> + auto ipc = std::make_unique(ipam->path().c_str(),\n> >> + proxyWorkerPath.c_str());\n> >> + if (!ipc->isConnected()) {\n> >> + LOG(IPAProxy, Error) << \"Failed to create IPCPipe\";\n> >> + return;\n> >> + }\n> >> +\n> >> + ipc->recv.connect(this, &{{proxy_name}}Remote::recvMessage);\n> >> +\n> >> + ipc_ = std::move(ipc);\n> >> + valid_ = true;\n> >> +}\n> >> +\n> >> +{{proxy_name}}Remote::~{{proxy_name}}Remote()\n> >> +{\n> >> + if (ipc_) {\n> >> + IPCMessage::Header header =\n> >> + { static_cast({{cmd_enum_name}}::Exit), seq_++ };\n> >> + IPCMessage msg(header);\n> >> + ipc_->sendAsync(msg);\n> >> + }\n> >> +}\n> >> +\n> >> +{% for method in interface_main.methods %}\n> >> +{{proxy_funcs.func_sig(proxy_name + \"Remote\", method)}}\n> >> {\n> >> {%- if method.mojom_name == \"configure\" %}\n> >> controlSerializer_.reset();\n> >> @@ -226,14 +210,25 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)\n> >> \n> >> {% endfor %}\n> >> \n> >> -{% for method in interface_event.methods %}\n> >> -{{proxy_funcs.func_sig(proxy_name, method, \"Thread\")}}\n> >> +void {{proxy_name}}Remote::recvMessage(const IPCMessage &data)\n> >> {\n> >> - ASSERT(state_ != ProxyStopped);\n> >> - {{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});\n> >> + size_t dataSize = data.data().size();\n> >> + {{cmd_event_enum_name}} _cmd = static_cast<{{cmd_event_enum_name}}>(data.header().cmd);\n> >> +\n> >> + switch (_cmd) {\n> >> +{%- for method in interface_event.methods %}\n> >> + case {{cmd_event_enum_name}}::{{method.mojom_name|cap}}: {\n> >> + {{method.mojom_name}}Handler(data.data().cbegin(), dataSize, data.fds());\n> >> + break;\n> >> + }\n> >> +{%- endfor %}\n> >> + default:\n> >> + LOG(IPAProxy, Error) << \"Unknown command \" << static_cast(_cmd);\n> >> + }\n> >> }\n> >> \n> >> -void {{proxy_name}}::{{method.mojom_name}}IPC(\n> >> +{% for method in interface_event.methods %}\n> >> +void {{proxy_name}}Remote::{{method.mojom_name}}Handler(\n> >> [[maybe_unused]] std::vector::const_iterator data,\n> >> [[maybe_unused]] size_t dataSize,\n> >> [[maybe_unused]] const std::vector &fds)\n> >> diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> >> index a0312a7c1..0c4ef985b 100644\n> >> --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> >> +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> >> @@ -34,29 +34,32 @@ namespace {{ns}} {\n> >> {% endfor %}\n> >> {%- endif %}\n> >> \n> >> +class {{proxy_name}}Local;\n> >> +class {{proxy_name}}Remote;\n> >> +\n> >> class {{proxy_name}} : public IPAProxy, public {{interface_name}}, public Object\n> >> {\n> >> public:\n> >> - {{proxy_name}}(IPAModule *ipam, bool isolate);\n> >> - ~{{proxy_name}}();\n> >> + using Local = {{proxy_name}}Local;\n> >> + using Remote = {{proxy_name}}Remote;\n> >> \n> >> -{% for method in interface_main.methods %}\n> >> -{{proxy_funcs.func_sig(proxy_name, method, \"\", false, true)|indent(8, true)}};\n> >> -{% endfor %}\n> >> +protected:\n> >> + using IPAProxy::IPAProxy;\n> >> +};\n> >> \n> >> -private:\n> >> - void recvMessage(const IPCMessage &data);\n> >> +class {{proxy_name}}Local : public {{proxy_name}}\n> >> +{\n> >> +public:\n> >> + {{proxy_name}}Local(IPAModule *ipam);\n> >> + ~{{proxy_name}}Local();\n> >> \n> >> {% for method in interface_main.methods %}\n> >> -{{proxy_funcs.func_sig(proxy_name, method, \"Thread\", false)|indent(8, true)}};\n> >> -{{proxy_funcs.func_sig(proxy_name, method, \"IPC\", false)|indent(8, true)}};\n> >> +{{proxy_funcs.func_sig(proxy_name + \"Local\", method, \"\", false, true)|indent(8, true)}};\n\nAs this is Python code I *think* you could also write\n\n{{proxy_funcs.func_sig(f\"{proxy_name}Local\", method, \"\", false, true)|indent(8, true)}};\n\nif you think it's clearer (sae elsewhere in the patch).\n\n> >> {% endfor %}\n> >> +\n> >> +private:\n> >> {% for method in interface_event.methods %}\n> >> -{{proxy_funcs.func_sig(proxy_name, method, \"Thread\", false)|indent(8, true)}};\n> >> - void {{method.mojom_name}}IPC(\n> >> - std::vector::const_iterator data,\n> >> - size_t dataSize,\n> >> - const std::vector &fds);\n> >> +{{proxy_funcs.func_sig(proxy_name + \"Local\", method, \"Handler\", false)|indent(8, true)}};\n> >> {% endfor %}\n> >> \n> >> /* Helper class to invoke async functions in another thread. */\n> >> @@ -79,12 +82,12 @@ private:\n> >> }\n> >> {% for method in interface_main.methods %}\n> >> {%- if method|is_async %}\n> >> - {{proxy_funcs.func_sig(proxy_name, method, \"\", false)|indent(16)}}\n> >> + {{proxy_funcs.func_sig(proxy_name + \"Local\", method, \"\", false)|indent(16)}}\n> >> {\n> >> ipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}});\n> >> }\n> >> {%- elif method.mojom_name == \"start\" %}\n> >> - {{proxy_funcs.func_sig(proxy_name, method, \"\", false)|indent(16)}}\n> >> + {{proxy_funcs.func_sig(proxy_name + \"Local\", method, \"\", false)|indent(16)}}\n> >> {\n> >> {%- if method|method_return_value != \"void\" %}\n> >> return ipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}});\n> >> @@ -104,8 +107,27 @@ private:\n> >> Thread thread_;\n> >> ThreadProxy proxy_;\n> >> std::unique_ptr<{{interface_name}}> ipa_;\n> >> +};\n> >> \n> >> - const bool isolate_;\n> >> +class {{proxy_name}}Remote : public {{proxy_name}}\n> >> +{\n> >> +public:\n> >> + {{proxy_name}}Remote(IPAModule *ipam);\n> >> + ~{{proxy_name}}Remote();\n> >> +\n> >> +{% for method in interface_main.methods %}\n> >> +{{proxy_funcs.func_sig(proxy_name + \"Local\", method, \"\", false, true)|indent(8, true)}};\n> > \n> > s/Local/Remote/ ?\n> > \n> > Other than that, looks good to me. This is exciting!\n> \n> Oops, fixed. I was wondering why it did not cause any issues... of course\n> the name of the class is not used due to the fourth argument being false.\n> \n> >> +{% endfor %}\n> >> +\n> >> +private:\n> >> + void recvMessage(const IPCMessage &data);\n> >> +\n> >> +{% for method in interface_event.methods %}\n> >> + void {{method.mojom_name}}Handler(\n> >> + std::vector::const_iterator data,\n> >> + size_t dataSize,\n> >> + const std::vector &fds);\n> >> +{% endfor %}\n> >> \n> >> std::unique_ptr ipc_;\n> >>","headers":{"Return-Path":"","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 A8AB5BEFBE\n\tfor ;\n\tThu, 14 Aug 2025 22:25:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BDFD96922C;\n\tFri, 15 Aug 2025 00:25:09 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 04EA96922C\n\tfor ;\n\tFri, 15 Aug 2025 00:25:08 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 674306DF;\n\tFri, 15 Aug 2025 00:24:13 +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=\"G0c6PVct\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1755210253;\n\tbh=0Nclevn5i7sKmDqxwdViZk8Bp+Alw6/DvCFk6Th9cFg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=G0c6PVctcMbkI9FlMicYYo3x4SzLrn9eW8pcFO2yDomxD6APDrzsJQnvNhzesofNR\n\tiLi3rPjXWdHb19rY5/9FR9NNZWOI6OUzAwRZPgw0sipWlk6V00eRyQJkdSh94saRfS\n\tJIrXpWUc/IF9WwFCoO3X/20+5lRe3eoCFy9Plpss=","Date":"Fri, 15 Aug 2025 01:24:48 +0300","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v1] utils: codegen: ipc: Split proxy types","Message-ID":"<20250814222448.GD6201@pendragon.ideasonboard.com>","References":"<20250804165226.63825-1-barnabas.pocze@ideasonboard.com>\n\t<175516195566.3701817.15703185411370250000@neptunite.rasen.tech>\n\t<0015f255-e725-42d7-aefb-1330b4940bc3@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<0015f255-e725-42d7-aefb-1330b4940bc3@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":35422,"web_url":"https://patchwork.libcamera.org/comment/35422/","msgid":"","date":"2025-08-15T07:10:02","subject":"Re: [RFC PATCH v1] utils: codegen: ipc: Split proxy types","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"2025. 08. 15. 0:24 keltezéssel, Laurent Pinchart írta:\n> On Thu, Aug 14, 2025 at 11:42:39AM +0200, Barnabás Pőcze wrote:\n>> 2025. 08. 14. 10:59 keltezéssel, Paul Elder írta:\n>>> Quoting Barnabás Pőcze (2025-08-05 01:52:26)\n>>>> Even though there is an abstract class to represent the interface of an IPA,\n>>>> the threaded and IPC versions are still multiplexed using the same type,\n>>>> which uses a boolean to actually dispatch to the right function.\n>>>>\n>>>> Instead of doing that, split the classes into a \"local\" and \"remote\" variant,\n>>>> and make `IPAManager` choose accordingly.\n> \n> \"Local\" and \"Remote\" are not the clearest names. Would \"Threaded\" and\n> \"Isolated\" be better ? I'm sure there are even better option.\n\nI'm sure there are endless opportunities. I will rename them.\n\n\n> \n>>> Wow, that is a cool upgrade!\n>>>\n>>>> Signed-off-by: Barnabás Pőcze \n>>>> ---\n>>>> include/libcamera/internal/ipa_manager.h | 8 +-\n>>>> .../module_ipa_proxy.cpp.tmpl | 149 +++++++++---------\n>>>> .../module_ipa_proxy.h.tmpl | 56 +++++--\n>>>> 3 files changed, 118 insertions(+), 95 deletions(-)\n>>>>\n>>>> diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h\n>>>> index a0d448cf9..847a69066 100644\n>>>> --- a/include/libcamera/internal/ipa_manager.h\n>>>> +++ b/include/libcamera/internal/ipa_manager.h\n>>>> @@ -42,7 +42,13 @@ public:\n>>>> if (!m)\n>>>> return nullptr;\n>>>> \n>>>> - std::unique_ptr proxy = std::make_unique(m, !self->isSignatureValid(m));\n>>>> + auto proxy = [&]() -> std::unique_ptr {\n>>>> + if (self->isSignatureValid(m))\n>>>> + return std::make_unique(m);\n>>>> + else\n>>>> + return std::make_unique(m);\n>>>> + } ();\n> \n> Is this using a lambda just for the sake of it ? :-)\n> \n> \t\tstd::unique_ptrisSignatureValid(m)\n> \t\t\t\t\t? std::make_unique(m)\n> \t\t\t\t\t: std::make_unique(m);\n\nThe lambda is there because the ternary doesn't work.\n\n\n> \n>>>> +\n>>>> if (!proxy->isValid()) {\n>>>> LOG(IPAManager, Error) << \"Failed to load proxy\";\n>>>> return nullptr;\n>>>> diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n>>>> index beb646e2d..8792b3b9e 100644\n>>>> --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n>>>> +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n>>>> @@ -45,36 +45,13 @@ namespace {{ns}} {\n>>>> {% endfor %}\n>>>> {%- endif %}\n>>>> \n>>>> -{{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate)\n>>>> - : IPAProxy(ipam), isolate_(isolate),\n>>>> - controlSerializer_(ControlSerializer::Role::Proxy), seq_(0)\n>>>> +{{proxy_name}}Local::{{proxy_name}}Local(IPAModule *ipam)\n>>>> + : {{proxy_name}}(ipam)\n>>>> {\n>>>> LOG(IPAProxy, Debug)\n>>>> << \"initializing {{module_name}} proxy: loading IPA from \"\n>>>> << ipam->path();\n>>>\n>>> Might it be worth it to also print information about isolation? Same for the\n>>> constructor below.\n>>>\n>>>> \n>>>> - if (isolate_) {\n>>>> - const std::string proxyWorkerPath = resolvePath(\"{{module_name}}_ipa_proxy\");\n>>>> - if (proxyWorkerPath.empty()) {\n>>>> - LOG(IPAProxy, Error)\n>>>> - << \"Failed to get proxy worker path\";\n>>>> - return;\n>>>> - }\n>>>> -\n>>>> - auto ipc = std::make_unique(ipam->path().c_str(),\n>>>> - proxyWorkerPath.c_str());\n>>>> - if (!ipc->isConnected()) {\n>>>> - LOG(IPAProxy, Error) << \"Failed to create IPCPipe\";\n>>>> - return;\n>>>> - }\n>>>> -\n>>>> - ipc->recv.connect(this, &{{proxy_name}}::recvMessage);\n>>>> -\n>>>> - ipc_ = std::move(ipc);\n>>>> - valid_ = true;\n>>>> - return;\n>>>> - }\n>>>> -\n>>>> if (!ipam->load())\n>>>> return;\n>>>> \n>>>> @@ -89,59 +66,16 @@ namespace {{ns}} {\n>>>> proxy_.setIPA(ipa_.get());\n>>>> \n>>>> {% for method in interface_event.methods %}\n>>>> - ipa_->{{method.mojom_name}}.connect(this, &{{proxy_name}}::{{method.mojom_name}}Thread);\n>>>> + ipa_->{{method.mojom_name}}.connect(this, &{{proxy_name}}Local::{{method.mojom_name}}Handler);\n>>>> {%- endfor %}\n>>>> \n>>>> valid_ = true;\n>>>> }\n>>>> \n>>>> -{{proxy_name}}::~{{proxy_name}}()\n>>>> -{\n>>>> - if (ipc_) {\n>>>> - IPCMessage::Header header =\n>>>> - { static_cast({{cmd_enum_name}}::Exit), seq_++ };\n>>>> - IPCMessage msg(header);\n>>>> - ipc_->sendAsync(msg);\n>>>> - }\n>>>> -}\n>>>> -\n>>>> -{% if interface_event.methods|length > 0 %}\n>>>> -void {{proxy_name}}::recvMessage(const IPCMessage &data)\n>>>> -{\n>>>> - size_t dataSize = data.data().size();\n>>>> - {{cmd_event_enum_name}} _cmd = static_cast<{{cmd_event_enum_name}}>(data.header().cmd);\n>>>> -\n>>>> - switch (_cmd) {\n>>>> -{%- for method in interface_event.methods %}\n>>>> - case {{cmd_event_enum_name}}::{{method.mojom_name|cap}}: {\n>>>> - {{method.mojom_name}}IPC(data.data().cbegin(), dataSize, data.fds());\n>>>> - break;\n>>>> - }\n>>>> -{%- endfor %}\n>>>> - default:\n>>>> - LOG(IPAProxy, Error) << \"Unknown command \" << static_cast(_cmd);\n>>>> - }\n>>>> -}\n>>>> -{%- endif %}\n>>>> +{{proxy_name}}Local::~{{proxy_name}}Local() = default;\n>>>> \n>>>> {% for method in interface_main.methods %}\n>>>> -{{proxy_funcs.func_sig(proxy_name, method)}}\n>>>> -{\n>>>> - if (isolate_)\n>>>> - return {{method.mojom_name}}IPC(\n>>>> -{%- for param in method|method_param_names -%}\n>>>> - {{param}}{{- \", \" if not loop.last}}\n>>>> -{%- endfor -%}\n>>>> -);\n>>>> - else\n>>>> - return {{method.mojom_name}}Thread(\n>>>> -{%- for param in method|method_param_names -%}\n>>>> - {{param}}{{- \", \" if not loop.last}}\n>>>> -{%- endfor -%}\n>>>> -);\n>>>> -}\n>>>> -\n>>>> -{{proxy_funcs.func_sig(proxy_name, method, \"Thread\")}}\n>>>> +{{proxy_funcs.func_sig(proxy_name + \"Local\", method)}}\n>>>> {\n>>>> {%- if method.mojom_name == \"stop\" %}\n>>>> {{proxy_funcs.stop_thread_body()}}\n>>>> @@ -181,8 +115,58 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)\n>>>> );\n>>>> {%- endif %}\n>>>> }\n>>>> +{% endfor %}\n>>>> \n>>>> -{{proxy_funcs.func_sig(proxy_name, method, \"IPC\")}}\n>>>> +{% for method in interface_event.methods %}\n>>>> +{{proxy_funcs.func_sig(proxy_name + \"Local\", method, \"Handler\")}}\n>>>> +{\n>>>> + ASSERT(state_ != ProxyStopped);\n>>>> + {{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});\n>>>> +}\n>>>> +{% endfor %}\n>>>> +\n>>>> +/* ========================================================================== */\n>>>> +\n>>>> +{{proxy_name}}Remote::{{proxy_name}}Remote(IPAModule *ipam)\n>>>> + : {{proxy_name}}(ipam),\n>>>> + controlSerializer_(ControlSerializer::Role::Proxy), seq_(0)\n>>>> +{\n>>>> + LOG(IPAProxy, Debug)\n>>>> + << \"initializing {{module_name}} proxy: loading IPA from \"\n>>>> + << ipam->path();\n>>>\n>>> Here ^\n>>\n>> I'll add \"locally\" and \"remotely\" to the messages. Or should it be more concrete?\n>>\n>>>> +\n>>>> + const std::string proxyWorkerPath = resolvePath(\"{{module_name}}_ipa_proxy\");\n>>>> + if (proxyWorkerPath.empty()) {\n>>>> + LOG(IPAProxy, Error)\n>>>> + << \"Failed to get proxy worker path\";\n>>>> + return;\n>>>> + }\n>>>> +\n>>>> + auto ipc = std::make_unique(ipam->path().c_str(),\n>>>> + proxyWorkerPath.c_str());\n>>>> + if (!ipc->isConnected()) {\n>>>> + LOG(IPAProxy, Error) << \"Failed to create IPCPipe\";\n>>>> + return;\n>>>> + }\n>>>> +\n>>>> + ipc->recv.connect(this, &{{proxy_name}}Remote::recvMessage);\n>>>> +\n>>>> + ipc_ = std::move(ipc);\n>>>> + valid_ = true;\n>>>> +}\n>>>> +\n>>>> +{{proxy_name}}Remote::~{{proxy_name}}Remote()\n>>>> +{\n>>>> + if (ipc_) {\n>>>> + IPCMessage::Header header =\n>>>> + { static_cast({{cmd_enum_name}}::Exit), seq_++ };\n>>>> + IPCMessage msg(header);\n>>>> + ipc_->sendAsync(msg);\n>>>> + }\n>>>> +}\n>>>> +\n>>>> +{% for method in interface_main.methods %}\n>>>> +{{proxy_funcs.func_sig(proxy_name + \"Remote\", method)}}\n>>>> {\n>>>> {%- if method.mojom_name == \"configure\" %}\n>>>> controlSerializer_.reset();\n>>>> @@ -226,14 +210,25 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)\n>>>> \n>>>> {% endfor %}\n>>>> \n>>>> -{% for method in interface_event.methods %}\n>>>> -{{proxy_funcs.func_sig(proxy_name, method, \"Thread\")}}\n>>>> +void {{proxy_name}}Remote::recvMessage(const IPCMessage &data)\n>>>> {\n>>>> - ASSERT(state_ != ProxyStopped);\n>>>> - {{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});\n>>>> + size_t dataSize = data.data().size();\n>>>> + {{cmd_event_enum_name}} _cmd = static_cast<{{cmd_event_enum_name}}>(data.header().cmd);\n>>>> +\n>>>> + switch (_cmd) {\n>>>> +{%- for method in interface_event.methods %}\n>>>> + case {{cmd_event_enum_name}}::{{method.mojom_name|cap}}: {\n>>>> + {{method.mojom_name}}Handler(data.data().cbegin(), dataSize, data.fds());\n>>>> + break;\n>>>> + }\n>>>> +{%- endfor %}\n>>>> + default:\n>>>> + LOG(IPAProxy, Error) << \"Unknown command \" << static_cast(_cmd);\n>>>> + }\n>>>> }\n>>>> \n>>>> -void {{proxy_name}}::{{method.mojom_name}}IPC(\n>>>> +{% for method in interface_event.methods %}\n>>>> +void {{proxy_name}}Remote::{{method.mojom_name}}Handler(\n>>>> [[maybe_unused]] std::vector::const_iterator data,\n>>>> [[maybe_unused]] size_t dataSize,\n>>>> [[maybe_unused]] const std::vector &fds)\n>>>> diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n>>>> index a0312a7c1..0c4ef985b 100644\n>>>> --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n>>>> +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n>>>> @@ -34,29 +34,32 @@ namespace {{ns}} {\n>>>> {% endfor %}\n>>>> {%- endif %}\n>>>> \n>>>> +class {{proxy_name}}Local;\n>>>> +class {{proxy_name}}Remote;\n>>>> +\n>>>> class {{proxy_name}} : public IPAProxy, public {{interface_name}}, public Object\n>>>> {\n>>>> public:\n>>>> - {{proxy_name}}(IPAModule *ipam, bool isolate);\n>>>> - ~{{proxy_name}}();\n>>>> + using Local = {{proxy_name}}Local;\n>>>> + using Remote = {{proxy_name}}Remote;\n>>>> \n>>>> -{% for method in interface_main.methods %}\n>>>> -{{proxy_funcs.func_sig(proxy_name, method, \"\", false, true)|indent(8, true)}};\n>>>> -{% endfor %}\n>>>> +protected:\n>>>> + using IPAProxy::IPAProxy;\n>>>> +};\n>>>> \n>>>> -private:\n>>>> - void recvMessage(const IPCMessage &data);\n>>>> +class {{proxy_name}}Local : public {{proxy_name}}\n>>>> +{\n>>>> +public:\n>>>> + {{proxy_name}}Local(IPAModule *ipam);\n>>>> + ~{{proxy_name}}Local();\n>>>> \n>>>> {% for method in interface_main.methods %}\n>>>> -{{proxy_funcs.func_sig(proxy_name, method, \"Thread\", false)|indent(8, true)}};\n>>>> -{{proxy_funcs.func_sig(proxy_name, method, \"IPC\", false)|indent(8, true)}};\n>>>> +{{proxy_funcs.func_sig(proxy_name + \"Local\", method, \"\", false, true)|indent(8, true)}};\n> \n> As this is Python code I *think* you could also write\n> \n> {{proxy_funcs.func_sig(f\"{proxy_name}Local\", method, \"\", false, true)|indent(8, true)}};\n> \n> if you think it's clearer (sae elsewhere in the patch).\n\nI'll check.\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \n>>>> {% endfor %}\n>>>> +\n>>>> +private:\n>>>> {% for method in interface_event.methods %}\n>>>> -{{proxy_funcs.func_sig(proxy_name, method, \"Thread\", false)|indent(8, true)}};\n>>>> - void {{method.mojom_name}}IPC(\n>>>> - std::vector::const_iterator data,\n>>>> - size_t dataSize,\n>>>> - const std::vector &fds);\n>>>> +{{proxy_funcs.func_sig(proxy_name + \"Local\", method, \"Handler\", false)|indent(8, true)}};\n>>>> {% endfor %}\n>>>> \n>>>> /* Helper class to invoke async functions in another thread. */\n>>>> @@ -79,12 +82,12 @@ private:\n>>>> }\n>>>> {% for method in interface_main.methods %}\n>>>> {%- if method|is_async %}\n>>>> - {{proxy_funcs.func_sig(proxy_name, method, \"\", false)|indent(16)}}\n>>>> + {{proxy_funcs.func_sig(proxy_name + \"Local\", method, \"\", false)|indent(16)}}\n>>>> {\n>>>> ipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}});\n>>>> }\n>>>> {%- elif method.mojom_name == \"start\" %}\n>>>> - {{proxy_funcs.func_sig(proxy_name, method, \"\", false)|indent(16)}}\n>>>> + {{proxy_funcs.func_sig(proxy_name + \"Local\", method, \"\", false)|indent(16)}}\n>>>> {\n>>>> {%- if method|method_return_value != \"void\" %}\n>>>> return ipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}});\n>>>> @@ -104,8 +107,27 @@ private:\n>>>> Thread thread_;\n>>>> ThreadProxy proxy_;\n>>>> std::unique_ptr<{{interface_name}}> ipa_;\n>>>> +};\n>>>> \n>>>> - const bool isolate_;\n>>>> +class {{proxy_name}}Remote : public {{proxy_name}}\n>>>> +{\n>>>> +public:\n>>>> + {{proxy_name}}Remote(IPAModule *ipam);\n>>>> + ~{{proxy_name}}Remote();\n>>>> +\n>>>> +{% for method in interface_main.methods %}\n>>>> +{{proxy_funcs.func_sig(proxy_name + \"Local\", method, \"\", false, true)|indent(8, true)}};\n>>>\n>>> s/Local/Remote/ ?\n>>>\n>>> Other than that, looks good to me. This is exciting!\n>>\n>> Oops, fixed. I was wondering why it did not cause any issues... of course\n>> the name of the class is not used due to the fourth argument being false.\n>>\n>>>> +{% endfor %}\n>>>> +\n>>>> +private:\n>>>> + void recvMessage(const IPCMessage &data);\n>>>> +\n>>>> +{% for method in interface_event.methods %}\n>>>> + void {{method.mojom_name}}Handler(\n>>>> + std::vector::const_iterator data,\n>>>> + size_t dataSize,\n>>>> + const std::vector &fds);\n>>>> +{% endfor %}\n>>>> \n>>>> std::unique_ptr ipc_;\n>>>> \n>","headers":{"Return-Path":"","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 855B9BDCC1\n\tfor ;\n\tFri, 15 Aug 2025 07:10:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8130A69251;\n\tFri, 15 Aug 2025 09:10:08 +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 D2E2E69244\n\tfor ;\n\tFri, 15 Aug 2025 09:10:06 +0200 (CEST)","from [192.168.33.21] (185.221.141.188.nat.pool.zt.hu\n\t[185.221.141.188])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 841D663B;\n\tFri, 15 Aug 2025 09:09:11 +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=\"dFV/C6As\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1755241751;\n\tbh=uIJQaHcHgFZd1Z2i5vLtl3cIesPQELwc1rRyZhXbV9I=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=dFV/C6Asufe15eV24ERRAOD/65z6SikOUFPhehUcnhvxSaVNW7A53RLmNc2tKvaWh\n\tK01hmGXF0B5Sl4PTq84Bo2NanwJoTDtef2SFv38dZBJ1sQKNxjj7i2rk+8xs4eYNpn\n\twJwKMcEc+4xsm2pc6e2a+QuKB/UHkT0+V062Plug=","Message-ID":"","Date":"Fri, 15 Aug 2025 09:10:02 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [RFC PATCH v1] utils: codegen: ipc: Split proxy types","To":"Laurent Pinchart ","Cc":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20250804165226.63825-1-barnabas.pocze@ideasonboard.com>\n\t<175516195566.3701817.15703185411370250000@neptunite.rasen.tech>\n\t<0015f255-e725-42d7-aefb-1330b4940bc3@ideasonboard.com>\n\t<20250814222448.GD6201@pendragon.ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20250814222448.GD6201@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":35425,"web_url":"https://patchwork.libcamera.org/comment/35425/","msgid":"<20250815082653.GH6201@pendragon.ideasonboard.com>","date":"2025-08-15T08:26:53","subject":"Re: [RFC PATCH v1] utils: codegen: ipc: Split proxy types","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Aug 15, 2025 at 09:10:02AM +0200, Barnabás Pőcze wrote:\n> 2025. 08. 15. 0:24 keltezéssel, Laurent Pinchart írta:\n> > On Thu, Aug 14, 2025 at 11:42:39AM +0200, Barnabás Pőcze wrote:\n> >> 2025. 08. 14. 10:59 keltezéssel, Paul Elder írta:\n> >>> Quoting Barnabás Pőcze (2025-08-05 01:52:26)\n> >>>> Even though there is an abstract class to represent the interface of an IPA,\n> >>>> the threaded and IPC versions are still multiplexed using the same type,\n> >>>> which uses a boolean to actually dispatch to the right function.\n> >>>>\n> >>>> Instead of doing that, split the classes into a \"local\" and \"remote\" variant,\n> >>>> and make `IPAManager` choose accordingly.\n> > \n> > \"Local\" and \"Remote\" are not the clearest names. Would \"Threaded\" and\n> > \"Isolated\" be better ? I'm sure there are even better option.\n> \n> I'm sure there are endless opportunities. I will rename them.\n> \n> >>> Wow, that is a cool upgrade!\n> >>>\n> >>>> Signed-off-by: Barnabás Pőcze \n> >>>> ---\n> >>>> include/libcamera/internal/ipa_manager.h | 8 +-\n> >>>> .../module_ipa_proxy.cpp.tmpl | 149 +++++++++---------\n> >>>> .../module_ipa_proxy.h.tmpl | 56 +++++--\n> >>>> 3 files changed, 118 insertions(+), 95 deletions(-)\n> >>>>\n> >>>> diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h\n> >>>> index a0d448cf9..847a69066 100644\n> >>>> --- a/include/libcamera/internal/ipa_manager.h\n> >>>> +++ b/include/libcamera/internal/ipa_manager.h\n> >>>> @@ -42,7 +42,13 @@ public:\n> >>>> if (!m)\n> >>>> return nullptr;\n> >>>> \n> >>>> - std::unique_ptr proxy = std::make_unique(m, !self->isSignatureValid(m));\n> >>>> + auto proxy = [&]() -> std::unique_ptr {\n> >>>> + if (self->isSignatureValid(m))\n> >>>> + return std::make_unique(m);\n> >>>> + else\n> >>>> + return std::make_unique(m);\n> >>>> + } ();\n> > \n> > Is this using a lambda just for the sake of it ? :-)\n> > \n> > \t\tstd::unique_ptrisSignatureValid(m)\n> > \t\t\t\t\t? std::make_unique(m)\n> > \t\t\t\t\t: std::make_unique(m);\n> \n> The lambda is there because the ternary doesn't work.\n\nAh yes, they're different types. We can also do\n\n\t\tstd::unique_ptr proxy;\n\n\t \tif (self->isSignatureValid(m))\n\t\t\tproxy = std::make_unique(m);\n\t\telse\n\t\t\tproxy = std::make_unique(m);\n\nI see the appeal in initializing the variable when declaring it though.\n\n> >>>> +\n> >>>> if (!proxy->isValid()) {\n> >>>> LOG(IPAManager, Error) << \"Failed to load proxy\";\n> >>>> return nullptr;\n> >>>> diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> >>>> index beb646e2d..8792b3b9e 100644\n> >>>> --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> >>>> +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> >>>> @@ -45,36 +45,13 @@ namespace {{ns}} {\n> >>>> {% endfor %}\n> >>>> {%- endif %}\n> >>>> \n> >>>> -{{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate)\n> >>>> - : IPAProxy(ipam), isolate_(isolate),\n> >>>> - controlSerializer_(ControlSerializer::Role::Proxy), seq_(0)\n> >>>> +{{proxy_name}}Local::{{proxy_name}}Local(IPAModule *ipam)\n> >>>> + : {{proxy_name}}(ipam)\n> >>>> {\n> >>>> LOG(IPAProxy, Debug)\n> >>>> << \"initializing {{module_name}} proxy: loading IPA from \"\n> >>>> << ipam->path();\n> >>>\n> >>> Might it be worth it to also print information about isolation? Same for the\n> >>> constructor below.\n> >>>\n> >>>> \n> >>>> - if (isolate_) {\n> >>>> - const std::string proxyWorkerPath = resolvePath(\"{{module_name}}_ipa_proxy\");\n> >>>> - if (proxyWorkerPath.empty()) {\n> >>>> - LOG(IPAProxy, Error)\n> >>>> - << \"Failed to get proxy worker path\";\n> >>>> - return;\n> >>>> - }\n> >>>> -\n> >>>> - auto ipc = std::make_unique(ipam->path().c_str(),\n> >>>> - proxyWorkerPath.c_str());\n> >>>> - if (!ipc->isConnected()) {\n> >>>> - LOG(IPAProxy, Error) << \"Failed to create IPCPipe\";\n> >>>> - return;\n> >>>> - }\n> >>>> -\n> >>>> - ipc->recv.connect(this, &{{proxy_name}}::recvMessage);\n> >>>> -\n> >>>> - ipc_ = std::move(ipc);\n> >>>> - valid_ = true;\n> >>>> - return;\n> >>>> - }\n> >>>> -\n> >>>> if (!ipam->load())\n> >>>> return;\n> >>>> \n> >>>> @@ -89,59 +66,16 @@ namespace {{ns}} {\n> >>>> proxy_.setIPA(ipa_.get());\n> >>>> \n> >>>> {% for method in interface_event.methods %}\n> >>>> - ipa_->{{method.mojom_name}}.connect(this, &{{proxy_name}}::{{method.mojom_name}}Thread);\n> >>>> + ipa_->{{method.mojom_name}}.connect(this, &{{proxy_name}}Local::{{method.mojom_name}}Handler);\n> >>>> {%- endfor %}\n> >>>> \n> >>>> valid_ = true;\n> >>>> }\n> >>>> \n> >>>> -{{proxy_name}}::~{{proxy_name}}()\n> >>>> -{\n> >>>> - if (ipc_) {\n> >>>> - IPCMessage::Header header =\n> >>>> - { static_cast({{cmd_enum_name}}::Exit), seq_++ };\n> >>>> - IPCMessage msg(header);\n> >>>> - ipc_->sendAsync(msg);\n> >>>> - }\n> >>>> -}\n> >>>> -\n> >>>> -{% if interface_event.methods|length > 0 %}\n> >>>> -void {{proxy_name}}::recvMessage(const IPCMessage &data)\n> >>>> -{\n> >>>> - size_t dataSize = data.data().size();\n> >>>> - {{cmd_event_enum_name}} _cmd = static_cast<{{cmd_event_enum_name}}>(data.header().cmd);\n> >>>> -\n> >>>> - switch (_cmd) {\n> >>>> -{%- for method in interface_event.methods %}\n> >>>> - case {{cmd_event_enum_name}}::{{method.mojom_name|cap}}: {\n> >>>> - {{method.mojom_name}}IPC(data.data().cbegin(), dataSize, data.fds());\n> >>>> - break;\n> >>>> - }\n> >>>> -{%- endfor %}\n> >>>> - default:\n> >>>> - LOG(IPAProxy, Error) << \"Unknown command \" << static_cast(_cmd);\n> >>>> - }\n> >>>> -}\n> >>>> -{%- endif %}\n> >>>> +{{proxy_name}}Local::~{{proxy_name}}Local() = default;\n> >>>> \n> >>>> {% for method in interface_main.methods %}\n> >>>> -{{proxy_funcs.func_sig(proxy_name, method)}}\n> >>>> -{\n> >>>> - if (isolate_)\n> >>>> - return {{method.mojom_name}}IPC(\n> >>>> -{%- for param in method|method_param_names -%}\n> >>>> - {{param}}{{- \", \" if not loop.last}}\n> >>>> -{%- endfor -%}\n> >>>> -);\n> >>>> - else\n> >>>> - return {{method.mojom_name}}Thread(\n> >>>> -{%- for param in method|method_param_names -%}\n> >>>> - {{param}}{{- \", \" if not loop.last}}\n> >>>> -{%- endfor -%}\n> >>>> -);\n> >>>> -}\n> >>>> -\n> >>>> -{{proxy_funcs.func_sig(proxy_name, method, \"Thread\")}}\n> >>>> +{{proxy_funcs.func_sig(proxy_name + \"Local\", method)}}\n> >>>> {\n> >>>> {%- if method.mojom_name == \"stop\" %}\n> >>>> {{proxy_funcs.stop_thread_body()}}\n> >>>> @@ -181,8 +115,58 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)\n> >>>> );\n> >>>> {%- endif %}\n> >>>> }\n> >>>> +{% endfor %}\n> >>>> \n> >>>> -{{proxy_funcs.func_sig(proxy_name, method, \"IPC\")}}\n> >>>> +{% for method in interface_event.methods %}\n> >>>> +{{proxy_funcs.func_sig(proxy_name + \"Local\", method, \"Handler\")}}\n> >>>> +{\n> >>>> + ASSERT(state_ != ProxyStopped);\n> >>>> + {{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});\n> >>>> +}\n> >>>> +{% endfor %}\n> >>>> +\n> >>>> +/* ========================================================================== */\n> >>>> +\n> >>>> +{{proxy_name}}Remote::{{proxy_name}}Remote(IPAModule *ipam)\n> >>>> + : {{proxy_name}}(ipam),\n> >>>> + controlSerializer_(ControlSerializer::Role::Proxy), seq_(0)\n> >>>> +{\n> >>>> + LOG(IPAProxy, Debug)\n> >>>> + << \"initializing {{module_name}} proxy: loading IPA from \"\n> >>>> + << ipam->path();\n> >>>\n> >>> Here ^\n> >>\n> >> I'll add \"locally\" and \"remotely\" to the messages. Or should it be more concrete?\n> >>\n> >>>> +\n> >>>> + const std::string proxyWorkerPath = resolvePath(\"{{module_name}}_ipa_proxy\");\n> >>>> + if (proxyWorkerPath.empty()) {\n> >>>> + LOG(IPAProxy, Error)\n> >>>> + << \"Failed to get proxy worker path\";\n> >>>> + return;\n> >>>> + }\n> >>>> +\n> >>>> + auto ipc = std::make_unique(ipam->path().c_str(),\n> >>>> + proxyWorkerPath.c_str());\n> >>>> + if (!ipc->isConnected()) {\n> >>>> + LOG(IPAProxy, Error) << \"Failed to create IPCPipe\";\n> >>>> + return;\n> >>>> + }\n> >>>> +\n> >>>> + ipc->recv.connect(this, &{{proxy_name}}Remote::recvMessage);\n> >>>> +\n> >>>> + ipc_ = std::move(ipc);\n> >>>> + valid_ = true;\n> >>>> +}\n> >>>> +\n> >>>> +{{proxy_name}}Remote::~{{proxy_name}}Remote()\n> >>>> +{\n> >>>> + if (ipc_) {\n> >>>> + IPCMessage::Header header =\n> >>>> + { static_cast({{cmd_enum_name}}::Exit), seq_++ };\n> >>>> + IPCMessage msg(header);\n> >>>> + ipc_->sendAsync(msg);\n> >>>> + }\n> >>>> +}\n> >>>> +\n> >>>> +{% for method in interface_main.methods %}\n> >>>> +{{proxy_funcs.func_sig(proxy_name + \"Remote\", method)}}\n> >>>> {\n> >>>> {%- if method.mojom_name == \"configure\" %}\n> >>>> controlSerializer_.reset();\n> >>>> @@ -226,14 +210,25 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)\n> >>>> \n> >>>> {% endfor %}\n> >>>> \n> >>>> -{% for method in interface_event.methods %}\n> >>>> -{{proxy_funcs.func_sig(proxy_name, method, \"Thread\")}}\n> >>>> +void {{proxy_name}}Remote::recvMessage(const IPCMessage &data)\n> >>>> {\n> >>>> - ASSERT(state_ != ProxyStopped);\n> >>>> - {{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});\n> >>>> + size_t dataSize = data.data().size();\n> >>>> + {{cmd_event_enum_name}} _cmd = static_cast<{{cmd_event_enum_name}}>(data.header().cmd);\n> >>>> +\n> >>>> + switch (_cmd) {\n> >>>> +{%- for method in interface_event.methods %}\n> >>>> + case {{cmd_event_enum_name}}::{{method.mojom_name|cap}}: {\n> >>>> + {{method.mojom_name}}Handler(data.data().cbegin(), dataSize, data.fds());\n> >>>> + break;\n> >>>> + }\n> >>>> +{%- endfor %}\n> >>>> + default:\n> >>>> + LOG(IPAProxy, Error) << \"Unknown command \" << static_cast(_cmd);\n> >>>> + }\n> >>>> }\n> >>>> \n> >>>> -void {{proxy_name}}::{{method.mojom_name}}IPC(\n> >>>> +{% for method in interface_event.methods %}\n> >>>> +void {{proxy_name}}Remote::{{method.mojom_name}}Handler(\n> >>>> [[maybe_unused]] std::vector::const_iterator data,\n> >>>> [[maybe_unused]] size_t dataSize,\n> >>>> [[maybe_unused]] const std::vector &fds)\n> >>>> diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> >>>> index a0312a7c1..0c4ef985b 100644\n> >>>> --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> >>>> +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> >>>> @@ -34,29 +34,32 @@ namespace {{ns}} {\n> >>>> {% endfor %}\n> >>>> {%- endif %}\n> >>>> \n> >>>> +class {{proxy_name}}Local;\n> >>>> +class {{proxy_name}}Remote;\n> >>>> +\n> >>>> class {{proxy_name}} : public IPAProxy, public {{interface_name}}, public Object\n> >>>> {\n> >>>> public:\n> >>>> - {{proxy_name}}(IPAModule *ipam, bool isolate);\n> >>>> - ~{{proxy_name}}();\n> >>>> + using Local = {{proxy_name}}Local;\n> >>>> + using Remote = {{proxy_name}}Remote;\n> >>>> \n> >>>> -{% for method in interface_main.methods %}\n> >>>> -{{proxy_funcs.func_sig(proxy_name, method, \"\", false, true)|indent(8, true)}};\n> >>>> -{% endfor %}\n> >>>> +protected:\n> >>>> + using IPAProxy::IPAProxy;\n> >>>> +};\n> >>>> \n> >>>> -private:\n> >>>> - void recvMessage(const IPCMessage &data);\n> >>>> +class {{proxy_name}}Local : public {{proxy_name}}\n> >>>> +{\n> >>>> +public:\n> >>>> + {{proxy_name}}Local(IPAModule *ipam);\n> >>>> + ~{{proxy_name}}Local();\n> >>>> \n> >>>> {% for method in interface_main.methods %}\n> >>>> -{{proxy_funcs.func_sig(proxy_name, method, \"Thread\", false)|indent(8, true)}};\n> >>>> -{{proxy_funcs.func_sig(proxy_name, method, \"IPC\", false)|indent(8, true)}};\n> >>>> +{{proxy_funcs.func_sig(proxy_name + \"Local\", method, \"\", false, true)|indent(8, true)}};\n> > \n> > As this is Python code I *think* you could also write\n> > \n> > {{proxy_funcs.func_sig(f\"{proxy_name}Local\", method, \"\", false, true)|indent(8, true)}};\n> > \n> > if you think it's clearer (sae elsewhere in the patch).\n> \n> I'll check.\n> \n> >>>> {% endfor %}\n> >>>> +\n> >>>> +private:\n> >>>> {% for method in interface_event.methods %}\n> >>>> -{{proxy_funcs.func_sig(proxy_name, method, \"Thread\", false)|indent(8, true)}};\n> >>>> - void {{method.mojom_name}}IPC(\n> >>>> - std::vector::const_iterator data,\n> >>>> - size_t dataSize,\n> >>>> - const std::vector &fds);\n> >>>> +{{proxy_funcs.func_sig(proxy_name + \"Local\", method, \"Handler\", false)|indent(8, true)}};\n> >>>> {% endfor %}\n> >>>> \n> >>>> /* Helper class to invoke async functions in another thread. */\n> >>>> @@ -79,12 +82,12 @@ private:\n> >>>> }\n> >>>> {% for method in interface_main.methods %}\n> >>>> {%- if method|is_async %}\n> >>>> - {{proxy_funcs.func_sig(proxy_name, method, \"\", false)|indent(16)}}\n> >>>> + {{proxy_funcs.func_sig(proxy_name + \"Local\", method, \"\", false)|indent(16)}}\n> >>>> {\n> >>>> ipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}});\n> >>>> }\n> >>>> {%- elif method.mojom_name == \"start\" %}\n> >>>> - {{proxy_funcs.func_sig(proxy_name, method, \"\", false)|indent(16)}}\n> >>>> + {{proxy_funcs.func_sig(proxy_name + \"Local\", method, \"\", false)|indent(16)}}\n> >>>> {\n> >>>> {%- if method|method_return_value != \"void\" %}\n> >>>> return ipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}});\n> >>>> @@ -104,8 +107,27 @@ private:\n> >>>> Thread thread_;\n> >>>> ThreadProxy proxy_;\n> >>>> std::unique_ptr<{{interface_name}}> ipa_;\n> >>>> +};\n> >>>> \n> >>>> - const bool isolate_;\n> >>>> +class {{proxy_name}}Remote : public {{proxy_name}}\n> >>>> +{\n> >>>> +public:\n> >>>> + {{proxy_name}}Remote(IPAModule *ipam);\n> >>>> + ~{{proxy_name}}Remote();\n> >>>> +\n> >>>> +{% for method in interface_main.methods %}\n> >>>> +{{proxy_funcs.func_sig(proxy_name + \"Local\", method, \"\", false, true)|indent(8, true)}};\n> >>>\n> >>> s/Local/Remote/ ?\n> >>>\n> >>> Other than that, looks good to me. This is exciting!\n> >>\n> >> Oops, fixed. I was wondering why it did not cause any issues... of course\n> >> the name of the class is not used due to the fourth argument being false.\n> >>\n> >>>> +{% endfor %}\n> >>>> +\n> >>>> +private:\n> >>>> + void recvMessage(const IPCMessage &data);\n> >>>> +\n> >>>> +{% for method in interface_event.methods %}\n> >>>> + void {{method.mojom_name}}Handler(\n> >>>> + std::vector::const_iterator data,\n> >>>> + size_t dataSize,\n> >>>> + const std::vector &fds);\n> >>>> +{% endfor %}\n> >>>> \n> >>>> std::unique_ptr ipc_;\n> >>>>","headers":{"Return-Path":"","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 6437FBDCC1\n\tfor ;\n\tFri, 15 Aug 2025 08:27:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3E48769257;\n\tFri, 15 Aug 2025 10:27:16 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 94FA469244\n\tfor ;\n\tFri, 15 Aug 2025 10:27:14 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 7B32663B;\n\tFri, 15 Aug 2025 10:26:19 +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=\"GLtmeR/+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1755246379;\n\tbh=WBNHlpaWakjyG/mOzKmD4OdIJmVziUnCsy1LXeMDDxQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=GLtmeR/+H6mjofmHX9M2l/NycWenGyvNJzvQyrymnZ45QxGrh+MCin6+LzID0RSvG\n\tysTcztDDa9LYYXTsTUvwGmdE64fderfV4e+dZXXclHOl46CdOXJFwXLzYUzARQyFcX\n\tFA84ZLWncqy7Qqxlb3zvloG+ClY0fXmUEzC0KKlw=","Date":"Fri, 15 Aug 2025 11:26:53 +0300","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v1] utils: codegen: ipc: Split proxy types","Message-ID":"<20250815082653.GH6201@pendragon.ideasonboard.com>","References":"<20250804165226.63825-1-barnabas.pocze@ideasonboard.com>\n\t<175516195566.3701817.15703185411370250000@neptunite.rasen.tech>\n\t<0015f255-e725-42d7-aefb-1330b4940bc3@ideasonboard.com>\n\t<20250814222448.GD6201@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":35426,"web_url":"https://patchwork.libcamera.org/comment/35426/","msgid":"<8ce2fb5a-2555-425e-91df-101c23e66bad@ideasonboard.com>","date":"2025-08-15T08:40:49","subject":"Re: [RFC PATCH v1] utils: codegen: ipc: Split proxy types","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"2025. 08. 15. 10:26 keltezéssel, Laurent Pinchart írta:\n> On Fri, Aug 15, 2025 at 09:10:02AM +0200, Barnabás Pőcze wrote:\n>> 2025. 08. 15. 0:24 keltezéssel, Laurent Pinchart írta:\n>>> On Thu, Aug 14, 2025 at 11:42:39AM +0200, Barnabás Pőcze wrote:\n>>>> 2025. 08. 14. 10:59 keltezéssel, Paul Elder írta:\n>>>>> Quoting Barnabás Pőcze (2025-08-05 01:52:26)\n>>>>>> Even though there is an abstract class to represent the interface of an IPA,\n>>>>>> the threaded and IPC versions are still multiplexed using the same type,\n>>>>>> which uses a boolean to actually dispatch to the right function.\n>>>>>>\n>>>>>> Instead of doing that, split the classes into a \"local\" and \"remote\" variant,\n>>>>>> and make `IPAManager` choose accordingly.\n>>>\n>>> \"Local\" and \"Remote\" are not the clearest names. Would \"Threaded\" and\n>>> \"Isolated\" be better ? I'm sure there are even better option.\n>>\n>> I'm sure there are endless opportunities. I will rename them.\n>>\n>>>>> Wow, that is a cool upgrade!\n>>>>>\n>>>>>> Signed-off-by: Barnabás Pőcze \n>>>>>> ---\n>>>>>> include/libcamera/internal/ipa_manager.h | 8 +-\n>>>>>> .../module_ipa_proxy.cpp.tmpl | 149 +++++++++---------\n>>>>>> .../module_ipa_proxy.h.tmpl | 56 +++++--\n>>>>>> 3 files changed, 118 insertions(+), 95 deletions(-)\n>>>>>>\n>>>>>> diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h\n>>>>>> index a0d448cf9..847a69066 100644\n>>>>>> --- a/include/libcamera/internal/ipa_manager.h\n>>>>>> +++ b/include/libcamera/internal/ipa_manager.h\n>>>>>> @@ -42,7 +42,13 @@ public:\n>>>>>> if (!m)\n>>>>>> return nullptr;\n>>>>>> \n>>>>>> - std::unique_ptr proxy = std::make_unique(m, !self->isSignatureValid(m));\n>>>>>> + auto proxy = [&]() -> std::unique_ptr {\n>>>>>> + if (self->isSignatureValid(m))\n>>>>>> + return std::make_unique(m);\n>>>>>> + else\n>>>>>> + return std::make_unique(m);\n>>>>>> + } ();\n>>>\n>>> Is this using a lambda just for the sake of it ? :-)\n>>>\n>>> \t\tstd::unique_ptrisSignatureValid(m)\n>>> \t\t\t\t\t? std::make_unique(m)\n>>> \t\t\t\t\t: std::make_unique(m);\n>>\n>> The lambda is there because the ternary doesn't work.\n> \n> Ah yes, they're different types. We can also do\n> \n> \t\tstd::unique_ptr proxy;\n> \n> \t \tif (self->isSignatureValid(m))\n> \t\t\tproxy = std::make_unique(m);\n> \t\telse\n> \t\t\tproxy = std::make_unique(m);\n> \n> I see the appeal in initializing the variable when declaring it though.\n\nYes, I prefer the \"immediately invoked function expression\" approach. In any case\nI don't think it matters much, so which one should it be?\n\n\n> \n>>>>>> +\n>>>>>> if (!proxy->isValid()) {\n>>>>>> LOG(IPAManager, Error) << \"Failed to load proxy\";\n>>>>>> return nullptr;\n>>>>>> diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n>>>>>> index beb646e2d..8792b3b9e 100644\n>>>>>> --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n>>>>>> +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n>>>>>> @@ -45,36 +45,13 @@ namespace {{ns}} {\n>>>>>> {% endfor %}\n>>>>>> {%- endif %}\n>>>>>> \n>>>>>> -{{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate)\n>>>>>> - : IPAProxy(ipam), isolate_(isolate),\n>>>>>> - controlSerializer_(ControlSerializer::Role::Proxy), seq_(0)\n>>>>>> +{{proxy_name}}Local::{{proxy_name}}Local(IPAModule *ipam)\n>>>>>> + : {{proxy_name}}(ipam)\n>>>>>> {\n>>>>>> LOG(IPAProxy, Debug)\n>>>>>> << \"initializing {{module_name}} proxy: loading IPA from \"\n>>>>>> << ipam->path();\n>>>>>\n>>>>> Might it be worth it to also print information about isolation? Same for the\n>>>>> constructor below.\n>>>>>\n>>>>>> \n>>>>>> - if (isolate_) {\n>>>>>> - const std::string proxyWorkerPath = resolvePath(\"{{module_name}}_ipa_proxy\");\n>>>>>> - if (proxyWorkerPath.empty()) {\n>>>>>> - LOG(IPAProxy, Error)\n>>>>>> - << \"Failed to get proxy worker path\";\n>>>>>> - return;\n>>>>>> - }\n>>>>>> -\n>>>>>> - auto ipc = std::make_unique(ipam->path().c_str(),\n>>>>>> - proxyWorkerPath.c_str());\n>>>>>> - if (!ipc->isConnected()) {\n>>>>>> - LOG(IPAProxy, Error) << \"Failed to create IPCPipe\";\n>>>>>> - return;\n>>>>>> - }\n>>>>>> -\n>>>>>> - ipc->recv.connect(this, &{{proxy_name}}::recvMessage);\n>>>>>> -\n>>>>>> - ipc_ = std::move(ipc);\n>>>>>> - valid_ = true;\n>>>>>> - return;\n>>>>>> - }\n>>>>>> -\n>>>>>> if (!ipam->load())\n>>>>>> return;\n>>>>>> \n>>>>>> @@ -89,59 +66,16 @@ namespace {{ns}} {\n>>>>>> proxy_.setIPA(ipa_.get());\n>>>>>> \n>>>>>> {% for method in interface_event.methods %}\n>>>>>> - ipa_->{{method.mojom_name}}.connect(this, &{{proxy_name}}::{{method.mojom_name}}Thread);\n>>>>>> + ipa_->{{method.mojom_name}}.connect(this, &{{proxy_name}}Local::{{method.mojom_name}}Handler);\n>>>>>> {%- endfor %}\n>>>>>> \n>>>>>> valid_ = true;\n>>>>>> }\n>>>>>> \n>>>>>> -{{proxy_name}}::~{{proxy_name}}()\n>>>>>> -{\n>>>>>> - if (ipc_) {\n>>>>>> - IPCMessage::Header header =\n>>>>>> - { static_cast({{cmd_enum_name}}::Exit), seq_++ };\n>>>>>> - IPCMessage msg(header);\n>>>>>> - ipc_->sendAsync(msg);\n>>>>>> - }\n>>>>>> -}\n>>>>>> -\n>>>>>> -{% if interface_event.methods|length > 0 %}\n>>>>>> -void {{proxy_name}}::recvMessage(const IPCMessage &data)\n>>>>>> -{\n>>>>>> - size_t dataSize = data.data().size();\n>>>>>> - {{cmd_event_enum_name}} _cmd = static_cast<{{cmd_event_enum_name}}>(data.header().cmd);\n>>>>>> -\n>>>>>> - switch (_cmd) {\n>>>>>> -{%- for method in interface_event.methods %}\n>>>>>> - case {{cmd_event_enum_name}}::{{method.mojom_name|cap}}: {\n>>>>>> - {{method.mojom_name}}IPC(data.data().cbegin(), dataSize, data.fds());\n>>>>>> - break;\n>>>>>> - }\n>>>>>> -{%- endfor %}\n>>>>>> - default:\n>>>>>> - LOG(IPAProxy, Error) << \"Unknown command \" << static_cast(_cmd);\n>>>>>> - }\n>>>>>> -}\n>>>>>> -{%- endif %}\n>>>>>> +{{proxy_name}}Local::~{{proxy_name}}Local() = default;\n>>>>>> \n>>>>>> {% for method in interface_main.methods %}\n>>>>>> -{{proxy_funcs.func_sig(proxy_name, method)}}\n>>>>>> -{\n>>>>>> - if (isolate_)\n>>>>>> - return {{method.mojom_name}}IPC(\n>>>>>> -{%- for param in method|method_param_names -%}\n>>>>>> - {{param}}{{- \", \" if not loop.last}}\n>>>>>> -{%- endfor -%}\n>>>>>> -);\n>>>>>> - else\n>>>>>> - return {{method.mojom_name}}Thread(\n>>>>>> -{%- for param in method|method_param_names -%}\n>>>>>> - {{param}}{{- \", \" if not loop.last}}\n>>>>>> -{%- endfor -%}\n>>>>>> -);\n>>>>>> -}\n>>>>>> -\n>>>>>> -{{proxy_funcs.func_sig(proxy_name, method, \"Thread\")}}\n>>>>>> +{{proxy_funcs.func_sig(proxy_name + \"Local\", method)}}\n>>>>>> {\n>>>>>> {%- if method.mojom_name == \"stop\" %}\n>>>>>> {{proxy_funcs.stop_thread_body()}}\n>>>>>> @@ -181,8 +115,58 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)\n>>>>>> );\n>>>>>> {%- endif %}\n>>>>>> }\n>>>>>> +{% endfor %}\n>>>>>> \n>>>>>> -{{proxy_funcs.func_sig(proxy_name, method, \"IPC\")}}\n>>>>>> +{% for method in interface_event.methods %}\n>>>>>> +{{proxy_funcs.func_sig(proxy_name + \"Local\", method, \"Handler\")}}\n>>>>>> +{\n>>>>>> + ASSERT(state_ != ProxyStopped);\n>>>>>> + {{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});\n>>>>>> +}\n>>>>>> +{% endfor %}\n>>>>>> +\n>>>>>> +/* ========================================================================== */\n>>>>>> +\n>>>>>> +{{proxy_name}}Remote::{{proxy_name}}Remote(IPAModule *ipam)\n>>>>>> + : {{proxy_name}}(ipam),\n>>>>>> + controlSerializer_(ControlSerializer::Role::Proxy), seq_(0)\n>>>>>> +{\n>>>>>> + LOG(IPAProxy, Debug)\n>>>>>> + << \"initializing {{module_name}} proxy: loading IPA from \"\n>>>>>> + << ipam->path();\n>>>>>\n>>>>> Here ^\n>>>>\n>>>> I'll add \"locally\" and \"remotely\" to the messages. Or should it be more concrete?\n>>>>\n>>>>>> +\n>>>>>> + const std::string proxyWorkerPath = resolvePath(\"{{module_name}}_ipa_proxy\");\n>>>>>> + if (proxyWorkerPath.empty()) {\n>>>>>> + LOG(IPAProxy, Error)\n>>>>>> + << \"Failed to get proxy worker path\";\n>>>>>> + return;\n>>>>>> + }\n>>>>>> +\n>>>>>> + auto ipc = std::make_unique(ipam->path().c_str(),\n>>>>>> + proxyWorkerPath.c_str());\n>>>>>> + if (!ipc->isConnected()) {\n>>>>>> + LOG(IPAProxy, Error) << \"Failed to create IPCPipe\";\n>>>>>> + return;\n>>>>>> + }\n>>>>>> +\n>>>>>> + ipc->recv.connect(this, &{{proxy_name}}Remote::recvMessage);\n>>>>>> +\n>>>>>> + ipc_ = std::move(ipc);\n>>>>>> + valid_ = true;\n>>>>>> +}\n>>>>>> +\n>>>>>> +{{proxy_name}}Remote::~{{proxy_name}}Remote()\n>>>>>> +{\n>>>>>> + if (ipc_) {\n>>>>>> + IPCMessage::Header header =\n>>>>>> + { static_cast({{cmd_enum_name}}::Exit), seq_++ };\n>>>>>> + IPCMessage msg(header);\n>>>>>> + ipc_->sendAsync(msg);\n>>>>>> + }\n>>>>>> +}\n>>>>>> +\n>>>>>> +{% for method in interface_main.methods %}\n>>>>>> +{{proxy_funcs.func_sig(proxy_name + \"Remote\", method)}}\n>>>>>> {\n>>>>>> {%- if method.mojom_name == \"configure\" %}\n>>>>>> controlSerializer_.reset();\n>>>>>> @@ -226,14 +210,25 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)\n>>>>>> \n>>>>>> {% endfor %}\n>>>>>> \n>>>>>> -{% for method in interface_event.methods %}\n>>>>>> -{{proxy_funcs.func_sig(proxy_name, method, \"Thread\")}}\n>>>>>> +void {{proxy_name}}Remote::recvMessage(const IPCMessage &data)\n>>>>>> {\n>>>>>> - ASSERT(state_ != ProxyStopped);\n>>>>>> - {{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});\n>>>>>> + size_t dataSize = data.data().size();\n>>>>>> + {{cmd_event_enum_name}} _cmd = static_cast<{{cmd_event_enum_name}}>(data.header().cmd);\n>>>>>> +\n>>>>>> + switch (_cmd) {\n>>>>>> +{%- for method in interface_event.methods %}\n>>>>>> + case {{cmd_event_enum_name}}::{{method.mojom_name|cap}}: {\n>>>>>> + {{method.mojom_name}}Handler(data.data().cbegin(), dataSize, data.fds());\n>>>>>> + break;\n>>>>>> + }\n>>>>>> +{%- endfor %}\n>>>>>> + default:\n>>>>>> + LOG(IPAProxy, Error) << \"Unknown command \" << static_cast(_cmd);\n>>>>>> + }\n>>>>>> }\n>>>>>> \n>>>>>> -void {{proxy_name}}::{{method.mojom_name}}IPC(\n>>>>>> +{% for method in interface_event.methods %}\n>>>>>> +void {{proxy_name}}Remote::{{method.mojom_name}}Handler(\n>>>>>> [[maybe_unused]] std::vector::const_iterator data,\n>>>>>> [[maybe_unused]] size_t dataSize,\n>>>>>> [[maybe_unused]] const std::vector &fds)\n>>>>>> diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n>>>>>> index a0312a7c1..0c4ef985b 100644\n>>>>>> --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n>>>>>> +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n>>>>>> @@ -34,29 +34,32 @@ namespace {{ns}} {\n>>>>>> {% endfor %}\n>>>>>> {%- endif %}\n>>>>>> \n>>>>>> +class {{proxy_name}}Local;\n>>>>>> +class {{proxy_name}}Remote;\n>>>>>> +\n>>>>>> class {{proxy_name}} : public IPAProxy, public {{interface_name}}, public Object\n>>>>>> {\n>>>>>> public:\n>>>>>> - {{proxy_name}}(IPAModule *ipam, bool isolate);\n>>>>>> - ~{{proxy_name}}();\n>>>>>> + using Local = {{proxy_name}}Local;\n>>>>>> + using Remote = {{proxy_name}}Remote;\n>>>>>> \n>>>>>> -{% for method in interface_main.methods %}\n>>>>>> -{{proxy_funcs.func_sig(proxy_name, method, \"\", false, true)|indent(8, true)}};\n>>>>>> -{% endfor %}\n>>>>>> +protected:\n>>>>>> + using IPAProxy::IPAProxy;\n>>>>>> +};\n>>>>>> \n>>>>>> -private:\n>>>>>> - void recvMessage(const IPCMessage &data);\n>>>>>> +class {{proxy_name}}Local : public {{proxy_name}}\n>>>>>> +{\n>>>>>> +public:\n>>>>>> + {{proxy_name}}Local(IPAModule *ipam);\n>>>>>> + ~{{proxy_name}}Local();\n>>>>>> \n>>>>>> {% for method in interface_main.methods %}\n>>>>>> -{{proxy_funcs.func_sig(proxy_name, method, \"Thread\", false)|indent(8, true)}};\n>>>>>> -{{proxy_funcs.func_sig(proxy_name, method, \"IPC\", false)|indent(8, true)}};\n>>>>>> +{{proxy_funcs.func_sig(proxy_name + \"Local\", method, \"\", false, true)|indent(8, true)}};\n>>>\n>>> As this is Python code I *think* you could also write\n>>>\n>>> {{proxy_funcs.func_sig(f\"{proxy_name}Local\", method, \"\", false, true)|indent(8, true)}};\n>>>\n>>> if you think it's clearer (sae elsewhere in the patch).\n>>\n>> I'll check.\n\nApparently f-strings do not work in jinja, so I kept the \"+\".\n\n\n>>\n>>>>>> {% endfor %}\n>>>>>> +\n>>>>>> +private:\n>>>>>> {% for method in interface_event.methods %}\n>>>>>> -{{proxy_funcs.func_sig(proxy_name, method, \"Thread\", false)|indent(8, true)}};\n>>>>>> - void {{method.mojom_name}}IPC(\n>>>>>> - std::vector::const_iterator data,\n>>>>>> - size_t dataSize,\n>>>>>> - const std::vector &fds);\n>>>>>> +{{proxy_funcs.func_sig(proxy_name + \"Local\", method, \"Handler\", false)|indent(8, true)}};\n>>>>>> {% endfor %}\n>>>>>> \n>>>>>> /* Helper class to invoke async functions in another thread. */\n>>>>>> @@ -79,12 +82,12 @@ private:\n>>>>>> }\n>>>>>> {% for method in interface_main.methods %}\n>>>>>> {%- if method|is_async %}\n>>>>>> - {{proxy_funcs.func_sig(proxy_name, method, \"\", false)|indent(16)}}\n>>>>>> + {{proxy_funcs.func_sig(proxy_name + \"Local\", method, \"\", false)|indent(16)}}\n>>>>>> {\n>>>>>> ipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}});\n>>>>>> }\n>>>>>> {%- elif method.mojom_name == \"start\" %}\n>>>>>> - {{proxy_funcs.func_sig(proxy_name, method, \"\", false)|indent(16)}}\n>>>>>> + {{proxy_funcs.func_sig(proxy_name + \"Local\", method, \"\", false)|indent(16)}}\n>>>>>> {\n>>>>>> {%- if method|method_return_value != \"void\" %}\n>>>>>> return ipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}});\n>>>>>> @@ -104,8 +107,27 @@ private:\n>>>>>> Thread thread_;\n>>>>>> ThreadProxy proxy_;\n>>>>>> std::unique_ptr<{{interface_name}}> ipa_;\n>>>>>> +};\n>>>>>> \n>>>>>> - const bool isolate_;\n>>>>>> +class {{proxy_name}}Remote : public {{proxy_name}}\n>>>>>> +{\n>>>>>> +public:\n>>>>>> + {{proxy_name}}Remote(IPAModule *ipam);\n>>>>>> + ~{{proxy_name}}Remote();\n>>>>>> +\n>>>>>> +{% for method in interface_main.methods %}\n>>>>>> +{{proxy_funcs.func_sig(proxy_name + \"Local\", method, \"\", false, true)|indent(8, true)}};\n>>>>>\n>>>>> s/Local/Remote/ ?\n>>>>>\n>>>>> Other than that, looks good to me. This is exciting!\n>>>>\n>>>> Oops, fixed. I was wondering why it did not cause any issues... of course\n>>>> the name of the class is not used due to the fourth argument being false.\n>>>>\n>>>>>> +{% endfor %}\n>>>>>> +\n>>>>>> +private:\n>>>>>> + void recvMessage(const IPCMessage &data);\n>>>>>> +\n>>>>>> +{% for method in interface_event.methods %}\n>>>>>> + void {{method.mojom_name}}Handler(\n>>>>>> + std::vector::const_iterator data,\n>>>>>> + size_t dataSize,\n>>>>>> + const std::vector &fds);\n>>>>>> +{% endfor %}\n>>>>>> \n>>>>>> std::unique_ptr ipc_;\n>>>>>> \n>","headers":{"Return-Path":"","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 7A8FFBEFBE\n\tfor ;\n\tFri, 15 Aug 2025 08:40:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 394AC6925A;\n\tFri, 15 Aug 2025 10:40:55 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 97F7869251\n\tfor ;\n\tFri, 15 Aug 2025 10:40:53 +0200 (CEST)","from [192.168.33.21] (185.221.141.188.nat.pool.zt.hu\n\t[185.221.141.188])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AB23C63B;\n\tFri, 15 Aug 2025 10:39:58 +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=\"e4/8mBOi\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1755247198;\n\tbh=1vFXV9a5FZC1KeobRY2DL+ynUl88U0VYuhJl3/tX9iQ=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=e4/8mBOiPJGF3gT6OT4iLsETiP0mHlhLV1HuNgTUbWMDSvMJ/o8TvxaR/u/g9cpEF\n\t5NWmCtgigUndMewD7mPm+3r6vqYfMbn8U5w1Jub/i4qT8OFgSP8mz7V0MhnI5JBQ7E\n\tA4ug4Np6lMkStItS/n5RA5MlMHW2QuaTlOUWuVUE=","Message-ID":"<8ce2fb5a-2555-425e-91df-101c23e66bad@ideasonboard.com>","Date":"Fri, 15 Aug 2025 10:40:49 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [RFC PATCH v1] utils: codegen: ipc: Split proxy types","To":"Laurent Pinchart ","Cc":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20250804165226.63825-1-barnabas.pocze@ideasonboard.com>\n\t<175516195566.3701817.15703185411370250000@neptunite.rasen.tech>\n\t<0015f255-e725-42d7-aefb-1330b4940bc3@ideasonboard.com>\n\t<20250814222448.GD6201@pendragon.ideasonboard.com>\n\t\n\t<20250815082653.GH6201@pendragon.ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20250815082653.GH6201@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":35427,"web_url":"https://patchwork.libcamera.org/comment/35427/","msgid":"<20250815084741.GI6201@pendragon.ideasonboard.com>","date":"2025-08-15T08:47:41","subject":"Re: [RFC PATCH v1] utils: codegen: ipc: Split proxy types","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Aug 15, 2025 at 10:40:49AM +0200, Barnabás Pőcze wrote:\n> 2025. 08. 15. 10:26 keltezéssel, Laurent Pinchart írta:\n> > On Fri, Aug 15, 2025 at 09:10:02AM +0200, Barnabás Pőcze wrote:\n> >> 2025. 08. 15. 0:24 keltezéssel, Laurent Pinchart írta:\n> >>> On Thu, Aug 14, 2025 at 11:42:39AM +0200, Barnabás Pőcze wrote:\n> >>>> 2025. 08. 14. 10:59 keltezéssel, Paul Elder írta:\n> >>>>> Quoting Barnabás Pőcze (2025-08-05 01:52:26)\n> >>>>>> Even though there is an abstract class to represent the interface of an IPA,\n> >>>>>> the threaded and IPC versions are still multiplexed using the same type,\n> >>>>>> which uses a boolean to actually dispatch to the right function.\n> >>>>>>\n> >>>>>> Instead of doing that, split the classes into a \"local\" and \"remote\" variant,\n> >>>>>> and make `IPAManager` choose accordingly.\n> >>>\n> >>> \"Local\" and \"Remote\" are not the clearest names. Would \"Threaded\" and\n> >>> \"Isolated\" be better ? I'm sure there are even better option.\n> >>\n> >> I'm sure there are endless opportunities. I will rename them.\n> >>\n> >>>>> Wow, that is a cool upgrade!\n> >>>>>\n> >>>>>> Signed-off-by: Barnabás Pőcze \n> >>>>>> ---\n> >>>>>> include/libcamera/internal/ipa_manager.h | 8 +-\n> >>>>>> .../module_ipa_proxy.cpp.tmpl | 149 +++++++++---------\n> >>>>>> .../module_ipa_proxy.h.tmpl | 56 +++++--\n> >>>>>> 3 files changed, 118 insertions(+), 95 deletions(-)\n> >>>>>>\n> >>>>>> diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h\n> >>>>>> index a0d448cf9..847a69066 100644\n> >>>>>> --- a/include/libcamera/internal/ipa_manager.h\n> >>>>>> +++ b/include/libcamera/internal/ipa_manager.h\n> >>>>>> @@ -42,7 +42,13 @@ public:\n> >>>>>> if (!m)\n> >>>>>> return nullptr;\n> >>>>>> \n> >>>>>> - std::unique_ptr proxy = std::make_unique(m, !self->isSignatureValid(m));\n> >>>>>> + auto proxy = [&]() -> std::unique_ptr {\n> >>>>>> + if (self->isSignatureValid(m))\n> >>>>>> + return std::make_unique(m);\n> >>>>>> + else\n> >>>>>> + return std::make_unique(m);\n> >>>>>> + } ();\n> >>>\n> >>> Is this using a lambda just for the sake of it ? :-)\n> >>>\n> >>> \t\tstd::unique_ptrisSignatureValid(m)\n> >>> \t\t\t\t\t? std::make_unique(m)\n> >>> \t\t\t\t\t: std::make_unique(m);\n> >>\n> >> The lambda is there because the ternary doesn't work.\n> > \n> > Ah yes, they're different types. We can also do\n> > \n> > \t\tstd::unique_ptr proxy;\n> > \n> > \t \tif (self->isSignatureValid(m))\n> > \t\t\tproxy = std::make_unique(m);\n> > \t\telse\n> > \t\t\tproxy = std::make_unique(m);\n> > \n> > I see the appeal in initializing the variable when declaring it though.\n> \n> Yes, I prefer the \"immediately invoked function expression\" approach. In any case\n> I don't think it matters much, so which one should it be?\n\nIt took me some time when reading the patch to notice the \"()\" after the\nlambda. That's why I commented on this. It may be a matter of getting\nused to it though. You can decide.\n\n> >>>>>> +\n> >>>>>> if (!proxy->isValid()) {\n> >>>>>> LOG(IPAManager, Error) << \"Failed to load proxy\";\n> >>>>>> return nullptr;\n> >>>>>> diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> >>>>>> index beb646e2d..8792b3b9e 100644\n> >>>>>> --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> >>>>>> +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> >>>>>> @@ -45,36 +45,13 @@ namespace {{ns}} {\n> >>>>>> {% endfor %}\n> >>>>>> {%- endif %}\n> >>>>>> \n> >>>>>> -{{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate)\n> >>>>>> - : IPAProxy(ipam), isolate_(isolate),\n> >>>>>> - controlSerializer_(ControlSerializer::Role::Proxy), seq_(0)\n> >>>>>> +{{proxy_name}}Local::{{proxy_name}}Local(IPAModule *ipam)\n> >>>>>> + : {{proxy_name}}(ipam)\n> >>>>>> {\n> >>>>>> LOG(IPAProxy, Debug)\n> >>>>>> << \"initializing {{module_name}} proxy: loading IPA from \"\n> >>>>>> << ipam->path();\n> >>>>>\n> >>>>> Might it be worth it to also print information about isolation? Same for the\n> >>>>> constructor below.\n> >>>>>\n> >>>>>> \n> >>>>>> - if (isolate_) {\n> >>>>>> - const std::string proxyWorkerPath = resolvePath(\"{{module_name}}_ipa_proxy\");\n> >>>>>> - if (proxyWorkerPath.empty()) {\n> >>>>>> - LOG(IPAProxy, Error)\n> >>>>>> - << \"Failed to get proxy worker path\";\n> >>>>>> - return;\n> >>>>>> - }\n> >>>>>> -\n> >>>>>> - auto ipc = std::make_unique(ipam->path().c_str(),\n> >>>>>> - proxyWorkerPath.c_str());\n> >>>>>> - if (!ipc->isConnected()) {\n> >>>>>> - LOG(IPAProxy, Error) << \"Failed to create IPCPipe\";\n> >>>>>> - return;\n> >>>>>> - }\n> >>>>>> -\n> >>>>>> - ipc->recv.connect(this, &{{proxy_name}}::recvMessage);\n> >>>>>> -\n> >>>>>> - ipc_ = std::move(ipc);\n> >>>>>> - valid_ = true;\n> >>>>>> - return;\n> >>>>>> - }\n> >>>>>> -\n> >>>>>> if (!ipam->load())\n> >>>>>> return;\n> >>>>>> \n> >>>>>> @@ -89,59 +66,16 @@ namespace {{ns}} {\n> >>>>>> proxy_.setIPA(ipa_.get());\n> >>>>>> \n> >>>>>> {% for method in interface_event.methods %}\n> >>>>>> - ipa_->{{method.mojom_name}}.connect(this, &{{proxy_name}}::{{method.mojom_name}}Thread);\n> >>>>>> + ipa_->{{method.mojom_name}}.connect(this, &{{proxy_name}}Local::{{method.mojom_name}}Handler);\n> >>>>>> {%- endfor %}\n> >>>>>> \n> >>>>>> valid_ = true;\n> >>>>>> }\n> >>>>>> \n> >>>>>> -{{proxy_name}}::~{{proxy_name}}()\n> >>>>>> -{\n> >>>>>> - if (ipc_) {\n> >>>>>> - IPCMessage::Header header =\n> >>>>>> - { static_cast({{cmd_enum_name}}::Exit), seq_++ };\n> >>>>>> - IPCMessage msg(header);\n> >>>>>> - ipc_->sendAsync(msg);\n> >>>>>> - }\n> >>>>>> -}\n> >>>>>> -\n> >>>>>> -{% if interface_event.methods|length > 0 %}\n> >>>>>> -void {{proxy_name}}::recvMessage(const IPCMessage &data)\n> >>>>>> -{\n> >>>>>> - size_t dataSize = data.data().size();\n> >>>>>> - {{cmd_event_enum_name}} _cmd = static_cast<{{cmd_event_enum_name}}>(data.header().cmd);\n> >>>>>> -\n> >>>>>> - switch (_cmd) {\n> >>>>>> -{%- for method in interface_event.methods %}\n> >>>>>> - case {{cmd_event_enum_name}}::{{method.mojom_name|cap}}: {\n> >>>>>> - {{method.mojom_name}}IPC(data.data().cbegin(), dataSize, data.fds());\n> >>>>>> - break;\n> >>>>>> - }\n> >>>>>> -{%- endfor %}\n> >>>>>> - default:\n> >>>>>> - LOG(IPAProxy, Error) << \"Unknown command \" << static_cast(_cmd);\n> >>>>>> - }\n> >>>>>> -}\n> >>>>>> -{%- endif %}\n> >>>>>> +{{proxy_name}}Local::~{{proxy_name}}Local() = default;\n> >>>>>> \n> >>>>>> {% for method in interface_main.methods %}\n> >>>>>> -{{proxy_funcs.func_sig(proxy_name, method)}}\n> >>>>>> -{\n> >>>>>> - if (isolate_)\n> >>>>>> - return {{method.mojom_name}}IPC(\n> >>>>>> -{%- for param in method|method_param_names -%}\n> >>>>>> - {{param}}{{- \", \" if not loop.last}}\n> >>>>>> -{%- endfor -%}\n> >>>>>> -);\n> >>>>>> - else\n> >>>>>> - return {{method.mojom_name}}Thread(\n> >>>>>> -{%- for param in method|method_param_names -%}\n> >>>>>> - {{param}}{{- \", \" if not loop.last}}\n> >>>>>> -{%- endfor -%}\n> >>>>>> -);\n> >>>>>> -}\n> >>>>>> -\n> >>>>>> -{{proxy_funcs.func_sig(proxy_name, method, \"Thread\")}}\n> >>>>>> +{{proxy_funcs.func_sig(proxy_name + \"Local\", method)}}\n> >>>>>> {\n> >>>>>> {%- if method.mojom_name == \"stop\" %}\n> >>>>>> {{proxy_funcs.stop_thread_body()}}\n> >>>>>> @@ -181,8 +115,58 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)\n> >>>>>> );\n> >>>>>> {%- endif %}\n> >>>>>> }\n> >>>>>> +{% endfor %}\n> >>>>>> \n> >>>>>> -{{proxy_funcs.func_sig(proxy_name, method, \"IPC\")}}\n> >>>>>> +{% for method in interface_event.methods %}\n> >>>>>> +{{proxy_funcs.func_sig(proxy_name + \"Local\", method, \"Handler\")}}\n> >>>>>> +{\n> >>>>>> + ASSERT(state_ != ProxyStopped);\n> >>>>>> + {{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});\n> >>>>>> +}\n> >>>>>> +{% endfor %}\n> >>>>>> +\n> >>>>>> +/* ========================================================================== */\n> >>>>>> +\n> >>>>>> +{{proxy_name}}Remote::{{proxy_name}}Remote(IPAModule *ipam)\n> >>>>>> + : {{proxy_name}}(ipam),\n> >>>>>> + controlSerializer_(ControlSerializer::Role::Proxy), seq_(0)\n> >>>>>> +{\n> >>>>>> + LOG(IPAProxy, Debug)\n> >>>>>> + << \"initializing {{module_name}} proxy: loading IPA from \"\n> >>>>>> + << ipam->path();\n> >>>>>\n> >>>>> Here ^\n> >>>>\n> >>>> I'll add \"locally\" and \"remotely\" to the messages. Or should it be more concrete?\n> >>>>\n> >>>>>> +\n> >>>>>> + const std::string proxyWorkerPath = resolvePath(\"{{module_name}}_ipa_proxy\");\n> >>>>>> + if (proxyWorkerPath.empty()) {\n> >>>>>> + LOG(IPAProxy, Error)\n> >>>>>> + << \"Failed to get proxy worker path\";\n> >>>>>> + return;\n> >>>>>> + }\n> >>>>>> +\n> >>>>>> + auto ipc = std::make_unique(ipam->path().c_str(),\n> >>>>>> + proxyWorkerPath.c_str());\n> >>>>>> + if (!ipc->isConnected()) {\n> >>>>>> + LOG(IPAProxy, Error) << \"Failed to create IPCPipe\";\n> >>>>>> + return;\n> >>>>>> + }\n> >>>>>> +\n> >>>>>> + ipc->recv.connect(this, &{{proxy_name}}Remote::recvMessage);\n> >>>>>> +\n> >>>>>> + ipc_ = std::move(ipc);\n> >>>>>> + valid_ = true;\n> >>>>>> +}\n> >>>>>> +\n> >>>>>> +{{proxy_name}}Remote::~{{proxy_name}}Remote()\n> >>>>>> +{\n> >>>>>> + if (ipc_) {\n> >>>>>> + IPCMessage::Header header =\n> >>>>>> + { static_cast({{cmd_enum_name}}::Exit), seq_++ };\n> >>>>>> + IPCMessage msg(header);\n> >>>>>> + ipc_->sendAsync(msg);\n> >>>>>> + }\n> >>>>>> +}\n> >>>>>> +\n> >>>>>> +{% for method in interface_main.methods %}\n> >>>>>> +{{proxy_funcs.func_sig(proxy_name + \"Remote\", method)}}\n> >>>>>> {\n> >>>>>> {%- if method.mojom_name == \"configure\" %}\n> >>>>>> controlSerializer_.reset();\n> >>>>>> @@ -226,14 +210,25 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)\n> >>>>>> \n> >>>>>> {% endfor %}\n> >>>>>> \n> >>>>>> -{% for method in interface_event.methods %}\n> >>>>>> -{{proxy_funcs.func_sig(proxy_name, method, \"Thread\")}}\n> >>>>>> +void {{proxy_name}}Remote::recvMessage(const IPCMessage &data)\n> >>>>>> {\n> >>>>>> - ASSERT(state_ != ProxyStopped);\n> >>>>>> - {{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});\n> >>>>>> + size_t dataSize = data.data().size();\n> >>>>>> + {{cmd_event_enum_name}} _cmd = static_cast<{{cmd_event_enum_name}}>(data.header().cmd);\n> >>>>>> +\n> >>>>>> + switch (_cmd) {\n> >>>>>> +{%- for method in interface_event.methods %}\n> >>>>>> + case {{cmd_event_enum_name}}::{{method.mojom_name|cap}}: {\n> >>>>>> + {{method.mojom_name}}Handler(data.data().cbegin(), dataSize, data.fds());\n> >>>>>> + break;\n> >>>>>> + }\n> >>>>>> +{%- endfor %}\n> >>>>>> + default:\n> >>>>>> + LOG(IPAProxy, Error) << \"Unknown command \" << static_cast(_cmd);\n> >>>>>> + }\n> >>>>>> }\n> >>>>>> \n> >>>>>> -void {{proxy_name}}::{{method.mojom_name}}IPC(\n> >>>>>> +{% for method in interface_event.methods %}\n> >>>>>> +void {{proxy_name}}Remote::{{method.mojom_name}}Handler(\n> >>>>>> [[maybe_unused]] std::vector::const_iterator data,\n> >>>>>> [[maybe_unused]] size_t dataSize,\n> >>>>>> [[maybe_unused]] const std::vector &fds)\n> >>>>>> diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> >>>>>> index a0312a7c1..0c4ef985b 100644\n> >>>>>> --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> >>>>>> +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> >>>>>> @@ -34,29 +34,32 @@ namespace {{ns}} {\n> >>>>>> {% endfor %}\n> >>>>>> {%- endif %}\n> >>>>>> \n> >>>>>> +class {{proxy_name}}Local;\n> >>>>>> +class {{proxy_name}}Remote;\n> >>>>>> +\n> >>>>>> class {{proxy_name}} : public IPAProxy, public {{interface_name}}, public Object\n> >>>>>> {\n> >>>>>> public:\n> >>>>>> - {{proxy_name}}(IPAModule *ipam, bool isolate);\n> >>>>>> - ~{{proxy_name}}();\n> >>>>>> + using Local = {{proxy_name}}Local;\n> >>>>>> + using Remote = {{proxy_name}}Remote;\n> >>>>>> \n> >>>>>> -{% for method in interface_main.methods %}\n> >>>>>> -{{proxy_funcs.func_sig(proxy_name, method, \"\", false, true)|indent(8, true)}};\n> >>>>>> -{% endfor %}\n> >>>>>> +protected:\n> >>>>>> + using IPAProxy::IPAProxy;\n> >>>>>> +};\n> >>>>>> \n> >>>>>> -private:\n> >>>>>> - void recvMessage(const IPCMessage &data);\n> >>>>>> +class {{proxy_name}}Local : public {{proxy_name}}\n> >>>>>> +{\n> >>>>>> +public:\n> >>>>>> + {{proxy_name}}Local(IPAModule *ipam);\n> >>>>>> + ~{{proxy_name}}Local();\n> >>>>>> \n> >>>>>> {% for method in interface_main.methods %}\n> >>>>>> -{{proxy_funcs.func_sig(proxy_name, method, \"Thread\", false)|indent(8, true)}};\n> >>>>>> -{{proxy_funcs.func_sig(proxy_name, method, \"IPC\", false)|indent(8, true)}};\n> >>>>>> +{{proxy_funcs.func_sig(proxy_name + \"Local\", method, \"\", false, true)|indent(8, true)}};\n> >>>\n> >>> As this is Python code I *think* you could also write\n> >>>\n> >>> {{proxy_funcs.func_sig(f\"{proxy_name}Local\", method, \"\", false, true)|indent(8, true)}};\n> >>>\n> >>> if you think it's clearer (sae elsewhere in the patch).\n> >>\n> >> I'll check.\n> \n> Apparently f-strings do not work in jinja, so I kept the \"+\".\n\nGood to know, thanks for trying it out.\n\n> >>>>>> {% endfor %}\n> >>>>>> +\n> >>>>>> +private:\n> >>>>>> {% for method in interface_event.methods %}\n> >>>>>> -{{proxy_funcs.func_sig(proxy_name, method, \"Thread\", false)|indent(8, true)}};\n> >>>>>> - void {{method.mojom_name}}IPC(\n> >>>>>> - std::vector::const_iterator data,\n> >>>>>> - size_t dataSize,\n> >>>>>> - const std::vector &fds);\n> >>>>>> +{{proxy_funcs.func_sig(proxy_name + \"Local\", method, \"Handler\", false)|indent(8, true)}};\n> >>>>>> {% endfor %}\n> >>>>>> \n> >>>>>> /* Helper class to invoke async functions in another thread. */\n> >>>>>> @@ -79,12 +82,12 @@ private:\n> >>>>>> }\n> >>>>>> {% for method in interface_main.methods %}\n> >>>>>> {%- if method|is_async %}\n> >>>>>> - {{proxy_funcs.func_sig(proxy_name, method, \"\", false)|indent(16)}}\n> >>>>>> + {{proxy_funcs.func_sig(proxy_name + \"Local\", method, \"\", false)|indent(16)}}\n> >>>>>> {\n> >>>>>> ipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}});\n> >>>>>> }\n> >>>>>> {%- elif method.mojom_name == \"start\" %}\n> >>>>>> - {{proxy_funcs.func_sig(proxy_name, method, \"\", false)|indent(16)}}\n> >>>>>> + {{proxy_funcs.func_sig(proxy_name + \"Local\", method, \"\", false)|indent(16)}}\n> >>>>>> {\n> >>>>>> {%- if method|method_return_value != \"void\" %}\n> >>>>>> return ipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}});\n> >>>>>> @@ -104,8 +107,27 @@ private:\n> >>>>>> Thread thread_;\n> >>>>>> ThreadProxy proxy_;\n> >>>>>> std::unique_ptr<{{interface_name}}> ipa_;\n> >>>>>> +};\n> >>>>>> \n> >>>>>> - const bool isolate_;\n> >>>>>> +class {{proxy_name}}Remote : public {{proxy_name}}\n> >>>>>> +{\n> >>>>>> +public:\n> >>>>>> + {{proxy_name}}Remote(IPAModule *ipam);\n> >>>>>> + ~{{proxy_name}}Remote();\n> >>>>>> +\n> >>>>>> +{% for method in interface_main.methods %}\n> >>>>>> +{{proxy_funcs.func_sig(proxy_name + \"Local\", method, \"\", false, true)|indent(8, true)}};\n> >>>>>\n> >>>>> s/Local/Remote/ ?\n> >>>>>\n> >>>>> Other than that, looks good to me. This is exciting!\n> >>>>\n> >>>> Oops, fixed. I was wondering why it did not cause any issues... of course\n> >>>> the name of the class is not used due to the fourth argument being false.\n> >>>>\n> >>>>>> +{% endfor %}\n> >>>>>> +\n> >>>>>> +private:\n> >>>>>> + void recvMessage(const IPCMessage &data);\n> >>>>>> +\n> >>>>>> +{% for method in interface_event.methods %}\n> >>>>>> + void {{method.mojom_name}}Handler(\n> >>>>>> + std::vector::const_iterator data,\n> >>>>>> + size_t dataSize,\n> >>>>>> + const std::vector &fds);\n> >>>>>> +{% endfor %}\n> >>>>>> \n> >>>>>> std::unique_ptr ipc_;\n> >>>>>>","headers":{"Return-Path":"","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 A1CD2BDCC1\n\tfor ;\n\tFri, 15 Aug 2025 08:48:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 99AB669257;\n\tFri, 15 Aug 2025 10:48:05 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 331DC69244\n\tfor ;\n\tFri, 15 Aug 2025 10:48:04 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 0F6FA1121; \n\tFri, 15 Aug 2025 10:47:08 +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=\"SCJhnz/J\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1755247629;\n\tbh=I5AGKAwvp33KKHZFMeQ5NtJxvBNmOtueDoNYN7zEsys=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=SCJhnz/JB6/M91lD6Nwc19P9UXLc1ZtTCisLU8UMRSOg3Vl7Njb7GHCI5Mk8o7kvY\n\tD5DZUpVnnrlA9stc8ImtG+dI+S2lAgzQw2laZvD+KO+6b7cQ895UZBoyOooRxkzOcv\n\tR6FWP20fVl6mOwD5GGCHfTqSiEcTiTLQOV/qqonw=","Date":"Fri, 15 Aug 2025 11:47:41 +0300","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v1] utils: codegen: ipc: Split proxy types","Message-ID":"<20250815084741.GI6201@pendragon.ideasonboard.com>","References":"<20250804165226.63825-1-barnabas.pocze@ideasonboard.com>\n\t<175516195566.3701817.15703185411370250000@neptunite.rasen.tech>\n\t<0015f255-e725-42d7-aefb-1330b4940bc3@ideasonboard.com>\n\t<20250814222448.GD6201@pendragon.ideasonboard.com>\n\t\n\t<20250815082653.GH6201@pendragon.ideasonboard.com>\n\t<8ce2fb5a-2555-425e-91df-101c23e66bad@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<8ce2fb5a-2555-425e-91df-101c23e66bad@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":35428,"web_url":"https://patchwork.libcamera.org/comment/35428/","msgid":"","date":"2025-08-15T08:51:21","subject":"Re: [RFC PATCH v1] utils: codegen: ipc: Split proxy types","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"2025. 08. 15. 10:47 keltezéssel, Laurent Pinchart írta:\n> On Fri, Aug 15, 2025 at 10:40:49AM +0200, Barnabás Pőcze wrote:\n>> 2025. 08. 15. 10:26 keltezéssel, Laurent Pinchart írta:\n>>> On Fri, Aug 15, 2025 at 09:10:02AM +0200, Barnabás Pőcze wrote:\n>>>> 2025. 08. 15. 0:24 keltezéssel, Laurent Pinchart írta:\n>>>>> On Thu, Aug 14, 2025 at 11:42:39AM +0200, Barnabás Pőcze wrote:\n>>>>>> 2025. 08. 14. 10:59 keltezéssel, Paul Elder írta:\n>>>>>>> Quoting Barnabás Pőcze (2025-08-05 01:52:26)\n>>>>>>>> Even though there is an abstract class to represent the interface of an IPA,\n>>>>>>>> the threaded and IPC versions are still multiplexed using the same type,\n>>>>>>>> which uses a boolean to actually dispatch to the right function.\n>>>>>>>>\n>>>>>>>> Instead of doing that, split the classes into a \"local\" and \"remote\" variant,\n>>>>>>>> and make `IPAManager` choose accordingly.\n>>>>>\n>>>>> \"Local\" and \"Remote\" are not the clearest names. Would \"Threaded\" and\n>>>>> \"Isolated\" be better ? I'm sure there are even better option.\n>>>>\n>>>> I'm sure there are endless opportunities. I will rename them.\n>>>>\n>>>>>>> Wow, that is a cool upgrade!\n>>>>>>>\n>>>>>>>> Signed-off-by: Barnabás Pőcze \n>>>>>>>> ---\n>>>>>>>> include/libcamera/internal/ipa_manager.h | 8 +-\n>>>>>>>> .../module_ipa_proxy.cpp.tmpl | 149 +++++++++---------\n>>>>>>>> .../module_ipa_proxy.h.tmpl | 56 +++++--\n>>>>>>>> 3 files changed, 118 insertions(+), 95 deletions(-)\n>>>>>>>>\n>>>>>>>> diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h\n>>>>>>>> index a0d448cf9..847a69066 100644\n>>>>>>>> --- a/include/libcamera/internal/ipa_manager.h\n>>>>>>>> +++ b/include/libcamera/internal/ipa_manager.h\n>>>>>>>> @@ -42,7 +42,13 @@ public:\n>>>>>>>> if (!m)\n>>>>>>>> return nullptr;\n>>>>>>>> \n>>>>>>>> - std::unique_ptr proxy = std::make_unique(m, !self->isSignatureValid(m));\n>>>>>>>> + auto proxy = [&]() -> std::unique_ptr {\n>>>>>>>> + if (self->isSignatureValid(m))\n>>>>>>>> + return std::make_unique(m);\n>>>>>>>> + else\n>>>>>>>> + return std::make_unique(m);\n>>>>>>>> + } ();\n>>>>>\n>>>>> Is this using a lambda just for the sake of it ? :-)\n>>>>>\n>>>>> \t\tstd::unique_ptrisSignatureValid(m)\n>>>>> \t\t\t\t\t? std::make_unique(m)\n>>>>> \t\t\t\t\t: std::make_unique(m);\n>>>>\n>>>> The lambda is there because the ternary doesn't work.\n>>>\n>>> Ah yes, they're different types. We can also do\n>>>\n>>> \t\tstd::unique_ptr proxy;\n>>>\n>>> \t \tif (self->isSignatureValid(m))\n>>> \t\t\tproxy = std::make_unique(m);\n>>> \t\telse\n>>> \t\t\tproxy = std::make_unique(m);\n>>>\n>>> I see the appeal in initializing the variable when declaring it though.\n>>\n>> Yes, I prefer the \"immediately invoked function expression\" approach. In any case\n>> I don't think it matters much, so which one should it be?\n> \n> It took me some time when reading the patch to notice the \"()\" after the\n> lambda. That's why I commented on this. It may be a matter of getting\n> used to it though. You can decide.\n\nOkay, then I'll keep the current version and we'll see what others think.\n\n\n> \n>>>>>>>> +\n>>>>>>>> if (!proxy->isValid()) {\n>>>>>>>> LOG(IPAManager, Error) << \"Failed to load proxy\";\n>>>>>>>> return nullptr;\n>>>>>>>> diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n>>>>>>>> index beb646e2d..8792b3b9e 100644\n>>>>>>>> --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n>>>>>>>> +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n>>>>>>>> @@ -45,36 +45,13 @@ namespace {{ns}} {\n>>>>>>>> {% endfor %}\n>>>>>>>> {%- endif %}\n>>>>>>>> \n>>>>>>>> -{{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate)\n>>>>>>>> - : IPAProxy(ipam), isolate_(isolate),\n>>>>>>>> - controlSerializer_(ControlSerializer::Role::Proxy), seq_(0)\n>>>>>>>> +{{proxy_name}}Local::{{proxy_name}}Local(IPAModule *ipam)\n>>>>>>>> + : {{proxy_name}}(ipam)\n>>>>>>>> {\n>>>>>>>> LOG(IPAProxy, Debug)\n>>>>>>>> << \"initializing {{module_name}} proxy: loading IPA from \"\n>>>>>>>> << ipam->path();\n>>>>>>>\n>>>>>>> Might it be worth it to also print information about isolation? Same for the\n>>>>>>> constructor below.\n>>>>>>>\n>>>>>>>> \n>>>>>>>> - if (isolate_) {\n>>>>>>>> - const std::string proxyWorkerPath = resolvePath(\"{{module_name}}_ipa_proxy\");\n>>>>>>>> - if (proxyWorkerPath.empty()) {\n>>>>>>>> - LOG(IPAProxy, Error)\n>>>>>>>> - << \"Failed to get proxy worker path\";\n>>>>>>>> - return;\n>>>>>>>> - }\n>>>>>>>> -\n>>>>>>>> - auto ipc = std::make_unique(ipam->path().c_str(),\n>>>>>>>> - proxyWorkerPath.c_str());\n>>>>>>>> - if (!ipc->isConnected()) {\n>>>>>>>> - LOG(IPAProxy, Error) << \"Failed to create IPCPipe\";\n>>>>>>>> - return;\n>>>>>>>> - }\n>>>>>>>> -\n>>>>>>>> - ipc->recv.connect(this, &{{proxy_name}}::recvMessage);\n>>>>>>>> -\n>>>>>>>> - ipc_ = std::move(ipc);\n>>>>>>>> - valid_ = true;\n>>>>>>>> - return;\n>>>>>>>> - }\n>>>>>>>> -\n>>>>>>>> if (!ipam->load())\n>>>>>>>> return;\n>>>>>>>> \n>>>>>>>> @@ -89,59 +66,16 @@ namespace {{ns}} {\n>>>>>>>> proxy_.setIPA(ipa_.get());\n>>>>>>>> \n>>>>>>>> {% for method in interface_event.methods %}\n>>>>>>>> - ipa_->{{method.mojom_name}}.connect(this, &{{proxy_name}}::{{method.mojom_name}}Thread);\n>>>>>>>> + ipa_->{{method.mojom_name}}.connect(this, &{{proxy_name}}Local::{{method.mojom_name}}Handler);\n>>>>>>>> {%- endfor %}\n>>>>>>>> \n>>>>>>>> valid_ = true;\n>>>>>>>> }\n>>>>>>>> \n>>>>>>>> -{{proxy_name}}::~{{proxy_name}}()\n>>>>>>>> -{\n>>>>>>>> - if (ipc_) {\n>>>>>>>> - IPCMessage::Header header =\n>>>>>>>> - { static_cast({{cmd_enum_name}}::Exit), seq_++ };\n>>>>>>>> - IPCMessage msg(header);\n>>>>>>>> - ipc_->sendAsync(msg);\n>>>>>>>> - }\n>>>>>>>> -}\n>>>>>>>> -\n>>>>>>>> -{% if interface_event.methods|length > 0 %}\n>>>>>>>> -void {{proxy_name}}::recvMessage(const IPCMessage &data)\n>>>>>>>> -{\n>>>>>>>> - size_t dataSize = data.data().size();\n>>>>>>>> - {{cmd_event_enum_name}} _cmd = static_cast<{{cmd_event_enum_name}}>(data.header().cmd);\n>>>>>>>> -\n>>>>>>>> - switch (_cmd) {\n>>>>>>>> -{%- for method in interface_event.methods %}\n>>>>>>>> - case {{cmd_event_enum_name}}::{{method.mojom_name|cap}}: {\n>>>>>>>> - {{method.mojom_name}}IPC(data.data().cbegin(), dataSize, data.fds());\n>>>>>>>> - break;\n>>>>>>>> - }\n>>>>>>>> -{%- endfor %}\n>>>>>>>> - default:\n>>>>>>>> - LOG(IPAProxy, Error) << \"Unknown command \" << static_cast(_cmd);\n>>>>>>>> - }\n>>>>>>>> -}\n>>>>>>>> -{%- endif %}\n>>>>>>>> +{{proxy_name}}Local::~{{proxy_name}}Local() = default;\n>>>>>>>> \n>>>>>>>> {% for method in interface_main.methods %}\n>>>>>>>> -{{proxy_funcs.func_sig(proxy_name, method)}}\n>>>>>>>> -{\n>>>>>>>> - if (isolate_)\n>>>>>>>> - return {{method.mojom_name}}IPC(\n>>>>>>>> -{%- for param in method|method_param_names -%}\n>>>>>>>> - {{param}}{{- \", \" if not loop.last}}\n>>>>>>>> -{%- endfor -%}\n>>>>>>>> -);\n>>>>>>>> - else\n>>>>>>>> - return {{method.mojom_name}}Thread(\n>>>>>>>> -{%- for param in method|method_param_names -%}\n>>>>>>>> - {{param}}{{- \", \" if not loop.last}}\n>>>>>>>> -{%- endfor -%}\n>>>>>>>> -);\n>>>>>>>> -}\n>>>>>>>> -\n>>>>>>>> -{{proxy_funcs.func_sig(proxy_name, method, \"Thread\")}}\n>>>>>>>> +{{proxy_funcs.func_sig(proxy_name + \"Local\", method)}}\n>>>>>>>> {\n>>>>>>>> {%- if method.mojom_name == \"stop\" %}\n>>>>>>>> {{proxy_funcs.stop_thread_body()}}\n>>>>>>>> @@ -181,8 +115,58 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)\n>>>>>>>> );\n>>>>>>>> {%- endif %}\n>>>>>>>> }\n>>>>>>>> +{% endfor %}\n>>>>>>>> \n>>>>>>>> -{{proxy_funcs.func_sig(proxy_name, method, \"IPC\")}}\n>>>>>>>> +{% for method in interface_event.methods %}\n>>>>>>>> +{{proxy_funcs.func_sig(proxy_name + \"Local\", method, \"Handler\")}}\n>>>>>>>> +{\n>>>>>>>> + ASSERT(state_ != ProxyStopped);\n>>>>>>>> + {{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});\n>>>>>>>> +}\n>>>>>>>> +{% endfor %}\n>>>>>>>> +\n>>>>>>>> +/* ========================================================================== */\n>>>>>>>> +\n>>>>>>>> +{{proxy_name}}Remote::{{proxy_name}}Remote(IPAModule *ipam)\n>>>>>>>> + : {{proxy_name}}(ipam),\n>>>>>>>> + controlSerializer_(ControlSerializer::Role::Proxy), seq_(0)\n>>>>>>>> +{\n>>>>>>>> + LOG(IPAProxy, Debug)\n>>>>>>>> + << \"initializing {{module_name}} proxy: loading IPA from \"\n>>>>>>>> + << ipam->path();\n>>>>>>>\n>>>>>>> Here ^\n>>>>>>\n>>>>>> I'll add \"locally\" and \"remotely\" to the messages. Or should it be more concrete?\n>>>>>>\n>>>>>>>> +\n>>>>>>>> + const std::string proxyWorkerPath = resolvePath(\"{{module_name}}_ipa_proxy\");\n>>>>>>>> + if (proxyWorkerPath.empty()) {\n>>>>>>>> + LOG(IPAProxy, Error)\n>>>>>>>> + << \"Failed to get proxy worker path\";\n>>>>>>>> + return;\n>>>>>>>> + }\n>>>>>>>> +\n>>>>>>>> + auto ipc = std::make_unique(ipam->path().c_str(),\n>>>>>>>> + proxyWorkerPath.c_str());\n>>>>>>>> + if (!ipc->isConnected()) {\n>>>>>>>> + LOG(IPAProxy, Error) << \"Failed to create IPCPipe\";\n>>>>>>>> + return;\n>>>>>>>> + }\n>>>>>>>> +\n>>>>>>>> + ipc->recv.connect(this, &{{proxy_name}}Remote::recvMessage);\n>>>>>>>> +\n>>>>>>>> + ipc_ = std::move(ipc);\n>>>>>>>> + valid_ = true;\n>>>>>>>> +}\n>>>>>>>> +\n>>>>>>>> +{{proxy_name}}Remote::~{{proxy_name}}Remote()\n>>>>>>>> +{\n>>>>>>>> + if (ipc_) {\n>>>>>>>> + IPCMessage::Header header =\n>>>>>>>> + { static_cast({{cmd_enum_name}}::Exit), seq_++ };\n>>>>>>>> + IPCMessage msg(header);\n>>>>>>>> + ipc_->sendAsync(msg);\n>>>>>>>> + }\n>>>>>>>> +}\n>>>>>>>> +\n>>>>>>>> +{% for method in interface_main.methods %}\n>>>>>>>> +{{proxy_funcs.func_sig(proxy_name + \"Remote\", method)}}\n>>>>>>>> {\n>>>>>>>> {%- if method.mojom_name == \"configure\" %}\n>>>>>>>> controlSerializer_.reset();\n>>>>>>>> @@ -226,14 +210,25 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)\n>>>>>>>> \n>>>>>>>> {% endfor %}\n>>>>>>>> \n>>>>>>>> -{% for method in interface_event.methods %}\n>>>>>>>> -{{proxy_funcs.func_sig(proxy_name, method, \"Thread\")}}\n>>>>>>>> +void {{proxy_name}}Remote::recvMessage(const IPCMessage &data)\n>>>>>>>> {\n>>>>>>>> - ASSERT(state_ != ProxyStopped);\n>>>>>>>> - {{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});\n>>>>>>>> + size_t dataSize = data.data().size();\n>>>>>>>> + {{cmd_event_enum_name}} _cmd = static_cast<{{cmd_event_enum_name}}>(data.header().cmd);\n>>>>>>>> +\n>>>>>>>> + switch (_cmd) {\n>>>>>>>> +{%- for method in interface_event.methods %}\n>>>>>>>> + case {{cmd_event_enum_name}}::{{method.mojom_name|cap}}: {\n>>>>>>>> + {{method.mojom_name}}Handler(data.data().cbegin(), dataSize, data.fds());\n>>>>>>>> + break;\n>>>>>>>> + }\n>>>>>>>> +{%- endfor %}\n>>>>>>>> + default:\n>>>>>>>> + LOG(IPAProxy, Error) << \"Unknown command \" << static_cast(_cmd);\n>>>>>>>> + }\n>>>>>>>> }\n>>>>>>>> \n>>>>>>>> -void {{proxy_name}}::{{method.mojom_name}}IPC(\n>>>>>>>> +{% for method in interface_event.methods %}\n>>>>>>>> +void {{proxy_name}}Remote::{{method.mojom_name}}Handler(\n>>>>>>>> [[maybe_unused]] std::vector::const_iterator data,\n>>>>>>>> [[maybe_unused]] size_t dataSize,\n>>>>>>>> [[maybe_unused]] const std::vector &fds)\n>>>>>>>> diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n>>>>>>>> index a0312a7c1..0c4ef985b 100644\n>>>>>>>> --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n>>>>>>>> +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n>>>>>>>> @@ -34,29 +34,32 @@ namespace {{ns}} {\n>>>>>>>> {% endfor %}\n>>>>>>>> {%- endif %}\n>>>>>>>> \n>>>>>>>> +class {{proxy_name}}Local;\n>>>>>>>> +class {{proxy_name}}Remote;\n>>>>>>>> +\n>>>>>>>> class {{proxy_name}} : public IPAProxy, public {{interface_name}}, public Object\n>>>>>>>> {\n>>>>>>>> public:\n>>>>>>>> - {{proxy_name}}(IPAModule *ipam, bool isolate);\n>>>>>>>> - ~{{proxy_name}}();\n>>>>>>>> + using Local = {{proxy_name}}Local;\n>>>>>>>> + using Remote = {{proxy_name}}Remote;\n>>>>>>>> \n>>>>>>>> -{% for method in interface_main.methods %}\n>>>>>>>> -{{proxy_funcs.func_sig(proxy_name, method, \"\", false, true)|indent(8, true)}};\n>>>>>>>> -{% endfor %}\n>>>>>>>> +protected:\n>>>>>>>> + using IPAProxy::IPAProxy;\n>>>>>>>> +};\n>>>>>>>> \n>>>>>>>> -private:\n>>>>>>>> - void recvMessage(const IPCMessage &data);\n>>>>>>>> +class {{proxy_name}}Local : public {{proxy_name}}\n>>>>>>>> +{\n>>>>>>>> +public:\n>>>>>>>> + {{proxy_name}}Local(IPAModule *ipam);\n>>>>>>>> + ~{{proxy_name}}Local();\n>>>>>>>> \n>>>>>>>> {% for method in interface_main.methods %}\n>>>>>>>> -{{proxy_funcs.func_sig(proxy_name, method, \"Thread\", false)|indent(8, true)}};\n>>>>>>>> -{{proxy_funcs.func_sig(proxy_name, method, \"IPC\", false)|indent(8, true)}};\n>>>>>>>> +{{proxy_funcs.func_sig(proxy_name + \"Local\", method, \"\", false, true)|indent(8, true)}};\n>>>>>\n>>>>> As this is Python code I *think* you could also write\n>>>>>\n>>>>> {{proxy_funcs.func_sig(f\"{proxy_name}Local\", method, \"\", false, true)|indent(8, true)}};\n>>>>>\n>>>>> if you think it's clearer (sae elsewhere in the patch).\n>>>>\n>>>> I'll check.\n>>\n>> Apparently f-strings do not work in jinja, so I kept the \"+\".\n> \n> Good to know, thanks for trying it out.\n> \n>>>>>>>> {% endfor %}\n>>>>>>>> +\n>>>>>>>> +private:\n>>>>>>>> {% for method in interface_event.methods %}\n>>>>>>>> -{{proxy_funcs.func_sig(proxy_name, method, \"Thread\", false)|indent(8, true)}};\n>>>>>>>> - void {{method.mojom_name}}IPC(\n>>>>>>>> - std::vector::const_iterator data,\n>>>>>>>> - size_t dataSize,\n>>>>>>>> - const std::vector &fds);\n>>>>>>>> +{{proxy_funcs.func_sig(proxy_name + \"Local\", method, \"Handler\", false)|indent(8, true)}};\n>>>>>>>> {% endfor %}\n>>>>>>>> \n>>>>>>>> /* Helper class to invoke async functions in another thread. */\n>>>>>>>> @@ -79,12 +82,12 @@ private:\n>>>>>>>> }\n>>>>>>>> {% for method in interface_main.methods %}\n>>>>>>>> {%- if method|is_async %}\n>>>>>>>> - {{proxy_funcs.func_sig(proxy_name, method, \"\", false)|indent(16)}}\n>>>>>>>> + {{proxy_funcs.func_sig(proxy_name + \"Local\", method, \"\", false)|indent(16)}}\n>>>>>>>> {\n>>>>>>>> ipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}});\n>>>>>>>> }\n>>>>>>>> {%- elif method.mojom_name == \"start\" %}\n>>>>>>>> - {{proxy_funcs.func_sig(proxy_name, method, \"\", false)|indent(16)}}\n>>>>>>>> + {{proxy_funcs.func_sig(proxy_name + \"Local\", method, \"\", false)|indent(16)}}\n>>>>>>>> {\n>>>>>>>> {%- if method|method_return_value != \"void\" %}\n>>>>>>>> return ipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}});\n>>>>>>>> @@ -104,8 +107,27 @@ private:\n>>>>>>>> Thread thread_;\n>>>>>>>> ThreadProxy proxy_;\n>>>>>>>> std::unique_ptr<{{interface_name}}> ipa_;\n>>>>>>>> +};\n>>>>>>>> \n>>>>>>>> - const bool isolate_;\n>>>>>>>> +class {{proxy_name}}Remote : public {{proxy_name}}\n>>>>>>>> +{\n>>>>>>>> +public:\n>>>>>>>> + {{proxy_name}}Remote(IPAModule *ipam);\n>>>>>>>> + ~{{proxy_name}}Remote();\n>>>>>>>> +\n>>>>>>>> +{% for method in interface_main.methods %}\n>>>>>>>> +{{proxy_funcs.func_sig(proxy_name + \"Local\", method, \"\", false, true)|indent(8, true)}};\n>>>>>>>\n>>>>>>> s/Local/Remote/ ?\n>>>>>>>\n>>>>>>> Other than that, looks good to me. This is exciting!\n>>>>>>\n>>>>>> Oops, fixed. I was wondering why it did not cause any issues... of course\n>>>>>> the name of the class is not used due to the fourth argument being false.\n>>>>>>\n>>>>>>>> +{% endfor %}\n>>>>>>>> +\n>>>>>>>> +private:\n>>>>>>>> + void recvMessage(const IPCMessage &data);\n>>>>>>>> +\n>>>>>>>> +{% for method in interface_event.methods %}\n>>>>>>>> + void {{method.mojom_name}}Handler(\n>>>>>>>> + std::vector::const_iterator data,\n>>>>>>>> + size_t dataSize,\n>>>>>>>> + const std::vector &fds);\n>>>>>>>> +{% endfor %}\n>>>>>>>> \n>>>>>>>> std::unique_ptr ipc_;\n>>>>>>>> \n>","headers":{"Return-Path":"","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 B3D7ABDCC1\n\tfor ;\n\tFri, 15 Aug 2025 08:51:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5CB2069257;\n\tFri, 15 Aug 2025 10:51:27 +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 ACE1C69244\n\tfor ;\n\tFri, 15 Aug 2025 10:51:25 +0200 (CEST)","from [192.168.33.21] (185.221.141.188.nat.pool.zt.hu\n\t[185.221.141.188])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DCD7863B;\n\tFri, 15 Aug 2025 10:50:29 +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=\"R9r2lri5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1755247830;\n\tbh=UFRLqyRtrIWnTtfgUyvIOc46dsFwBuihlqIFn7nguWc=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=R9r2lri5eLPoZ02etSxRGR641pZV1pI6V30y81SoYVPTv5mcqNGd2HuZPD3ItG1cC\n\tfNP0xAV/hlwnMdSra6j9mKiQCn6bnKrpQ7cDCTzPICimeNv/lpwByYOM7tgxQD3l7w\n\tLF8JIVAmXLJJP/iczgObZQvlz+VV4rcifsGF7v34=","Message-ID":"","Date":"Fri, 15 Aug 2025 10:51:21 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [RFC PATCH v1] utils: codegen: ipc: Split proxy types","To":"Laurent Pinchart ","Cc":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20250804165226.63825-1-barnabas.pocze@ideasonboard.com>\n\t<175516195566.3701817.15703185411370250000@neptunite.rasen.tech>\n\t<0015f255-e725-42d7-aefb-1330b4940bc3@ideasonboard.com>\n\t<20250814222448.GD6201@pendragon.ideasonboard.com>\n\t\n\t<20250815082653.GH6201@pendragon.ideasonboard.com>\n\t<8ce2fb5a-2555-425e-91df-101c23e66bad@ideasonboard.com>\n\t<20250815084741.GI6201@pendragon.ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20250815084741.GI6201@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]