[PATCH v1 2/2] Revert "controls: Add boolean constructors for ControlInfo"
Paul Elder
paul.elder at ideasonboard.com
Fri May 16 14:21:40 CEST 2025
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?
> > >> -
> > >> - 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.
> > >> -
> > >> - 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