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

[Laurent Pinchart]

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

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

-- 
Regards,

Laurent Pinchart