[RFC PATCH v1 7/8] libcamera: ipa_data_serializer: Add specialization for enums
Barnabás Pőcze
barnabas.pocze at ideasonboard.com
Tue May 20 15:56:10 CEST 2025
Hi
2025. 05. 20. 15:11 keltezéssel, Jacopo Mondi írta:
> Hi Barnabás
>
> On Thu, May 15, 2025 at 02:00:11PM +0200, Barnabás Pőcze wrote:
>> Instead of handling enums specially in the code generation templates,
>> create a specialization of `IPADataSerializer` that handles enums.
>>
>> To stay compatible, encode every enum as a `uint32_t`, and also
>> static_assert that the given enum type is smaller.
>>
>> Signed-off-by: Barnabás Pőcze <barnabas.pocze at ideasonboard.com>
>> ---
>> .../libcamera/internal/ipa_data_serializer.h | 48 ++++++++++++++++++-
>> .../libcamera_templates/proxy_functions.tmpl | 17 +------
>> .../libcamera_templates/serializer.tmpl | 14 ------
>> 3 files changed, 48 insertions(+), 31 deletions(-)
>>
>> diff --git a/include/libcamera/internal/ipa_data_serializer.h b/include/libcamera/internal/ipa_data_serializer.h
>> index b1fefba58..564f59e25 100644
>> --- a/include/libcamera/internal/ipa_data_serializer.h
>> +++ b/include/libcamera/internal/ipa_data_serializer.h
>> @@ -61,7 +61,7 @@ T readPOD(std::vector<uint8_t> &vec, size_t pos)
>>
>> } /* namespace */
>>
>> -template<typename T>
>> +template<typename T, typename = void>
>
> It took me a bit of time to understand why, give the above
Yes, this could be better in C++20:
template<typename E>
requires (std::is_enum_v<E>)
class IPADataSerializer<E> { ... };
which is easier to understand and does not require modification of the main template.
>
>> class IPADataSerializer
>> {
>> public:
>> @@ -344,6 +344,52 @@ public:
>> }
>> };
>>
>> +template<typename E>
>> +class IPADataSerializer<E, std::enable_if_t<std::is_enum_v<E>>>
>
> This could work for other E that are not enums, or in other words, I
> was expecting that we need to have an overload for !std::is_enum<>
>
> Later I realized that all other specializations here are for containers
> like vector<E> map<K, E> and Flag<E>, and all other types will have a
> generated specialization.
>
> Do you know how many copies of this function will be generated by the
> compiler ? One for each enum type specified in the interface file ?
This specialization will be instantiated for every enum type that goes
through (de)serialization. (At the moment such enums are only used in vimc,
as far as I remember.) But I generally expect the functions to be inlined.
Regards,
Barnabás Pőcze
>
> Anyway, looks good to me
> Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
>
>
>> +{
>> + using U = uint32_t;
>> + static_assert(sizeof(E) <= sizeof(U));
>> +
>> +public:
>> + static std::tuple<std::vector<uint8_t>, std::vector<SharedFD>>
>> + serialize(const E &data, [[maybe_unused]] ControlSerializer *cs = nullptr)
>> + {
>> + std::vector<uint8_t> dataVec;
>> + appendPOD<U>(dataVec, static_cast<U>(data));
>> +
>> + return { dataVec, {} };
>> + }
>> +
>> + static E deserialize(std::vector<uint8_t> &data,
>> + [[maybe_unused]] ControlSerializer *cs = nullptr)
>> + {
>> + return deserialize(data.cbegin(), data.cend());
>> + }
>> +
>> + static E deserialize(std::vector<uint8_t>::const_iterator dataBegin,
>> + std::vector<uint8_t>::const_iterator dataEnd,
>> + [[maybe_unused]] ControlSerializer *cs = nullptr)
>> + {
>> + return static_cast<E>(readPOD<U>(dataBegin, 0, dataEnd));
>> + }
>> +
>> + static E deserialize(std::vector<uint8_t> &data,
>> + [[maybe_unused]] std::vector<SharedFD> &fds,
>> + [[maybe_unused]] ControlSerializer *cs = nullptr)
>> + {
>> + return deserialize(data.cbegin(), data.cend());
>> + }
>> +
>> + static E deserialize(std::vector<uint8_t>::const_iterator dataBegin,
>> + std::vector<uint8_t>::const_iterator dataEnd,
>> + [[maybe_unused]] std::vector<SharedFD>::const_iterator fdsBegin,
>> + [[maybe_unused]] std::vector<SharedFD>::const_iterator fdsEnd,
>> + [[maybe_unused]] ControlSerializer *cs = nullptr)
>> + {
>> + return deserialize(dataBegin, dataEnd);
>> + }
>> +};
>> +
>> #endif /* __DOXYGEN__ */
>>
>> } /* namespace libcamera */
>> diff --git a/utils/codegen/ipc/generators/libcamera_templates/proxy_functions.tmpl b/utils/codegen/ipc/generators/libcamera_templates/proxy_functions.tmpl
>> index 25476990e..01e2567ca 100644
>> --- a/utils/codegen/ipc/generators/libcamera_templates/proxy_functions.tmpl
>> +++ b/utils/codegen/ipc/generators/libcamera_templates/proxy_functions.tmpl
>> @@ -52,9 +52,6 @@
>> #}
>> {%- macro serialize_call(params, buf, fds) %}
>> {%- for param in params %}
>> -{%- if param|is_enum %}
>> - static_assert(sizeof({{param|name_full}}) <= 4);
>> -{%- endif %}
>> std::vector<uint8_t> {{param.mojom_name}}Buf;
>> {%- if param|has_fd %}
>> std::vector<SharedFD> {{param.mojom_name}}Fds;
>> @@ -62,13 +59,7 @@
>> {%- else %}
>> std::tie({{param.mojom_name}}Buf, std::ignore) =
>> {%- endif %}
>> -{%- if param|is_flags %}
>> IPADataSerializer<{{param|name_full}}>::serialize({{param.mojom_name}}
>> -{%- elif param|is_enum %}
>> - IPADataSerializer<uint32_t>::serialize(static_cast<uint32_t>({{param.mojom_name}})
>> -{%- else %}
>> - IPADataSerializer<{{param|name}}>::serialize({{param.mojom_name}}
>> -{% endif -%}
>> {{- ", &controlSerializer_" if param|needs_control_serializer -}}
>> );
>> {%- endfor %}
>> @@ -107,13 +98,7 @@
>> #}
>> {%- macro deserialize_param(param, pointer, loop, buf, fds, iter, data_size) -%}
>> {{"*" if pointer}}{{param.mojom_name}} =
>> -{%- if param|is_flags %}
>> IPADataSerializer<{{param|name_full}}>::deserialize(
>> -{%- elif param|is_enum %}
>> -static_cast<{{param|name_full}}>(IPADataSerializer<uint32_t>::deserialize(
>> -{%- else %}
>> -IPADataSerializer<{{param|name}}>::deserialize(
>> -{%- endif %}
>> {{buf}}{{- ".cbegin()" if not iter}} + {{param.mojom_name}}Start,
>> {%- if loop.last and not iter %}
>> {{buf}}.cend()
>> @@ -137,7 +122,7 @@ IPADataSerializer<{{param|name}}>::deserialize(
>> {%- if param|needs_control_serializer %}
>> &controlSerializer_
>> {%- endif -%}
>> -){{")" if param|is_enum and not param|is_flags}};
>> +);
>> {%- endmacro -%}
>>
>>
>> diff --git a/utils/codegen/ipc/generators/libcamera_templates/serializer.tmpl b/utils/codegen/ipc/generators/libcamera_templates/serializer.tmpl
>> index d07836cc1..e316dd88a 100644
>> --- a/utils/codegen/ipc/generators/libcamera_templates/serializer.tmpl
>> +++ b/utils/codegen/ipc/generators/libcamera_templates/serializer.tmpl
>> @@ -32,15 +32,7 @@
>> {%- 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_flags %}
>> IPADataSerializer<{{field|name_full}}>::serialize(data.{{field.mojom_name}});
>> - {%- elif field|is_enum_scoped %}
>> - IPADataSerializer<uint{{field|bit_width}}_t>::serialize(static_cast<uint{{field|bit_width}}_t>(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}};
>> @@ -98,13 +90,7 @@
>> {% 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}} = IPADataSerializer<{{field|name}}>::deserialize(m, m + {{field_size}});
>> - {%- elif field|is_flags %}
>> ret.{{field.mojom_name}} = IPADataSerializer<{{field|name_full}}>::deserialize(m, m + {{field_size}});
>> - {%- else %}
>> - ret.{{field.mojom_name}} = static_cast<{{field|name_full}}>(IPADataSerializer<uint{{field|bit_width}}_t>::deserialize(m, m + {{field_size}}));
>> - {%- endif %}
>> {%- if not loop.last %}
>> m += {{field_size}};
>> dataSize -= {{field_size}};
>> --
>> 2.49.0
>>
More information about the libcamera-devel
mailing list