[libcamera-devel] [RFC PATCH v2 5/9] libcamera: Implement a ControlList
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed Jun 26 12:59:00 CEST 2019
Hi Laurent, Niklas,
On 24/06/2019 17:27, Laurent Pinchart wrote:
> Hi Kieran,
>
> On Mon, Jun 24, 2019 at 04:48:05PM +0100, Kieran Bingham wrote:
>> On 23/06/2019 16:35, 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
>>>> 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...
>
> But it's less efficient as it has to go through the whole list in all
> cases.
Hrm... I thought with the guaranteed single key entry, and known hash
algorithm this would be optimised internally : However:
www.cplusplus.com/reference/unordered_map/unordered_map/{count,find}:
Count elements with a specific key
Searches the container for elements whose key is k and returns the
number of elements found. Because unordered_map containers do not allow
for duplicate keys, this means that the function actually returns 1 if
an element with that key exists in the container, and zero otherwise.
But:
Complexity[Count]
Average case: linear in the number of elements counted.
Worst case: linear in container size.
vs
Complexity[Find]
Average case: constant.
Worst case: linear in container size.
Yup... that certainly points to using find();.
>>>> + 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>
Thankyou, currently trying to work out if I can drop the whole wrapper
easily though :S
>>>
>>>> + */
>>>> +
>>>> +/**
>>>> + * \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 */
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list