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

10687 diff mbox series

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
at construction time, allowing the declaration of populated
ControlList instances.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 include/libcamera/controls.h |  2 ++
 src/libcamera/controls.cpp   | 11 +++++++++++
 2 files changed, 13 insertions(+)

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@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 ?

> +}
> +
>  /**
>   * \brief Construct a ControlList with an optional control validator
>   * \param[in] idmap The ControlId map for the control list target object

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@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