[libcamera-devel] [PATCH v2 6/9] libcamera: controls: List-initialize ControlList

[Jacopo Mondi]

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