[RFC,v3] utils: codegen: ipc: Split proxy types
diff mbox series

Message ID 20250922100339.1455688-1-barnabas.pocze@ideasonboard.com
State Accepted
Headers show
Series
  • [RFC,v3] utils: codegen: ipc: Split proxy types
Related show

Commit Message

Barnabás Pőcze Sept. 22, 2025, 10:03 a.m. UTC 
Even though there is an abstract class to represent the interface of an IPA,
the threaded and IPC versions are still multiplexed using the same type,
which uses a boolean to actually dispatch to the right function.

Instead of doing that, split the classes into "threaded" and "isolated"
variants, and make `IPAManager` choose accordingly.

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
---
changes in v3:
  * rebase

changes in v2:
  * fix typo
  * rename local -> threaded, remote -> isolated
  * have different log messages in the two variants

v2: https://patchwork.libcamera.org/patch/24108/
v1: https://patchwork.libcamera.org/patch/24056/
---
 include/libcamera/internal/ipa_manager.h      |   9 +-
 .../module_ipa_proxy.cpp.tmpl                 | 151 +++++++++---------
 .../module_ipa_proxy.h.tmpl                   |  56 +++++--
 3 files changed, 120 insertions(+), 96 deletions(-)

--
2.51.0

Comments

Laurent Pinchart Sept. 29, 2025, 7:19 a.m. UTC | #1 
Hi Barnabás,

On Mon, Sep 22, 2025 at 12:03:39PM +0200, Barnabás Pőcze wrote:
> Even though there is an abstract class to represent the interface of an IPA,
> the threaded and IPC versions are still multiplexed using the same type,
> which uses a boolean to actually dispatch to the right function.
> 
> Instead of doing that, split the classes into "threaded" and "isolated"
> variants, and make `IPAManager` choose accordingly.
> 
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
> changes in v3:
>   * rebase
> 
> changes in v2:
>   * fix typo
>   * rename local -> threaded, remote -> isolated
>   * have different log messages in the two variants
> 
> v2: https://patchwork.libcamera.org/patch/24108/
> v1: https://patchwork.libcamera.org/patch/24056/
> ---
>  include/libcamera/internal/ipa_manager.h      |   9 +-
>  .../module_ipa_proxy.cpp.tmpl                 | 151 +++++++++---------
>  .../module_ipa_proxy.h.tmpl                   |  56 +++++--
>  3 files changed, 120 insertions(+), 96 deletions(-)
> 
> diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h
> index b0b44c74a..f8ce78016 100644
> --- a/include/libcamera/internal/ipa_manager.h
> +++ b/include/libcamera/internal/ipa_manager.h
> @@ -44,7 +44,14 @@ public:
>  			return nullptr;
> 
>  		const GlobalConfiguration &configuration = cm->_d()->configuration();
> -		std::unique_ptr<T> proxy = std::make_unique<T>(m, !self->isSignatureValid(m), configuration);
> +
> +		auto proxy = [&]() -> std::unique_ptr<T> {
> +			if (self->isSignatureValid(m))
> +				return std::make_unique<typename T::Threaded>(m, configuration);
> +			else
> +				return std::make_unique<typename T::Isolated>(m, configuration);
> +		}();
> +
>  		if (!proxy->isValid()) {
>  			LOG(IPAManager, Error) << "Failed to load proxy";
>  			return nullptr;
> 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
> index 18b4ab5e5..9bf2c58d3 100644
> --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> @@ -45,36 +45,13 @@ namespace {{ns}} {
>  {% endfor %}
>  {%- endif %}
> 
> -{{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate, const GlobalConfiguration &configuration)
> -	: IPAProxy(ipam, configuration), isolate_(isolate),
> -	  controlSerializer_(ControlSerializer::Role::Proxy), seq_(0)
> +{{proxy_name}}Threaded::{{proxy_name}}Threaded(IPAModule *ipam, const GlobalConfiguration &configuration)
> +	: {{proxy_name}}(ipam, configuration)
>  {
>  	LOG(IPAProxy, Debug)
> -		<< "initializing {{module_name}} proxy: loading IPA from "
> +		<< "initializing {{module_name}} proxy in thread: loading IPA from "
>  		<< ipam->path();
> 
> -	if (isolate_) {
> -		const std::string proxyWorkerPath = resolvePath("{{module_name}}_ipa_proxy");
> -		if (proxyWorkerPath.empty()) {
> -			LOG(IPAProxy, Error)
> -				<< "Failed to get proxy worker path";
> -			return;
> -		}
> -
> -		auto ipc = std::make_unique<IPCPipeUnixSocket>(ipam->path().c_str(),
> -							       proxyWorkerPath.c_str());
> -		if (!ipc->isConnected()) {
> -			LOG(IPAProxy, Error) << "Failed to create IPCPipe";
> -			return;
> -		}
> -
> -		ipc->recv.connect(this, &{{proxy_name}}::recvMessage);
> -
> -		ipc_ = std::move(ipc);
> -		valid_ = true;
> -		return;
> -	}
> -
>  	if (!ipam->load())
>  		return;
> 
> @@ -89,59 +66,16 @@ namespace {{ns}} {
>  	proxy_.setIPA(ipa_.get());
> 
>  {% for method in interface_event.methods %}
> -	ipa_->{{method.mojom_name}}.connect(this, &{{proxy_name}}::{{method.mojom_name}}Thread);
> +	ipa_->{{method.mojom_name}}.connect(this, &{{proxy_name}}Threaded::{{method.mojom_name}}Handler);
>  {%- endfor %}
> 
>  	valid_ = true;
>  }
> 
> -{{proxy_name}}::~{{proxy_name}}()
> -{
> -	if (ipc_) {
> -		IPCMessage::Header header =
> -			{ static_cast<uint32_t>({{cmd_enum_name}}::Exit), seq_++ };
> -		IPCMessage msg(header);
> -		ipc_->sendAsync(msg);
> -	}
> -}
> -
> -{% if interface_event.methods|length > 0 %}
> -void {{proxy_name}}::recvMessage(const IPCMessage &data)
> -{
> -	size_t dataSize = data.data().size();
> -	{{cmd_event_enum_name}} _cmd = static_cast<{{cmd_event_enum_name}}>(data.header().cmd);
> -
> -	switch (_cmd) {
> -{%- for method in interface_event.methods %}
> -	case {{cmd_event_enum_name}}::{{method.mojom_name|cap}}: {
> -		{{method.mojom_name}}IPC(data.data().cbegin(), dataSize, data.fds());
> -		break;
> -	}
> -{%- endfor %}
> -	default:
> -		LOG(IPAProxy, Error) << "Unknown command " << static_cast<uint32_t>(_cmd);
> -	}
> -}
> -{%- endif %}
> +{{proxy_name}}Threaded::~{{proxy_name}}Threaded() = default;

Can't you omit the destructor from the class definition and drop this
line, to let the compiler generate the default destructor ?

> 
>  {% for method in interface_main.methods %}
> -{{proxy_funcs.func_sig(proxy_name, method)}}
> -{
> -	if (isolate_)
> -		return {{method.mojom_name}}IPC(
> -{%- for param in method|method_param_names -%}
> -		{{param}}{{- ", " if not loop.last}}
> -{%- endfor -%}
> -);
> -	else
> -		return {{method.mojom_name}}Thread(
> -{%- for param in method|method_param_names -%}
> -		{{param}}{{- ", " if not loop.last}}
> -{%- endfor -%}
> -);
> -}
> -
> -{{proxy_funcs.func_sig(proxy_name, method, "Thread")}}
> +{{proxy_funcs.func_sig(proxy_name + "Threaded", method)}}
>  {
>  {%- if method.mojom_name == "stop" %}
>  	{{proxy_funcs.stop_thread_body()}}
> @@ -181,8 +115,58 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)
>  );
>  {%- endif %}
>  }
> +{% endfor %}
> +
> +{% for method in interface_event.methods %}
> +{{proxy_funcs.func_sig(proxy_name + "Threaded", method, "Handler")}}
> +{
> +	ASSERT(state_ != ProxyStopped);
> +	{{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});
> +}
> +{% endfor %}
> +
> +/* ========================================================================== */
> +
> +{{proxy_name}}Isolated::{{proxy_name}}Isolated(IPAModule *ipam, const GlobalConfiguration &configuration)
> +	: {{proxy_name}}(ipam, configuration),
> +	  controlSerializer_(ControlSerializer::Role::Proxy), seq_(0)
> +{
> +	LOG(IPAProxy, Debug)
> +		<< "initializing {{module_name}} proxy in isolation: loading IPA from "
> +		<< ipam->path();
> +
> +	const std::string proxyWorkerPath = resolvePath("{{module_name}}_ipa_proxy");
> +	if (proxyWorkerPath.empty()) {
> +		LOG(IPAProxy, Error)
> +			<< "Failed to get proxy worker path";

This now holds on a single line.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +		return;
> +	}
> +
> +	auto ipc = std::make_unique<IPCPipeUnixSocket>(ipam->path().c_str(),
> +						       proxyWorkerPath.c_str());
> +	if (!ipc->isConnected()) {
> +		LOG(IPAProxy, Error) << "Failed to create IPCPipe";
> +		return;
> +	}
> +
> +	ipc->recv.connect(this, &{{proxy_name}}Isolated::recvMessage);
> +
> +	ipc_ = std::move(ipc);
> +	valid_ = true;
> +}
> 
> -{{proxy_funcs.func_sig(proxy_name, method, "IPC")}}
> +{{proxy_name}}Isolated::~{{proxy_name}}Isolated()
> +{
> +	if (ipc_) {
> +		IPCMessage::Header header =
> +			{ static_cast<uint32_t>({{cmd_enum_name}}::Exit), seq_++ };
> +		IPCMessage msg(header);
> +		ipc_->sendAsync(msg);
> +	}
> +}
> +
> +{% for method in interface_main.methods %}
> +{{proxy_funcs.func_sig(proxy_name + "Isolated", method)}}
>  {
>  {%- if method.mojom_name == "configure" %}
>  	controlSerializer_.reset();
> @@ -226,14 +210,25 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)
> 
>  {% endfor %}
> 
> -{% for method in interface_event.methods %}
> -{{proxy_funcs.func_sig(proxy_name, method, "Thread")}}
> +void {{proxy_name}}Isolated::recvMessage(const IPCMessage &data)
>  {
> -	ASSERT(state_ != ProxyStopped);
> -	{{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});
> +	size_t dataSize = data.data().size();
> +	{{cmd_event_enum_name}} _cmd = static_cast<{{cmd_event_enum_name}}>(data.header().cmd);
> +
> +	switch (_cmd) {
> +{%- for method in interface_event.methods %}
> +	case {{cmd_event_enum_name}}::{{method.mojom_name|cap}}: {
> +		{{method.mojom_name}}Handler(data.data().cbegin(), dataSize, data.fds());
> +		break;
> +	}
> +{%- endfor %}
> +	default:
> +		LOG(IPAProxy, Error) << "Unknown command " << static_cast<uint32_t>(_cmd);
> +	}
>  }
> 
> -void {{proxy_name}}::{{method.mojom_name}}IPC(
> +{% for method in interface_event.methods %}
> +void {{proxy_name}}Isolated::{{method.mojom_name}}Handler(
>  	[[maybe_unused]] std::vector<uint8_t>::const_iterator data,
>  	[[maybe_unused]] size_t dataSize,
>  	[[maybe_unused]] const std::vector<SharedFD> &fds)
> 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
> index 057c3ab03..ef280ca42 100644
> --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
> +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
> @@ -34,29 +34,32 @@ namespace {{ns}} {
>  {% endfor %}
>  {%- endif %}
> 
> +class {{proxy_name}}Threaded;
> +class {{proxy_name}}Isolated;
> +
>  class {{proxy_name}} : public IPAProxy, public {{interface_name}}, public Object
>  {
>  public:
> -	{{proxy_name}}(IPAModule *ipam, bool isolate, const GlobalConfiguration &configuration);
> -	~{{proxy_name}}();
> +	using Threaded = {{proxy_name}}Threaded;
> +	using Isolated = {{proxy_name}}Isolated;
> 
> -{% for method in interface_main.methods %}
> -{{proxy_funcs.func_sig(proxy_name, method, "", false, true)|indent(8, true)}};
> -{% endfor %}
> +protected:
> +	using IPAProxy::IPAProxy;
> +};
> 
> -private:
> -	void recvMessage(const IPCMessage &data);
> +class {{proxy_name}}Threaded : public {{proxy_name}}
> +{
> +public:
> +	{{proxy_name}}Threaded(IPAModule *ipam, const GlobalConfiguration &configuration);
> +	~{{proxy_name}}Threaded();
> 
>  {% for method in interface_main.methods %}
> -{{proxy_funcs.func_sig(proxy_name, method, "Thread", false)|indent(8, true)}};
> -{{proxy_funcs.func_sig(proxy_name, method, "IPC", false)|indent(8, true)}};
> +{{proxy_funcs.func_sig(proxy_name + "Threaded", method, "", false, true)|indent(8, true)}};
>  {% endfor %}
> +
> +private:
>  {% for method in interface_event.methods %}
> -{{proxy_funcs.func_sig(proxy_name, method, "Thread", false)|indent(8, true)}};
> -	void {{method.mojom_name}}IPC(
> -		std::vector<uint8_t>::const_iterator data,
> -		size_t dataSize,
> -		const std::vector<SharedFD> &fds);
> +{{proxy_funcs.func_sig(proxy_name + "Threaded", method, "Handler", false)|indent(8, true)}};
>  {% endfor %}
> 
>  	/* Helper class to invoke async functions in another thread. */
> @@ -79,12 +82,12 @@ private:
>  		}
>  {% for method in interface_main.methods %}
>  {%- if method|is_async %}
> -		{{proxy_funcs.func_sig(proxy_name, method, "", false)|indent(16)}}
> +		{{proxy_funcs.func_sig(proxy_name + "Threaded", method, "", false)|indent(16)}}
>  		{
>  			ipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}});
>  		}
>  {%- elif method.mojom_name == "start" %}
> -		{{proxy_funcs.func_sig(proxy_name, method, "", false)|indent(16)}}
> +		{{proxy_funcs.func_sig(proxy_name + "Threaded", method, "", false)|indent(16)}}
>  		{
>  {%- if method|method_return_value != "void" %}
>  			return ipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}});
> @@ -104,8 +107,27 @@ private:
>  	Thread thread_;
>  	ThreadProxy proxy_;
>  	std::unique_ptr<{{interface_name}}> ipa_;
> +};
> 
> -	const bool isolate_;
> +class {{proxy_name}}Isolated : public {{proxy_name}}
> +{
> +public:
> +	{{proxy_name}}Isolated(IPAModule *ipam, const GlobalConfiguration &configuration);
> +	~{{proxy_name}}Isolated();
> +
> +{% for method in interface_main.methods %}
> +{{proxy_funcs.func_sig(proxy_name + "Isolated", method, "", false, true)|indent(8, true)}};
> +{% endfor %}
> +
> +private:
> +	void recvMessage(const IPCMessage &data);
> +
> +{% for method in interface_event.methods %}
> +	void {{method.mojom_name}}Handler(
> +		std::vector<uint8_t>::const_iterator data,
> +		size_t dataSize,
> +		const std::vector<SharedFD> &fds);
> +{% endfor %}
> 
>  	std::unique_ptr<IPCPipeUnixSocket> ipc_;
>
Barnabás Pőcze Sept. 29, 2025, 7:54 a.m. UTC | #2 
Hi


2025. 09. 29. 9:19 keltezéssel, Laurent Pinchart írta:
> Hi Barnabás,
> 
> On Mon, Sep 22, 2025 at 12:03:39PM +0200, Barnabás Pőcze wrote:
>> Even though there is an abstract class to represent the interface of an IPA,
>> the threaded and IPC versions are still multiplexed using the same type,
>> which uses a boolean to actually dispatch to the right function.
>>
>> Instead of doing that, split the classes into "threaded" and "isolated"
>> variants, and make `IPAManager` choose accordingly.
>>
>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
>> ---
>> changes in v3:
>>    * rebase
>>
>> changes in v2:
>>    * fix typo
>>    * rename local -> threaded, remote -> isolated
>>    * have different log messages in the two variants
>>
>> v2: https://patchwork.libcamera.org/patch/24108/
>> v1: https://patchwork.libcamera.org/patch/24056/
>> ---
>>   include/libcamera/internal/ipa_manager.h      |   9 +-
>>   .../module_ipa_proxy.cpp.tmpl                 | 151 +++++++++---------
>>   .../module_ipa_proxy.h.tmpl                   |  56 +++++--
>>   3 files changed, 120 insertions(+), 96 deletions(-)
>>
>> diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h
>> index b0b44c74a..f8ce78016 100644
>> --- a/include/libcamera/internal/ipa_manager.h
>> +++ b/include/libcamera/internal/ipa_manager.h
>> @@ -44,7 +44,14 @@ public:
>>   			return nullptr;
>>
>>   		const GlobalConfiguration &configuration = cm->_d()->configuration();
>> -		std::unique_ptr<T> proxy = std::make_unique<T>(m, !self->isSignatureValid(m), configuration);
>> +
>> +		auto proxy = [&]() -> std::unique_ptr<T> {
>> +			if (self->isSignatureValid(m))
>> +				return std::make_unique<typename T::Threaded>(m, configuration);
>> +			else
>> +				return std::make_unique<typename T::Isolated>(m, configuration);
>> +		}();
>> +
>>   		if (!proxy->isValid()) {
>>   			LOG(IPAManager, Error) << "Failed to load proxy";
>>   			return nullptr;
>> 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
>> index 18b4ab5e5..9bf2c58d3 100644
>> --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
>> +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
>> @@ -45,36 +45,13 @@ namespace {{ns}} {
>>   {% endfor %}
>>   {%- endif %}
>>
>> -{{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate, const GlobalConfiguration &configuration)
>> -	: IPAProxy(ipam, configuration), isolate_(isolate),
>> -	  controlSerializer_(ControlSerializer::Role::Proxy), seq_(0)
>> +{{proxy_name}}Threaded::{{proxy_name}}Threaded(IPAModule *ipam, const GlobalConfiguration &configuration)
>> +	: {{proxy_name}}(ipam, configuration)
>>   {
>>   	LOG(IPAProxy, Debug)
>> -		<< "initializing {{module_name}} proxy: loading IPA from "
>> +		<< "initializing {{module_name}} proxy in thread: loading IPA from "
>>   		<< ipam->path();
>>
>> -	if (isolate_) {
>> -		const std::string proxyWorkerPath = resolvePath("{{module_name}}_ipa_proxy");
>> -		if (proxyWorkerPath.empty()) {
>> -			LOG(IPAProxy, Error)
>> -				<< "Failed to get proxy worker path";
>> -			return;
>> -		}
>> -
>> -		auto ipc = std::make_unique<IPCPipeUnixSocket>(ipam->path().c_str(),
>> -							       proxyWorkerPath.c_str());
>> -		if (!ipc->isConnected()) {
>> -			LOG(IPAProxy, Error) << "Failed to create IPCPipe";
>> -			return;
>> -		}
>> -
>> -		ipc->recv.connect(this, &{{proxy_name}}::recvMessage);
>> -
>> -		ipc_ = std::move(ipc);
>> -		valid_ = true;
>> -		return;
>> -	}
>> -
>>   	if (!ipam->load())
>>   		return;
>>
>> @@ -89,59 +66,16 @@ namespace {{ns}} {
>>   	proxy_.setIPA(ipa_.get());
>>
>>   {% for method in interface_event.methods %}
>> -	ipa_->{{method.mojom_name}}.connect(this, &{{proxy_name}}::{{method.mojom_name}}Thread);
>> +	ipa_->{{method.mojom_name}}.connect(this, &{{proxy_name}}Threaded::{{method.mojom_name}}Handler);
>>   {%- endfor %}
>>
>>   	valid_ = true;
>>   }
>>
>> -{{proxy_name}}::~{{proxy_name}}()
>> -{
>> -	if (ipc_) {
>> -		IPCMessage::Header header =
>> -			{ static_cast<uint32_t>({{cmd_enum_name}}::Exit), seq_++ };
>> -		IPCMessage msg(header);
>> -		ipc_->sendAsync(msg);
>> -	}
>> -}
>> -
>> -{% if interface_event.methods|length > 0 %}
>> -void {{proxy_name}}::recvMessage(const IPCMessage &data)
>> -{
>> -	size_t dataSize = data.data().size();
>> -	{{cmd_event_enum_name}} _cmd = static_cast<{{cmd_event_enum_name}}>(data.header().cmd);
>> -
>> -	switch (_cmd) {
>> -{%- for method in interface_event.methods %}
>> -	case {{cmd_event_enum_name}}::{{method.mojom_name|cap}}: {
>> -		{{method.mojom_name}}IPC(data.data().cbegin(), dataSize, data.fds());
>> -		break;
>> -	}
>> -{%- endfor %}
>> -	default:
>> -		LOG(IPAProxy, Error) << "Unknown command " << static_cast<uint32_t>(_cmd);
>> -	}
>> -}
>> -{%- endif %}
>> +{{proxy_name}}Threaded::~{{proxy_name}}Threaded() = default;
> 
> Can't you omit the destructor from the class definition and drop this
> line, to let the compiler generate the default destructor ?

Yes, it could be omitted. The only difference is that it would be an inline
destructor, generated in the translation unit(s) where it is used. Should I
change it?


> 
>>
>>   {% for method in interface_main.methods %}
>> -{{proxy_funcs.func_sig(proxy_name, method)}}
>> -{
>> -	if (isolate_)
>> -		return {{method.mojom_name}}IPC(
>> -{%- for param in method|method_param_names -%}
>> -		{{param}}{{- ", " if not loop.last}}
>> -{%- endfor -%}
>> -);
>> -	else
>> -		return {{method.mojom_name}}Thread(
>> -{%- for param in method|method_param_names -%}
>> -		{{param}}{{- ", " if not loop.last}}
>> -{%- endfor -%}
>> -);
>> -}
>> -
>> -{{proxy_funcs.func_sig(proxy_name, method, "Thread")}}
>> +{{proxy_funcs.func_sig(proxy_name + "Threaded", method)}}
>>   {
>>   {%- if method.mojom_name == "stop" %}
>>   	{{proxy_funcs.stop_thread_body()}}
>> @@ -181,8 +115,58 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)
>>   );
>>   {%- endif %}
>>   }
>> +{% endfor %}
>> +
>> +{% for method in interface_event.methods %}
>> +{{proxy_funcs.func_sig(proxy_name + "Threaded", method, "Handler")}}
>> +{
>> +	ASSERT(state_ != ProxyStopped);
>> +	{{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});
>> +}
>> +{% endfor %}
>> +
>> +/* ========================================================================== */
>> +
>> +{{proxy_name}}Isolated::{{proxy_name}}Isolated(IPAModule *ipam, const GlobalConfiguration &configuration)
>> +	: {{proxy_name}}(ipam, configuration),
>> +	  controlSerializer_(ControlSerializer::Role::Proxy), seq_(0)
>> +{
>> +	LOG(IPAProxy, Debug)
>> +		<< "initializing {{module_name}} proxy in isolation: loading IPA from "
>> +		<< ipam->path();
>> +
>> +	const std::string proxyWorkerPath = resolvePath("{{module_name}}_ipa_proxy");
>> +	if (proxyWorkerPath.empty()) {
>> +		LOG(IPAProxy, Error)
>> +			<< "Failed to get proxy worker path";
> 
> This now holds on a single line.

Fixed.


Regards,
Barnabás Pőcze


> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
>> +		return;
>> +	}
>> +
>> +	auto ipc = std::make_unique<IPCPipeUnixSocket>(ipam->path().c_str(),
>> +						       proxyWorkerPath.c_str());
>> +	if (!ipc->isConnected()) {
>> +		LOG(IPAProxy, Error) << "Failed to create IPCPipe";
>> +		return;
>> +	}
>> +
>> +	ipc->recv.connect(this, &{{proxy_name}}Isolated::recvMessage);
>> +
>> +	ipc_ = std::move(ipc);
>> +	valid_ = true;
>> +}
>>
>> -{{proxy_funcs.func_sig(proxy_name, method, "IPC")}}
>> +{{proxy_name}}Isolated::~{{proxy_name}}Isolated()
>> +{
>> +	if (ipc_) {
>> +		IPCMessage::Header header =
>> +			{ static_cast<uint32_t>({{cmd_enum_name}}::Exit), seq_++ };
>> +		IPCMessage msg(header);
>> +		ipc_->sendAsync(msg);
>> +	}
>> +}
>> +
>> +{% for method in interface_main.methods %}
>> +{{proxy_funcs.func_sig(proxy_name + "Isolated", method)}}
>>   {
>>   {%- if method.mojom_name == "configure" %}
>>   	controlSerializer_.reset();
>> @@ -226,14 +210,25 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)
>>
>>   {% endfor %}
>>
>> -{% for method in interface_event.methods %}
>> -{{proxy_funcs.func_sig(proxy_name, method, "Thread")}}
>> +void {{proxy_name}}Isolated::recvMessage(const IPCMessage &data)
>>   {
>> -	ASSERT(state_ != ProxyStopped);
>> -	{{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});
>> +	size_t dataSize = data.data().size();
>> +	{{cmd_event_enum_name}} _cmd = static_cast<{{cmd_event_enum_name}}>(data.header().cmd);
>> +
>> +	switch (_cmd) {
>> +{%- for method in interface_event.methods %}
>> +	case {{cmd_event_enum_name}}::{{method.mojom_name|cap}}: {
>> +		{{method.mojom_name}}Handler(data.data().cbegin(), dataSize, data.fds());
>> +		break;
>> +	}
>> +{%- endfor %}
>> +	default:
>> +		LOG(IPAProxy, Error) << "Unknown command " << static_cast<uint32_t>(_cmd);
>> +	}
>>   }
>>
>> -void {{proxy_name}}::{{method.mojom_name}}IPC(
>> +{% for method in interface_event.methods %}
>> +void {{proxy_name}}Isolated::{{method.mojom_name}}Handler(
>>   	[[maybe_unused]] std::vector<uint8_t>::const_iterator data,
>>   	[[maybe_unused]] size_t dataSize,
>>   	[[maybe_unused]] const std::vector<SharedFD> &fds)
>> 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
>> index 057c3ab03..ef280ca42 100644
>> --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
>> +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
>> @@ -34,29 +34,32 @@ namespace {{ns}} {
>>   {% endfor %}
>>   {%- endif %}
>>
>> +class {{proxy_name}}Threaded;
>> +class {{proxy_name}}Isolated;
>> +
>>   class {{proxy_name}} : public IPAProxy, public {{interface_name}}, public Object
>>   {
>>   public:
>> -	{{proxy_name}}(IPAModule *ipam, bool isolate, const GlobalConfiguration &configuration);
>> -	~{{proxy_name}}();
>> +	using Threaded = {{proxy_name}}Threaded;
>> +	using Isolated = {{proxy_name}}Isolated;
>>
>> -{% for method in interface_main.methods %}
>> -{{proxy_funcs.func_sig(proxy_name, method, "", false, true)|indent(8, true)}};
>> -{% endfor %}
>> +protected:
>> +	using IPAProxy::IPAProxy;
>> +};
>>
>> -private:
>> -	void recvMessage(const IPCMessage &data);
>> +class {{proxy_name}}Threaded : public {{proxy_name}}
>> +{
>> +public:
>> +	{{proxy_name}}Threaded(IPAModule *ipam, const GlobalConfiguration &configuration);
>> +	~{{proxy_name}}Threaded();
>>
>>   {% for method in interface_main.methods %}
>> -{{proxy_funcs.func_sig(proxy_name, method, "Thread", false)|indent(8, true)}};
>> -{{proxy_funcs.func_sig(proxy_name, method, "IPC", false)|indent(8, true)}};
>> +{{proxy_funcs.func_sig(proxy_name + "Threaded", method, "", false, true)|indent(8, true)}};
>>   {% endfor %}
>> +
>> +private:
>>   {% for method in interface_event.methods %}
>> -{{proxy_funcs.func_sig(proxy_name, method, "Thread", false)|indent(8, true)}};
>> -	void {{method.mojom_name}}IPC(
>> -		std::vector<uint8_t>::const_iterator data,
>> -		size_t dataSize,
>> -		const std::vector<SharedFD> &fds);
>> +{{proxy_funcs.func_sig(proxy_name + "Threaded", method, "Handler", false)|indent(8, true)}};
>>   {% endfor %}
>>
>>   	/* Helper class to invoke async functions in another thread. */
>> @@ -79,12 +82,12 @@ private:
>>   		}
>>   {% for method in interface_main.methods %}
>>   {%- if method|is_async %}
>> -		{{proxy_funcs.func_sig(proxy_name, method, "", false)|indent(16)}}
>> +		{{proxy_funcs.func_sig(proxy_name + "Threaded", method, "", false)|indent(16)}}
>>   		{
>>   			ipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}});
>>   		}
>>   {%- elif method.mojom_name == "start" %}
>> -		{{proxy_funcs.func_sig(proxy_name, method, "", false)|indent(16)}}
>> +		{{proxy_funcs.func_sig(proxy_name + "Threaded", method, "", false)|indent(16)}}
>>   		{
>>   {%- if method|method_return_value != "void" %}
>>   			return ipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}});
>> @@ -104,8 +107,27 @@ private:
>>   	Thread thread_;
>>   	ThreadProxy proxy_;
>>   	std::unique_ptr<{{interface_name}}> ipa_;
>> +};
>>
>> -	const bool isolate_;
>> +class {{proxy_name}}Isolated : public {{proxy_name}}
>> +{
>> +public:
>> +	{{proxy_name}}Isolated(IPAModule *ipam, const GlobalConfiguration &configuration);
>> +	~{{proxy_name}}Isolated();
>> +
>> +{% for method in interface_main.methods %}
>> +{{proxy_funcs.func_sig(proxy_name + "Isolated", method, "", false, true)|indent(8, true)}};
>> +{% endfor %}
>> +
>> +private:
>> +	void recvMessage(const IPCMessage &data);
>> +
>> +{% for method in interface_event.methods %}
>> +	void {{method.mojom_name}}Handler(
>> +		std::vector<uint8_t>::const_iterator data,
>> +		size_t dataSize,
>> +		const std::vector<SharedFD> &fds);
>> +{% endfor %}
>>
>>   	std::unique_ptr<IPCPipeUnixSocket> ipc_;
>>
>
Laurent Pinchart Sept. 29, 2025, 11:23 a.m. UTC | #3 
On Mon, Sep 29, 2025 at 09:54:49AM +0200, Barnabás Pőcze wrote:
> 2025. 09. 29. 9:19 keltezéssel, Laurent Pinchart írta:
> > On Mon, Sep 22, 2025 at 12:03:39PM +0200, Barnabás Pőcze wrote:
> >> Even though there is an abstract class to represent the interface of an IPA,
> >> the threaded and IPC versions are still multiplexed using the same type,
> >> which uses a boolean to actually dispatch to the right function.
> >>
> >> Instead of doing that, split the classes into "threaded" and "isolated"
> >> variants, and make `IPAManager` choose accordingly.
> >>
> >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> >> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> >> ---
> >> changes in v3:
> >>    * rebase
> >>
> >> changes in v2:
> >>    * fix typo
> >>    * rename local -> threaded, remote -> isolated
> >>    * have different log messages in the two variants
> >>
> >> v2: https://patchwork.libcamera.org/patch/24108/
> >> v1: https://patchwork.libcamera.org/patch/24056/
> >> ---
> >>   include/libcamera/internal/ipa_manager.h      |   9 +-
> >>   .../module_ipa_proxy.cpp.tmpl                 | 151 +++++++++---------
> >>   .../module_ipa_proxy.h.tmpl                   |  56 +++++--
> >>   3 files changed, 120 insertions(+), 96 deletions(-)
> >>
> >> diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h
> >> index b0b44c74a..f8ce78016 100644
> >> --- a/include/libcamera/internal/ipa_manager.h
> >> +++ b/include/libcamera/internal/ipa_manager.h
> >> @@ -44,7 +44,14 @@ public:
> >>   			return nullptr;
> >>
> >>   		const GlobalConfiguration &configuration = cm->_d()->configuration();
> >> -		std::unique_ptr<T> proxy = std::make_unique<T>(m, !self->isSignatureValid(m), configuration);
> >> +
> >> +		auto proxy = [&]() -> std::unique_ptr<T> {
> >> +			if (self->isSignatureValid(m))
> >> +				return std::make_unique<typename T::Threaded>(m, configuration);
> >> +			else
> >> +				return std::make_unique<typename T::Isolated>(m, configuration);
> >> +		}();
> >> +
> >>   		if (!proxy->isValid()) {
> >>   			LOG(IPAManager, Error) << "Failed to load proxy";
> >>   			return nullptr;
> >> 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
> >> index 18b4ab5e5..9bf2c58d3 100644
> >> --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> >> +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> >> @@ -45,36 +45,13 @@ namespace {{ns}} {
> >>   {% endfor %}
> >>   {%- endif %}
> >>
> >> -{{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate, const GlobalConfiguration &configuration)
> >> -	: IPAProxy(ipam, configuration), isolate_(isolate),
> >> -	  controlSerializer_(ControlSerializer::Role::Proxy), seq_(0)
> >> +{{proxy_name}}Threaded::{{proxy_name}}Threaded(IPAModule *ipam, const GlobalConfiguration &configuration)
> >> +	: {{proxy_name}}(ipam, configuration)
> >>   {
> >>   	LOG(IPAProxy, Debug)
> >> -		<< "initializing {{module_name}} proxy: loading IPA from "
> >> +		<< "initializing {{module_name}} proxy in thread: loading IPA from "
> >>   		<< ipam->path();
> >>
> >> -	if (isolate_) {
> >> -		const std::string proxyWorkerPath = resolvePath("{{module_name}}_ipa_proxy");
> >> -		if (proxyWorkerPath.empty()) {
> >> -			LOG(IPAProxy, Error)
> >> -				<< "Failed to get proxy worker path";
> >> -			return;
> >> -		}
> >> -
> >> -		auto ipc = std::make_unique<IPCPipeUnixSocket>(ipam->path().c_str(),
> >> -							       proxyWorkerPath.c_str());
> >> -		if (!ipc->isConnected()) {
> >> -			LOG(IPAProxy, Error) << "Failed to create IPCPipe";
> >> -			return;
> >> -		}
> >> -
> >> -		ipc->recv.connect(this, &{{proxy_name}}::recvMessage);
> >> -
> >> -		ipc_ = std::move(ipc);
> >> -		valid_ = true;
> >> -		return;
> >> -	}
> >> -
> >>   	if (!ipam->load())
> >>   		return;
> >>
> >> @@ -89,59 +66,16 @@ namespace {{ns}} {
> >>   	proxy_.setIPA(ipa_.get());
> >>
> >>   {% for method in interface_event.methods %}
> >> -	ipa_->{{method.mojom_name}}.connect(this, &{{proxy_name}}::{{method.mojom_name}}Thread);
> >> +	ipa_->{{method.mojom_name}}.connect(this, &{{proxy_name}}Threaded::{{method.mojom_name}}Handler);
> >>   {%- endfor %}
> >>
> >>   	valid_ = true;
> >>   }
> >>
> >> -{{proxy_name}}::~{{proxy_name}}()
> >> -{
> >> -	if (ipc_) {
> >> -		IPCMessage::Header header =
> >> -			{ static_cast<uint32_t>({{cmd_enum_name}}::Exit), seq_++ };
> >> -		IPCMessage msg(header);
> >> -		ipc_->sendAsync(msg);
> >> -	}
> >> -}
> >> -
> >> -{% if interface_event.methods|length > 0 %}
> >> -void {{proxy_name}}::recvMessage(const IPCMessage &data)
> >> -{
> >> -	size_t dataSize = data.data().size();
> >> -	{{cmd_event_enum_name}} _cmd = static_cast<{{cmd_event_enum_name}}>(data.header().cmd);
> >> -
> >> -	switch (_cmd) {
> >> -{%- for method in interface_event.methods %}
> >> -	case {{cmd_event_enum_name}}::{{method.mojom_name|cap}}: {
> >> -		{{method.mojom_name}}IPC(data.data().cbegin(), dataSize, data.fds());
> >> -		break;
> >> -	}
> >> -{%- endfor %}
> >> -	default:
> >> -		LOG(IPAProxy, Error) << "Unknown command " << static_cast<uint32_t>(_cmd);
> >> -	}
> >> -}
> >> -{%- endif %}
> >> +{{proxy_name}}Threaded::~{{proxy_name}}Threaded() = default;
> > 
> > Can't you omit the destructor from the class definition and drop this
> > line, to let the compiler generate the default destructor ?
> 
> Yes, it could be omitted. The only difference is that it would be an inline
> destructor, generated in the translation unit(s) where it is used. Should I
> change it?

Ah right. You can keep it here if you think it's better. Up to you.

> >>
> >>   {% for method in interface_main.methods %}
> >> -{{proxy_funcs.func_sig(proxy_name, method)}}
> >> -{
> >> -	if (isolate_)
> >> -		return {{method.mojom_name}}IPC(
> >> -{%- for param in method|method_param_names -%}
> >> -		{{param}}{{- ", " if not loop.last}}
> >> -{%- endfor -%}
> >> -);
> >> -	else
> >> -		return {{method.mojom_name}}Thread(
> >> -{%- for param in method|method_param_names -%}
> >> -		{{param}}{{- ", " if not loop.last}}
> >> -{%- endfor -%}
> >> -);
> >> -}
> >> -
> >> -{{proxy_funcs.func_sig(proxy_name, method, "Thread")}}
> >> +{{proxy_funcs.func_sig(proxy_name + "Threaded", method)}}
> >>   {
> >>   {%- if method.mojom_name == "stop" %}
> >>   	{{proxy_funcs.stop_thread_body()}}
> >> @@ -181,8 +115,58 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)
> >>   );
> >>   {%- endif %}
> >>   }
> >> +{% endfor %}
> >> +
> >> +{% for method in interface_event.methods %}
> >> +{{proxy_funcs.func_sig(proxy_name + "Threaded", method, "Handler")}}
> >> +{
> >> +	ASSERT(state_ != ProxyStopped);
> >> +	{{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});
> >> +}
> >> +{% endfor %}
> >> +
> >> +/* ========================================================================== */
> >> +
> >> +{{proxy_name}}Isolated::{{proxy_name}}Isolated(IPAModule *ipam, const GlobalConfiguration &configuration)
> >> +	: {{proxy_name}}(ipam, configuration),
> >> +	  controlSerializer_(ControlSerializer::Role::Proxy), seq_(0)
> >> +{
> >> +	LOG(IPAProxy, Debug)
> >> +		<< "initializing {{module_name}} proxy in isolation: loading IPA from "
> >> +		<< ipam->path();
> >> +
> >> +	const std::string proxyWorkerPath = resolvePath("{{module_name}}_ipa_proxy");
> >> +	if (proxyWorkerPath.empty()) {
> >> +		LOG(IPAProxy, Error)
> >> +			<< "Failed to get proxy worker path";
> > 
> > This now holds on a single line.
> 
> Fixed.
> 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> >> +		return;
> >> +	}
> >> +
> >> +	auto ipc = std::make_unique<IPCPipeUnixSocket>(ipam->path().c_str(),
> >> +						       proxyWorkerPath.c_str());
> >> +	if (!ipc->isConnected()) {
> >> +		LOG(IPAProxy, Error) << "Failed to create IPCPipe";
> >> +		return;
> >> +	}
> >> +
> >> +	ipc->recv.connect(this, &{{proxy_name}}Isolated::recvMessage);
> >> +
> >> +	ipc_ = std::move(ipc);
> >> +	valid_ = true;
> >> +}
> >>
> >> -{{proxy_funcs.func_sig(proxy_name, method, "IPC")}}
> >> +{{proxy_name}}Isolated::~{{proxy_name}}Isolated()
> >> +{
> >> +	if (ipc_) {
> >> +		IPCMessage::Header header =
> >> +			{ static_cast<uint32_t>({{cmd_enum_name}}::Exit), seq_++ };
> >> +		IPCMessage msg(header);
> >> +		ipc_->sendAsync(msg);
> >> +	}
> >> +}
> >> +
> >> +{% for method in interface_main.methods %}
> >> +{{proxy_funcs.func_sig(proxy_name + "Isolated", method)}}
> >>   {
> >>   {%- if method.mojom_name == "configure" %}
> >>   	controlSerializer_.reset();
> >> @@ -226,14 +210,25 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)
> >>
> >>   {% endfor %}
> >>
> >> -{% for method in interface_event.methods %}
> >> -{{proxy_funcs.func_sig(proxy_name, method, "Thread")}}
> >> +void {{proxy_name}}Isolated::recvMessage(const IPCMessage &data)
> >>   {
> >> -	ASSERT(state_ != ProxyStopped);
> >> -	{{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});
> >> +	size_t dataSize = data.data().size();
> >> +	{{cmd_event_enum_name}} _cmd = static_cast<{{cmd_event_enum_name}}>(data.header().cmd);
> >> +
> >> +	switch (_cmd) {
> >> +{%- for method in interface_event.methods %}
> >> +	case {{cmd_event_enum_name}}::{{method.mojom_name|cap}}: {
> >> +		{{method.mojom_name}}Handler(data.data().cbegin(), dataSize, data.fds());
> >> +		break;
> >> +	}
> >> +{%- endfor %}
> >> +	default:
> >> +		LOG(IPAProxy, Error) << "Unknown command " << static_cast<uint32_t>(_cmd);
> >> +	}
> >>   }
> >>
> >> -void {{proxy_name}}::{{method.mojom_name}}IPC(
> >> +{% for method in interface_event.methods %}
> >> +void {{proxy_name}}Isolated::{{method.mojom_name}}Handler(
> >>   	[[maybe_unused]] std::vector<uint8_t>::const_iterator data,
> >>   	[[maybe_unused]] size_t dataSize,
> >>   	[[maybe_unused]] const std::vector<SharedFD> &fds)
> >> 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
> >> index 057c3ab03..ef280ca42 100644
> >> --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
> >> +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
> >> @@ -34,29 +34,32 @@ namespace {{ns}} {
> >>   {% endfor %}
> >>   {%- endif %}
> >>
> >> +class {{proxy_name}}Threaded;
> >> +class {{proxy_name}}Isolated;
> >> +
> >>   class {{proxy_name}} : public IPAProxy, public {{interface_name}}, public Object
> >>   {
> >>   public:
> >> -	{{proxy_name}}(IPAModule *ipam, bool isolate, const GlobalConfiguration &configuration);
> >> -	~{{proxy_name}}();
> >> +	using Threaded = {{proxy_name}}Threaded;
> >> +	using Isolated = {{proxy_name}}Isolated;
> >>
> >> -{% for method in interface_main.methods %}
> >> -{{proxy_funcs.func_sig(proxy_name, method, "", false, true)|indent(8, true)}};
> >> -{% endfor %}
> >> +protected:
> >> +	using IPAProxy::IPAProxy;
> >> +};
> >>
> >> -private:
> >> -	void recvMessage(const IPCMessage &data);
> >> +class {{proxy_name}}Threaded : public {{proxy_name}}
> >> +{
> >> +public:
> >> +	{{proxy_name}}Threaded(IPAModule *ipam, const GlobalConfiguration &configuration);
> >> +	~{{proxy_name}}Threaded();
> >>
> >>   {% for method in interface_main.methods %}
> >> -{{proxy_funcs.func_sig(proxy_name, method, "Thread", false)|indent(8, true)}};
> >> -{{proxy_funcs.func_sig(proxy_name, method, "IPC", false)|indent(8, true)}};
> >> +{{proxy_funcs.func_sig(proxy_name + "Threaded", method, "", false, true)|indent(8, true)}};
> >>   {% endfor %}
> >> +
> >> +private:
> >>   {% for method in interface_event.methods %}
> >> -{{proxy_funcs.func_sig(proxy_name, method, "Thread", false)|indent(8, true)}};
> >> -	void {{method.mojom_name}}IPC(
> >> -		std::vector<uint8_t>::const_iterator data,
> >> -		size_t dataSize,
> >> -		const std::vector<SharedFD> &fds);
> >> +{{proxy_funcs.func_sig(proxy_name + "Threaded", method, "Handler", false)|indent(8, true)}};
> >>   {% endfor %}
> >>
> >>   	/* Helper class to invoke async functions in another thread. */
> >> @@ -79,12 +82,12 @@ private:
> >>   		}
> >>   {% for method in interface_main.methods %}
> >>   {%- if method|is_async %}
> >> -		{{proxy_funcs.func_sig(proxy_name, method, "", false)|indent(16)}}
> >> +		{{proxy_funcs.func_sig(proxy_name + "Threaded", method, "", false)|indent(16)}}
> >>   		{
> >>   			ipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}});
> >>   		}
> >>   {%- elif method.mojom_name == "start" %}
> >> -		{{proxy_funcs.func_sig(proxy_name, method, "", false)|indent(16)}}
> >> +		{{proxy_funcs.func_sig(proxy_name + "Threaded", method, "", false)|indent(16)}}
> >>   		{
> >>   {%- if method|method_return_value != "void" %}
> >>   			return ipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}});
> >> @@ -104,8 +107,27 @@ private:
> >>   	Thread thread_;
> >>   	ThreadProxy proxy_;
> >>   	std::unique_ptr<{{interface_name}}> ipa_;
> >> +};
> >>
> >> -	const bool isolate_;
> >> +class {{proxy_name}}Isolated : public {{proxy_name}}
> >> +{
> >> +public:
> >> +	{{proxy_name}}Isolated(IPAModule *ipam, const GlobalConfiguration &configuration);
> >> +	~{{proxy_name}}Isolated();
> >> +
> >> +{% for method in interface_main.methods %}
> >> +{{proxy_funcs.func_sig(proxy_name + "Isolated", method, "", false, true)|indent(8, true)}};
> >> +{% endfor %}
> >> +
> >> +private:
> >> +	void recvMessage(const IPCMessage &data);
> >> +
> >> +{% for method in interface_event.methods %}
> >> +	void {{method.mojom_name}}Handler(
> >> +		std::vector<uint8_t>::const_iterator data,
> >> +		size_t dataSize,
> >> +		const std::vector<SharedFD> &fds);
> >> +{% endfor %}
> >>
> >>   	std::unique_ptr<IPCPipeUnixSocket> ipc_;
> >>

Patch
diff mbox series 

diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h
index b0b44c74a..f8ce78016 100644
--- a/include/libcamera/internal/ipa_manager.h
+++ b/include/libcamera/internal/ipa_manager.h
@@ -44,7 +44,14 @@  public:
 			return nullptr;

 		const GlobalConfiguration &configuration = cm->_d()->configuration();
-		std::unique_ptr<T> proxy = std::make_unique<T>(m, !self->isSignatureValid(m), configuration);
+
+		auto proxy = [&]() -> std::unique_ptr<T> {
+			if (self->isSignatureValid(m))
+				return std::make_unique<typename T::Threaded>(m, configuration);
+			else
+				return std::make_unique<typename T::Isolated>(m, configuration);
+		}();
+
 		if (!proxy->isValid()) {
 			LOG(IPAManager, Error) << "Failed to load proxy";
 			return nullptr;
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
index 18b4ab5e5..9bf2c58d3 100644
--- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
+++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
@@ -45,36 +45,13 @@  namespace {{ns}} {
 {% endfor %}
 {%- endif %}

-{{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate, const GlobalConfiguration &configuration)
-	: IPAProxy(ipam, configuration), isolate_(isolate),
-	  controlSerializer_(ControlSerializer::Role::Proxy), seq_(0)
+{{proxy_name}}Threaded::{{proxy_name}}Threaded(IPAModule *ipam, const GlobalConfiguration &configuration)
+	: {{proxy_name}}(ipam, configuration)
 {
 	LOG(IPAProxy, Debug)
-		<< "initializing {{module_name}} proxy: loading IPA from "
+		<< "initializing {{module_name}} proxy in thread: loading IPA from "
 		<< ipam->path();

-	if (isolate_) {
-		const std::string proxyWorkerPath = resolvePath("{{module_name}}_ipa_proxy");
-		if (proxyWorkerPath.empty()) {
-			LOG(IPAProxy, Error)
-				<< "Failed to get proxy worker path";
-			return;
-		}
-
-		auto ipc = std::make_unique<IPCPipeUnixSocket>(ipam->path().c_str(),
-							       proxyWorkerPath.c_str());
-		if (!ipc->isConnected()) {
-			LOG(IPAProxy, Error) << "Failed to create IPCPipe";
-			return;
-		}
-
-		ipc->recv.connect(this, &{{proxy_name}}::recvMessage);
-
-		ipc_ = std::move(ipc);
-		valid_ = true;
-		return;
-	}
-
 	if (!ipam->load())
 		return;

@@ -89,59 +66,16 @@  namespace {{ns}} {
 	proxy_.setIPA(ipa_.get());

 {% for method in interface_event.methods %}
-	ipa_->{{method.mojom_name}}.connect(this, &{{proxy_name}}::{{method.mojom_name}}Thread);
+	ipa_->{{method.mojom_name}}.connect(this, &{{proxy_name}}Threaded::{{method.mojom_name}}Handler);
 {%- endfor %}

 	valid_ = true;
 }

-{{proxy_name}}::~{{proxy_name}}()
-{
-	if (ipc_) {
-		IPCMessage::Header header =
-			{ static_cast<uint32_t>({{cmd_enum_name}}::Exit), seq_++ };
-		IPCMessage msg(header);
-		ipc_->sendAsync(msg);
-	}
-}
-
-{% if interface_event.methods|length > 0 %}
-void {{proxy_name}}::recvMessage(const IPCMessage &data)
-{
-	size_t dataSize = data.data().size();
-	{{cmd_event_enum_name}} _cmd = static_cast<{{cmd_event_enum_name}}>(data.header().cmd);
-
-	switch (_cmd) {
-{%- for method in interface_event.methods %}
-	case {{cmd_event_enum_name}}::{{method.mojom_name|cap}}: {
-		{{method.mojom_name}}IPC(data.data().cbegin(), dataSize, data.fds());
-		break;
-	}
-{%- endfor %}
-	default:
-		LOG(IPAProxy, Error) << "Unknown command " << static_cast<uint32_t>(_cmd);
-	}
-}
-{%- endif %}
+{{proxy_name}}Threaded::~{{proxy_name}}Threaded() = default;

 {% for method in interface_main.methods %}
-{{proxy_funcs.func_sig(proxy_name, method)}}
-{
-	if (isolate_)
-		return {{method.mojom_name}}IPC(
-{%- for param in method|method_param_names -%}
-		{{param}}{{- ", " if not loop.last}}
-{%- endfor -%}
-);
-	else
-		return {{method.mojom_name}}Thread(
-{%- for param in method|method_param_names -%}
-		{{param}}{{- ", " if not loop.last}}
-{%- endfor -%}
-);
-}
-
-{{proxy_funcs.func_sig(proxy_name, method, "Thread")}}
+{{proxy_funcs.func_sig(proxy_name + "Threaded", method)}}
 {
 {%- if method.mojom_name == "stop" %}
 	{{proxy_funcs.stop_thread_body()}}
@@ -181,8 +115,58 @@  void {{proxy_name}}::recvMessage(const IPCMessage &data)
 );
 {%- endif %}
 }
+{% endfor %}
+
+{% for method in interface_event.methods %}
+{{proxy_funcs.func_sig(proxy_name + "Threaded", method, "Handler")}}
+{
+	ASSERT(state_ != ProxyStopped);
+	{{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});
+}
+{% endfor %}
+
+/* ========================================================================== */
+
+{{proxy_name}}Isolated::{{proxy_name}}Isolated(IPAModule *ipam, const GlobalConfiguration &configuration)
+	: {{proxy_name}}(ipam, configuration),
+	  controlSerializer_(ControlSerializer::Role::Proxy), seq_(0)
+{
+	LOG(IPAProxy, Debug)
+		<< "initializing {{module_name}} proxy in isolation: loading IPA from "
+		<< ipam->path();
+
+	const std::string proxyWorkerPath = resolvePath("{{module_name}}_ipa_proxy");
+	if (proxyWorkerPath.empty()) {
+		LOG(IPAProxy, Error)
+			<< "Failed to get proxy worker path";
+		return;
+	}
+
+	auto ipc = std::make_unique<IPCPipeUnixSocket>(ipam->path().c_str(),
+						       proxyWorkerPath.c_str());
+	if (!ipc->isConnected()) {
+		LOG(IPAProxy, Error) << "Failed to create IPCPipe";
+		return;
+	}
+
+	ipc->recv.connect(this, &{{proxy_name}}Isolated::recvMessage);
+
+	ipc_ = std::move(ipc);
+	valid_ = true;
+}

-{{proxy_funcs.func_sig(proxy_name, method, "IPC")}}
+{{proxy_name}}Isolated::~{{proxy_name}}Isolated()
+{
+	if (ipc_) {
+		IPCMessage::Header header =
+			{ static_cast<uint32_t>({{cmd_enum_name}}::Exit), seq_++ };
+		IPCMessage msg(header);
+		ipc_->sendAsync(msg);
+	}
+}
+
+{% for method in interface_main.methods %}
+{{proxy_funcs.func_sig(proxy_name + "Isolated", method)}}
 {
 {%- if method.mojom_name == "configure" %}
 	controlSerializer_.reset();
@@ -226,14 +210,25 @@  void {{proxy_name}}::recvMessage(const IPCMessage &data)

 {% endfor %}

-{% for method in interface_event.methods %}
-{{proxy_funcs.func_sig(proxy_name, method, "Thread")}}
+void {{proxy_name}}Isolated::recvMessage(const IPCMessage &data)
 {
-	ASSERT(state_ != ProxyStopped);
-	{{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});
+	size_t dataSize = data.data().size();
+	{{cmd_event_enum_name}} _cmd = static_cast<{{cmd_event_enum_name}}>(data.header().cmd);
+
+	switch (_cmd) {
+{%- for method in interface_event.methods %}
+	case {{cmd_event_enum_name}}::{{method.mojom_name|cap}}: {
+		{{method.mojom_name}}Handler(data.data().cbegin(), dataSize, data.fds());
+		break;
+	}
+{%- endfor %}
+	default:
+		LOG(IPAProxy, Error) << "Unknown command " << static_cast<uint32_t>(_cmd);
+	}
 }

-void {{proxy_name}}::{{method.mojom_name}}IPC(
+{% for method in interface_event.methods %}
+void {{proxy_name}}Isolated::{{method.mojom_name}}Handler(
 	[[maybe_unused]] std::vector<uint8_t>::const_iterator data,
 	[[maybe_unused]] size_t dataSize,
 	[[maybe_unused]] const std::vector<SharedFD> &fds)
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
index 057c3ab03..ef280ca42 100644
--- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
+++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
@@ -34,29 +34,32 @@  namespace {{ns}} {
 {% endfor %}
 {%- endif %}

+class {{proxy_name}}Threaded;
+class {{proxy_name}}Isolated;
+
 class {{proxy_name}} : public IPAProxy, public {{interface_name}}, public Object
 {
 public:
-	{{proxy_name}}(IPAModule *ipam, bool isolate, const GlobalConfiguration &configuration);
-	~{{proxy_name}}();
+	using Threaded = {{proxy_name}}Threaded;
+	using Isolated = {{proxy_name}}Isolated;

-{% for method in interface_main.methods %}
-{{proxy_funcs.func_sig(proxy_name, method, "", false, true)|indent(8, true)}};
-{% endfor %}
+protected:
+	using IPAProxy::IPAProxy;
+};

-private:
-	void recvMessage(const IPCMessage &data);
+class {{proxy_name}}Threaded : public {{proxy_name}}
+{
+public:
+	{{proxy_name}}Threaded(IPAModule *ipam, const GlobalConfiguration &configuration);
+	~{{proxy_name}}Threaded();

 {% for method in interface_main.methods %}
-{{proxy_funcs.func_sig(proxy_name, method, "Thread", false)|indent(8, true)}};
-{{proxy_funcs.func_sig(proxy_name, method, "IPC", false)|indent(8, true)}};
+{{proxy_funcs.func_sig(proxy_name + "Threaded", method, "", false, true)|indent(8, true)}};
 {% endfor %}
+
+private:
 {% for method in interface_event.methods %}
-{{proxy_funcs.func_sig(proxy_name, method, "Thread", false)|indent(8, true)}};
-	void {{method.mojom_name}}IPC(
-		std::vector<uint8_t>::const_iterator data,
-		size_t dataSize,
-		const std::vector<SharedFD> &fds);
+{{proxy_funcs.func_sig(proxy_name + "Threaded", method, "Handler", false)|indent(8, true)}};
 {% endfor %}

 	/* Helper class to invoke async functions in another thread. */
@@ -79,12 +82,12 @@  private:
 		}
 {% for method in interface_main.methods %}
 {%- if method|is_async %}
-		{{proxy_funcs.func_sig(proxy_name, method, "", false)|indent(16)}}
+		{{proxy_funcs.func_sig(proxy_name + "Threaded", method, "", false)|indent(16)}}
 		{
 			ipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}});
 		}
 {%- elif method.mojom_name == "start" %}
-		{{proxy_funcs.func_sig(proxy_name, method, "", false)|indent(16)}}
+		{{proxy_funcs.func_sig(proxy_name + "Threaded", method, "", false)|indent(16)}}
 		{
 {%- if method|method_return_value != "void" %}
 			return ipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}});
@@ -104,8 +107,27 @@  private:
 	Thread thread_;
 	ThreadProxy proxy_;
 	std::unique_ptr<{{interface_name}}> ipa_;
+};

-	const bool isolate_;
+class {{proxy_name}}Isolated : public {{proxy_name}}
+{
+public:
+	{{proxy_name}}Isolated(IPAModule *ipam, const GlobalConfiguration &configuration);
+	~{{proxy_name}}Isolated();
+
+{% for method in interface_main.methods %}
+{{proxy_funcs.func_sig(proxy_name + "Isolated", method, "", false, true)|indent(8, true)}};
+{% endfor %}
+
+private:
+	void recvMessage(const IPCMessage &data);
+
+{% for method in interface_event.methods %}
+	void {{method.mojom_name}}Handler(
+		std::vector<uint8_t>::const_iterator data,
+		size_t dataSize,
+		const std::vector<SharedFD> &fds);
+{% endfor %}

 	std::unique_ptr<IPCPipeUnixSocket> ipc_;