[libcamera-devel] [PATCH 3/4] libcamera: controls: Store array control size in Control class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Aug 11 00:52:58 CEST 2022
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.
> 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
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list