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

1498 diff mbox series

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
Request or stored locally to establish a set of default values for later
reuse.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 include/libcamera/controls.h |  41 +++++++++++
 src/libcamera/controls.cpp   | 128 +++++++++++++++++++++++++++++++++++
 2 files changed, 169 insertions(+)

Hi Kieran,

Thanks for your patch.

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
> Request or stored locally to establish a set of default values for later
> reuse.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@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;

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(); }
> +	void reset() { controls_.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@ragnatech.se>

> + */
> +
> +/**
> + * \brief Construct a ControlList
> + */
> +ControlList::ControlList()
> +{
> +}
> +
> +/**
> + * \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;
> +}
> +
>  } /* namespace libcamera */
> -- 
> 2.20.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

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

Hi Niklas,

On 23/06/2019 16:35, Niklas Söderlund wrote:
> Hi Kieran,
> 
> Thanks for your patch.
> 
> 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
>> Request or stored locally to establish a set of default values for later
>> reuse.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham@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;
> 
> Is this needed to go first or can it be moved down to the other private 
> section?

It has to be defined before the usages below.

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

I thought this was elegant.

Count == 0 = false,
count > 0 = true...


>> +	bool empty() const { return controls_.empty(); }
>> +	size_t size() const { return controls_.size(); }
>> +	void reset() { controls_.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.

No - it updates myNewValue. I'll just remove this. I was trying to put
in that the the [] is the accessor.


> 
> 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@ragnatech.se>
> 
>> + */
>> +
>> +/**
>> + * \brief Construct a ControlList
>> + */
>> +ControlList::ControlList()
>> +{
>> +}
>> +
>> +/**
>> + * \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;
>> +}
>> +
>>  } /* namespace libcamera */
>> -- 
>> 2.20.1
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel@lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
>

Hi Laurent,

On 23/06/2019 23:21, Laurent Pinchart wrote:
> 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/  / /

I wish vim reflow/automatic wrap would do that....

>>> Request or stored locally to establish a set of default values for later
>>> reuse.
>>>
>>> Signed-off-by: Kieran Bingham <kieran.bingham@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 prefer this :-D

Updated.

> 
> I don't think you should copy the ControlInfo in the ControlList. 

I was trying not to - It was supposed to be a ControlInfo & but creating
the original ControlInfo's has caused me headache ...

> If I
> understand your patch series correctly, ControlInfo should be created
> with the camera, and remain constant for the lifetime of the camera. 

Yes, I want to create ControlInfo structures which are constant for the
camera lifetime, and contain any values appropriate to that camera instance.


> The
> map should thus use either the control ID or a pointer to the control
> info as the key.


An ID should be easy to support, but means code will have to look up the
ControlInfo later, whereas a pointer to the ControlInfo needs to look it
up internally.

This is essentially why this topic is still RFC for me.


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


Sure.

> 
>>> +	void reset() { controls_.clear(); }
> 
> Should we call this clear() ?


Sure.


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


Good point, removed.

> 
>>> +/**
>>> + * \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>;

Hrm ... well I think the intention was to be able to add some extra
validation later... but I now wonder if just the ControlList 'using'
might be easier, and remove a lot of rubbish passthrough code ...


I'll give it a go - but I don't want to throw away a whole bunch of code
to re-add it again soon after ...

Best save this as a branch (well although it's saved on list at least)....


> ? 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 );

'just like that' ?

I'll play there next.

Removing the ControlList class would remove my desire to have all the
tests that you disagree with later in the series ...


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

Hi Kieran,

On Mon, Jun 24, 2019 at 04:48:05PM +0100, Kieran Bingham wrote:
> On 23/06/2019 16:35, 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
> >> Request or stored locally to establish a set of default values for later
> >> reuse.
> >>
> >> Signed-off-by: Kieran Bingham <kieran.bingham@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;
> > 
> > Is this needed to go first or can it be moved down to the other private 
> > section?
> 
> It has to be defined before the usages below.
> 
> >> +
> >> +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?
> 
> I thought this was elegant.
> 
> Count == 0 = false,
> count > 0 = true...

But it's less efficient as it has to go through the whole list in all
cases.

> >> +	bool empty() const { return controls_.empty(); }
> >> +	size_t size() const { return controls_.size(); }
> >> +	void reset() { controls_.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.
> 
> No - it updates myNewValue. I'll just remove this. I was trying to put
> in that the the [] is the accessor.
> 
> > 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@ragnatech.se>
> > 
> >> + */
> >> +
> >> +/**
> >> + * \brief Construct a ControlList
> >> + */
> >> +ControlList::ControlList()
> >> +{
> >> +}
> >> +
> >> +/**
> >> + * \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;
> >> +}
> >> +
> >>  } /* namespace libcamera */

Hi Kieran,

On Mon, Jun 24, 2019 at 05:02:55PM +0100, Kieran Bingham wrote:
> On 23/06/2019 23:21, Laurent Pinchart wrote:
> > 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/  / /
> 
> I wish vim reflow/automatic wrap would do that....
> 
> >>> Request or stored locally to establish a set of default values for later
> >>> reuse.
> >>>
> >>> Signed-off-by: Kieran Bingham <kieran.bingham@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 prefer this :-D
> 
> Updated.
> 
> > I don't think you should copy the ControlInfo in the ControlList. 
> 
> I was trying not to - It was supposed to be a ControlInfo & but creating
> the original ControlInfo's has caused me headache ...
> 
> > If I
> > understand your patch series correctly, ControlInfo should be created
> > with the camera, and remain constant for the lifetime of the camera. 
> 
> Yes, I want to create ControlInfo structures which are constant for the
> camera lifetime, and contain any values appropriate to that camera instance.
> 
> > The
> > map should thus use either the control ID or a pointer to the control
> > info as the key.
> 
> An ID should be easy to support, but means code will have to look up the
> ControlInfo later, whereas a pointer to the ControlInfo needs to look it
> up internally.
> 
> This is essentially why this topic is still RFC for me.
> 
> >> 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
> 
> Sure.
> 
> >>> +	void reset() { controls_.clear(); }
> > 
> > Should we call this clear() ?
> 
> Sure.
> 
> >>> +
> >>> +	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@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.
> 
> Good point, removed.
> 
> >>> +/**
> >>> + * \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>;
> 
> Hrm ... well I think the intention was to be able to add some extra
> validation later... but I now wonder if just the ControlList 'using'
> might be easier, and remove a lot of rubbish passthrough code ...
> 
> I'll give it a go - but I don't want to throw away a whole bunch of code
> to re-add it again soon after ...

git history to the rescue :-) (and mailing list archives)

> Best save this as a branch (well although it's saved on list at least)....
> 
> > ? 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 );
> 
> 'just like that' ?

controls.insert(otherControls.begin(), otherControls.end());

> I'll play there next.
> 
> Removing the ControlList class would remove my desire to have all the
> tests that you disagree with later in the series ...

I swear the proposal has nothing to do with the test discussion :-)

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

Hi Laurent, Niklas,

On 24/06/2019 17:27, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Mon, Jun 24, 2019 at 04:48:05PM +0100, Kieran Bingham wrote:
>> On 23/06/2019 16:35, 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
>>>> Request or stored locally to establish a set of default values for later
>>>> reuse.
>>>>
>>>> Signed-off-by: Kieran Bingham <kieran.bingham@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;
>>>
>>> Is this needed to go first or can it be moved down to the other private 
>>> section?
>>
>> It has to be defined before the usages below.
>>
>>>> +
>>>> +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?
>>
>> I thought this was elegant.
>>
>> Count == 0 = false,
>> count > 0 = true...
> 
> But it's less efficient as it has to go through the whole list in all
> cases.

Hrm... I thought with the guaranteed single key entry, and known hash
algorithm this would be optimised internally : However:

www.cplusplus.com/reference/unordered_map/unordered_map/{count,find}:

Count elements with a specific key
Searches the container for elements whose key is k and returns the
number of elements found. Because unordered_map containers do not allow
for duplicate keys, this means that the function actually returns 1 if
an element with that key exists in the container, and zero otherwise.

But:

Complexity[Count]
Average case: linear in the number of elements counted.
Worst case: linear in container size.

vs

Complexity[Find]
Average case: constant.
Worst case: linear in container size.


Yup... that certainly points to using find();.


>>>> +	bool empty() const { return controls_.empty(); }
>>>> +	size_t size() const { return controls_.size(); }
>>>> +	void reset() { controls_.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.
>>
>> No - it updates myNewValue. I'll just remove this. I was trying to put
>> in that the the [] is the accessor.
>>
>>> 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@ragnatech.se>

Thankyou, currently trying to work out if I can drop the whole wrapper
easily though :S

>>>
>>>> + */
>>>> +
>>>> +/**
>>>> + * \brief Construct a ControlList
>>>> + */
>>>> +ControlList::ControlList()
>>>> +{
>>>> +}
>>>> +
>>>> +/**
>>>> + * \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;
>>>> +}
>>>> +
>>>>  } /* namespace libcamera */
>

Hi Laurent,

A whole bunch of my development thoughts as I went through
converting/removing the ControlList abstraction.

TLDR: I'm removing the Class abstraction and just using the
std::unordered_map directly through the typedef/using.

Seems like it might be worth going back to being a map of straight
ControlID, {Control}Value pairs too - and keep the ControlInfo entirely
separated. Otherwise anytime a control is added to the list directly - a
lookup will have to be done on the ControlInfo even if it's not needed...


On 24/06/2019 17:30, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Mon, Jun 24, 2019 at 05:02:55PM +0100, Kieran Bingham wrote:
>> On 23/06/2019 23:21, Laurent Pinchart wrote:
>>> 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/  / /
>>
>> I wish vim reflow/automatic wrap would do that....
>>
>>>>> Request or stored locally to establish a set of default values for later
>>>>> reuse.
>>>>>
>>>>> Signed-off-by: Kieran Bingham <kieran.bingham@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 prefer this :-D
>>
>> Updated.
>>
>>> I don't think you should copy the ControlInfo in the ControlList. 
>>
>> I was trying not to - It was supposed to be a ControlInfo & but creating
>> the original ControlInfo's has caused me headache ...
>>
>>> If I
>>> understand your patch series correctly, ControlInfo should be created
>>> with the camera, and remain constant for the lifetime of the camera. 
>>
>> Yes, I want to create ControlInfo structures which are constant for the
>> camera lifetime, and contain any values appropriate to that camera instance.
>>
>>> The
>>> map should thus use either the control ID or a pointer to the control
>>> info as the key.
>>
>> An ID should be easy to support, but means code will have to look up the
>> ControlInfo later, whereas a pointer to the ControlInfo needs to look it
>> up internally.
>>
>> This is essentially why this topic is still RFC for me.
>>
>>>> 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
>>
>> Sure.
>>
>>>>> +	void reset() { controls_.clear(); }
>>>
>>> Should we call this clear() ?
>>
>> Sure.
>>
>>>>> +
>>>>> +	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@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.
>>
>> Good point, removed.
>>
>>>>> +/**
>>>>> + * \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>;
>>
>> Hrm ... well I think the intention was to be able to add some extra
>> validation later... but I now wonder if just the ControlList 'using'
>> might be easier, and remove a lot of rubbish passthrough code ...
>>
>> I'll give it a go - but I don't want to throw away a whole bunch of code
>> to re-add it again soon after ...
> 
> git history to the rescue :-) (and mailing list archives)
> 
>> Best save this as a branch (well although it's saved on list at least)....
>>
>>> ? 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 );
>>
>> 'just like that' ?
> 
> controls.insert(otherControls.begin(), otherControls.end());

I wonder if some helpers would make it 'nicer', but because there is no
class - it feels a bit more ugly (loses it's OO?):

  void controlListUpdate(targetList, sourceList) {
	targetList.insert(sourceList.begin(), sourceList.end());
  }

Any ideas or suggestions on how to do these helpers nicely? Ideally I'd
want the ControlList to look like a class object - but I don't think I
can just extend the std::unordered_map() interface ...

I'd also want a helper for the .contains() method

(which is a shame, because in C++20 .contains() is available directly on
the unordered_map)


Perhaps it's a non-issue anyway, as if we only care about initialising a
list with a 'preset' set of controls, then that can be done with the
operator=():

  ControlList newList = defaultList;

But that removes any existing items in the new list, so can't be used to
update an existing list.

Still if that becomes necessary - then we can worry about adding the helper.

Likewise, perhaps the usage of .contains() won't be really necessary
just yet, and it would be more optimal to use the find() directly so
that in the event that it is found, you don't then have to 'search again'.


TLDR: I've cancelled out my concerns :) - dropping the Class and
replacing with the 'using' typedef.


>> I'll play there next.
>>
>> Removing the ControlList class would remove my desire to have all the
>> tests that you disagree with later in the series ...
> 
> I swear the proposal has nothing to do with the test discussion :-)

Hahah - of course, I actually like the idea of just using the typdef on
the map itself. I don't want to pointlessly wrap the standard library.

If there's a good way to implement helpers - then this looks like a good
approach ...

Or we just open-code the .insert() for update() and .find() for
contains() but it would be nice if those were friendlier.


This doesn't seem as nice anymore:

	if ((newList.find(ManualGain) == newList.end()) ||
	    (newList.find(ManualBrightness) == newList.end())) {}

was:
	if (!(newList.contains(ManualGain) &&
	     (newList.contains(ManualBrightness))) {}

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

Hi Kieran,

On Wed, Jun 26, 2019 at 02:14:34PM +0100, Kieran Bingham wrote:
> Hi Laurent,
> 
> A whole bunch of my development thoughts as I went through
> converting/removing the ControlList abstraction.
> 
> TLDR: I'm removing the Class abstraction and just using the
> std::unordered_map directly through the typedef/using.
> 
> Seems like it might be worth going back to being a map of straight
> ControlID, {Control}Value pairs too - and keep the ControlInfo entirely
> separated. Otherwise anytime a control is added to the list directly - a
> lookup will have to be done on the ControlInfo even if it's not needed...

I think that will depend on the usage patterns, but my gut feeling at
the moment is that you're right.

> On 24/06/2019 17:30, Laurent Pinchart wrote:
> > On Mon, Jun 24, 2019 at 05:02:55PM +0100, Kieran Bingham wrote:
> >> On 23/06/2019 23:21, Laurent Pinchart wrote:
> >>> 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/  / /
> >>
> >> I wish vim reflow/automatic wrap would do that....
> >>
> >>>>> Request or stored locally to establish a set of default values for later
> >>>>> reuse.
> >>>>>
> >>>>> Signed-off-by: Kieran Bingham <kieran.bingham@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 prefer this :-D
> >>
> >> Updated.
> >>
> >>> I don't think you should copy the ControlInfo in the ControlList. 
> >>
> >> I was trying not to - It was supposed to be a ControlInfo & but creating
> >> the original ControlInfo's has caused me headache ...
> >>
> >>> If I
> >>> understand your patch series correctly, ControlInfo should be created
> >>> with the camera, and remain constant for the lifetime of the camera. 
> >>
> >> Yes, I want to create ControlInfo structures which are constant for the
> >> camera lifetime, and contain any values appropriate to that camera instance.
> >>
> >>> The
> >>> map should thus use either the control ID or a pointer to the control
> >>> info as the key.
> >>
> >> An ID should be easy to support, but means code will have to look up the
> >> ControlInfo later, whereas a pointer to the ControlInfo needs to look it
> >> up internally.
> >>
> >> This is essentially why this topic is still RFC for me.
> >>
> >>>> 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
> >>
> >> Sure.
> >>
> >>>>> +	void reset() { controls_.clear(); }
> >>>
> >>> Should we call this clear() ?
> >>
> >> Sure.
> >>
> >>>>> +
> >>>>> +	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@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.
> >>
> >> Good point, removed.
> >>
> >>>>> +/**
> >>>>> + * \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>;
> >>
> >> Hrm ... well I think the intention was to be able to add some extra
> >> validation later... but I now wonder if just the ControlList 'using'
> >> might be easier, and remove a lot of rubbish passthrough code ...
> >>
> >> I'll give it a go - but I don't want to throw away a whole bunch of code
> >> to re-add it again soon after ...
> > 
> > git history to the rescue :-) (and mailing list archives)
> > 
> >> Best save this as a branch (well although it's saved on list at least)....
> >>
> >>> ? 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 );
> >>
> >> 'just like that' ?
> > 
> > controls.insert(otherControls.begin(), otherControls.end());
> 
> I wonder if some helpers would make it 'nicer', but because there is no
> class - it feels a bit more ugly (loses it's OO?):
> 
>   void controlListUpdate(targetList, sourceList) {
> 	targetList.insert(sourceList.begin(), sourceList.end());
>   }
> 
> Any ideas or suggestions on how to do these helpers nicely? Ideally I'd
> want the ControlList to look like a class object - but I don't think I
> can just extend the std::unordered_map() interface ...
> 
> I'd also want a helper for the .contains() method
> 
> (which is a shame, because in C++20 .contains() is available directly on
> the unordered_map)

How about keeping the ControlList class, but making it derive from the
container ? That would allow you to add the few helpers you need without
a need to proxy everything else. You also won't need to document or test
the proxy methods.

> Perhaps it's a non-issue anyway, as if we only care about initialising a
> list with a 'preset' set of controls, then that can be done with the
> operator=():
> 
>   ControlList newList = defaultList;
> 
> But that removes any existing items in the new list, so can't be used to
> update an existing list.
> 
> Still if that becomes necessary - then we can worry about adding the helper.
> 
> Likewise, perhaps the usage of .contains() won't be really necessary
> just yet, and it would be more optimal to use the find() directly so
> that in the event that it is found, you don't then have to 'search again'.
> 
> 
> TLDR: I've cancelled out my concerns :) - dropping the Class and
> replacing with the 'using' typedef.
> 
> 
> >> I'll play there next.
> >>
> >> Removing the ControlList class would remove my desire to have all the
> >> tests that you disagree with later in the series ...
> > 
> > I swear the proposal has nothing to do with the test discussion :-)
> 
> Hahah - of course, I actually like the idea of just using the typdef on
> the map itself. I don't want to pointlessly wrap the standard library.
> 
> If there's a good way to implement helpers - then this looks like a good
> approach ...
> 
> Or we just open-code the .insert() for update() and .find() for
> contains() but it would be nice if those were friendlier.
> 
> 
> This doesn't seem as nice anymore:
> 
> 	if ((newList.find(ManualGain) == newList.end()) ||
> 	    (newList.find(ManualBrightness) == newList.end())) {}
> 
> was:
> 	if (!(newList.contains(ManualGain) &&
> 	     (newList.contains(ManualBrightness))) {}
> 
> >>>>> +
> >>>>>  } /* namespace libcamera */

Hi Laurent,

On 26/06/2019 14:51, Laurent Pinchart wrote:
 <snip>

>> Any ideas or suggestions on how to do these helpers nicely? Ideally I'd
>> want the ControlList to look like a class object - but I don't think I
>> can just extend the std::unordered_map() interface ...
>>
>> I'd also want a helper for the .contains() method
>>
>> (which is a shame, because in C++20 .contains() is available directly on
>> the unordered_map)
> 
> How about keeping the ControlList class, but making it derive from the
> container ? That would allow you to add the few helpers you need without
> a need to proxy everything else. You also won't need to document or test
> the proxy methods.

I sort of want to do exactly that - but too many comments online say
"Never derive the STL containers":

https://stackoverflow.com/questions/10477839/c-inheriting-from-stdmap
https://www.researchgate.net/post/I_really_want_to_understand_if_there_is_any_reason_why_one_should_not_inherit_from_the_stl_vector_class_in_c2


I think the reasoning is that the STL does not implement virtual
destructors, but does this matter if I don't implement a custom destructor?

<snip>

Hi Kieran,

On Wed, Jun 26, 2019 at 03:02:59PM +0100, Kieran Bingham wrote:
> On 26/06/2019 14:51, Laurent Pinchart wrote:
>  <snip>
> 
> >> Any ideas or suggestions on how to do these helpers nicely? Ideally I'd
> >> want the ControlList to look like a class object - but I don't think I
> >> can just extend the std::unordered_map() interface ...
> >>
> >> I'd also want a helper for the .contains() method
> >>
> >> (which is a shame, because in C++20 .contains() is available directly on
> >> the unordered_map)
> > 
> > How about keeping the ControlList class, but making it derive from the
> > container ? That would allow you to add the few helpers you need without
> > a need to proxy everything else. You also won't need to document or test
> > the proxy methods.
> 
> I sort of want to do exactly that - but too many comments online say
> "Never derive the STL containers":
> 
> https://stackoverflow.com/questions/10477839/c-inheriting-from-stdmap
> https://www.researchgate.net/post/I_really_want_to_understand_if_there_is_any_reason_why_one_should_not_inherit_from_the_stl_vector_class_in_c2
> 
> I think the reasoning is that the STL does not implement virtual
> destructors, but does this matter if I don't implement a custom destructor?

It's one of the reasons. Another one seems that the standard says not to
do so, but I believe the reasons behind it are not relevant in this
case. If the only purpose of this inheritance is to add helper methods
to the container, without adding data members, I think it should be
fine. If we have to add data members or want to override existing
methods, then we're reaching grey waters, and I wouldn't be comfortable
going there.