[PATCH v1 2/2] Revert "controls: Add boolean constructors for ControlInfo"
Barnabás Pőcze
barnabas.pocze at ideasonboard.com
Fri May 16 17:32:22 CEST 2025
Hi
2025. 05. 16. 14:21 keltezéssel, Paul Elder írta:
> Quoting Paul Elder (2025-05-16 14:18:39)
>> Quoting Barnabás Pőcze (2025-05-15 15:27:02)
>>> 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...
>>
>> Hm, good point.
>>
>>>
>>>
>>>>
>>>> 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.
>>
>> Right.
>>
>>>
>>>
>>>>
>>>> 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.
>>
>> Oops yeah that's what I meant.
>>
>>> `CameraSession::listControls()` does not use it at the moment.
>>
>> Hm it indeed does not.
>>
>> imo it probably should, but now I see where you're going with this. I guess it
>> doesn't really make sense either to enumerate possible values of a boolen,and
>> min/max is indeed enough.
>>
>>> 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.)
>>
>> Yes, that's true. I think maybe an upgrade to the documentation about checking
>> types of controls and how to retrieve the possible values of a ControlInfo
>> might be good, plus fixing how our apps do these. But we can do this on top.
>>
>> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
>>
>>>
>>>
>>> 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 at 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;
>>>>> - }
>
> Wait, can't we keep this test?
The constructor needs to be modified as follows:
ControlInfo aeEnable(false, true, false);
>
>>>>> -
>>>>> - if (aeEnable.values()[0].get<bool>() != false ||
>>>>> - aeEnable.values()[1].get<bool>() != true) {
>>>>> - cout << "Invalid control values for AeEnable" << endl;
>>>>> - return TestFail;
>>>>> - }
>
> ig we can drop this one.
>
>>>>> -
>>>>> - 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;
>>>>> - }
>
> I also want to keep this one.
This constructor also needs to be modified:
ControlInfo aeEnable(true, true, true);
So should I make these changes?
Regards,
Barnabás Pőcze
>
>>>>> -
>>>>> - if (awbEnable.values()[0].get<bool>() != true) {
>>>>> - cout << "Invalid control values for AwbEnable" << endl;
>>>>> - return TestFail;
>>>>> - }
>
> And this one can be dropped too ig.
>
>
> Paul
>
>>>>> -
>>>>> return TestPass;
>>>>> }
>>>>> };
>>>>> --
>>>>> 2.49.0
>>>>>
>>>
More information about the libcamera-devel
mailing list