[libcamera-devel] [PATCH v2 6/9] libcamera: controls: List-initialize ControlList
Jacopo Mondi
jacopo at jmondi.org
Wed Dec 23 17:34:34 CET 2020
Hi Laurent,
On Wed, Dec 23, 2020 at 07:56:50AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Fri, Dec 18, 2020 at 05:47:51PM +0100, Jacopo Mondi wrote:
> > Provide an additional constructor to the ControlList class to
> > allow the construction of instances of the class with the usage
> > of an std::intializer_list<> of ControlId and an associated
> > ControlValue.
> >
> > The new constructor allows to intialize a ControlList instance
>
> s/intialize/initialize/
>
> > at construction time, allowing the declaration of populated
> > ControlList instances.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> > include/libcamera/controls.h | 2 ++
> > src/libcamera/controls.cpp | 11 +++++++++++
> > 2 files changed, 13 insertions(+)
> >
> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > index 3634dc431618..4df6615a3c7b 100644
> > --- a/include/libcamera/controls.h
> > +++ b/include/libcamera/controls.h
> > @@ -348,9 +348,11 @@ class ControlList
> > {
> > private:
> > using ControlListMap = std::unordered_map<unsigned int, ControlValue>;
> > + using ControlsMap = std::unordered_map<const ControlId *, ControlValue>;
>
> As the ControlId should never be null, should this be a reference ?
>
> >
> > public:
> > ControlList();
> > + ControlList(std::initializer_list<ControlsMap::value_type> controls);
> > ControlList(const ControlIdMap &idmap, ControlValidator *validator = nullptr);
> > ControlList(const ControlInfoMap &infoMap, ControlValidator *validator = nullptr);
> >
> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> > index c58ed3946f3b..7650057dde7d 100644
> > --- a/src/libcamera/controls.cpp
> > +++ b/src/libcamera/controls.cpp
> > @@ -797,6 +797,17 @@ ControlList::ControlList()
> > {
> > }
> >
> > +/**
> > + * \brief Construct a ControlList with a list of values
> > + * \param[in] controls The controls and associated values to initialize the list with
> > + */
> > +ControlList::ControlList(std::initializer_list<ControlsMap::value_type> controls)
> > + : ControlList()
> > +{
> > + for (const auto &ctrl : controls)
> > + set(ctrl.first->id(), ctrl.second);
>
> The annoying part here is to we lose type safety. The ControlValue in
> the controls variable is constructor without any type inference based on
> a Control<T>. This leads to the explicit construction of, for instance,
> Span<const float>, in patch 7/9. Is there a way we could do better ?
>
I'll keep the patch like it is right now in next version as I've got
so many piled up patches this seems like a minor issue, as the only
use case for statically initialized ControlList is the sensor database
current implementation. It can be changed on top if desired.
> > +}
> > +
> > /**
> > * \brief Construct a ControlList with an optional control validator
> > * \param[in] idmap The ControlId map for the control list target object
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list