[libcamera-devel] [RFC PATCH v2 5/9] libcamera: Implement a ControlList

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Sun, Jun 23, 2019 at 05:35:01PM +0200, Niklas Söderlund wrote:
> On 2019-06-21 17:13:57 +0100, Kieran Bingham wrote:
> > Define a ControlList which wraps the STL std::unordered_map to contain
> > a list of associative pairs of ControlInfo and Value items.
> > 
> > A ControlInfo (or represented by its ControlId) together with a Value
> > provide a control item which can be interpreted by the internal IPA and
> > PipelineHandler components.
> > 
> > To pass a set of controls around, a ControlList  can be added to a


s/  / /

> > Request or stored locally to establish a set of default values for later
> > reuse.
> > 
> > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > ---
> >  include/libcamera/controls.h |  41 +++++++++++
> >  src/libcamera/controls.cpp   | 128 +++++++++++++++++++++++++++++++++++
> >  2 files changed, 169 insertions(+)
> > 
> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > index 95198d41c4cf..5835b840a31b 100644
> > --- a/include/libcamera/controls.h
> > +++ b/include/libcamera/controls.h
> > @@ -4,6 +4,9 @@
> >   *
> >   * controls.h - Control handling
> >   */
> > +
> > +#include <unordered_map>
> > +
> >  #ifndef __LIBCAMERA_CONTROLS_H__
> >  #define __LIBCAMERA_CONTROLS_H__
> >  
> > @@ -53,6 +56,44 @@ private:
> >  
> >  std::ostream &operator<<(std::ostream &stream, const ControlInfo &value);
> >  
> > +class ControlList
> > +{
> > +private:
> > +	struct ControlInfoHasher {
> > +		std::size_t operator()(const ControlInfo &ci) const
> > +		{
> > +			return std::hash<int>()(ci.id());
> > +		}
> > +	};
> > +
> > +	typedef std::unordered_map<ControlInfo, Value, ControlInfoHasher> ControlListMap;

We usually use the C++11 syntax

	using ControlListMap = std::unordered_map<ControlInfo, Value, ControlInfoHasher>;

I don't think you should copy the ControlInfo in the ControlList. If I
understand your patch series correctly, ControlInfo should be created
with the camera, and remain constant for the lifetime of the camera. The
map should thus use either the control ID or a pointer to the control
info as the key.

> Is this needed to go first or can it be moved down to the other private 
> section?
> 
> > +
> > +public:
> > +	ControlList();
> > +
> > +	using iterator = ControlListMap::iterator;
> > +	using const_iterator = ControlListMap::const_iterator;
> > +
> > +	iterator begin() { return controls_.begin(); }
> > +	iterator end() { return controls_.end(); }
> > +	const_iterator begin() const { return controls_.begin(); }
> > +	const_iterator end() const { return controls_.end(); }
> > +
> > +	bool contains(ControlId id) const { return controls_.count(id); };
> 
> Would not return controls_.find(id) != controls_.end() be better?
> 
> > +	bool empty() const { return controls_.empty(); }
> > +	size_t size() const { return controls_.size(); }

std::size_t

> > +	void reset() { controls_.clear(); }

Should we call this clear() ?

> > +
> > +	Value &operator[](ControlId id) { return controls_[id]; }
> > +
> > +	void update(const ControlList &list);
> > +	void update(enum ControlId id, const Value &value);
> > +	void update(const ControlInfo &info, const Value &value);
> > +
> > +private:
> > +	ControlListMap controls_;
> > +};
> > +
> >  } /* namespace libcamera */
> >  
> >  #endif /* __LIBCAMERA_CONTROLS_H__ */
> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> > index b1be46ddb55e..7c55afffe4ca 100644
> > --- a/src/libcamera/controls.cpp
> > +++ b/src/libcamera/controls.cpp
> > @@ -157,4 +157,132 @@ std::ostream &operator<<(std::ostream &stream, const ControlInfo &info)
> >  	return stream << info.toString();
> >  }
> >  
> > +/**
> > + * \class ControlList
> > + * \brief Associates a list of ControlIds with their values.
> > + *
> > + * The ControlList stores a pair of ControlInfo and Value classes as an
> > + * unordered map. Entries can be updated using array syntax such as
> > + *   myControlList[MyControlId] = myValue;
> > + * or
> > + *   myNewValue = myControlList[MyControlId];
> 
> I don't think this will update the value.
> 
> I think you can drop the examples and the last sentence from the 
> documentation.
> 
> With these issues addressed,
> 
> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> 
> > + */
> > +
> > +/**
> > + * \brief Construct a ControlList
> > + */
> > +ControlList::ControlList()
> > +{
> > +}
> > +

If the constructor is empty and is the only one you can omit it, the
compiler will generate one for your.

> > +/**
> > + * \typedef ControlList::iterator
> > + * \brief Iterator for the controls contained within the list.
> > + */
> > +
> > +/**
> > + * \typedef ControlList::const_iterator
> > + * \brief Const iterator for the controls contained within the list.
> > + */
> > +
> > +/**
> > + * \fn iterator ControlList::begin()
> > + * \brief Retrieve an iterator to the first Control in the list
> > + * \return An iterator to the first Control in the list
> > + */
> > +
> > +/**
> > + * \fn iterator ControlList::end()
> > + * \brief Retrieve an iterator to the next element after the last controls in
> > + * the instance.
> > + * \return An iterator to the element following the last control in the instance
> > + */
> > +
> > +/**
> > + * \fn const_iterator ControlList::begin() const
> > + * \brief Retrieve a const_iterator to the first Control in the list
> > + * \return A const_iterator to the first Control in the list
> > + */
> > +
> > +/**
> > + * \fn const_iterator ControlList::end() const
> > + * \brief Retrieve a constant iterator pointing to an empty element after the
> > + * \return A const iterator to the element following the last control in the
> > + * instance
> > + */
> > +
> > +/**
> > + * \fn ControlList::contains()
> > + * \brief Identify if the List contains a control with the specified ControlId
> > + * \return True if the list contains a matching control, false otherwise
> > + */
> > +
> > +/**
> > + * \fn ControlList::empty()
> > + * \brief Identify if the list is empty
> > + * \return True if the list does not contain any control, false otherwise
> > + */
> > +
> > +/**
> > + * \fn ControlList::size()
> > + * \brief Retrieve the number of controls in the list
> > + * \return The number of Control entries stored in the list
> > + */
> > +
> > +/**
> > + * \fn ControlList::reset()
> > + * \brief Removes all controls from the list
> > + */
> > +
> > +/**
> > + * \fn ControlList::operator[]()
> > + * \brief Access the control with the given Id
> > + * \param id The id of the control to access
> > + * \return The Value of the control with a ControlId of \a Id
> > + */
> > +
> > +/**
> > + * \brief Update all Control values with the value from the given \a list
> > + * \param list The list of controls to update or append to this list
> > + *
> > + * Update all controls in the ControlList, by the values given by \a list
> > + * If the list already contains a control of this ID then it will be overwritten
> > + */
> > +void ControlList::update(const ControlList &list)
> > +{
> > +	for (auto it : list) {
> > +		const ControlInfo &info = it.first;
> > +		const Value &value = it.second;
> > +
> > +		controls_[info] = value;
> > +	}
> > +}
> > +
> > +/**
> > + * \brief Update a control value in the list
> > + * \param id The ControlId of the control to update
> > + * \param value The new value for the updated control
> > + *
> > + * Set the Control value in the list to the appropriate value
> > + * If the list already contains a control of this ID then it will be overwritten
> > + */
> > +void ControlList::update(enum ControlId id, const Value &value)
> > +{
> > +	controls_[id] = value;
> > +}
> > +
> > +/**
> > + * \brief Update a control value in the list.
> > + * \param info The ControlInfo of the control to update
> > + * \param value The new value for the updated control
> > + *
> > + * Set the Control value in the list to the appropriate value.
> > + * If the list already contains a control of the Id matching the ControlInfo
> > + * then it will be overwritten.
> > + */
> > +void ControlList::update(const ControlInfo &info, const Value &value)
> > +{
> > +	controls_[info] = value;
> > +}

Do we really need this class ? Can't we just expose 

	using ControlList = std::unordered_map<ControlInfo, Value, ControlInfoHasher>;

? The last two update functions would be replaced by usage of
operator[], and the first one with

template< class InputIt >
void std::unordered_map::insert( InputIt first, InputIt last );

> > +
> >  } /* namespace libcamera */

-- 
Regards,

Laurent Pinchart