[PATCH v1] libcamera: controls: Check size of enum

[Barnabás Pőcze]

Hi


2025. 03. 17. 18:56 keltezéssel, Jacopo Mondi írta:
> Hi Barnabás
> 
> On Mon, Mar 10, 2025 at 06:02:54PM +0100, Barnabás Pőcze wrote:
>> Only enums whose sizes match that of `int32_t` can be directly
>> supported. Otherwise the expected size and the real size would
>> disagree, leading to an assertion failure in `ControlValue::set()`.
>>
>> Signed-off-by: Barnabás Pőcze <barnabas.pocze at ideasonboard.com>
> 
> Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> 
> Thanks!
> 
>> ---
>>   include/libcamera/controls.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
>> index 7c7828ae5..4bfe9615c 100644
>> --- a/include/libcamera/controls.h
>> +++ b/include/libcamera/controls.h
>> @@ -125,7 +125,7 @@ struct control_type<Span<T, N>> : public control_type<std::remove_cv_t<T>> {
>>   };
>>
>>   template<typename T>
>> -struct control_type<T, std::enable_if_t<std::is_enum_v<T>>> : public control_type<int32_t> {
>> +struct control_type<T, std::enable_if_t<std::is_enum_v<T> && sizeof(T) == sizeof(int32_t)>> : public control_type<int32_t> {
> 
> However, for extra paranoia, the controls and properties enumerations
> are unscoped enum without an underlying specified type.
> 
> I read that if an underlying type is not specified:
> "the underlying type is an implementation-defined integral type"
> https://en.cppreference.com/w/cpp/language/enum
> 
> which makes me wonder if we should go extra paranoid and:
> 
> --- a/include/libcamera/control_ids.h.in
> +++ b/include/libcamera/control_ids.h.in
> @@ -31,7 +31,7 @@ namespace {{vendor}} {
>   {%- endif %}
> 
>   {% if ctrls %}
> -enum {
> +enum : int32_t {
>   {%- for ctrl in ctrls %}
>          {{ctrl.name|snake_case|upper}} = {{ctrl.id}},
>   {%- endfor %}
> 
>>   };
>>
>>   } /* namespace details */

I think that could be a good idea, maybe something like this:

diff --git a/include/libcamera/control_ids.h.in b/include/libcamera/control_ids.h.in
index 8f037589d..d56cb08ee 100644
--- a/include/libcamera/control_ids.h.in
+++ b/include/libcamera/control_ids.h.in
@@ -31,7 +31,7 @@ namespace {{vendor}} {
  {%- endif %}
  
  {% if ctrls %}
-enum {
+enum : unsigned int {
  {%- for ctrl in ctrls %}
         {{ctrl.name|snake_case|upper}} = {{ctrl.id}},
  {%- endfor %}
@@ -40,7 +40,7 @@ enum {
  
  {% for ctrl in ctrls -%}
  {% if ctrl.is_enum -%}
-enum {{ctrl.name}}Enum {
+enum {{ctrl.name}}Enum : int32_t {
  {%- for enum in ctrl.enum_values %}
         {{enum.name}} = {{enum.value}},
  {%- endfor %}


that is, control identifiers have the underlying type `unsigned int` since that
is what `ControlList`, etc. use for this purpose, and the specific enumerators
have type `int32_t` since that is what `ControlValue`, etc. use.


Regards,
Barnabás Pőcze


>> --
>> 2.48.1
>>