[PATCH v1 2/2] Revert "controls: Add boolean constructors for ControlInfo"
Paul Elder
paul.elder at ideasonboard.com
Fri May 16 14:18:39 CEST 2025
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;
> >> - }
> >> -
> >> - 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
> >>
>
More information about the libcamera-devel
mailing list