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

[Laurent Pinchart]

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

-- 
Regards,

Laurent Pinchart