[libcamera-devel] [PATCH v4 04/37] utils: ipc: add templates for code generation for IPC mechanism
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Nov 26 17:07:54 CET 2020
Hi Paul,
On Thu, Nov 26, 2020 at 03:49:18PM +0900, paul.elder at ideasonboard.com wrote:
> On Thu, Nov 19, 2020 at 07:21:00PM +0200, Laurent Pinchart wrote:
> > On Fri, Nov 06, 2020 at 07:36:34PM +0900, Paul Elder wrote:
> > > Add templates to mojo to generate code for the IPC mechanism. These
> > > templates generate:
> > > - module header
> > > - module serializer
> > > - IPA proxy cpp, header, and worker
> > >
> > > Given an input data definition mojom file for a pipeline.
> > >
> > > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > >
> > > ---
> > > Changes in v4:
> > > For the non-template files:
> > > - rename IPA{pipeline_name}CallbackInterface to
> > > IPA{pipeline_name}EventInterface
> > > - to avoid the notion of "callback" and emphasize that it's an event
> > > - add support for strings in custom structs
> > > - add validation, that async methods must not have return values
> > > - it throws exception and isn't very clear though...?
> > > - rename controls to libcamera::{pipeline_name}::controls (controls is
> > > now lowercase)
> > > - rename {pipeline_name}_generated.h to {pipeline_name}_ipa_interface.h,
> > > and {pipeline_name}_serializer.h to {pipeline_name}_ipa_serializer.h
> > > - same for their corresponding template files
> > > For the template files:
> > > - fix spacing (now it's all {{var}} instead of some {{ var }})
> > > - except if it's code, so code is still {{ code }}
> > > - move inclusion of corresponding header to first in the inclusion list
> > > - fix copy&paste errors
> > > - change snake_case to camelCase in the generated code
> > > - template code still uses snake_case
> > > - change the generated command enums to an enum class, and make it
> > > capitalized (instead of allcaps)
> > > - add length checks to recvIPC (in proxy)
> > > - fix some template spacing
> > > - don't use const for PODs in function/signal parameters
> > > - add the proper length checks to readPOD/appendPOD
> > > - the helper functions for reading and writing PODs to and from
> > > serialized data
> > > - rename readUInt/appendUInt to readPOD/appendPOD
> > > - add support for strings in custom structs
> > >
> > > Changes in v3:
> > > - add support for namespaces
> > > - fix enum assignment (used to have +1 for CMD applied to all enums)
> > > - use readHeader, writeHeader, and eraseHeader as static class functions
> > > of IPAIPCUnixSocket (in the proxy worker)
> > > - add requirement that base controls *must* be defined in
> > > libcamera::{pipeline_name}::Controls
> > >
> > > Changes in v2:
> > > - mandate the main and callback interfaces, and init(), start(), stop()
> > > and their parameters
> > > - fix returning single pod value from IPC-called function
> > > - add licenses
> > > - improve auto-generated message
> > > - other fixes related to serdes
> > > ---
> > > .../module_ipa_interface.h.tmpl | 113 ++++
> > > .../module_ipa_proxy.cpp.tmpl | 238 +++++++++
> > > .../module_ipa_proxy.h.tmpl | 118 +++++
> > > .../module_ipa_proxy_worker.cpp.tmpl | 187 +++++++
> > > .../module_ipa_serializer.h.tmpl | 44 ++
> > > .../libcamera_templates/proxy_functions.tmpl | 205 ++++++++
> > > .../libcamera_templates/serializer.tmpl | 280 ++++++++++
> > > .../generators/mojom_libcamera_generator.py | 488 ++++++++++++++++++
> > > 8 files changed, 1673 insertions(+)
> > > create mode 100644 utils/ipc/generators/libcamera_templates/module_ipa_interface.h.tmpl
> > > create mode 100644 utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> > > create mode 100644 utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
> > > create mode 100644 utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
> >
> > The proxy .h and .cpp files are named ipa_proxy_{pipeline}.{h,cpp},
> > while the other generated files are named {pipeline}_ipa_interface.h and
> > {pipeline}_ipa_serializer.h. Could you pick one naming convention, and
> > apply it to all files ? The .tmpl files should then follow the same
> > convention.
>
> Ah, yeah. I'll make them all {pipeline}_ipa_{proxy,proxy_worker,interface,serializer}.{h,cpp}
>
> > > create mode 100644 utils/ipc/generators/libcamera_templates/module_ipa_serializer.h.tmpl
> > > create mode 100644 utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> > > create mode 100644 utils/ipc/generators/libcamera_templates/serializer.tmpl
> > > create mode 100644 utils/ipc/generators/mojom_libcamera_generator.py
> >
> > And now, for the templates.
> >
> > > diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_interface.h.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_interface.h.tmpl
> > > new file mode 100644
> > > index 00000000..a470b99e
> > > --- /dev/null
> > > +++ b/utils/ipc/generators/libcamera_templates/module_ipa_interface.h.tmpl
> > > @@ -0,0 +1,113 @@
> > > +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) {{year}}, Google Inc.
> >
> > Coming back to the question of copyright on generated code.
> >
> > First of all, the template itself is a creative work, which should have
> > a copyright notice, with a fixed date. This should go to the comment at
> > the beginning of the file.
> >
> > The generated file is the result of a non-creative process that takes
> > creative work as input. As such, it is covered by the copyright of the
> > template, and the mojom file. Would it be difficult to extract the
> > copyright data of the .mojom file and add it here ? I'm thnking about
> > the following at the beginning of the template:
> >
> > {#-
> > # SPDX-License-Identifier: LGPL-2.1-or-later
> > # Copyright (C) 2020, Google Inc.
> > -#}
> > /* SPDX-License-Identifier: LGPL-2.1-or-later */
> > /*
> > * Copyright (C) 2020, Google Inc.
> > * {{copyright}}
> > *
> > * {{module_name}}_ipa_interface.h - Image Processing Algorithm interface for {{module_name}}
> > *
> > * This file is auto-generated. Do not edit.
> > */
> >
> > with {{copyright}} being replaced with the copyright statement from the
> > .mojom file. Mojo won't give you the information, but if you pass the
> > template name to _GetJinjaExport(), it may not be too difficult to
> > extract the copyright line.
> >
> > This may not be worth it, so feel free to just have a fixed copyright
> > here:
> >
> > /* SPDX-License-Identifier: LGPL-2.1-or-later */
> > /*
> > * Copyright (C) 2020, Google Inc.
> > *
> > * {{module_name}}_ipa_interface.h - Image Processing Algorithm interface for {{module_name}}
> > *
> > * This file is auto-generated. Do not edit.
> > */
>
> Okay. I'll take the latter.
>
> > > + *
> > > + * {{module_name}}_ipa_interface.h - Image Processing Algorithm interface for {{module_name}}
> > > + *
> > > + * This file is auto-generated. Do not edit.
> > > + */
> > > +
> > > +#ifndef __LIBCAMERA_IPA_INTERFACE_{{module_name|upper}}_GENERATED_H__
> > > +#define __LIBCAMERA_IPA_INTERFACE_{{module_name|upper}}_GENERATED_H__
> > > +
> > > +#include <libcamera/ipa/ipa_interface.h>
> > > +#include <libcamera/ipa/{{module_name}}.h>
> > > +
> > > +{% if has_map %}#include <map>{% endif %}
> > > +{% if has_array %}#include <vector>{% endif %}
> > > +
> > > +namespace libcamera {
> > > +{%- if has_namespace %}
> > > +{% for ns in namespace %}
> > > +namespace {{ns}} {
> > > +{% endfor %}
> > > +{%- endif %}
> > > +
> > > +enum class {{cmd_enum_name}} {
> > > + Exit = 0,
> > > +{%- for method in interface_main.methods %}
> > > + {{method.mojom_name|cap}} = {{loop.index}},
> > > +{%- endfor %}
> > > +};
> > > +
> > > +enum class {{cmd_event_enum_name}} {
> > > +{%- for method in interface_cb.methods %}
> > > + {{method.mojom_name|cap}} = {{loop.index}},
> > > +{%- endfor %}
> > > +};
> > > +
> > > +{% for enum in enums %}
> > > +enum {{enum.mojom_name}} {
> > > +{%- for field in enum.fields %}
> > > + {{field.mojom_name}} = {{field.numeric_value}},
> > > +{%- endfor %}
> > > +};
> > > +{% endfor %}
> > > +
> > > +{%- for struct in structs_nonempty %}
> > > +struct {{struct.mojom_name}}
> > > +{
> > > +public:
> > > + {{struct.mojom_name}}() {%- if struct|has_default_fields %}
> > > + :{% endif %}
> > > +{%- for field in struct.fields|with_default_values -%}
> > > +{{" " if loop.first}}{{field.mojom_name}}_({{field|default_value}}){{", " if not loop.last}}
> > > +{%- endfor %}
> > > + {
> > > + }
> > > +
> > > + ~{{struct.mojom_name}}() {}
> >
> > I think you can skip empty destructors.
> >
> > > +
> > > + {{struct.mojom_name}}(
> > > +{%- for field in struct.fields -%}
> > > +{{field|name}} {{field.mojom_name}}{{", " if not loop.last}}
> >
> > Should non-trivial parameters be passed by const reference instead of
> > value ?
>
> They should.
>
> > > +{%- endfor -%}
> > > +)
> > > + :
> > > +{%- for field in struct.fields -%}
> > > +{{" " if loop.first}}{{field.mojom_name}}_({{field.mojom_name}}){{", " if not loop.last}}
> > > +{%- endfor %}
> > > + {
> > > + }
> > > +{% for field in struct.fields %}
> > > + {{field|name}} {{field.mojom_name}}_;
> > > +{%- endfor %}
> > > +};
> > > +{% endfor %}
> > > +
> > > +{#-
> > > +Any consts or #defines should be moved to the mojom file when possible.
> > > +If anything needs to be #included, then {{module_name}}.h needs to have the
> > > +#include.
> > > +#}
> > > +class {{interface_name}} : public IPAInterface
> > > +{
> > > +public:
> > > + virtual ~{{interface_name}}() {}
> >
> > The base class has a virtual destructor already, so you can drop this
> > one.
> >
> > > +{% for method in interface_main.methods %}
> > > + virtual {{method|method_return_value}} {{method.mojom_name}}(
> > > +{%- for param in method|method_parameters %}
> > > + {{param}}{{- "," if not loop.last}}
> > > +{%- endfor -%}
> > > +) = 0;
> > > +{% endfor %}
> > > +
> > > +{%- for method in interface_cb.methods %}
> > > + Signal<
> > > +{%- for param in method.parameters -%}
> > > + {{"const " if not param|is_pod}}{{param|name}}{{" &" if not param|is_pod}}
> > > + {{- ", " if not loop.last}}
> > > +{%- endfor -%}
> > > +> {{method.mojom_name}};
> > > +{% endfor -%}
> > > +};
> > > +
> > > +{%- if has_namespace %}
> > > +{% for ns in namespace|reverse %}
> > > +} /* {{ns}} */
> >
> > This should be
> >
> > } /* namespace {{ns}} */
> >
> > Same in a few locations below.
> >
> > > +{% endfor %}
> > > +{%- endif %}
> > > +} /* namespace libcamera */
> > > +
> > > +#endif /* __LIBCAMERA_IPA_INTERFACE_{{module_name|upper}}_GENERATED_H__ */
> > > diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> > > new file mode 100644
> > > index 00000000..9328c7ca
> > > --- /dev/null
> > > +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> > > @@ -0,0 +1,238 @@
> > > +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}
> > > +{%- import "proxy_functions.tmpl" as proxy_funcs -%}
> > > +
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) {{year}}, Google Inc.
> > > + *
> > > + * ipa_proxy_{{module_name}}.cpp - Image Processing Algorithm proxy for {{module_name}}
> > > + *
> > > + * This file is auto-generated. Do not edit.
> > > + */
> > > +
> > > +#include <libcamera/ipa/ipa_proxy_{{module_name}}.h>
> > > +
> > > +#include <vector>
> > > +
> > > +#include <libcamera/ipa/ipa_module_info.h>
> > > +#include <libcamera/ipa/{{module_name}}.h>
> > > +#include <libcamera/ipa/{{module_name}}_ipa_interface.h>
> > > +#include <libcamera/ipa/{{module_name}}_ipa_serializer.h>
> > > +
> > > +#include "libcamera/internal/control_serializer.h"
> > > +#include "libcamera/internal/ipa_ipc.h"
> > > +#include "libcamera/internal/ipa_ipc_unixsocket.h"
> > > +#include "libcamera/internal/ipa_data_serializer.h"
> > > +#include "libcamera/internal/ipa_module.h"
> > > +#include "libcamera/internal/ipa_proxy.h"
> > > +#include "libcamera/internal/ipc_unixsocket.h"
> > > +#include "libcamera/internal/log.h"
> > > +#include "libcamera/internal/process.h"
> > > +#include "libcamera/internal/thread.h"
> >
> > YOu got the alphabetical order nearly right :-)
> >
> > > +
> > > +namespace libcamera {
> > > +
> > > +LOG_DECLARE_CATEGORY(IPAProxy)
> > > +
> > > +{%- if has_namespace %}
> > > +{% for ns in namespace %}
> > > +namespace {{ns}} {
> > > +{% endfor %}
> > > +{%- endif %}
> > > +
> > > +{{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate)
> > > + : IPAProxy(ipam), running_(false),
> > > + isolate_(isolate)
> > > +{
> > > + LOG(IPAProxy, Debug)
> > > + << "initializing {{module_name}} proxy: loading IPA from "
> > > + << ipam->path();
> > > +
> > > + if (isolate_) {
> > > + const std::string proxyWorkerPath = resolvePath("ipa_proxy_{{module_name}}");
> > > + if (proxyWorkerPath.empty()) {
> > > + LOG(IPAProxy, Error)
> > > + << "Failed to get proxy worker path";
> > > + return;
> > > + }
> > > +
> > > + ipc_ = std::make_unique<IPAIPCUnixSocket>(ipam->path().c_str(), proxyWorkerPath.c_str());
> >
> > Line wrap maybe ?
> >
> > > + if (!ipc_->isValid()) {
> > > + LOG(IPAProxy, Error) << "Failed to create IPAIPC";
> > > + return;
> > > + }
> > > +
> > > +{% if interface_cb.methods|length > 0 %}
> > > + ipc_->recvIPC.connect(this, &{{proxy_name}}::recvIPC);
> > > +{% endif %}
> >
> > I think we can assume there will always be events. An IPA module that
> > only consumes data would be a bit useless, wouldn't it ? :-)
> >
> > > +
> > > + valid_ = true;
> > > + return;
> > > + }
> > > +
> > > + if (!ipam->load())
> > > + return;
> > > +
> > > + IPAInterface *ipai = ipam->createInterface();
> > > + if (!ipai) {
> > > + LOG(IPAProxy, Error)
> > > + << "Failed to create IPA context for " << ipam->path();
> > > + return;
> > > + }
> > > +
> > > + ipa_ = std::unique_ptr<{{interface_name}}>(dynamic_cast<{{interface_name}} *>(ipai));
> >
> > Do we need a dynamic cast ?
>
> Yeah.
>
> src/libcamera/proxy/raspberrypi_ipa_proxy.cpp:78:46: error: no matching function for call to
> ‘std::unique_ptr<libcamera::ipa::rpi::IPARPiInterface>::unique_ptr(libcamera::IPAInterface*&)’
> 78 | ipa_ = std::unique_ptr<IPARPiInterface>(ipai);
>
> ipai is pointer to IPAInterface, but we need unique pointer to IPA{pipeline}Interface.
I mean, can't you use a static_cast ?
> > > + proxy_.setIPA(ipa_.get());
> > > +
> > > +{% for method in interface_cb.methods %}
> > > + ipa_->{{method.mojom_name}}.connect(this, &{{proxy_name}}::{{method.mojom_name}}Thread);
> > > +{%- endfor %}
> > > +
> > > + valid_ = true;
> > > +}
> > > +
> > > +{{proxy_name}}::~{{proxy_name}}()
> > > +{
> > > + if (isolate_)
> > > + ipc_->sendAsync(static_cast<uint32_t>({{cmd_enum_name}}::Exit), {}, {});
> > > +}
> > > +
> > > +{% if interface_cb.methods|length > 0 %}
> > > +void {{proxy_name}}::recvIPC(std::vector<uint8_t> &data, std::vector<int32_t> &fds)
> >
> > How about calling this receiveMessage() ?
>
> Yeah. Is recvMessage fine?
We usually try to avoid abbreviations when possible, but if
receiveMessage() is too long, exceptions are allowed.
> > > +{
> > > + if (data.size() < 8) {
> > > + LOG(IPAProxy, Error)
> > > + << "Didn't receive enough bytes to parse event";
> > > + return;
> > > + }
> > > +
> > > + {{cmd_event_enum_name}} cmd = static_cast<{{cmd_event_enum_name}}>((
> > > + data[0]) | (data[1] << 8) | (data[2] << 16) | (data[3] << 24));
> > > +
> > > + /* Need to skip another 4 bytes for the sequence number. */
> >
> > Is the protocol documented somewhere ?
>
> No...
I think this should be fixed, but I also wonder if we shouldn't use the
same protocol as mojo. Documentation would mostly be free then :-) It's
something that can be done on top, can you record it in a \todo ?
> > There's a bit of an imbalance in the API, with recvIPC() having to deal
> > with unpacking the command and skipping the sequence number, and
> > sendSync() and sendAsync() dealing with those internally. Could
> > recvIPC() be called with the command and two spans instead ?
>
> Oh shoot, yeah that is imbalanced.
>
> > > + std::vector<uint8_t>::iterator vec = data.begin() + 8;
> > > + size_t dataSize = data.size() - 8;
> > > + switch (cmd) {
> > > +{%- for method in interface_cb.methods %}
> > > + case {{cmd_event_enum_name}}::{{method.mojom_name|cap}}: {
> > > + {{method.mojom_name}}IPC(vec, dataSize, fds);
> > > + break;
> > > + }
> > > +{%- endfor %}
> > > + default:
> > > + LOG(IPAProxy, Error) << "Unknown command " << static_cast<uint32_t>(cmd);
> > > + }
> > > +}
> > > +{%- endif %}
> > > +
> > > +{% for method in interface_main.methods %}
> > > +{{proxy_funcs.func_sig(proxy_name, method)}}
> > > +{
> > > + if (isolate_)
> > > + {{"return " if method|method_return_value != "void"}}{{method.mojom_name}}IPC(
> > > +{%- for param in method|method_param_names -%}
> > > + {{param}}{{- ", " if not loop.last}}
> > > +{%- endfor -%}
> > > +);
> > > + else
> > > + {{"return " if method|method_return_value != "void"}}{{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")}}
> > > +{
> > > +{%- if method.mojom_name == "init" %}
> > > + {{proxy_funcs.init_thread_body()}}
> > > +{%- elif method.mojom_name == "start" %}
> > > + {{proxy_funcs.start_thread_body()}}
> > > +{%- elif method.mojom_name == "stop" %}
> > > + {{proxy_funcs.stop_thread_body()}}
> > > +{%- elif not method|is_async %}
> > > + ipa_->{{method.mojom_name}}(
> > > + {%- for param in method|method_param_names -%}
> > > + {{param}}{{- ", " if not loop.last}}
> > > + {%- endfor -%}
> > > +);
> > > +{% elif method|is_async %}
> > > + proxy_.invokeMethod(&ThreadProxy::{{method.mojom_name}}, ConnectionTypeQueued,
> > > + {%- for param in method|method_param_names -%}
> > > + {{param}}{{- ", " if not loop.last}}
> > > + {%- endfor -%}
> > > +);
> > > +{%- endif %}
> > > +}
> > > +
> > > +{{proxy_funcs.func_sig(proxy_name, method, "IPC")}}
> > > +{
> > > +{%- set has_input = true if method|method_param_inputs|length > 0 %}
> > > +{%- set has_output = true if method|method_param_outputs|length > 0 or method|method_return_value != "void" %}
> > > +{%- if has_input %}
> > > + std::vector<uint8_t> _ipcInputBuf;
> > > +{%- if method|method_input_has_fd %}
> > > + std::vector<int32_t> _ipcInputFds;
> > > +{%- endif %}
> > > +{%- endif %}
> > > +{%- if has_output %}
> > > + std::vector<uint8_t> _ipcOutputBuf;
> > > +{%- if method|method_output_has_fd %}
> > > + std::vector<int32_t> _ipcOutputFds;
> > > +{%- endif %}
> > > +{%- endif %}
> > > +
> > > +{{proxy_funcs.serialize_call(method|method_param_inputs, '_ipcInputBuf', '_ipcInputFds', base_controls)}}
> > > +
> > > +{%- set input_buf = "_ipcInputBuf" if has_input else "{}" %}
> > > +{%- set fds_buf = "_ipcInputFds" if method|method_input_has_fd else "{}" %}
> > > +{%- set cmd = cmd_enum_name + "::" + method.mojom_name|cap %}
> > > +{% if method|is_async %}
> > > + int ret = ipc_->sendAsync(static_cast<uint32_t>({{cmd}}), {{input_buf}}, {{fds_buf}});
> > > +{%- else %}
> > > + int ret = ipc_->sendSync(static_cast<uint32_t>({{cmd}}), {{input_buf}}, {{fds_buf}}
> > > +{{- ", &_ipcOutputBuf" if has_output -}}
> > > +{{- ", &_ipcOutputFds" if has_output and method|method_output_has_fd -}}
> > > +);
> > > +{%- endif %}
> > > + if (ret < 0) {
> > > + LOG(IPAProxy, Error) << "Failed to call {{method.mojom_name}}";
> > > +{%- if method|method_return_value != "void" %}
> > > + return static_cast<{{method|method_return_value}}>(ret);
> > > +{%- else %}
> > > + return;
> > > +{%- endif %}
> > > + }
> > > +{% if method|method_return_value != "void" %}
> > > + return IPADataSerializer<{{method.response_parameters|first|name}}>::deserialize(_ipcOutputBuf, 0);
> > > +{% elif method|method_param_outputs|length > 0 %}
> > > +{{proxy_funcs.deserialize_call(method|method_param_outputs, '_ipcOutputBuf', '_ipcOutputFds')}}
> > > +{% endif -%}
> > > +}
> > > +
> > > +{% endfor %}
> > > +
> > > +{% for method in interface_cb.methods %}
> > > +{{proxy_funcs.func_sig(proxy_name, method, "Thread")}}
> > > +{
> > > + {{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});
> > > +}
> > > +
> > > +void {{proxy_name}}::{{method.mojom_name}}IPC(
> > > + std::vector<uint8_t>::iterator data,
> > > + size_t dataSize,
> > > + [[maybe_unused]] std::vector<int32_t> &fds)
> > > +{
> > > +{%- for param in method.parameters %}
> > > + {{param|name}} {{param.mojom_name}};
> > > +{%- endfor %}
> > > +{{proxy_funcs.deserialize_call(method.parameters, 'data', 'fds', false, false, true, 'dataSize')}}
> > > + {{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});
> > > +}
> > > +{% endfor %}
> > > +
> > > +{%- if has_namespace %}
> > > +{% for ns in namespace|reverse %}
> > > +} /* {{ns}} */
> > > +{% endfor %}
> > > +{%- endif %}
> > > +} /* namespace libcamera */
> > > diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
> > > new file mode 100644
> > > index 00000000..3fb7192f
> > > --- /dev/null
> > > +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
> > > @@ -0,0 +1,118 @@
> > > +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}
> > > +{%- import "proxy_functions.tmpl" as proxy_funcs -%}
> > > +
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) {{year}}, Google Inc.
> > > + *
> > > + * ipa_proxy_{{module_name}}.h - Image Processing Algorithm proxy for {{module_name}}
> > > + *
> > > + * This file is auto-generated. Do not edit.
> > > + */
> > > +
> > > +#ifndef __LIBCAMERA_INTERNAL_IPA_PROXY_{{module_name|upper}}_H__
> > > +#define __LIBCAMERA_INTERNAL_IPA_PROXY_{{module_name|upper}}_H__
> > > +
> > > +#include <libcamera/ipa/ipa_interface.h>
> > > +#include <libcamera/ipa/{{module_name}}.h>
> > > +#include <libcamera/ipa/{{module_name}}_ipa_interface.h>
> > > +
> > > +#include "libcamera/internal/control_serializer.h"
> > > +#include "libcamera/internal/ipa_ipc.h"
> > > +#include "libcamera/internal/ipa_ipc_unixsocket.h"
> > > +#include "libcamera/internal/ipa_proxy.h"
> > > +#include "libcamera/internal/ipc_unixsocket.h"
> > > +#include "libcamera/internal/thread.h"
> > > +
> > > +namespace libcamera {
> > > +{%- if has_namespace %}
> > > +{% for ns in namespace %}
> > > +namespace {{ns}} {
> > > +{% endfor %}
> > > +{%- endif %}
> > > +
> > > +class {{proxy_name}} : public IPAProxy, public {{interface_name}}, public Object
> > > +{
> > > +public:
> > > + {{proxy_name}}(IPAModule *ipam, bool isolate);
> > > + ~{{proxy_name}}();
> > > +
> > > +{% for method in interface_main.methods %}
> > > +{{proxy_funcs.func_sig(proxy_name, method, "", false, true)|indent(8, true)}};
> > > +{% endfor %}
> > > +
> > > +{%- for method in interface_cb.methods %}
> > > + Signal<
> > > +{%- for param in method.parameters -%}
> > > + {{"const " if not param|is_pod}}{{param|name}}{{" &" if not param|is_pod}}
> > > + {{- ", " if not loop.last}}
> > > +{%- endfor -%}
> > > +> {{method.mojom_name}};
> > > +{% endfor %}
> > > +
> > > +private:
> > > + void recvIPC(std::vector<uint8_t> &data, std::vector<int32_t> &fds);
> >
> > data and fds shouldn't be modified, let's make them const. I'd even go
> > one step further, and make them Span<const uint8_t> and Span<int32_t>.
>
> I'll make them const for now.
>
> > > +
> > > +{% 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)}};
> > > +{% endfor %}
> > > +{% for method in interface_cb.methods %}
> > > +{{proxy_funcs.func_sig(proxy_name, method, "Thread", false)|indent(8, true)}};
> > > + void {{method.mojom_name}}IPC(
> > > + std::vector<uint8_t>::iterator data,
> > > + size_t dataSize,
> > > + std::vector<int32_t> &fds);
> > > +{% endfor %}
> > > +
> > > + /* Helper class to invoke async functions in another thread. */
> > > + class ThreadProxy : public Object
> > > + {
> > > + public:
> > > + void setIPA({{interface_name}} *ipa)
> > > + {
> > > + ipa_ = ipa;
> > > + }
> > > +
> > > + int start()
> > > + {
> > > + return ipa_->start();
> > > + }
> > > +
> > > + void stop()
> > > + {
> > > + ipa_->stop();
> > > + }
> > > +{% for method in interface_main.methods %}
> > > +{%- if method|is_async %}
> > > + {{proxy_funcs.func_sig(proxy_name, method, "", false)|indent(16)}}
> > > + {
> > > + ipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}});
> > > + }
> > > +{%- endif %}
> > > +{%- endfor %}
> > > +
> > > + private:
> > > + {{interface_name}} *ipa_;
> > > + };
> > > +
> > > + bool running_;
> > > + Thread thread_;
> > > + ThreadProxy proxy_;
> > > + std::unique_ptr<{{interface_name}}> ipa_;
> > > +
> > > + const bool isolate_;
> > > +
> > > + std::unique_ptr<IPAIPCUnixSocket> ipc_;
> > > +
> > > + ControlSerializer controlSerializer_;
> >
> > I wonder if it would make sense to move some of the fields to the
> > IPAProxy class (and possible some of the functions too). That can be
> > done on top in a separate series.
>
> Yeah, probably.
>
> > > +};
> > > +
> > > +{%- if has_namespace %}
> > > +{% for ns in namespace|reverse %}
> > > +} /* {{ns}} */
> > > +{% endfor %}
> > > +{%- endif %}
> > > +} /* namespace libcamera */
> > > +
> > > +#endif /* __LIBCAMERA_INTERNAL_IPA_PROXY_{{module_name|upper}}_H__ */
> > > diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
> > > new file mode 100644
> > > index 00000000..dca4f99d
> > > --- /dev/null
> > > +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
> > > @@ -0,0 +1,187 @@
> > > +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}
> > > +{%- import "proxy_functions.tmpl" as proxy_funcs -%}
> > > +
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) {{year}}, Google Inc.
> > > + *
> > > + * ipa_proxy_{{module_name}}_worker.cpp - Image Processing Algorithm proxy worker for {{module_name}}
> > > + *
> > > + * This file is auto-generated. Do not edit.
> > > + */
> > > +
> > > +#include <algorithm>
> > > +#include <iostream>
> > > +#include <sys/types.h>
> > > +#include <tuple>
> > > +#include <unistd.h>
> > > +
> > > +#include <libcamera/event_dispatcher.h>
> > > +#include <libcamera/ipa/ipa_interface.h>
> > > +#include <libcamera/ipa/{{module_name}}_ipa_interface.h>
> > > +#include <libcamera/ipa/{{module_name}}_ipa_serializer.h>
> > > +#include <libcamera/logging.h>
> > > +
> > > +#include "libcamera/internal/camera_sensor.h"
> > > +#include "libcamera/internal/control_serializer.h"
> > > +#include "libcamera/internal/ipa_data_serializer.h"
> > > +#include "libcamera/internal/ipa_ipc_unixsocket.h"
> > > +#include "libcamera/internal/ipa_module.h"
> > > +#include "libcamera/internal/ipa_proxy.h"
> > > +#include "libcamera/internal/ipc_unixsocket.h"
> > > +#include "libcamera/internal/log.h"
> > > +#include "libcamera/internal/thread.h"
> > > +
> > > +using namespace libcamera;
> > > +
> > > +LOG_DEFINE_CATEGORY({{proxy_name}}Worker)
> > > +
> > > +{%- if has_namespace %}
> > > +{% for ns in namespace -%}
> > > +using namespace {{ns}};
> > > +{% endfor %}
> > > +{%- endif %}
> > > +
> > > +struct CallData {
> > > + IPCUnixSocket::Payload *response;
> > > + bool done;
> > > +};
> >
> > This doesn't seem to be used.
>
> Ah indeed it's not.
>
> > > +
> > > +{{interface_name}} *ipa_;
> > > +IPCUnixSocket socket_;
> > > +
> > > +ControlSerializer controlSerializer_;
> > > +
> > > +bool exit_ = false;
> >
> > I still think it would be nicer to move the data and functions to a
> > class, with a single instance created in the main() function.
>
> Hm, okay.
>
> > > +
> > > +void readyRead(IPCUnixSocket *socket)
> > > +{
> > > + IPCUnixSocket::Payload _message, _response;
> > > + int _retRecv = socket->receive(&_message);
> > > + if (_retRecv) {
> > > + LOG({{proxy_name}}Worker, Error)
> > > + << "Receive message failed" << _retRecv;
> > > + return;
> > > + }
> > > +
> > > + uint32_t _cmdUint, _seq;
> > > + std::tie(_cmdUint, _seq) = IPAIPCUnixSocket::readHeader(_message);
> >
> > I think there's some room to refactor the IPC-related classes. The
> > payload and header should probably not be tied to the IPAIPCUnixSocket
> > class for instance, as they should be independent of the underlying
> > mechanism. I'll try to comment more on this topic when reviewing the
> > corresponding patches.
> >
> > > + IPAIPCUnixSocket::eraseHeader(_message);
> > > +
> > > + {{cmd_enum_name}} _cmd = static_cast<{{cmd_enum_name}}>(_cmdUint);
> > > +
> > > + switch (_cmd) {
> > > + case {{cmd_enum_name}}::Exit: {
> > > + exit_ = true;
> > > + break;
> > > + }
> > > +
> > > +{% for method in interface_main.methods %}
> > > + case {{cmd_enum_name}}::{{method.mojom_name|cap}}: {
> > > + {{proxy_funcs.deserialize_call(method|method_param_inputs, '_message.data', '_message.fds', false, true)|indent(8, true)}}
> > > +{% for param in method|method_param_outputs %}
> > > + {{param|name}} {{param.mojom_name}};
> > > +{% endfor %}
> > > +{%- if method|method_return_value != "void" %}
> > > + {{method|method_return_value}} _callRet = ipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}});
> > > +{%- else %}
> > > + ipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}}
> > > +{{- ", " if method|method_param_outputs|params_comma_sep -}}
> > > +{%- for param in method|method_param_outputs -%}
> > > +&{{param.mojom_name}}{{", " if not loop.last}}
> > > +{%- endfor -%}
> > > +);
> > > +{%- endif %}
> > > +{% if not method|is_async %}
> > > + IPAIPCUnixSocket::writeHeader(_response, _cmdUint, _seq);
> > > +{%- if method|method_return_value != "void" %}
> > > + std::vector<uint8_t> _callRetBuf;
> > > + std::tie(_callRetBuf, std::ignore) =
> > > + IPADataSerializer<{{method|method_return_value}}>::serialize(_callRet);
> > > + _response.data.insert(_response.data.end(), _callRetBuf.begin(), _callRetBuf.end());
> > > +{%- else %}
> > > + {{proxy_funcs.serialize_call(method|method_param_outputs, "_response.data", "_response.fds", base_controls)|indent(8, true)}}
> > > +{%- endif %}
> > > + int _ret = socket_.send(_response);
> > > + if (_ret < 0) {
> > > + LOG({{proxy_name}}Worker, Error)
> > > + << "Reply to {{method.mojom_name}}() failed" << _ret;
> > > + }
> > > + LOG({{proxy_name}}Worker, Debug) << "Done replying to {{method.mojom_name}}()";
> > > +{%- endif %}
> > > + break;
> > > + }
> > > +{% endfor %}
> > > + default:
> > > + LOG({{proxy_name}}Worker, Error) << "Unknown command " << _cmdUint;
> > > + }
> > > +}
> > > +
> > > +{% for method in interface_cb.methods %}
> > > +{{proxy_funcs.func_sig(proxy_name, method, "", false)}}
> > > +{
> > > + IPCUnixSocket::Payload _message;
> > > +
> > > + IPAIPCUnixSocket::writeHeader(_message, static_cast<uint32_t>({{cmd_event_enum_name}}::{{method.mojom_name|cap}}), 0);
> > > + {{proxy_funcs.serialize_call(method|method_param_inputs, "_message.data", "_message.fds", base_controls)}}
> > > +
> > > + socket_.send(_message);
> > > +
> > > + LOG({{proxy_name}}Worker, Debug) << "{{method.mojom_name}} done";
> > > +}
> > > +{% endfor %}
> > > +
> > > +int main(int argc, char **argv)
> > > +{
> > > + /* Uncomment this for debugging. */
> > > +#if 0
> > > + std::string logPath = "/tmp/libcamera.worker." +
> > > + std::to_string(getpid()) + ".log";
> > > + logSetFile(logPath.c_str());
> > > +#endif
> >
> > Likely something we'll have to handle more dynamically, but it doesn't
> > have to be done now. A \todo comment would be good though.
>
> ack
>
> > > +
> > > + if (argc < 3) {
> > > + LOG({{proxy_name}}Worker, Error)
> > > + << "Tried to start worker with no args";
> > > + return EXIT_FAILURE;
> > > + }
> > > +
> > > + int fd = std::stoi(argv[2]);
> > > + LOG({{proxy_name}}Worker, Info)
> > > + << "Starting worker for IPA module " << argv[1]
> > > + << " with IPC fd = " << fd;
> > > +
> > > + std::unique_ptr<IPAModule> ipam = std::make_unique<IPAModule>(argv[1]);
> > > + if (!ipam->isValid() || !ipam->load()) {
> > > + LOG({{proxy_name}}Worker, Error)
> > > + << "IPAModule " << argv[1] << " isn't valid";
> > > + return EXIT_FAILURE;
> > > + }
> > > +
> > > + if (socket_.bind(fd) < 0) {
> > > + LOG({{proxy_name}}Worker, Error) << "IPC socket binding failed";
> > > + return EXIT_FAILURE;
> > > + }
> > > + socket_.readyRead.connect(&readyRead);
> > > +
> > > + ipa_ = dynamic_cast<{{interface_name}} *>(ipam->createInterface());
> > > + if (!ipa_) {
> > > + LOG({{proxy_name}}Worker, Error) << "Failed to create IPA interface instance";
> > > + return EXIT_FAILURE;
> > > + }
> > > +{% for method in interface_cb.methods %}
> > > + ipa_->{{method.mojom_name}}.connect(&{{method.mojom_name}});
> > > +{%- endfor %}
> > > +
> > > + LOG({{proxy_name}}Worker, Debug) << "Proxy worker successfully started";
> > > +
> > > + /* \todo upgrade listening loop */
> >
> > Upgrade to what ? :-)
>
> I copied this from the old IPAProxyLinuxWorker :)
Maybe it's not needed anymore, if nobody remembers what it is ? :-)
> > > + EventDispatcher *dispatcher = Thread::current()->eventDispatcher();
> > > + while (!exit_)
> > > + dispatcher->processEvents();
> > > +
> > > + delete ipa_;
> > > + socket_.close();
> > > +
> > > + return 0;
> > > +}
> > > diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_serializer.h.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_serializer.h.tmpl
> > > new file mode 100644
> > > index 00000000..675f9adf
> > > --- /dev/null
> > > +++ b/utils/ipc/generators/libcamera_templates/module_ipa_serializer.h.tmpl
> > > @@ -0,0 +1,44 @@
> > > +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}
> > > +{%- import "serializer.tmpl" as serializer -%}
> > > +
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) {{year}}, Google Inc.
> > > + *
> > > + * {{module_name}}_serializer.h - Image Processing Algorithm data serializer for {{module_name}}
> > > + *
> > > + * This file is auto-generated. Do not edit.
> > > + */
> > > +
> > > +#ifndef __LIBCAMERA_INTERNAL_IPA_DATA_SERIALIZER_{{module_name|upper}}_H__
> > > +#define __LIBCAMERA_INTERNAL_IPA_DATA_SERIALIZER_{{module_name|upper}}_H__
> > > +
> > > +#include <libcamera/ipa/{{module_name}}.h>
> > > +#include <libcamera/ipa/{{module_name}}_ipa_interface.h>
> > > +
> > > +#include "libcamera/internal/control_serializer.h"
> > > +#include "libcamera/internal/ipa_data_serializer.h"
> > > +
> > > +#include <tuple>
> > > +#include <vector>
> >
> > These should go first.
> >
> > > +
> > > +namespace libcamera {
> > > +
> > > +LOG_DECLARE_CATEGORY(IPADataSerializer)
> > > +{% for struct in structs_nonempty %}
> > > +template<>
> > > +class IPADataSerializer<{{struct|name_full(namespace_str)}}>
> > > +{
> > > +public:
> > > +{{- serializer.serializer(struct, base_controls, namespace_str)}}
> > > +{%- if struct|has_fd %}
> > > +{{serializer.deserializer_fd(struct, namespace_str)}}
> > > +{%- else %}
> > > +{{serializer.deserializer_no_fd(struct, namespace_str)}}
> > > +{%- endif %}
> > > +};
> > > +{% endfor %}
> > > +
> > > +} /* namespace libcamera */
> > > +
> > > +#endif /* __LIBCAMERA_INTERNAL_IPA_DATA_SERIALIZER_{{module_name|upper}}_H__ */
> > > diff --git a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> > > new file mode 100644
> > > index 00000000..f6836034
> > > --- /dev/null
> > > +++ b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> > > @@ -0,0 +1,205 @@
> > > +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}
> > > +{#
> > > + # \brief Generate fuction prototype
> > > + #
> > > + # \param class Class name
> > > + # \param method mojom Method object
> > > + # \param suffix Suffix to append to \a method function name
> > > + # \param need_class_name True to generate class name with function
> > > + # \param override True to generate override tag after the function prototype
> > > + #}
> > > +{%- macro func_sig(class, method, suffix, need_class_name = true, override = false) -%}
> >
> > There's one occurrence of a call to this macro with two arguments only.
> > Should suffix have a default of "" ?
>
> That's the default default value of macro parameters in jinja, that's why
> I left it blank.
I thought it would be something like that. Maybe making it explicit
would be more readable, or maybe that's just because I don't have
experience with jinja. Up to you.
> > > +{{method|method_return_value}} {{class + "::" if need_class_name}}{{method.mojom_name}}{{suffix}}(
> > > +{%- for param in method|method_parameters %}
> > > + {{param}}{{- "," if not loop.last}}
> > > +{%- endfor -%}
> > > +){{" override" if override}}
> > > +{%- endmacro -%}
> > > +
> > > +{#
> > > + # \brief Generate function body for IPA init() function for thread
> > > + #}
> > > +{%- macro init_thread_body() -%}
> > > + int ret = ipa_->init(settings);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + proxy_.moveToThread(&thread_);
> > > +
> > > + return 0;
> > > +{%- endmacro -%}
> > > +
> > > +{#
> > > + # \brief Generate function body for IPA start() function for thread
> > > + #}
> > > +{%- macro start_thread_body() -%}
> > > + running_ = true;
> > > + thread_.start();
> > > +
> > > + return proxy_.invokeMethod(&ThreadProxy::start, ConnectionTypeBlocking);
> > > +{%- endmacro -%}
> > > +
> > > +{#
> > > + # \brief Generate function body for IPA stop() function for thread
> > > + #}
> > > +{%- macro stop_thread_body() -%}
> > > + if (!running_)
> > > + return;
> > > +
> > > + running_ = false;
> > > +
> > > + proxy_.invokeMethod(&ThreadProxy::stop, ConnectionTypeBlocking);
> > > +
> > > + thread_.exit();
> > > + thread_.wait();
> > > +{%- endmacro -%}
> > > +
> > > +
> > > +{#
> > > + # \brief Serialize multiple objects into data buffer and fd vector
> > > + #
> > > + # Generate code to serialize multiple objects, as specified in \a params
> > > + # (which are the parameters to some function), into \a buf data buffer and
> > > + # \a fds fd vector.
> > > + # This code is meant to be used by the proxy, for serializing prior to IPC calls.
> > > + #}
> > > +{%- macro serialize_call(params, buf, fds, base_controls) %}
> > > +{%- for param in params %}
> > > + std::vector<uint8_t> {{param.mojom_name}}Buf;
> > > +{%- if param|has_fd %}
> > > + std::vector<int32_t> {{param.mojom_name}}Fds;
> > > + std::tie({{param.mojom_name}}Buf, {{param.mojom_name}}Fds) =
> > > +{%- else %}
> > > + std::tie({{param.mojom_name}}Buf, std::ignore) =
> > > +{%- endif %}
> > > +{%- if param|is_controls %}
> > > + IPADataSerializer<{{param|name}}>::serialize({{param.mojom_name}}, {{param.mojom_name}}.infoMap() ? *{{param.mojom_name}}.infoMap() : {{base_controls}}
> > > +{%- else %}
> > > + IPADataSerializer<{{param|name}}>::serialize({{param.mojom_name}}
> > > +{%- endif %}
> > > +{{- ", &controlSerializer_" if param|needs_control_serializer -}}
> > > +);
> > > +{%- endfor %}
> > > +
> > > +{%- if params|length > 1 %}
> > > +{%- for param in params %}
> > > + appendPOD<uint32_t>({{buf}}, {{param.mojom_name}}Buf.size());
> > > +{%- if param|has_fd %}
> > > + appendPOD<uint32_t>({{buf}}, {{param.mojom_name}}Fds.size());
> > > +{%- endif %}
> > > +{%- endfor %}
> > > +{%- endif %}
> > > +
> > > +{%- for param in params %}
> > > + {{buf}}.insert({{buf}}.end(), {{param.mojom_name}}Buf.begin(), {{param.mojom_name}}Buf.end());
> > > +{%- endfor %}
> > > +
> > > +{%- for param in params %}
> > > +{%- if param|has_fd %}
> > > + {{fds}}.insert({{fds}}.end(), {{param.mojom_name}}Fds.begin(), {{param.mojom_name}}Fds.end());
> > > +{%- endif %}
> > > +{%- endfor %}
> > > +{%- endmacro -%}
> >
> > I wonder if we could optimize this by avoiding the intermediate vectors,
> > but that's a candidate for a later optimization.
>
> I'll add it to the todo list.
>
> > > +
> > > +
> > > +{#
> > > + # \brief Deserialize a single object from data buffer and fd vector
> > > + #
> > > + # \param pointer True deserializes the object into a dereferenced pointer
> >
> > s/True deserializes/If true, deserialize/ ?
> >
> > Same in other locations below.
>
> Yes.
>
> > > + # \param iter True treats \a buf as an iterator instead of a vector
> > > + # \param data_size Variable that holds the size of the vector referenced by \a buf
> > > + #
> > > + # Generate code to deserialize a single object, as specified in \a param,
> > > + # from \a buf data buffer and \a fds fd vector.
> > > + # This code is meant to be used by macro deserialize_call.
> > > + #}
> > > +{%- macro deserialize_param(param, pointer, loop, buf, fds, iter, data_size) -%}
> > > +{{"*" if pointer}}{{param.mojom_name}} = IPADataSerializer<{{param|name}}>::deserialize(
> > > +{%- if not iter %}
> > > + {{buf}}.begin() + {{param.mojom_name}}Start,
> > > +{%- else %}
> > > + {{buf}} + {{param.mojom_name}}Start,
> > > +{%- endif %}
> >
> > Maybe
> >
> > {{buf}}{{- ".begin()" if not iter}} + {{param.mojom_name}}Start,
> >
> > ?
>
> Yeah that's better.
>
> > > +{%- if loop.last and not iter %}
> > > + {{buf}}.end()
> > > +{%- elif not iter %}
> > > + {{buf}}.begin() + {{param.mojom_name}}Start + {{param.mojom_name}}BufSize
> > > +{%- elif iter and loop.length == 1 %}
> > > + {{buf}} + {{data_size}}
> > > +{%- else %}
> > > + {{buf}} + {{param.mojom_name}}Start + {{param.mojom_name}}BufSize
> > > +{%- endif -%}
> >
> > I wonder if it would simplify the generated code if we standardized on
> > Span<uint8_t>. Passing both the vector iterator and the size in separate
> > arguments can more easily result in them becoming out of sync. I was
> > thinking the Span<>::subspan() function could be quite helpful here, but
> > it doesn't handle overflows (requesting a subspan with offset > size is
> > ill-defined) :-S
> >
> > Not necessarily something to be addressed now, but do you think it would
> > make sense ? And of course if it helps fixing some of the issues
> > outlined elsewhere in this review, we could already switch to span.
>
> It probably would make sense... but it'll be a pain to convert :/
Let's record it in a \todo then.
> > > +{{- "," if param|has_fd}}
> > > +{%- if param|has_fd %}
> > > + {{fds}}.begin() + {{param.mojom_name}}FdStart,
> > > +{%- if loop.last %}
> > > + {{fds}}.end()
> > > +{%- else %}
> > > + {{fds}}.begin() + {{param.mojom_name}}FdStart + {{param.mojom_name}}FdsSize
> > > +{%- endif -%}
> > > +{%- endif -%}
> > > +{{- "," if param|needs_control_serializer}}
> > > +{%- if param|needs_control_serializer %}
> > > + &controlSerializer_
> > > +{%- endif -%}
> > > +);
> > > +{%- endmacro -%}
> > > +
> > > +
> > > +{#
> > > + # \brief Deserialize multiple objects from data buffer and fd vector
> > > + #
> > > + # \param pointer True deserializes objects into pointers, and adds a null check.
> > > + # \param declare True declares the objects in addition to deserialization.
> > > + # \param iter True treats \a buf as an iterator instead of a vector
> > > + # \param data_size Variable that holds the size of the vector referenced by \a buf
> > > + #
> > > + # Generate code to deserialize multiple objects, as specified in \a params
> > > + # (which are the parameters to some function), from \a buf data buffer and
> > > + # \a fds fd vector.
> > > + # This code is meant to be used by the proxy, for deserializing after IPC calls.
> > > + #}
> > > +{%- macro deserialize_call(params, buf, fds, pointer = true, declare = false, iter = false, data_size = '') -%}
> > > +{% set ns = namespace(size_offset = 0) %}
> > > +{%- if params|length > 1 %}
> > > +{%- for param in params %}
> > > + [[maybe_unused]] size_t {{param.mojom_name}}BufSize = readPOD<uint32_t>({{buf}}, {{ns.size_offset}}
> >
> > const ?
> >
> > > +{%- if iter -%}
> > > +, {{buf}} + {{data_size}}
> > > +{%- endif -%}
> > > +);
> > > + {%- set ns.size_offset = ns.size_offset + 4 %}
> > > +{%- if param|has_fd %}
> > > + [[maybe_unused]] size_t {{param.mojom_name}}FdsSize = readPOD<uint32_t>({{buf}}, {{ns.size_offset}}
> >
> > const ?
> >
> > > +{%- if iter -%}
> > > +, {{buf}} + {{data_size}}
> > > +{%- endif -%}
> > > +);
> > > + {%- set ns.size_offset = ns.size_offset + 4 %}
> > > +{%- endif %}
> > > +{%- endfor %}
> > > +{%- endif %}
> > > +{% for param in params %}
> > > +{%- if loop.first %}
> > > + size_t {{param.mojom_name}}Start = {{ns.size_offset}};
> >
> > const ?
> >
> > Same in a few locations below.
>
> Yes.
>
> > > +{%- else %}
> > > + size_t {{param.mojom_name}}Start = {{loop.previtem.mojom_name}}Start + {{loop.previtem.mojom_name}}BufSize;
> > > +{%- endif %}
> > > +{%- endfor %}
> > > +{% for param in params|with_fds %}
> > > +{%- if loop.first %}
> > > + size_t {{param.mojom_name}}FdStart = 0;
> > > +{%- elif not loop.last %}
> > > + size_t {{param.mojom_name}}FdStart = {{loop.previtem.mojom_name}}FdStart + {{loop.previtem.mojom_name}}FdsSize;
> > > +{%- endif %}
> > > +{%- endfor %}
> > > +{% for param in params %}
> > > + {%- if pointer %}
> > > + if ({{param.mojom_name}}) {
> > > +{{deserialize_param(param, pointer, loop, buf, fds, iter, data_size)|indent(16, True)}}
> > > + }
> > > + {%- else %}
> > > + {{param|name + " " if declare}}{{deserialize_param(param, pointer, loop, buf, fds, iter, data_size)|indent(8)}}
> > > + {%- endif %}
> > > +{% endfor %}
> > > +{%- endmacro -%}
> > > diff --git a/utils/ipc/generators/libcamera_templates/serializer.tmpl b/utils/ipc/generators/libcamera_templates/serializer.tmpl
> > > new file mode 100644
> > > index 00000000..51dbeb0e
> > > --- /dev/null
> > > +++ b/utils/ipc/generators/libcamera_templates/serializer.tmpl
> > > @@ -0,0 +1,280 @@
> > > +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}
> > > +{# Turn this into a C macro? #}
> > > +{#
> > > + # \brief Verify that there is enough bytes to deserialize
> > > + #
> > > + # Generate code that verifies that \a size is not greater than \a dataSize.
> > > + # Otherwise log an error with \a name and \a typename.
> > > + #}
> > > +{%- macro check_data_size(size, dataSize, name, typename) %}
> > > + if ({{size}} > {{dataSize}}) {
> >
> >
> > Maybe
> > if ({{dataSize}} < {{size}}) {
> >
> > ?
> >
> > > + LOG(IPADataSerializer, Error)
> > > + << "Failed to deserialize {{name}}: not enough {{typename}}, expected "
> >
> > << "Failed to deserialize " << "{{name}}"
> > << ": not enough {{typename}}, expected "
> >
> > This should allow the compiler to deduplicate strings.
> >
> > > + << ({{size}}) << ", got " << ({{dataSize}});
> > > + return ret;
> > > + }
> > > +{%- endmacro %}
> > > +
> > > +
> > > +{#
> > > + # \brief Serialize some field into return vector
> > > + #
> > > + # Generate code to serialize \a field into retData, including size of the
> > > + # field and fds (where appropriate). \a base_controls indicates the
> > > + # default ControlInfoMap in the event that the ControlList does not have one.
> > > + # This code is meant to be used by the IPADataSerializer specialization.
> > > + #}
> > > +{%- macro serializer_field(field, base_controls, namespace, loop) %}
> > > +{%- if field|is_pod or field|is_enum %}
> > > + std::vector<uint8_t> {{field.mojom_name}};
> > > + std::tie({{field.mojom_name}}, std::ignore) =
> > > + {%- if field|is_pod %}
> > > + IPADataSerializer<{{field|name}}>::serialize(data.{{field.mojom_name}}_);
> > > + {%- elif field|is_enum %}
> > > + IPADataSerializer<uint{{field|bit_width}}_t>::serialize(data.{{field.mojom_name}}_);
> > > + {%- endif %}
> > > + retData.insert(retData.end(), {{field.mojom_name}}.begin(), {{field.mojom_name}}.end());
> > > +{%- elif field|is_fd %}
> > > + std::vector<uint8_t> {{field.mojom_name}};
> > > + std::vector<int32_t> {{field.mojom_name}}Fds;
> > > + std::tie({{field.mojom_name}}, {{field.mojom_name}}Fds) =
> > > + IPADataSerializer<{{field|name}}>::serialize(data.{{field.mojom_name}}_);
> > > + retData.insert(retData.end(), {{field.mojom_name}}.begin(), {{field.mojom_name}}.end());
> > > + retFds.insert(retFds.end(), {{field.mojom_name}}Fds.begin(), {{field.mojom_name}}Fds.end());
> > > +{%- elif field|is_controls %}
> > > + if (data.{{field.mojom_name}}_.size() > 0) {
> > > + std::vector<uint8_t> {{field.mojom_name}};
> > > + std::tie({{field.mojom_name}}, std::ignore) =
> > > + IPADataSerializer<{{field|name}}>::serialize(data.{{field.mojom_name}}_,
> > > + data.{{field.mojom_name}}_.infoMap() ? *data.{{field.mojom_name}}_.infoMap() : {{base_controls}},
> > > + cs);
> > > + appendPOD<uint32_t>(retData, {{field.mojom_name}}.size());
> > > + retData.insert(retData.end(), {{field.mojom_name}}.begin(), {{field.mojom_name}}.end());
> > > + } else {
> > > + appendPOD<uint32_t>(retData, 0);
> > > + }
> > > +{%- elif field|is_plain_struct or field|is_array or field|is_map or field|is_str %}
> > > + std::vector<uint8_t> {{field.mojom_name}};
> > > + {%- if field|has_fd %}
> > > + std::vector<int32_t> {{field.mojom_name}}Fds;
> > > + std::tie({{field.mojom_name}}, {{field.mojom_name}}Fds) =
> > > + {%- else %}
> > > + std::tie({{field.mojom_name}}, std::ignore) =
> > > + {%- endif %}
> > > + {%- if field|is_array or field|is_map %}
> > > + IPADataSerializer<{{field|name}}>::serialize(data.{{field.mojom_name}}_, cs);
> > > + {%- elif field|is_str %}
> > > + IPADataSerializer<{{field|name}}>::serialize(data.{{field.mojom_name}}_);
> > > + {%- else %}
> > > + IPADataSerializer<{{field|name_full(namespace)}}>::serialize(data.{{field.mojom_name}}_, cs);
> > > + {%- endif %}
> > > + appendPOD<uint32_t>(retData, {{field.mojom_name}}.size());
> > > + {%- if field|has_fd %}
> > > + appendPOD<uint32_t>(retData, {{field.mojom_name}}Fds.size());
> > > + {%- endif %}
> > > + retData.insert(retData.end(), {{field.mojom_name}}.begin(), {{field.mojom_name}}.end());
> > > + {%- if field|has_fd %}
> > > + retFds.insert(retFds.end(), {{field.mojom_name}}Fds.begin(), {{field.mojom_name}}Fds.end());
> > > + {%- endif %}
> > > +{%- else %}
> > > + /* Unknown serialization for {{field.mojom_name}}. */
> > > +{%- endif %}
> > > +{%- endmacro %}
> >
> > Same here, I think there's room for optimization if we could avoid the
> > intermediate vectors.
> >
> > > +
> > > +
> > > +{#
> > > + # \brief Deserialize some field into return struct
> > > + #
> > > + # Generate code to deserialize \a field into object ret.
> > > + # This code is meant to be used by the IPADataSerializer specialization.
> > > + #}
> > > +{%- macro deserializer_field(field, namespace, loop) %}
> > > +{% if field|is_pod or field|is_enum %}
> > > + {%- set field_size = (field|bit_width|int / 8)|int %}
> > > + {{- check_data_size(field_size, 'dataSize', field.mojom_name, 'data')}}
> > > + {%- if field|is_pod %}
> > > + ret.{{field.mojom_name}}_ = static_cast<{{field|name}}>(IPADataSerializer<{{field|name}}>::deserialize(m, m + {{field_size}}));
> >
> > Do we need the static_cast<>, doesn't
> > IPADataSerializer<{{field|name}}>::deserialize() return a {{field|name}}
> > ?
>
> Ah yes, this one doesn't need the static_cast. The next one does due to
> enums though.
>
> > > + {%- else %}
> > > + ret.{{field.mojom_name}}_ = static_cast<{{field|name}}>(IPADataSerializer<uint{{field|bit_width}}_t>::deserialize(m, m + {{field_size}}));
> > > + {%- endif %}
> > > + {%- if not loop.last %}
> > > + m += {{field_size}};
> > > + dataSize -= {{field_size}};
> > > + {%- endif %}
> > > +{% elif field|is_fd %}
> > > + {%- set field_size = 1 %}
> > > + {{- check_data_size(field_size, 'dataSize', field.mojom_name, 'data')}}
> > > + ret.{{field.mojom_name}}_ = IPADataSerializer<{{field|name}}>::deserialize(m, m + 1, n, n + 1, cs);
> > > + {%- if not loop.last %}
> > > + m += {{field_size}};
> > > + dataSize -= {{field_size}};
> > > + n += ret.{{field.mojom_name}}_.isValid() ? 1 : 0;
> > > + fdsSize -= ret.{{field.mojom_name}}_.isValid() ? 1 : 0;
> > > + {%- endif %}
> > > +{% elif field|is_controls %}
> > > + {%- set field_size = 4 %}
> > > + {{- check_data_size(field_size, 'dataSize', field.mojom_name + 'Size', 'data')}}
> > > + size_t {{field.mojom_name}}Size = readPOD<uint32_t>(m, 0, data.end());
> >
> > const ?
> >
> > You call IPADataSerializer<{{field|name}}>::deserialize() above for POD
> > types, and readPOD() here. Do we need the IPADataSerializer<>
> > specializations for all the PODs, can't we call readPOD() directly ?
>
> No we can't get rid of the POD specializations of IPADataSerializer,
> because the IPADataSerializer for vectors and maps needs them. So for
> serdesing fields that are PODs we use IPADataSerializer, and for
> serdesing metadata like field size, we use readPOD.
>
> > > + {%- set field_size = '4 + ' + field.mojom_name + 'Size' -%}
> > > + {{- check_data_size(field_size, 'dataSize', field.mojom_name, 'data')}}
> > > + if ({{field.mojom_name}}Size > 0)
> > > + ret.{{field.mojom_name}}_ =
> > > + IPADataSerializer<{{field|name}}>::deserialize(m + 4, m + 4 + {{field.mojom_name}}Size, cs);
> > > + {%- if not loop.last %}
> > > + m += {{field_size}};
> > > + dataSize -= {{field_size}};
> > > + {%- endif %}
> > > +{% elif field|is_plain_struct or field|is_array or field|is_map or field|is_str %}
> > > + {%- set field_size = 4 %}
> > > + {{- check_data_size(field_size, 'dataSize', field.mojom_name + 'Size', 'data')}}
> > > + size_t {{field.mojom_name}}Size = readPOD<uint32_t>(m, 0, data.end());
> >
> > const ?
> >
> > > + {%- if field|has_fd %}
> > > + {%- set field_size = 8 %}
> > > + {{- check_data_size(field_size, 'dataSize', field.mojom_name + 'FdsSize', 'data')}}
> > > + size_t {{field.mojom_name}}FdsSize = readPOD<uint32_t>(m, 4, data.end());
> >
> > const ?
> >
> > > + {{- check_data_size(field.mojom_name + 'FdsSize', 'fdsSize', field.mojom_name, 'fds')}}
> > > + {%- endif %}
> > > + {%- set field_size = field|has_fd|choose('8 + ', '4 + ') + field.mojom_name + 'Size' -%}
> >
> > I'm afraid you've got quite a few integer overflow issues here :-S
>
> :S
>
> > > + {{- check_data_size(field_size, 'dataSize', field.mojom_name, 'data')}}
> > > + ret.{{field.mojom_name}}_ =
> > > + {%- if field|is_str %}
> > > + IPADataSerializer<{{field|name}}>::deserialize(m + 4, m + 4 + {{field.mojom_name}}Size);
> > > + {%- elif field|has_fd and (field|is_array or field|is_map) %}
> > > + IPADataSerializer<{{field|name}}>::deserialize(m + 8, m + 8 + {{field.mojom_name}}Size, n, n + {{field.mojom_name}}FdsSize, cs);
> > > + {%- elif field|has_fd and (not (field|is_array or field|is_map)) %}
> > > + IPADataSerializer<{{field|name_full(namespace)}}>::deserialize(m + 8, m + 8 + {{field.mojom_name}}Size, n, n + {{field.mojom_name}}FdsSize, cs);
> > > + {%- elif (not field|has_fd) and (field|is_array or field|is_map) %}
> > > + IPADataSerializer<{{field|name}}>::deserialize(m + 4, m + 4 + {{field.mojom_name}}Size, cs);
> > > + {%- else %}
> > > + IPADataSerializer<{{field|name_full(namespace)}}>::deserialize(m + 4, m + 4 + {{field.mojom_name}}Size, cs);
> > > + {%- endif %}
> >
> > Can't we advance m after reading the size fields ? That would avoid all
> > the error-prone calculation, and get rid of at least some of the integer
> > overflow issues.
>
> Oh, right.
>
> > > + {%- if not loop.last %}
> > > + m += {{field_size}};
> > > + dataSize -= {{field_size}};
> > > + {%- if field|has_fd %}
> > > + n += {{field.mojom_name}}FdsSize;
> > > + fdsSize -= {{field.mojom_name}}FdsSize;
> > > + {%- endif %}
> > > + {%- endif %}
> > > +{% else %}
> > > + /* Unknown deserialization for {{field.mojom_name}}. */
> > > +{%- endif %}
> > > +{%- endmacro %}
> > > +
> > > +
> > > +{#
> > > + # \brief Serialize a struct
> > > + #
> > > + # Generate code for IPADataSerializer specialization, for serializing
> > > + # \a struct. \a base_controls indicates the default ControlInfoMap
> > > + # in the event that the ControlList does not have one.
> > > + #}
> > > +{%- macro serializer(struct, base_controls, namespace) %}
> > > + static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> > > + serialize(const {{struct|name_full(namespace)}} &data,
> > > +{%- if struct|needs_control_serializer %}
> > > + ControlSerializer *cs)
> > > +{%- else %}
> > > + [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > +{%- endif %}
> > > + {
> > > + std::vector<uint8_t> retData;
> > > +{%- if struct|has_fd %}
> > > + std::vector<int32_t> retFds;
> > > +{%- endif %}
> > > +{%- for field in struct.fields %}
> > > +{{serializer_field(field, base_controls, namespace, loop)}}
> > > +{%- endfor %}
> > > +{% if struct|has_fd %}
> > > + return {retData, retFds};
> > > +{%- else %}
> > > + return {retData, {}};
> > > +{%- endif %}
> > > + }
> > > +{%- endmacro %}
> > > +
> > > +
> > > +{#
> > > + # \brief Deserialize a struct that has fds
> > > + #
> > > + # Generate code for IPADataSerializer specialization, for deserializing
> > > + # \a struct, in the case that \a struct has file descriptors.
> > > + #}
> > > +{%- macro deserializer_fd(struct, namespace) %}
> > > + static {{struct|name_full(namespace)}}
> > > + deserialize(std::vector<uint8_t> &data,
> > > + std::vector<int32_t> &fds,
> > > +{%- if struct|needs_control_serializer %}
> > > + ControlSerializer *cs)
> > > +{%- else %}
> > > + [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > +{%- endif %}
> > > + {
> > > + {{struct|name_full(namespace)}} ret;
> > > + std::vector<uint8_t>::iterator m = data.begin();
> > > + std::vector<int32_t>::iterator n = fds.begin();
> > > +
> > > + size_t dataSize = data.size();
> > > + size_t fdsSize = fds.size();
> > > +{%- for field in struct.fields -%}
> > > +{{deserializer_field(field, namespace, loop)}}
> > > +{%- endfor %}
> > > + return ret;
> > > + }
> > > +
> > > + static {{struct|name_full(namespace)}}
> > > + deserialize(std::vector<uint8_t>::iterator dataBegin,
> > > + std::vector<uint8_t>::iterator dataEnd,
> > > + std::vector<int32_t>::iterator fdsBegin,
> > > + std::vector<int32_t>::iterator fdsEnd,
> >
> > All these should const const_iterator. There are quite a few locations
> > in the deserializer where the data and fds vectors or iterators are not
> > const. Could you fix that ?
>
> Yeah, I'll fix all of that.
>
> > > +{%- if struct|needs_control_serializer %}
> > > + ControlSerializer *cs)
> > > +{%- else %}
> > > + [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > +{%- endif %}
> > > + {
> > > + std::vector<uint8_t> data(dataBegin, dataEnd);
> >
> > This creates a copy of the data, do we really need that ? I think you
> > should turn this around, implement the deserialization in this function,
> > and implement the previous function as a wrapper around this one.
>
> Ah, okay.
>
> > > + std::vector<int32_t> fds(fdsBegin, fdsEnd);
> > > + return IPADataSerializer<{{struct|name_full(namespace)}}>::deserialize(data, fds, cs);
> > > + }
> > > +{%- endmacro %}
> > > +
> > > +
> > > +{#
> > > + # \brief Deserialize a struct that has no fds
> > > + #
> > > + # Generate code for IPADataSerializer specialization, for deserializing
> > > + # \a struct, in the case that \a struct does not have file descriptors.
> > > + #}
> > > +{%- macro deserializer_no_fd(struct, namespace) %}
> > > + static {{struct|name_full(namespace)}}
> > > + deserialize(std::vector<uint8_t> &data,
> > > +{%- if struct|needs_control_serializer %}
> > > + ControlSerializer *cs)
> > > +{%- else %}
> > > + [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > +{%- endif %}
> > > + {
> > > + {{struct|name_full(namespace)}} ret;
> > > + std::vector<uint8_t>::iterator m = data.begin();
> > > +
> > > + size_t dataSize = data.size();
> > > +{%- for field in struct.fields -%}
> > > +{{deserializer_field(field, namespace, loop)}}
> > > +{%- endfor %}
> > > + return ret;
> > > + }
> > > +
> > > + static {{struct|name_full(namespace)}}
> > > + deserialize(std::vector<uint8_t>::iterator dataBegin,
> > > + std::vector<uint8_t>::iterator dataEnd,
> > > +{%- if struct|needs_control_serializer %}
> > > + ControlSerializer *cs)
> > > +{%- else %}
> > > + [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > +{%- endif %}
> > > + {
> > > + std::vector<uint8_t> data(dataBegin, dataEnd);
> > > + return IPADataSerializer<{{struct|name_full(namespace)}}>::deserialize(data, cs);
> > > + }
> > > +{%- endmacro %}
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list