[libcamera-devel] [RFC PATCH v2 5/9] libcamera: Implement a ControlList
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed Jun 26 15:14:34 CEST 2019
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 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)
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
--
Kieran
More information about the libcamera-devel
mailing list