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

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

Commit Message

Barnabás Pőcze Aug. 15, 2025, 8:51 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>
---
changes in v2:
  * fix typo
  * rename local -> threaded, remote -> isolated
  * have different log messages in the two variants

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

--
2.50.1

Comments

Paul Elder Aug. 26, 2025, 3:14 a.m. UTC | #1 
Hi Barnabás,

Thanks for the patch.

Quoting Barnabás Pőcze (2025-08-15 17:51:14)
> 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>
> ---
> changes in v2:
>   * fix typo
>   * rename local -> threaded, remote -> isolated
>   * have different log messages in the two variants
> 
> v1: https://patchwork.libcamera.org/patch/24056/
> ---
>  include/libcamera/internal/ipa_manager.h      |   8 +-
>  .../module_ipa_proxy.cpp.tmpl                 | 151 +++++++++---------
>  .../module_ipa_proxy.h.tmpl                   |  56 +++++--
>  3 files changed, 119 insertions(+), 96 deletions(-)
> 
> diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h
> index a0d448cf9..ae832b645 100644
> --- a/include/libcamera/internal/ipa_manager.h
> +++ b/include/libcamera/internal/ipa_manager.h
> @@ -42,7 +42,13 @@ public:
>                 if (!m)
>                         return nullptr;
> 
> -               std::unique_ptr<T> proxy = std::make_unique<T>(m, !self->isSignatureValid(m));
> +               auto proxy = [&]() -> std::unique_ptr<T> {
> +                       if (self->isSignatureValid(m))
> +                               return std::make_unique<typename T::Threaded>(m);
> +                       else
> +                               return std::make_unique<typename T::Isolated>(m);
> +               }();
> +
>                 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 beb646e2d..7ccde1d56 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)
> -       : IPAProxy(ipam), isolate_(isolate),
> -         controlSerializer_(ControlSerializer::Role::Proxy), seq_(0)
> +{{proxy_name}}Threaded::{{proxy_name}}Threaded(IPAModule *ipam)
> +       : {{proxy_name}}(ipam)
>  {
>         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)
> +       : {{proxy_name}}(ipam),
> +         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 %}

I just realized that you removed the {% if interface_event.methods|length > 0
%} guard that was around recvMessage (and that I didn't have the same guard in
the header file). Would the compiler complain here that the switch has no cases here?

Or maybe we don't have to worry about that scenario since IPAs will never not
have events, since an IPA that doesn't return anything to the pipeline handler
is the same as no IPA. So maybe it's fine.

Ok, looks good to me.

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> +       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 a0312a7c1..caf8392da 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);
> -       ~{{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);
> +       ~{{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);
> +       ~{{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_;
> 
> --
> 2.50.1
Barnabás Pőcze Aug. 26, 2025, 10:03 a.m. UTC | #2 
2025. 08. 26. 5:14 keltezéssel, Paul Elder írta:
> Hi Barnabás,
> 
> Thanks for the patch.
> 
> Quoting Barnabás Pőcze (2025-08-15 17:51:14)
>> 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>
>> ---
>> changes in v2:
>>    * fix typo
>>    * rename local -> threaded, remote -> isolated
>>    * have different log messages in the two variants
>>
>> v1: https://patchwork.libcamera.org/patch/24056/
>> ---
>>   include/libcamera/internal/ipa_manager.h      |   8 +-
>>   .../module_ipa_proxy.cpp.tmpl                 | 151 +++++++++---------
>>   .../module_ipa_proxy.h.tmpl                   |  56 +++++--
>>   3 files changed, 119 insertions(+), 96 deletions(-)
>>
>> diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h
>> index a0d448cf9..ae832b645 100644
>> --- a/include/libcamera/internal/ipa_manager.h
>> +++ b/include/libcamera/internal/ipa_manager.h
>> @@ -42,7 +42,13 @@ public:
>>                  if (!m)
>>                          return nullptr;
>>
>> -               std::unique_ptr<T> proxy = std::make_unique<T>(m, !self->isSignatureValid(m));
>> +               auto proxy = [&]() -> std::unique_ptr<T> {
>> +                       if (self->isSignatureValid(m))
>> +                               return std::make_unique<typename T::Threaded>(m);
>> +                       else
>> +                               return std::make_unique<typename T::Isolated>(m);
>> +               }();
>> +
>>                  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 beb646e2d..7ccde1d56 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)
>> -       : IPAProxy(ipam), isolate_(isolate),
>> -         controlSerializer_(ControlSerializer::Role::Proxy), seq_(0)
>> +{{proxy_name}}Threaded::{{proxy_name}}Threaded(IPAModule *ipam)
>> +       : {{proxy_name}}(ipam)
>>   {
>>          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)
>> +       : {{proxy_name}}(ipam),
>> +         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 %}
> 
> I just realized that you removed the {% if interface_event.methods|length > 0
> %} guard that was around recvMessage (and that I didn't have the same guard in
> the header file). Would the compiler complain here that the switch has no cases here?
> 
> Or maybe we don't have to worry about that scenario since IPAs will never not
> have events, since an IPA that doesn't return anything to the pipeline handler
> is the same as no IPA. So maybe it's fine.

The enum type seems to always be generated, so I am pretty sure it should be ok.
I don't expect any compiler to complain about the lack of `case` labels in any case.


Regards,
Barnabás Pőcze

> 
> Ok, looks good to me.
> 
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> 
>> +       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 a0312a7c1..caf8392da 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);
>> -       ~{{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);
>> +       ~{{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);
>> +       ~{{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_;
>>
>> --
>> 2.50.1

Patch
diff mbox series 

diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h
index a0d448cf9..ae832b645 100644
--- a/include/libcamera/internal/ipa_manager.h
+++ b/include/libcamera/internal/ipa_manager.h
@@ -42,7 +42,13 @@  public:
 		if (!m)
 			return nullptr;

-		std::unique_ptr<T> proxy = std::make_unique<T>(m, !self->isSignatureValid(m));
+		auto proxy = [&]() -> std::unique_ptr<T> {
+			if (self->isSignatureValid(m))
+				return std::make_unique<typename T::Threaded>(m);
+			else
+				return std::make_unique<typename T::Isolated>(m);
+		}();
+
 		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 beb646e2d..7ccde1d56 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)
-	: IPAProxy(ipam), isolate_(isolate),
-	  controlSerializer_(ControlSerializer::Role::Proxy), seq_(0)
+{{proxy_name}}Threaded::{{proxy_name}}Threaded(IPAModule *ipam)
+	: {{proxy_name}}(ipam)
 {
 	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)
+	: {{proxy_name}}(ipam),
+	  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 a0312a7c1..caf8392da 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);
-	~{{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);
+	~{{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);
+	~{{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_;