[{"id":34248,"web_url":"https://patchwork.libcamera.org/comment/34248/","msgid":"<174731637525.209691.10547963723108491405@calcite>","date":"2025-05-15T13:39:35","subject":"Re: [RFC PATCH v1 7/8] libcamera: ipa_data_serializer: Add\n\tspecialization for enums","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Quoting Barnabás Pőcze (2025-05-15 14:00:11)\n> Instead of handling enums specially in the code generation templates,\n> create a specialization of `IPADataSerializer` that handles enums.\n> \n> To stay compatible, encode every enum as a `uint32_t`, and also\n> static_assert that the given enum type is smaller.\n> \n> Signed-off-by: Barnabás Pőcze \n> ---\n> .../libcamera/internal/ipa_data_serializer.h | 48 ++++++++++++++++++-\n> .../libcamera_templates/proxy_functions.tmpl | 17 +------\n> .../libcamera_templates/serializer.tmpl | 14 ------\n\nThat is a nice cleanup.\n\nReviewed-by: Paul Elder \n\n> 3 files changed, 48 insertions(+), 31 deletions(-)\n> \n> diff --git a/include/libcamera/internal/ipa_data_serializer.h b/include/libcamera/internal/ipa_data_serializer.h\n> index b1fefba58..564f59e25 100644\n> --- a/include/libcamera/internal/ipa_data_serializer.h\n> +++ b/include/libcamera/internal/ipa_data_serializer.h\n> @@ -61,7 +61,7 @@ T readPOD(std::vector &vec, size_t pos)\n> \n> } /* namespace */\n> \n> -template\n> +template\n> class IPADataSerializer\n> {\n> public:\n> @@ -344,6 +344,52 @@ public:\n> }\n> };\n> \n> +template\n> +class IPADataSerializer>>\n> +{\n> + using U = uint32_t;\n> + static_assert(sizeof(E) <= sizeof(U));\n> +\n> +public:\n> + static std::tuple, std::vector>\n> + serialize(const E &data, [[maybe_unused]] ControlSerializer *cs = nullptr)\n> + {\n> + std::vector dataVec;\n> + appendPOD(dataVec, static_cast(data));\n> +\n> + return { dataVec, {} };\n> + }\n> +\n> + static E deserialize(std::vector &data,\n> + [[maybe_unused]] ControlSerializer *cs = nullptr)\n> + {\n> + return deserialize(data.cbegin(), data.cend());\n> + }\n> +\n> + static E deserialize(std::vector::const_iterator dataBegin,\n> + std::vector::const_iterator dataEnd,\n> + [[maybe_unused]] ControlSerializer *cs = nullptr)\n> + {\n> + return static_cast(readPOD(dataBegin, 0, dataEnd));\n> + }\n> +\n> + static E deserialize(std::vector &data,\n> + [[maybe_unused]] std::vector &fds,\n> + [[maybe_unused]] ControlSerializer *cs = nullptr)\n> + {\n> + return deserialize(data.cbegin(), data.cend());\n> + }\n> +\n> + static E deserialize(std::vector::const_iterator dataBegin,\n> + std::vector::const_iterator dataEnd,\n> + [[maybe_unused]] std::vector::const_iterator fdsBegin,\n> + [[maybe_unused]] std::vector::const_iterator fdsEnd,\n> + [[maybe_unused]] ControlSerializer *cs = nullptr)\n> + {\n> + return deserialize(dataBegin, dataEnd);\n> + }\n> +};\n> +\n> #endif /* __DOXYGEN__ */\n> \n> } /* namespace libcamera */\n> diff --git a/utils/codegen/ipc/generators/libcamera_templates/proxy_functions.tmpl b/utils/codegen/ipc/generators/libcamera_templates/proxy_functions.tmpl\n> index 25476990e..01e2567ca 100644\n> --- a/utils/codegen/ipc/generators/libcamera_templates/proxy_functions.tmpl\n> +++ b/utils/codegen/ipc/generators/libcamera_templates/proxy_functions.tmpl\n> @@ -52,9 +52,6 @@\n> #}\n> {%- macro serialize_call(params, buf, fds) %}\n> {%- for param in params %}\n> -{%- if param|is_enum %}\n> - static_assert(sizeof({{param|name_full}}) <= 4);\n> -{%- endif %}\n> std::vector {{param.mojom_name}}Buf;\n> {%- if param|has_fd %}\n> std::vector {{param.mojom_name}}Fds;\n> @@ -62,13 +59,7 @@\n> {%- else %}\n> std::tie({{param.mojom_name}}Buf, std::ignore) =\n> {%- endif %}\n> -{%- if param|is_flags %}\n> IPADataSerializer<{{param|name_full}}>::serialize({{param.mojom_name}}\n> -{%- elif param|is_enum %}\n> - IPADataSerializer::serialize(static_cast({{param.mojom_name}})\n> -{%- else %}\n> - IPADataSerializer<{{param|name}}>::serialize({{param.mojom_name}}\n> -{% endif -%}\n> {{- \", &controlSerializer_\" if param|needs_control_serializer -}}\n> );\n> {%- endfor %}\n> @@ -107,13 +98,7 @@\n> #}\n> {%- macro deserialize_param(param, pointer, loop, buf, fds, iter, data_size) -%}\n> {{\"*\" if pointer}}{{param.mojom_name}} =\n> -{%- if param|is_flags %}\n> IPADataSerializer<{{param|name_full}}>::deserialize(\n> -{%- elif param|is_enum %}\n> -static_cast<{{param|name_full}}>(IPADataSerializer::deserialize(\n> -{%- else %}\n> -IPADataSerializer<{{param|name}}>::deserialize(\n> -{%- endif %}\n> {{buf}}{{- \".cbegin()\" if not iter}} + {{param.mojom_name}}Start,\n> {%- if loop.last and not iter %}\n> {{buf}}.cend()\n> @@ -137,7 +122,7 @@ IPADataSerializer<{{param|name}}>::deserialize(\n> {%- if param|needs_control_serializer %}\n> &controlSerializer_\n> {%- endif -%}\n> -){{\")\" if param|is_enum and not param|is_flags}};\n> +);\n> {%- endmacro -%}\n> \n> \n> diff --git a/utils/codegen/ipc/generators/libcamera_templates/serializer.tmpl b/utils/codegen/ipc/generators/libcamera_templates/serializer.tmpl\n> index d07836cc1..e316dd88a 100644\n> --- a/utils/codegen/ipc/generators/libcamera_templates/serializer.tmpl\n> +++ b/utils/codegen/ipc/generators/libcamera_templates/serializer.tmpl\n> @@ -32,15 +32,7 @@\n> {%- if field|is_pod or field|is_enum %}\n> std::vector {{field.mojom_name}};\n> std::tie({{field.mojom_name}}, std::ignore) =\n> - {%- if field|is_pod %}\n> - IPADataSerializer<{{field|name}}>::serialize(data.{{field.mojom_name}});\n> - {%- elif field|is_flags %}\n> IPADataSerializer<{{field|name_full}}>::serialize(data.{{field.mojom_name}});\n> - {%- elif field|is_enum_scoped %}\n> - IPADataSerializer::serialize(static_cast(data.{{field.mojom_name}}));\n> - {%- elif field|is_enum %}\n> - IPADataSerializer::serialize(data.{{field.mojom_name}});\n> - {%- endif %}\n> retData.insert(retData.end(), {{field.mojom_name}}.begin(), {{field.mojom_name}}.end());\n> {%- elif field|is_fd %}\n> std::vector {{field.mojom_name}};\n> @@ -98,13 +90,7 @@\n> {% if field|is_pod or field|is_enum %}\n> {%- set field_size = (field|bit_width|int / 8)|int %}\n> {{- check_data_size(field_size, 'dataSize', field.mojom_name, 'data')}}\n> - {%- if field|is_pod %}\n> - ret.{{field.mojom_name}} = IPADataSerializer<{{field|name}}>::deserialize(m, m + {{field_size}});\n> - {%- elif field|is_flags %}\n> ret.{{field.mojom_name}} = IPADataSerializer<{{field|name_full}}>::deserialize(m, m + {{field_size}});\n> - {%- else %}\n> - ret.{{field.mojom_name}} = static_cast<{{field|name_full}}>(IPADataSerializer::deserialize(m, m + {{field_size}}));\n> - {%- endif %}\n> {%- if not loop.last %}\n> m += {{field_size}};\n> dataSize -= {{field_size}};\n> -- \n> 2.49.0\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id AD9CAC3200\n\tfor ;\n\tThu, 15 May 2025 13:39:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 99B4C68B6C;\n\tThu, 15 May 2025 15:39:39 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 249C168B6C\n\tfor ;\n\tThu, 15 May 2025 15:39:38 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2a01:cb16:204c:4644:2597:989f:e013:9296])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B6827886;\n\tThu, 15 May 2025 15:39:20 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"tPYM5VrT\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1747316360;\n\tbh=xLIQEoHuUT6za4EwDfGnjTnu2EfXdNhrvRDiRZ4iBZ0=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=tPYM5VrTior8Jha3t9Jul/+5YBKKqBXWAzTkCpTqv5Zdg8rhQKvmVnCqfOP01ySkE\n\t/beMSLudhspbB4NOJoBGl9gnFKeTfW5uCN3XIL849s7HZ1ARtN0D2NJYSoe0OvXXai\n\ts5qoPvSzhcCbuM+N6rX0kb+2fGC20HSCR3Z4fh1k=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250515120012.3127231-8-barnabas.pocze@ideasonboard.com>","References":"<20250515120012.3127231-1-barnabas.pocze@ideasonboard.com>\n\t<20250515120012.3127231-8-barnabas.pocze@ideasonboard.com>","Subject":"Re: [RFC PATCH v1 7/8] libcamera: ipa_data_serializer: Add\n\tspecialization for enums","From":"Paul Elder ","Cc":"","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 15 May 2025 15:39:35 +0200","Message-ID":"<174731637525.209691.10547963723108491405@calcite>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":34297,"web_url":"https://patchwork.libcamera.org/comment/34297/","msgid":"","date":"2025-05-20T13:11:42","subject":"Re: [RFC PATCH v1 7/8] libcamera: ipa_data_serializer: Add\n\tspecialization for enums","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Barnabás\n\nOn Thu, May 15, 2025 at 02:00:11PM +0200, Barnabás Pőcze wrote:\n> Instead of handling enums specially in the code generation templates,\n> create a specialization of `IPADataSerializer` that handles enums.\n>\n> To stay compatible, encode every enum as a `uint32_t`, and also\n> static_assert that the given enum type is smaller.\n>\n> Signed-off-by: Barnabás Pőcze \n> ---\n> .../libcamera/internal/ipa_data_serializer.h | 48 ++++++++++++++++++-\n> .../libcamera_templates/proxy_functions.tmpl | 17 +------\n> .../libcamera_templates/serializer.tmpl | 14 ------\n> 3 files changed, 48 insertions(+), 31 deletions(-)\n>\n> diff --git a/include/libcamera/internal/ipa_data_serializer.h b/include/libcamera/internal/ipa_data_serializer.h\n> index b1fefba58..564f59e25 100644\n> --- a/include/libcamera/internal/ipa_data_serializer.h\n> +++ b/include/libcamera/internal/ipa_data_serializer.h\n> @@ -61,7 +61,7 @@ T readPOD(std::vector &vec, size_t pos)\n>\n> } /* namespace */\n>\n> -template\n> +template\n\nIt took me a bit of time to understand why, give the above\n\n> class IPADataSerializer\n> {\n> public:\n> @@ -344,6 +344,52 @@ public:\n> \t}\n> };\n>\n> +template\n> +class IPADataSerializer>>\n\nThis could work for other E that are not enums, or in other words, I\nwas expecting that we need to have an overload for !std::is_enum<>\n\nLater I realized that all other specializations here are for containers\nlike vector map and Flag, and all other types will have a\ngenerated specialization.\n\nDo you know how many copies of this function will be generated by the\ncompiler ? One for each enum type specified in the interface file ?\n\nAnyway, looks good to me\nReviewed-by: Jacopo Mondi \n\n\n> +{\n> +\tusing U = uint32_t;\n> +\tstatic_assert(sizeof(E) <= sizeof(U));\n> +\n> +public:\n> +\tstatic std::tuple, std::vector>\n> +\tserialize(const E &data, [[maybe_unused]] ControlSerializer *cs = nullptr)\n> +\t{\n> +\t\tstd::vector dataVec;\n> +\t\tappendPOD(dataVec, static_cast(data));\n> +\n> +\t\treturn { dataVec, {} };\n> +\t}\n> +\n> +\tstatic E deserialize(std::vector &data,\n> +\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> +\t{\n> +\t\treturn deserialize(data.cbegin(), data.cend());\n> +\t}\n> +\n> +\tstatic E deserialize(std::vector::const_iterator dataBegin,\n> +\t\t\t std::vector::const_iterator dataEnd,\n> +\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> +\t{\n> +\t\treturn static_cast(readPOD(dataBegin, 0, dataEnd));\n> +\t}\n> +\n> +\tstatic E deserialize(std::vector &data,\n> +\t\t\t [[maybe_unused]] std::vector &fds,\n> +\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> +\t{\n> +\t\treturn deserialize(data.cbegin(), data.cend());\n> +\t}\n> +\n> +\tstatic E deserialize(std::vector::const_iterator dataBegin,\n> +\t\t\t std::vector::const_iterator dataEnd,\n> +\t\t\t [[maybe_unused]] std::vector::const_iterator fdsBegin,\n> +\t\t\t [[maybe_unused]] std::vector::const_iterator fdsEnd,\n> +\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> +\t{\n> +\t\treturn deserialize(dataBegin, dataEnd);\n> +\t}\n> +};\n> +\n> #endif /* __DOXYGEN__ */\n>\n> } /* namespace libcamera */\n> diff --git a/utils/codegen/ipc/generators/libcamera_templates/proxy_functions.tmpl b/utils/codegen/ipc/generators/libcamera_templates/proxy_functions.tmpl\n> index 25476990e..01e2567ca 100644\n> --- a/utils/codegen/ipc/generators/libcamera_templates/proxy_functions.tmpl\n> +++ b/utils/codegen/ipc/generators/libcamera_templates/proxy_functions.tmpl\n> @@ -52,9 +52,6 @@\n> #}\n> {%- macro serialize_call(params, buf, fds) %}\n> {%- for param in params %}\n> -{%- if param|is_enum %}\n> -\tstatic_assert(sizeof({{param|name_full}}) <= 4);\n> -{%- endif %}\n> \tstd::vector {{param.mojom_name}}Buf;\n> {%- if param|has_fd %}\n> \tstd::vector {{param.mojom_name}}Fds;\n> @@ -62,13 +59,7 @@\n> {%- else %}\n> \tstd::tie({{param.mojom_name}}Buf, std::ignore) =\n> {%- endif %}\n> -{%- if param|is_flags %}\n> \t\tIPADataSerializer<{{param|name_full}}>::serialize({{param.mojom_name}}\n> -{%- elif param|is_enum %}\n> -\t\tIPADataSerializer::serialize(static_cast({{param.mojom_name}})\n> -{%- else %}\n> -\t\tIPADataSerializer<{{param|name}}>::serialize({{param.mojom_name}}\n> -{% endif -%}\n> {{- \", &controlSerializer_\" if param|needs_control_serializer -}}\n> );\n> {%- endfor %}\n> @@ -107,13 +98,7 @@\n> #}\n> {%- macro deserialize_param(param, pointer, loop, buf, fds, iter, data_size) -%}\n> {{\"*\" if pointer}}{{param.mojom_name}} =\n> -{%- if param|is_flags %}\n> IPADataSerializer<{{param|name_full}}>::deserialize(\n> -{%- elif param|is_enum %}\n> -static_cast<{{param|name_full}}>(IPADataSerializer::deserialize(\n> -{%- else %}\n> -IPADataSerializer<{{param|name}}>::deserialize(\n> -{%- endif %}\n> \t{{buf}}{{- \".cbegin()\" if not iter}} + {{param.mojom_name}}Start,\n> {%- if loop.last and not iter %}\n> \t{{buf}}.cend()\n> @@ -137,7 +122,7 @@ IPADataSerializer<{{param|name}}>::deserialize(\n> {%- if param|needs_control_serializer %}\n> \t&controlSerializer_\n> {%- endif -%}\n> -){{\")\" if param|is_enum and not param|is_flags}};\n> +);\n> {%- endmacro -%}\n>\n>\n> diff --git a/utils/codegen/ipc/generators/libcamera_templates/serializer.tmpl b/utils/codegen/ipc/generators/libcamera_templates/serializer.tmpl\n> index d07836cc1..e316dd88a 100644\n> --- a/utils/codegen/ipc/generators/libcamera_templates/serializer.tmpl\n> +++ b/utils/codegen/ipc/generators/libcamera_templates/serializer.tmpl\n> @@ -32,15 +32,7 @@\n> {%- if field|is_pod or field|is_enum %}\n> \t\tstd::vector {{field.mojom_name}};\n> \t\tstd::tie({{field.mojom_name}}, std::ignore) =\n> -\t{%- if field|is_pod %}\n> -\t\t\tIPADataSerializer<{{field|name}}>::serialize(data.{{field.mojom_name}});\n> -\t{%- elif field|is_flags %}\n> \t\t\tIPADataSerializer<{{field|name_full}}>::serialize(data.{{field.mojom_name}});\n> -\t{%- elif field|is_enum_scoped %}\n> -\t\t\tIPADataSerializer::serialize(static_cast(data.{{field.mojom_name}}));\n> -\t{%- elif field|is_enum %}\n> -\t\t\tIPADataSerializer::serialize(data.{{field.mojom_name}});\n> -\t{%- endif %}\n> \t\tretData.insert(retData.end(), {{field.mojom_name}}.begin(), {{field.mojom_name}}.end());\n> {%- elif field|is_fd %}\n> \t\tstd::vector {{field.mojom_name}};\n> @@ -98,13 +90,7 @@\n> {% if field|is_pod or field|is_enum %}\n> \t{%- set field_size = (field|bit_width|int / 8)|int %}\n> \t\t{{- check_data_size(field_size, 'dataSize', field.mojom_name, 'data')}}\n> -\t\t{%- if field|is_pod %}\n> -\t\tret.{{field.mojom_name}} = IPADataSerializer<{{field|name}}>::deserialize(m, m + {{field_size}});\n> -\t\t{%- elif field|is_flags %}\n> \t\tret.{{field.mojom_name}} = IPADataSerializer<{{field|name_full}}>::deserialize(m, m + {{field_size}});\n> -\t\t{%- else %}\n> -\t\tret.{{field.mojom_name}} = static_cast<{{field|name_full}}>(IPADataSerializer::deserialize(m, m + {{field_size}}));\n> -\t\t{%- endif %}\n> \t{%- if not loop.last %}\n> \t\tm += {{field_size}};\n> \t\tdataSize -= {{field_size}};\n> --\n> 2.49.0\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 68940C31E9\n\tfor ;\n\tTue, 20 May 2025 13:11:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 13F9C68D85;\n\tTue, 20 May 2025 15:11:50 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C2E57614DE\n\tfor ;\n\tTue, 20 May 2025 15:11:48 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9CB8A74C;\n\tTue, 20 May 2025 15:11:27 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"LVO5y+h/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1747746687;\n\tbh=m5Kxqk+Kuf2vvqlAUxgOsrX+xsrCmdQ+qLDVMCBc3Q8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LVO5y+h/hx8xwHMdNgc7PjPcT4XUNkZz3VinzuxcONhTwWVjb29TWyFicZbp8QHYY\n\tdUW/QB6swB9L4YT5+StgTjNaIYE5qt6aKSs/GiaMy8RF3vsfT1itiGO3nG8w4vnhkE\n\tKSf/YPt/zCXMWlAOFC4CTEqIoYzK++W2R/49FuN8=","Date":"Tue, 20 May 2025 15:11:42 +0200","From":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org, \n\tPaul Elder ","Subject":"Re: [RFC PATCH v1 7/8] libcamera: ipa_data_serializer: Add\n\tspecialization for enums","Message-ID":"","References":"<20250515120012.3127231-1-barnabas.pocze@ideasonboard.com>\n\t<20250515120012.3127231-8-barnabas.pocze@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20250515120012.3127231-8-barnabas.pocze@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":34298,"web_url":"https://patchwork.libcamera.org/comment/34298/","msgid":"<9b92d70f-cc04-484a-949c-1ff939d374bc@ideasonboard.com>","date":"2025-05-20T13:56:10","subject":"Re: [RFC PATCH v1 7/8] libcamera: ipa_data_serializer: Add\n\tspecialization for enums","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n\n2025. 05. 20. 15:11 keltezéssel, Jacopo Mondi írta:\n> Hi Barnabás\n> \n> On Thu, May 15, 2025 at 02:00:11PM +0200, Barnabás Pőcze wrote:\n>> Instead of handling enums specially in the code generation templates,\n>> create a specialization of `IPADataSerializer` that handles enums.\n>>\n>> To stay compatible, encode every enum as a `uint32_t`, and also\n>> static_assert that the given enum type is smaller.\n>>\n>> Signed-off-by: Barnabás Pőcze \n>> ---\n>> .../libcamera/internal/ipa_data_serializer.h | 48 ++++++++++++++++++-\n>> .../libcamera_templates/proxy_functions.tmpl | 17 +------\n>> .../libcamera_templates/serializer.tmpl | 14 ------\n>> 3 files changed, 48 insertions(+), 31 deletions(-)\n>>\n>> diff --git a/include/libcamera/internal/ipa_data_serializer.h b/include/libcamera/internal/ipa_data_serializer.h\n>> index b1fefba58..564f59e25 100644\n>> --- a/include/libcamera/internal/ipa_data_serializer.h\n>> +++ b/include/libcamera/internal/ipa_data_serializer.h\n>> @@ -61,7 +61,7 @@ T readPOD(std::vector &vec, size_t pos)\n>>\n>> } /* namespace */\n>>\n>> -template\n>> +template\n> \n> It took me a bit of time to understand why, give the above\n\nYes, this could be better in C++20:\n\n template\n \trequires (std::is_enum_v)\n class IPADataSerializer { ... };\n\nwhich is easier to understand and does not require modification of the main template.\n\n\n> \n>> class IPADataSerializer\n>> {\n>> public:\n>> @@ -344,6 +344,52 @@ public:\n>> \t}\n>> };\n>>\n>> +template\n>> +class IPADataSerializer>>\n> \n> This could work for other E that are not enums, or in other words, I\n> was expecting that we need to have an overload for !std::is_enum<>\n> \n> Later I realized that all other specializations here are for containers\n> like vector map and Flag, and all other types will have a\n> generated specialization.\n> \n> Do you know how many copies of this function will be generated by the\n> compiler ? One for each enum type specified in the interface file ?\n\nThis specialization will be instantiated for every enum type that goes\nthrough (de)serialization. (At the moment such enums are only used in vimc,\nas far as I remember.) But I generally expect the functions to be inlined.\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \n> Anyway, looks good to me\n> Reviewed-by: Jacopo Mondi \n> \n> \n>> +{\n>> +\tusing U = uint32_t;\n>> +\tstatic_assert(sizeof(E) <= sizeof(U));\n>> +\n>> +public:\n>> +\tstatic std::tuple, std::vector>\n>> +\tserialize(const E &data, [[maybe_unused]] ControlSerializer *cs = nullptr)\n>> +\t{\n>> +\t\tstd::vector dataVec;\n>> +\t\tappendPOD(dataVec, static_cast(data));\n>> +\n>> +\t\treturn { dataVec, {} };\n>> +\t}\n>> +\n>> +\tstatic E deserialize(std::vector &data,\n>> +\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n>> +\t{\n>> +\t\treturn deserialize(data.cbegin(), data.cend());\n>> +\t}\n>> +\n>> +\tstatic E deserialize(std::vector::const_iterator dataBegin,\n>> +\t\t\t std::vector::const_iterator dataEnd,\n>> +\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n>> +\t{\n>> +\t\treturn static_cast(readPOD(dataBegin, 0, dataEnd));\n>> +\t}\n>> +\n>> +\tstatic E deserialize(std::vector &data,\n>> +\t\t\t [[maybe_unused]] std::vector &fds,\n>> +\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n>> +\t{\n>> +\t\treturn deserialize(data.cbegin(), data.cend());\n>> +\t}\n>> +\n>> +\tstatic E deserialize(std::vector::const_iterator dataBegin,\n>> +\t\t\t std::vector::const_iterator dataEnd,\n>> +\t\t\t [[maybe_unused]] std::vector::const_iterator fdsBegin,\n>> +\t\t\t [[maybe_unused]] std::vector::const_iterator fdsEnd,\n>> +\t\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n>> +\t{\n>> +\t\treturn deserialize(dataBegin, dataEnd);\n>> +\t}\n>> +};\n>> +\n>> #endif /* __DOXYGEN__ */\n>>\n>> } /* namespace libcamera */\n>> diff --git a/utils/codegen/ipc/generators/libcamera_templates/proxy_functions.tmpl b/utils/codegen/ipc/generators/libcamera_templates/proxy_functions.tmpl\n>> index 25476990e..01e2567ca 100644\n>> --- a/utils/codegen/ipc/generators/libcamera_templates/proxy_functions.tmpl\n>> +++ b/utils/codegen/ipc/generators/libcamera_templates/proxy_functions.tmpl\n>> @@ -52,9 +52,6 @@\n>> #}\n>> {%- macro serialize_call(params, buf, fds) %}\n>> {%- for param in params %}\n>> -{%- if param|is_enum %}\n>> -\tstatic_assert(sizeof({{param|name_full}}) <= 4);\n>> -{%- endif %}\n>> \tstd::vector {{param.mojom_name}}Buf;\n>> {%- if param|has_fd %}\n>> \tstd::vector {{param.mojom_name}}Fds;\n>> @@ -62,13 +59,7 @@\n>> {%- else %}\n>> \tstd::tie({{param.mojom_name}}Buf, std::ignore) =\n>> {%- endif %}\n>> -{%- if param|is_flags %}\n>> \t\tIPADataSerializer<{{param|name_full}}>::serialize({{param.mojom_name}}\n>> -{%- elif param|is_enum %}\n>> -\t\tIPADataSerializer::serialize(static_cast({{param.mojom_name}})\n>> -{%- else %}\n>> -\t\tIPADataSerializer<{{param|name}}>::serialize({{param.mojom_name}}\n>> -{% endif -%}\n>> {{- \", &controlSerializer_\" if param|needs_control_serializer -}}\n>> );\n>> {%- endfor %}\n>> @@ -107,13 +98,7 @@\n>> #}\n>> {%- macro deserialize_param(param, pointer, loop, buf, fds, iter, data_size) -%}\n>> {{\"*\" if pointer}}{{param.mojom_name}} =\n>> -{%- if param|is_flags %}\n>> IPADataSerializer<{{param|name_full}}>::deserialize(\n>> -{%- elif param|is_enum %}\n>> -static_cast<{{param|name_full}}>(IPADataSerializer::deserialize(\n>> -{%- else %}\n>> -IPADataSerializer<{{param|name}}>::deserialize(\n>> -{%- endif %}\n>> \t{{buf}}{{- \".cbegin()\" if not iter}} + {{param.mojom_name}}Start,\n>> {%- if loop.last and not iter %}\n>> \t{{buf}}.cend()\n>> @@ -137,7 +122,7 @@ IPADataSerializer<{{param|name}}>::deserialize(\n>> {%- if param|needs_control_serializer %}\n>> \t&controlSerializer_\n>> {%- endif -%}\n>> -){{\")\" if param|is_enum and not param|is_flags}};\n>> +);\n>> {%- endmacro -%}\n>>\n>>\n>> diff --git a/utils/codegen/ipc/generators/libcamera_templates/serializer.tmpl b/utils/codegen/ipc/generators/libcamera_templates/serializer.tmpl\n>> index d07836cc1..e316dd88a 100644\n>> --- a/utils/codegen/ipc/generators/libcamera_templates/serializer.tmpl\n>> +++ b/utils/codegen/ipc/generators/libcamera_templates/serializer.tmpl\n>> @@ -32,15 +32,7 @@\n>> {%- if field|is_pod or field|is_enum %}\n>> \t\tstd::vector {{field.mojom_name}};\n>> \t\tstd::tie({{field.mojom_name}}, std::ignore) =\n>> -\t{%- if field|is_pod %}\n>> -\t\t\tIPADataSerializer<{{field|name}}>::serialize(data.{{field.mojom_name}});\n>> -\t{%- elif field|is_flags %}\n>> \t\t\tIPADataSerializer<{{field|name_full}}>::serialize(data.{{field.mojom_name}});\n>> -\t{%- elif field|is_enum_scoped %}\n>> -\t\t\tIPADataSerializer::serialize(static_cast(data.{{field.mojom_name}}));\n>> -\t{%- elif field|is_enum %}\n>> -\t\t\tIPADataSerializer::serialize(data.{{field.mojom_name}});\n>> -\t{%- endif %}\n>> \t\tretData.insert(retData.end(), {{field.mojom_name}}.begin(), {{field.mojom_name}}.end());\n>> {%- elif field|is_fd %}\n>> \t\tstd::vector {{field.mojom_name}};\n>> @@ -98,13 +90,7 @@\n>> {% if field|is_pod or field|is_enum %}\n>> \t{%- set field_size = (field|bit_width|int / 8)|int %}\n>> \t\t{{- check_data_size(field_size, 'dataSize', field.mojom_name, 'data')}}\n>> -\t\t{%- if field|is_pod %}\n>> -\t\tret.{{field.mojom_name}} = IPADataSerializer<{{field|name}}>::deserialize(m, m + {{field_size}});\n>> -\t\t{%- elif field|is_flags %}\n>> \t\tret.{{field.mojom_name}} = IPADataSerializer<{{field|name_full}}>::deserialize(m, m + {{field_size}});\n>> -\t\t{%- else %}\n>> -\t\tret.{{field.mojom_name}} = static_cast<{{field|name_full}}>(IPADataSerializer::deserialize(m, m + {{field_size}}));\n>> -\t\t{%- endif %}\n>> \t{%- if not loop.last %}\n>> \t\tm += {{field_size}};\n>> \t\tdataSize -= {{field_size}};\n>> --\n>> 2.49.0\n>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 33488C31E9\n\tfor ;\n\tTue, 20 May 2025 13:56:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 18ADF68D85;\n\tTue, 20 May 2025 15:56:20 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E35B7614DE\n\tfor ;\n\tTue, 20 May 2025 15:56:18 +0200 (CEST)","from [192.168.33.24] (185.182.214.47.nat.pool.zt.hu\n\t[185.182.214.47])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9A33C74C;\n\tTue, 20 May 2025 15:55:56 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"pYCCGaLz\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1747749358;\n\tbh=CTO+uiJAeDCKshCISJ5Mgk6hYGUhZOQXBvxw7lVlyF4=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=pYCCGaLzyf88As6bu3ags3/ZmAFT8JupchIYxKBnUSfXslDJgw+763dwufaycJ0OD\n\tExJrdvapMLgQ/PTh+1uflH3ohZ1gdEn3IMSJ58xi0cHPWKQtLBOtY6Ff9ZcZeLm/H7\n\tKzp84JY+VBDChsH8v4j4l5Gg8o/llhuxBu1IpAps=","Message-ID":"<9b92d70f-cc04-484a-949c-1ff939d374bc@ideasonboard.com>","Date":"Tue, 20 May 2025 15:56:10 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [RFC PATCH v1 7/8] libcamera: ipa_data_serializer: Add\n\tspecialization for enums","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org,\n\tPaul Elder ","References":"<20250515120012.3127231-1-barnabas.pocze@ideasonboard.com>\n\t<20250515120012.3127231-8-barnabas.pocze@ideasonboard.com>\n\t","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]