[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 19 18:21:00 CET 2020
Hi Paul,
Thank you for the patch.
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.
> 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.
*/
> + *
> + * {{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 ?
> +{%- 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 ?
> + 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() ?
> +{
> + 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 ?
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 ?
> + 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>.
> +
> +{% 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.
> +};
> +
> +{%- 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.
> +
> +{{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.
> +
> +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.
> +
> + 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 ? :-)
> + 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 "" ?
> +{{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.
> +
> +
> +{#
> + # \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.
> + # \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,
?
> +{%- 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.
> +{{- "," 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.
> +{%- 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}}
?
> + {%- 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 ?
> + {%- 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
> + {{- 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.
> + {%- 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 ?
> +{%- 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.
> + 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