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

[Kieran Bingham]

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 at ideasonboard.com>
>>> ---
>>>  include/libcamera/controls.h |  41 +++++++++++
>>>  src/libcamera/controls.cpp   | 128 +++++++++++++++++++++++++++++++++++
>>>  2 files changed, 169 insertions(+)
>>>
>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
>>> index 95198d41c4cf..5835b840a31b 100644
>>> --- a/include/libcamera/controls.h
>>> +++ b/include/libcamera/controls.h
>>> @@ -4,6 +4,9 @@
>>>   *
>>>   * controls.h - Control handling
>>>   */
>>> +
>>> +#include <unordered_map>
>>> +
>>>  #ifndef __LIBCAMERA_CONTROLS_H__
>>>  #define __LIBCAMERA_CONTROLS_H__
>>>  
>>> @@ -53,6 +56,44 @@ private:
>>>  
>>>  std::ostream &operator<<(std::ostream &stream, const ControlInfo &value);
>>>  
>>> +class ControlList
>>> +{
>>> +private:
>>> +	struct ControlInfoHasher {
>>> +		std::size_t operator()(const ControlInfo &ci) const
>>> +		{
>>> +			return std::hash<int>()(ci.id());
>>> +		}
>>> +	};
>>> +
>>> +	typedef std::unordered_map<ControlInfo, Value, ControlInfoHasher> ControlListMap;
> 
> We usually use the C++11 syntax
> 
> 	using ControlListMap = std::unordered_map<ControlInfo, Value, ControlInfoHasher>;

I 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 at ragnatech.se>
>>
>>> + */
>>> +
>>> +/**
>>> + * \brief Construct a ControlList
>>> + */
>>> +ControlList::ControlList()
>>> +{
>>> +}
>>> +
> 
> If the constructor is empty and is the only one you can omit it, the
> compiler will generate one for your.


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

-- 
Regards
--
Kieran