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

[Kieran Bingham]

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 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;
> 
> 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 at 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 at lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
> 

-- 
Regards
--
Kieran