[v1,2/2] Revert "controls: Add boolean constructors for ControlInfo"

23375 diff mbox series

This reverts commit 10cdc914dad282b4ca0ad11067d5c6d446af1fcc.

The contstructors introduced by that commit are not used anywhere,
and they do not match the existing practice for boolean controls.

Specifically, every single boolean control is described by calling
the `ControlInfo(ControlValue, ControlValue, ControlValue = {})`
constructor. Crucially, that constructor does not set `values_`,
while the two removed contructors do. And whether or not `values_`
has any elements is currently used as an implicit sign to decide
whether or not the control is "enum-like", and those are assumed
to have type `int32_t`.

For example, any boolean control described using any of the two
removed constructors would cause an assertion in failure in
`CameraSession::listControls()` when calling `value.get<int32_t>()`.

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 include/libcamera/controls.h   |  3 ---
 src/libcamera/controls.cpp     | 29 -----------------------------
 test/controls/control_info.cpp | 33 ---------------------------------
 3 files changed, 65 deletions(-)

Hi Barnabás,

Thanks for the patch.

Quoting Barnabás Pőcze (2025-05-15 14:30:39)
> This reverts commit 10cdc914dad282b4ca0ad11067d5c6d446af1fcc.
> 
> The contstructors introduced by that commit are not used anywhere,
> and they do not match the existing practice for boolean controls.
> 
> Specifically, every single boolean control is described by calling
> the `ControlInfo(ControlValue, ControlValue, ControlValue = {})`
> constructor. Crucially, that constructor does not set `values_`,
> while the two removed contructors do. And whether or not `values_`
> has any elements is currently used as an implicit sign to decide
> whether or not the control is "enum-like", and those are assumed
> to have type `int32_t`.

I remember adding these constructors because if values_ is not populated then
applications can't enumerate the possible values of the control. (I suppose it
was an oversight that those constructors didn't get selected properly, though).

There might be debate whether or not boolean controls needs to be iterated, but
I had imagined that some platforms might want to report a control to be
supported, but only one boolean value is supported. Something like a uvc camera
declaring that it doesn't have ae but it supports manual controls. (Even though
we might be moving away from boolean enable flags to enum mode flags)

As for determining whether or not a control is enum-like, I remember adding
ControlId::isArray(). Although that acts on ControlId as opposed to
ControlInfo... Is that insufficient?


Thanks,

Paul

> 
> For example, any boolean control described using any of the two
> removed constructors would cause an assertion in failure in
> `CameraSession::listControls()` when calling `value.get<int32_t>()`.
> 
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  include/libcamera/controls.h   |  3 ---
>  src/libcamera/controls.cpp     | 29 -----------------------------
>  test/controls/control_info.cpp | 33 ---------------------------------
>  3 files changed, 65 deletions(-)
> 
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index 2ae4ec3d4..32fb31f78 100644
> --- a/include/libcamera/controls.h
> +++ b/include/libcamera/controls.h
> @@ -10,7 +10,6 @@
>  #include <assert.h>
>  #include <map>
>  #include <optional>
> -#include <set>
>  #include <stdint.h>
>  #include <string>
>  #include <unordered_map>
> @@ -334,8 +333,6 @@ public:
>                              const ControlValue &def = {});
>         explicit ControlInfo(Span<const ControlValue> values,
>                              const ControlValue &def = {});
> -       explicit ControlInfo(std::set<bool> values, bool def);
> -       explicit ControlInfo(bool value);
>  
>         const ControlValue &min() const { return min_; }
>         const ControlValue &max() const { return max_; }
> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> index 70f6f6092..eaa34e70b 100644
> --- a/src/libcamera/controls.cpp
> +++ b/src/libcamera/controls.cpp
> @@ -625,35 +625,6 @@ ControlInfo::ControlInfo(Span<const ControlValue> values,
>                 values_.push_back(value);
>  }
>  
> -/**
> - * \brief Construct a boolean ControlInfo with both boolean values
> - * \param[in] values The control valid boolean values (both true and false)
> - * \param[in] def The control default boolean value
> - *
> - * Construct a ControlInfo for a boolean control, where both true and false are
> - * valid values. \a values must be { false, true } (the order is irrelevant).
> - * The minimum value will always be false, and the maximum always true. The
> - * default value is \a def.
> - */
> -ControlInfo::ControlInfo(std::set<bool> values, bool def)
> -       : min_(false), max_(true), def_(def), values_({ false, true })
> -{
> -       ASSERT(values.count(def) && values.size() == 2);
> -}
> -
> -/**
> - * \brief Construct a boolean ControlInfo with only one valid value
> - * \param[in] value The control valid boolean value
> - *
> - * Construct a ControlInfo for a boolean control, where there is only valid
> - * value. The minimum, maximum, and default values will all be \a value.
> - */
> -ControlInfo::ControlInfo(bool value)
> -       : min_(value), max_(value), def_(value)
> -{
> -       values_ = { value };
> -}
> -
>  /**
>   * \fn ControlInfo::min()
>   * \brief Retrieve the minimum value of the control
> diff --git a/test/controls/control_info.cpp b/test/controls/control_info.cpp
> index e1bb43f0e..09c17ae63 100644
> --- a/test/controls/control_info.cpp
> +++ b/test/controls/control_info.cpp
> @@ -46,39 +46,6 @@ protected:
>                         return TestFail;
>                 }
>  
> -               /*
> -                * Test information retrieval from a control with boolean
> -                * values.
> -                */
> -               ControlInfo aeEnable({ false, true }, false);
> -
> -               if (aeEnable.min().get<bool>() != false ||
> -                   aeEnable.def().get<bool>() != false ||
> -                   aeEnable.max().get<bool>() != true) {
> -                       cout << "Invalid control range for AeEnable" << endl;
> -                       return TestFail;
> -               }
> -
> -               if (aeEnable.values()[0].get<bool>() != false ||
> -                   aeEnable.values()[1].get<bool>() != true) {
> -                       cout << "Invalid control values for AeEnable" << endl;
> -                       return TestFail;
> -               }
> -
> -               ControlInfo awbEnable(true);
> -
> -               if (awbEnable.min().get<bool>() != true ||
> -                   awbEnable.def().get<bool>() != true ||
> -                   awbEnable.max().get<bool>() != true) {
> -                       cout << "Invalid control range for AwbEnable" << endl;
> -                       return TestFail;
> -               }
> -
> -               if (awbEnable.values()[0].get<bool>() != true) {
> -                       cout << "Invalid control values for AwbEnable" << endl;
> -                       return TestFail;
> -               }
> -
>                 return TestPass;
>         }
>  };
> -- 
> 2.49.0
>

Hi

2025. 05. 15. 15:05 keltezéssel, Paul Elder írta:
> Hi Barnabás,
> 
> Thanks for the patch.
> 
> Quoting Barnabás Pőcze (2025-05-15 14:30:39)
>> This reverts commit 10cdc914dad282b4ca0ad11067d5c6d446af1fcc.
>>
>> The contstructors introduced by that commit are not used anywhere,
>> and they do not match the existing practice for boolean controls.
>>
>> Specifically, every single boolean control is described by calling
>> the `ControlInfo(ControlValue, ControlValue, ControlValue = {})`
>> constructor. Crucially, that constructor does not set `values_`,
>> while the two removed contructors do. And whether or not `values_`
>> has any elements is currently used as an implicit sign to decide
>> whether or not the control is "enum-like", and those are assumed
>> to have type `int32_t`.
> 
> I remember adding these constructors because if values_ is not populated then
> applications can't enumerate the possible values of the control. (I suppose it
> was an oversight that those constructors didn't get selected properly, though).

One could argue that for boolean controls min()/max() can be sufficient...


> 
> There might be debate whether or not boolean controls needs to be iterated, but
> I had imagined that some platforms might want to report a control to be
> supported, but only one boolean value is supported. Something like a uvc camera
> declaring that it doesn't have ae but it supports manual controls. (Even though
> we might be moving away from boolean enable flags to enum mode flags)

... min()/max() can be the same if only a single boolean value is actually allowed.


> 
> As for determining whether or not a control is enum-like, I remember adding
> ControlId::isArray(). Although that acts on ControlId as opposed to
> ControlInfo... Is that insufficient?

I'm not sure if `ControlId::isArray()` does that. Maybe you meant
`ControlId::enumerators()`? I think that could work, but e.g.
`CameraSession::listControls()` does not use it at the moment.

In any case, I'm just saying that the current situation is not ideal,
and something should be changed. And this seemed like the least intrusive
first step. (Although admittedly I do not have further steps in mind.)


Regards,
Barnabás Pőcze

> 
> 
> Thanks,
> 
> Paul
> 
>>
>> For example, any boolean control described using any of the two
>> removed constructors would cause an assertion in failure in
>> `CameraSession::listControls()` when calling `value.get<int32_t>()`.
>>
>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>> ---
>>   include/libcamera/controls.h   |  3 ---
>>   src/libcamera/controls.cpp     | 29 -----------------------------
>>   test/controls/control_info.cpp | 33 ---------------------------------
>>   3 files changed, 65 deletions(-)
>>
>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
>> index 2ae4ec3d4..32fb31f78 100644
>> --- a/include/libcamera/controls.h
>> +++ b/include/libcamera/controls.h
>> @@ -10,7 +10,6 @@
>>   #include <assert.h>
>>   #include <map>
>>   #include <optional>
>> -#include <set>
>>   #include <stdint.h>
>>   #include <string>
>>   #include <unordered_map>
>> @@ -334,8 +333,6 @@ public:
>>                               const ControlValue &def = {});
>>          explicit ControlInfo(Span<const ControlValue> values,
>>                               const ControlValue &def = {});
>> -       explicit ControlInfo(std::set<bool> values, bool def);
>> -       explicit ControlInfo(bool value);
>>   
>>          const ControlValue &min() const { return min_; }
>>          const ControlValue &max() const { return max_; }
>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
>> index 70f6f6092..eaa34e70b 100644
>> --- a/src/libcamera/controls.cpp
>> +++ b/src/libcamera/controls.cpp
>> @@ -625,35 +625,6 @@ ControlInfo::ControlInfo(Span<const ControlValue> values,
>>                  values_.push_back(value);
>>   }
>>   
>> -/**
>> - * \brief Construct a boolean ControlInfo with both boolean values
>> - * \param[in] values The control valid boolean values (both true and false)
>> - * \param[in] def The control default boolean value
>> - *
>> - * Construct a ControlInfo for a boolean control, where both true and false are
>> - * valid values. \a values must be { false, true } (the order is irrelevant).
>> - * The minimum value will always be false, and the maximum always true. The
>> - * default value is \a def.
>> - */
>> -ControlInfo::ControlInfo(std::set<bool> values, bool def)
>> -       : min_(false), max_(true), def_(def), values_({ false, true })
>> -{
>> -       ASSERT(values.count(def) && values.size() == 2);
>> -}
>> -
>> -/**
>> - * \brief Construct a boolean ControlInfo with only one valid value
>> - * \param[in] value The control valid boolean value
>> - *
>> - * Construct a ControlInfo for a boolean control, where there is only valid
>> - * value. The minimum, maximum, and default values will all be \a value.
>> - */
>> -ControlInfo::ControlInfo(bool value)
>> -       : min_(value), max_(value), def_(value)
>> -{
>> -       values_ = { value };
>> -}
>> -
>>   /**
>>    * \fn ControlInfo::min()
>>    * \brief Retrieve the minimum value of the control
>> diff --git a/test/controls/control_info.cpp b/test/controls/control_info.cpp
>> index e1bb43f0e..09c17ae63 100644
>> --- a/test/controls/control_info.cpp
>> +++ b/test/controls/control_info.cpp
>> @@ -46,39 +46,6 @@ protected:
>>                          return TestFail;
>>                  }
>>   
>> -               /*
>> -                * Test information retrieval from a control with boolean
>> -                * values.
>> -                */
>> -               ControlInfo aeEnable({ false, true }, false);
>> -
>> -               if (aeEnable.min().get<bool>() != false ||
>> -                   aeEnable.def().get<bool>() != false ||
>> -                   aeEnable.max().get<bool>() != true) {
>> -                       cout << "Invalid control range for AeEnable" << endl;
>> -                       return TestFail;
>> -               }
>> -
>> -               if (aeEnable.values()[0].get<bool>() != false ||
>> -                   aeEnable.values()[1].get<bool>() != true) {
>> -                       cout << "Invalid control values for AeEnable" << endl;
>> -                       return TestFail;
>> -               }
>> -
>> -               ControlInfo awbEnable(true);
>> -
>> -               if (awbEnable.min().get<bool>() != true ||
>> -                   awbEnable.def().get<bool>() != true ||
>> -                   awbEnable.max().get<bool>() != true) {
>> -                       cout << "Invalid control range for AwbEnable" << endl;
>> -                       return TestFail;
>> -               }
>> -
>> -               if (awbEnable.values()[0].get<bool>() != true) {
>> -                       cout << "Invalid control values for AwbEnable" << endl;
>> -                       return TestFail;
>> -               }
>> -
>>                  return TestPass;
>>          }
>>   };
>> -- 
>> 2.49.0
>>