[RFC,v1] utils: ipc: Do not define variables in signal handler upfront

23046 diff mbox series

Defining the variables at the beginning of the function forces the types
to be default constructible, which may not be desirable; furthermore, it
also forces the move/copy assignment operator to be used when the
deserialized value is retrieved.

Having `T val = f()` has the advantage of benefitting from potential RVO
as well as not requiring `T` to be default constructible, so generate
code in that form by calling `deserialize_call()` with `declare=true`.

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 .../generators/libcamera_templates/module_ipa_proxy.cpp.tmpl | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

Hi Barnabás,

Thanks for the patch.

On Wed, Mar 26, 2025 at 11:13:24AM +0100, Barnabás Pőcze wrote:
> Defining the variables at the beginning of the function forces the types
> to be default constructible, which may not be desirable; furthermore, it
> also forces the move/copy assignment operator to be used when the
> deserialized value is retrieved.

Ah, I see. Nice catch.

> 
> Having `T val = f()` has the advantage of benefitting from potential RVO
> as well as not requiring `T` to be default constructible, so generate
> code in that form by calling `deserialize_call()` with `declare=true`.
> 
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>

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

> ---
>  .../generators/libcamera_templates/module_ipa_proxy.cpp.tmpl | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> index ce3cc5ab6..07165821e 100644
> --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> @@ -239,10 +239,7 @@ void {{proxy_name}}::{{method.mojom_name}}IPC(
>  	[[maybe_unused]] size_t dataSize,
>  	[[maybe_unused]] const std::vector<SharedFD> &fds)
>  {
> -{%- for param in method.parameters %}
> -	{{param|name}} {{param.mojom_name}};
> -{%- endfor %}
> -{{proxy_funcs.deserialize_call(method.parameters, 'data', 'fds', false, false, true, 'dataSize')}}
> +{{proxy_funcs.deserialize_call(method.parameters, 'data', 'fds', false, true, true, 'dataSize')}}
>  	{{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});
>  }
>  {% endfor %}
> -- 
> 2.49.0
>

Hi Barnabás,

Thank you for the patch.

On Wed, Mar 26, 2025 at 11:13:24AM +0100, Barnabás Pőcze wrote:
> Defining the variables at the beginning of the function forces the types
> to be default constructible, which may not be desirable; furthermore, it
> also forces the move/copy assignment operator to be used when the
> deserialized value is retrieved.
> 
> Having `T val = f()` has the advantage of benefitting from potential RVO
> as well as not requiring `T` to be default constructible, so generate
> code in that form by calling `deserialize_call()` with `declare=true`.

I looked at the diff in generated files:

diff -Nur a/src/libcamera/proxy/ipu3_ipa_proxy.cpp b/src/libcamera/proxy/ipu3_ipa_proxy.cpp
--- a/src/libcamera/proxy/ipu3_ipa_proxy.cpp	2025-03-20 17:00:30.998772793 +0200
+++ b/src/libcamera/proxy/ipu3_ipa_proxy.cpp	2025-04-22 18:26:58.982280071 +0300
@@ -609,9 +609,6 @@
 	[[maybe_unused]] size_t dataSize,
 	[[maybe_unused]] const std::vector<SharedFD> &fds)
 {
-	uint32_t frame;
-	ControlList sensorControls;
-	ControlList lensControls;
 
 	[[maybe_unused]] const size_t frameBufSize = readPOD<uint32_t>(data, 0, data + dataSize);
 	[[maybe_unused]] const size_t sensorControlsBufSize = readPOD<uint32_t>(data, 4, data + dataSize);
@@ -622,18 +619,18 @@
 	const size_t lensControlsStart = sensorControlsStart + sensorControlsBufSize;
 
 
-	frame =
+	uint32_t frame =
         IPADataSerializer<uint32_t>::deserialize(
         	data + frameStart,
         	data + frameStart + frameBufSize);
 
-	sensorControls =
+	ControlList sensorControls =
         IPADataSerializer<ControlList>::deserialize(
         	data + sensorControlsStart,
         	data + sensorControlsStart + sensorControlsBufSize,
         	&controlSerializer_);
 
-	lensControls =
+	ControlList lensControls =
         IPADataSerializer<ControlList>::deserialize(
         	data + lensControlsStart,
         	data + lensControlsStart + lensControlsBufSize,
@@ -654,13 +651,12 @@
 	[[maybe_unused]] size_t dataSize,
 	[[maybe_unused]] const std::vector<SharedFD> &fds)
 {
-	uint32_t frame;
 
 
 	const size_t frameStart = 0;
 
 
-	frame =
+	uint32_t frame =
         IPADataSerializer<uint32_t>::deserialize(
         	data + frameStart,
         	data + dataSize);
@@ -681,8 +677,6 @@
 	[[maybe_unused]] size_t dataSize,
 	[[maybe_unused]] const std::vector<SharedFD> &fds)
 {
-	uint32_t frame;
-	ControlList metadata;
 
 	[[maybe_unused]] const size_t frameBufSize = readPOD<uint32_t>(data, 0, data + dataSize);
 	[[maybe_unused]] const size_t metadataBufSize = readPOD<uint32_t>(data, 4, data + dataSize);
@@ -691,12 +685,12 @@
 	const size_t metadataStart = frameStart + frameBufSize;
 
 
-	frame =
+	uint32_t frame =
         IPADataSerializer<uint32_t>::deserialize(
         	data + frameStart,
         	data + frameStart + frameBufSize);
 
-	metadata =
+	ControlList metadata =
         IPADataSerializer<ControlList>::deserialize(
         	data + metadataStart,
         	data + metadataStart + metadataBufSize,

This looks right.

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

> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  .../generators/libcamera_templates/module_ipa_proxy.cpp.tmpl | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> index ce3cc5ab6..07165821e 100644
> --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> @@ -239,10 +239,7 @@ void {{proxy_name}}::{{method.mojom_name}}IPC(
>  	[[maybe_unused]] size_t dataSize,
>  	[[maybe_unused]] const std::vector<SharedFD> &fds)
>  {
> -{%- for param in method.parameters %}
> -	{{param|name}} {{param.mojom_name}};
> -{%- endfor %}
> -{{proxy_funcs.deserialize_call(method.parameters, 'data', 'fds', false, false, true, 'dataSize')}}
> +{{proxy_funcs.deserialize_call(method.parameters, 'data', 'fds', false, true, true, 'dataSize')}}
>  	{{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});
>  }
>  {% endfor %}