[PATCH v1 2/2] Revert "controls: Add boolean constructors for ControlInfo"
Paul Elder
paul.elder at ideasonboard.com
Fri May 16 23:45:48 CEST 2025
Quoting Barnabás Pőcze (2025-05-16 17:32:22)
> 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);
Ah indeed, you're right.
>
>
> So should I make these changes?
Yes, please.
Thanks,
Paul
>
>
> 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