[libcamera-devel] [PATCH 3/4] libcamera: controls: Store array control size in Control class
Christian Rauch
Rauch.Christian at gmx.de
Fri Aug 12 00:11:21 CEST 2022
Hi Laurent,
Am 11.08.22 um 00:52 schrieb Laurent Pinchart:
> Hi Christian,
>
> On Thu, Aug 11, 2022 at 01:23:18AM +0300, Laurent Pinchart via libcamera-devel wrote:
>> On Wed, Aug 10, 2022 at 11:44:06PM +0200, Christian Rauch via libcamera-devel wrote:
>>> Dear Laurent,
>>>
>>> I don't see yet, how the "ControlId::dynamic_size" would be used
>>> additionally to the "Span::extent".
>>
>> The two have the same nuumerical value, but they differ in their intent.
>> ControlId::dynamic_size is meant to indicate that the array control has
>> a dynamic size, while utils::dynamic_extent is meant to indicate that a
>> span has a dynamic number of elements.
>>
>>> Since the size of the Span is
>>> already encoded in the type, what is the benefit of duplicating this
>>> information in the ControlId? Will this information be identical?
>>
>> It allows the ControlList::set() function to access the size, and
>> construct the Span with the right size, removing the need for explicit
>> Span construction in the caller as shown by patch 4/4.
>>
>> Now that I'm writing this, I wonder if the following could work
>>
>> template<typename T, size_t Size>
>> void set(const Control<Span<T, Size>> &ctrl, const std::initializer_list<T> &value)
>> {
>> ControlValue *val = find(ctrl.id());
>> if (!val)
>> return;
>>
>> val->set(Span<const typename std::remove_cv_t<T>, Size>{ value.begin(), value.size() });
>> }
>>
>> without adding the Size template argument to the Control class. I'll
>> give it a try.
>
> I can replace this patch with
>
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index ebc168fc28b7..2305202c4570 100644
> --- a/include/libcamera/controls.h
> +++ b/include/libcamera/controls.h
> @@ -393,14 +393,14 @@ public:
> val->set<T>(value);
> }
>
> - template<typename T, typename V>
> - void set(const Control<T> &ctrl, const std::initializer_list<V> &value)
> + template<typename T, typename V, size_t Size>
> + void set(const Control<Span<T, Size>> &ctrl, const std::initializer_list<V> &value)
> {
> ControlValue *val = find(ctrl.id());
> if (!val)
> return;
>
> - val->set<T>(Span<const typename std::remove_cv_t<V>>{ value.begin(), value.size() });
> + val->set(Span<const typename std::remove_cv_t<V>, Size>{ value.begin(), value.size() });
> }
>
> const ControlValue &get(unsigned int id) const;
> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> index bc3db4f69388..6dbf9b348709 100644
> --- a/src/libcamera/controls.cpp
> +++ b/src/libcamera/controls.cpp
> @@ -970,7 +970,7 @@ bool ControlList::contains(unsigned int id) const
> */
>
> /**
> - * \fn ControlList::set(const Control<T> &ctrl, const std::initializer_list<V> &value)
> + * \fn ControlList::set(const Control<Span<T, Size>> &ctrl, const std::initializer_list<V> &value)
> * \copydoc ControlList::set(const Control<T> &ctrl, const V &value)
> */
>
> and 4/4 still compiles. I think I'll go with this for the time being. I
> may revive the idea of adding a size directly to the Control class in
> the future when a use case will arise though, but that's for later.
If all you want to know is the size of the stored Span (or scalar),
wouldn't something like
template<typename T>
class Control : public ControlId
{
std::size_t extent() const
{
return T::extent;
}
}
be sufficient? You could use "is_span<T>::value" to implement an "array"
(returning 'extent') and "scalar" (returning 0) version of this.
>
>> I'm also tempted to figure out if there would be a way to define a
>> control as Control<T, Size> with T being the type of each element, not a
>> Span. Size would be 1 for non-array controls, >1 for array controls with
>> a fixed size, and std::numeric_limits<std::size_t>::max() for array
>> controls with a dynamic size. I'm not sure of the implications yet, but
>> having the size available as a template argument of the Control class
>> would allow passing it easily to the ControlId constructor, to make it
>> available at runtime. I think there are use cases for validation for
>> instance.
>>
>>> And
>>> what would be the meaning of "Control::size" when its T is a scalar and
>>> not a Span?
>>
>> It has no meaning in that case, which is indicated by setting it to 0.
>> Jacopo mentioned that 0 may be a confusing value, I agree with that.
>>
>>> Am 10.08.22 um 02:29 schrieb Laurent Pinchart:
>>>> Baside just identifying a control, the Control class enables usage of
>>>> the control type for metaprogramming. This is mainly used by the
>>>> ControlList get() and set() functions to type-check and cast the control
>>>> value automatically at compilation time.
>>>>
>>>> Extend this with a new Size template argument for the Control class that
>>>> specifies the size of array controls. Use it already in the
>>>> ControlList::set() overload that takes an std::initializer list to
>>>> construct the Span with an explicit size, enabling usage of the function
>>>> for fixed-size array controls.
>>>>
>>>> A possible future extension would be to pass the size to the ControlId
>>>> constructor and store it internally, enabling access to the size at
>>>> runtime, for instance to perform validity checks.
>>>>
>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>>>> ---
>>>> include/libcamera/controls.h | 20 +++++++++++--------
>>>> src/libcamera/controls.cpp | 37 +++++++++++++++++++++++++++---------
>>>> utils/gen-controls.py | 24 +++++++++++++++++++++--
>>>> 3 files changed, 62 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
>>>> index ebc168fc28b7..dd474a807d68 100644
>>>> --- a/include/libcamera/controls.h
>>>> +++ b/include/libcamera/controls.h
>>>> @@ -8,6 +8,7 @@
>>>> #pragma once
>>>>
>>>> #include <assert.h>
>>>> +#include <limits>
>>>> #include <optional>
>>>> #include <set>
>>>> #include <stdint.h>
>>>> @@ -213,6 +214,8 @@ private:
>>>> class ControlId
>>>> {
>>>> public:
>>>> + static constexpr size_t dynamic_size = std::numeric_limits<std::size_t>::max();
>>>> +
>>>> ControlId(unsigned int id, const std::string &name, ControlType type)
>>>> : id_(id), name_(name), type_(type)
>>>> {
>>>> @@ -250,11 +253,12 @@ static inline bool operator!=(const ControlId &lhs, unsigned int rhs)
>>>> return !(lhs == rhs);
>>>> }
>>>>
>>>> -template<typename T>
>>>> +template<typename T, size_t Size = 0>
>>>> class Control : public ControlId
>>>> {
>>>> public:
>>>> using type = T;
>>>> + static constexpr size_t size = Size;
>>>>
>>>> Control(unsigned int id, const char *name)
>>>> : ControlId(id, name, details::control_type<std::remove_cv_t<T>>::value)
>>>> @@ -372,8 +376,8 @@ public:
>>>>
>>>> bool contains(unsigned int id) const;
>>>>
>>>> - template<typename T>
>>>> - std::optional<T> get(const Control<T> &ctrl) const
>>>> + template<typename T, size_t Size>
>>>> + std::optional<T> get(const Control<T, Size> &ctrl) const
>>>> {
>>>> const auto entry = controls_.find(ctrl.id());
>>>> if (entry == controls_.end())
>>>> @@ -383,8 +387,8 @@ public:
>>>> return val.get<T>();
>>>> }
>>>>
>>>> - template<typename T, typename V>
>>>> - void set(const Control<T> &ctrl, const V &value)
>>>> + template<typename T, typename V, size_t Size>
>>>> + void set(const Control<T, Size> &ctrl, const V &value)
>>>> {
>>>> ControlValue *val = find(ctrl.id());
>>>> if (!val)
>>>> @@ -393,14 +397,14 @@ public:
>>>> val->set<T>(value);
>>>> }
>>>>
>>>> - template<typename T, typename V>
>>>> - void set(const Control<T> &ctrl, const std::initializer_list<V> &value)
>>>> + template<typename T, typename V, size_t Size>
>>>> + void set(const Control<T, Size> &ctrl, const std::initializer_list<V> &value)
>>>> {
>>>> ControlValue *val = find(ctrl.id());
>>>> if (!val)
>>>> return;
>>>>
>>>> - val->set<T>(Span<const typename std::remove_cv_t<V>>{ value.begin(), value.size() });
>>>> + val->set<T>(Span<const typename std::remove_cv_t<V>, Size>{ value.begin(), value.size() });
>>>> }
>>>>
>>>> const ControlValue &get(unsigned int id) const;
>>>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
>>>> index bc3db4f69388..24c836382f10 100644
>>>> --- a/src/libcamera/controls.cpp
>>>> +++ b/src/libcamera/controls.cpp
>>>> @@ -384,6 +384,11 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen
>>>> * Control class for more information.
>>>> */
>>>>
>>>> +/**
>>>> + * \var ControlId::dynamic_size
>>>> + * \brief Size value for dynamic array controls
>>>> + */
>>>> +
>>>> /**
>>>> * \fn ControlId::ControlId(unsigned int id, const std::string &name, ControlType type)
>>>> * \brief Construct a ControlId instance
>>>> @@ -431,12 +436,17 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen
>>>> /**
>>>> * \class Control
>>>> * \brief Describe a control and its intrinsic properties
>>>> + * \tparam T The control data type
>>>> + * \tparam Size The number of elements (for array controls only)
>>>> *
>>>> - * The Control class models a control exposed by an object. Its template type
>>>> - * name T refers to the control data type, and allows functions that operate on
>>>> - * control values to be defined as template functions using the same type T for
>>>> - * the control value. See for instance how the ControlList::get() function
>>>> - * returns a value corresponding to the type of the requested control.
>>>> + * The Control class models a control exposed by an object. Its template
>>>> + * paramter \a T refers to the control data type, and allows functions that
>>>> + * operate on control values to be defined as template functions using the same
>>>> + * type T for the control value. See for instance how the ControlList::get()
>>>> + * function returns a value corresponding to the type of the requested control.
>>>> + *
>>>> + * Similarly, for array controls the template parameter \a Size indicates the
>>>> + * number of elements in the array.
>>>> *
>>>> * While this class is the main means to refer to a control, the control
>>>> * identifying information is stored in the non-template base ControlId class.
>>>> @@ -469,6 +479,15 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen
>>>> * \brief The Control template type T
>>>> */
>>>>
>>>> +/**
>>>> + * \var Control::size
>>>> + * \brief The number of elements for array controls
>>>> + *
>>>> + * The \a size reports the number of elements stored by an array Control. It is
>>>> + * equal to 0 for non-array controls, and to ControlId::dynamic_size for
>>>> + * variable-size controls.
>>>> + */
>>>> +
>>>> /**
>>>> * \class ControlInfo
>>>> * \brief Describe the limits of valid values for a Control
>>>> @@ -943,7 +962,7 @@ bool ControlList::contains(unsigned int id) const
>>>> }
>>>>
>>>> /**
>>>> - * \fn ControlList::get(const Control<T> &ctrl) const
>>>> + * \fn ControlList::get(const Control<T, Size> &ctrl) const
>>>> * \brief Get the value of control \a ctrl
>>>> * \param[in] ctrl The control
>>>> *
>>>> @@ -956,7 +975,7 @@ bool ControlList::contains(unsigned int id) const
>>>> */
>>>>
>>>> /**
>>>> - * \fn ControlList::set(const Control<T> &ctrl, const V &value)
>>>> + * \fn ControlList::set(const Control<T, Size> &ctrl, const V &value)
>>>> * \brief Set the control \a ctrl value to \a value
>>>> * \param[in] ctrl The control
>>>> * \param[in] value The control value
>>>> @@ -970,8 +989,8 @@ bool ControlList::contains(unsigned int id) const
>>>> */
>>>>
>>>> /**
>>>> - * \fn ControlList::set(const Control<T> &ctrl, const std::initializer_list<V> &value)
>>>> - * \copydoc ControlList::set(const Control<T> &ctrl, const V &value)
>>>> + * \fn ControlList::set(const Control<T, Size> &ctrl, const std::initializer_list<V> &value)
>>>> + * \copydoc ControlList::set(const Control<T, Size> &ctrl, const V &value)
>>>> */
>>>>
>>>> /**
>>>> diff --git a/utils/gen-controls.py b/utils/gen-controls.py
>>>> index bcfbeb7f9f17..a3b9c58beec6 100755
>>>> --- a/utils/gen-controls.py
>>>> +++ b/utils/gen-controls.py
>>>> @@ -100,6 +100,10 @@ class Control(object):
>>>> ns = 'draft::' if self.is_draft else ''
>>>> return ns + self.__name
>>>>
>>>> + @property
>>>> + def size(self):
>>>> + return self.__size
>>>> +
>>>> @property
>>>> def type(self):
>>>> typ = self.__data.get('type')
>>>> @@ -137,7 +141,7 @@ ${description}''')
>>>> * \\var ${name}
>>>> ${description}
>>>> */''')
>>>> - def_template = string.Template('extern const Control<${type}> ${name}(${id_name}, "${name}");')
>>>> + def_template = string.Template('extern const Control<${type}${size}> ${name}(${id_name}, "${name}");')
>>>> enum_values_doc = string.Template('''/**
>>>> * \\var ${name}Values
>>>> * \\brief List of all $name supported values
>>>> @@ -154,9 +158,17 @@ ${description}
>>>> for ctrl in controls:
>>>> id_name = snake_case(ctrl.name).upper()
>>>>
>>>> + if ctrl.size is None:
>>>> + size = ''
>>>> + elif ctrl.size == 0:
>>>> + size = ', ControlId::dynamic_size'
>>>> + else:
>>>> + size = f', {ctrl.size}'
>>>> +
>>>> info = {
>>>> 'name': ctrl.name,
>>>> 'type': ctrl.type,
>>>> + 'size': size,
>>>> 'description': format_description(ctrl.description),
>>>> 'id_name': id_name,
>>>> }
>>>> @@ -216,7 +228,7 @@ def generate_h(controls):
>>>> enum_template_start = string.Template('''enum ${name}Enum {''')
>>>> enum_value_template = string.Template('''\t${name} = ${value},''')
>>>> enum_values_template = string.Template('''extern const std::array<const ControlValue, ${size}> ${name}Values;''')
>>>> - template = string.Template('''extern const Control<${type}> ${name};''')
>>>> + template = string.Template('''extern const Control<${type}${size}> ${name};''')
>>>>
>>>> ctrls = []
>>>> draft_ctrls = []
>>>> @@ -228,9 +240,17 @@ def generate_h(controls):
>>>>
>>>> ids.append('\t' + id_name + ' = ' + str(id_value) + ',')
>>>>
>>>> + if ctrl.size is None:
>>>> + size = ''
>>>> + elif ctrl.size == 0:
>>>> + size = ', ControlId::dynamic_size'
>>>> + else:
>>>> + size = f', {ctrl.size}'
>>>> +
>>>> info = {
>>>> 'name': ctrl.name,
>>>> 'type': ctrl.type,
>>>> + 'size': size,
>>>> }
>>>>
>>>> target_ctrls = ctrls
>
More information about the libcamera-devel
mailing list