[libcamera-devel] [PATCH 03/11] libcamera: controls: Add support for string controls
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Mar 20 13:12:14 CET 2020
Hi Jacopo,
On Fri, Mar 20, 2020 at 01:08:44PM +0100, Jacopo Mondi wrote:
> On Mon, Mar 09, 2020 at 05:24:06PM +0100, Jacopo Mondi wrote:
> > From: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >
> > String controls are stored internally as an array of char, but the
> > ControlValue constructor, get() and set() functions operate on an
> > std::string for convenience. Array of strings are thus not supported.
> >
> > Unlike for other control types, the ControlInfo range reports the
> > minimum and maximum allowed lengths of the string (the minimum will
> > usually be 0), not the minimum and maximum value of each element.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > include/libcamera/controls.h | 30 +++++++++++++++++++-----
> > src/libcamera/control_serializer.cpp | 22 +++++++++++++++++
> > src/libcamera/controls.cpp | 35 ++++++++++++++++++++++++----
> > src/libcamera/gen-controls.py | 18 +++++++-------
> > 4 files changed, 87 insertions(+), 18 deletions(-)
> >
> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > index 9c6cbffb88b5..5cf6280e612b 100644
> > --- a/include/libcamera/controls.h
> > +++ b/include/libcamera/controls.h
> > @@ -26,6 +26,7 @@ enum ControlType {
> > ControlTypeInteger32,
> > ControlTypeInteger64,
> > ControlTypeFloat,
> > + ControlTypeString,
> > };
> >
> > namespace details {
> > @@ -64,6 +65,11 @@ struct control_type<float> {
> > static constexpr ControlType value = ControlTypeFloat;
> > };
> >
> > +template<>
> > +struct control_type<std::string> {
> > + static constexpr ControlType value = ControlTypeString;
> > +};
> > +
> > template<typename T, std::size_t N>
> > struct control_type<Span<T, N>> : public control_type<std::remove_cv_t<T>> {
> > };
> > @@ -76,7 +82,9 @@ public:
> > ControlValue();
> >
> > #ifndef __DOXYGEN__
> > - template<typename T, typename std::enable_if_t<!details::is_span<T>::value, std::nullptr_t> = nullptr>
> > + template<typename T, typename std::enable_if_t<!details::is_span<T>::value &&
> > + !std::is_same<std::string, std::remove_cv_t<T>>::value,
> > + std::nullptr_t> = nullptr>
> > ControlValue(const T &value)
> > : type_(ControlTypeNone), numElements_(0)
> > {
> > @@ -84,7 +92,9 @@ public:
> > &value, 1, sizeof(T));
> > }
> >
> > - template<typename T, typename std::enable_if_t<details::is_span<T>::value, std::nullptr_t> = nullptr>
> > + template<typename T, typename std::enable_if_t<details::is_span<T>::value ||
> > + std::is_same<std::string, std::remove_cv_t<T>>::value,
> > + std::nullptr_t> = nullptr>
> > #else
> > template<typename T>
> > #endif
> > @@ -115,7 +125,9 @@ public:
> > }
> >
> > #ifndef __DOXYGEN__
> > - template<typename T, typename std::enable_if_t<!details::is_span<T>::value, std::nullptr_t> = nullptr>
> > + template<typename T, typename std::enable_if_t<!details::is_span<T>::value &&
> > + !std::is_same<std::string, std::remove_cv_t<T>>::value,
> > + std::nullptr_t> = nullptr>
> > T get() const
> > {
> > assert(type_ == details::control_type<std::remove_cv_t<T>>::value);
> > @@ -124,7 +136,9 @@ public:
> > return *reinterpret_cast<const T *>(data().data());
> > }
> >
> > - template<typename T, typename std::enable_if_t<details::is_span<T>::value, std::nullptr_t> = nullptr>
> > + template<typename T, typename std::enable_if_t<details::is_span<T>::value ||
> > + std::is_same<std::string, std::remove_cv_t<T>>::value,
> > + std::nullptr_t> = nullptr>
> > #else
> > template<typename T>
> > #endif
> > @@ -139,14 +153,18 @@ public:
> > }
> >
> > #ifndef __DOXYGEN__
> > - template<typename T, typename std::enable_if_t<!details::is_span<T>::value, std::nullptr_t> = nullptr>
> > + template<typename T, typename std::enable_if_t<!details::is_span<T>::value &&
> > + !std::is_same<std::string, std::remove_cv_t<T>>::value,
> > + std::nullptr_t> = nullptr>
> > void set(const T &value)
> > {
> > set(details::control_type<std::remove_cv_t<T>>::value, false,
> > reinterpret_cast<const void *>(&value), 1, sizeof(T));
> > }
> >
> > - template<typename T, typename std::enable_if_t<details::is_span<T>::value, std::nullptr_t> = nullptr>
> > + template<typename T, typename std::enable_if_t<details::is_span<T>::value ||
> > + std::is_same<std::string, std::remove_cv_t<T>>::value,
> > + std::nullptr_t> = nullptr>
> > #else
> > template<typename T>
> > #endif
> > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp
> > index eef875f4d96c..808419f246c0 100644
> > --- a/src/libcamera/control_serializer.cpp
> > +++ b/src/libcamera/control_serializer.cpp
> > @@ -314,6 +314,22 @@ ControlValue ControlSerializer::loadControlValue(ByteStreamBuffer &buffer,
> > return value;
> > }
> >
> > +template<>
> > +ControlValue ControlSerializer::loadControlValue<std::string>(ByteStreamBuffer &buffer,
> > + bool isArray,
> > + unsigned int count)
>
> Not may way around this specialization I gueess..
> I see std::string being defined as std::basic_string<char> and I was
> wondering if we could reuse the <char> template argument, but I don't
> see a clear way to do so
This is also the part of this patch that I dislike, but for now I think
it's the best option.
> Anyway, ControlTypeFloat documentation removal apart
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>
> > +{
> > + ControlValue value;
> > +
> > + const char *data = buffer.read<char>(count);
> > + if (!data)
> > + return value;
> > +
> > + value.set(std::string{ data, count });
> > +
> > + return value;
> > +}
> > +
> > ControlValue ControlSerializer::loadControlValue(ControlType type,
> > ByteStreamBuffer &buffer,
> > bool isArray,
> > @@ -335,6 +351,9 @@ ControlValue ControlSerializer::loadControlValue(ControlType type,
> > case ControlTypeFloat:
> > return loadControlValue<float>(buffer, isArray, count);
> >
> > + case ControlTypeString:
> > + return loadControlValue<std::string>(buffer, isArray, count);
> > +
> > case ControlTypeNone:
> > return ControlValue();
> > }
> > @@ -345,6 +364,9 @@ ControlValue ControlSerializer::loadControlValue(ControlType type,
> > ControlInfo ControlSerializer::loadControlInfo(ControlType type,
> > ByteStreamBuffer &b)
> > {
> > + if (type == ControlTypeString)
> > + type = ControlTypeInteger32;
> > +
> > ControlValue min = loadControlValue(type, b);
> > ControlValue max = loadControlValue(type, b);
> >
> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> > index 53649fe897db..68d040db4daa 100644
> > --- a/src/libcamera/controls.cpp
> > +++ b/src/libcamera/controls.cpp
> > @@ -57,6 +57,7 @@ static constexpr size_t ControlValueSize[] = {
> > [ControlTypeInteger32] = sizeof(int32_t),
> > [ControlTypeInteger64] = sizeof(int64_t),
> > [ControlTypeFloat] = sizeof(float),
> > + [ControlTypeString] = sizeof(char),
> > };
> >
> > } /* namespace */
> > @@ -74,8 +75,8 @@ static constexpr size_t ControlValueSize[] = {
> > * The control stores a 32-bit integer value
> > * \var ControlTypeInteger64
> > * The control stores a 64-bit integer value
> > - * \var ControlTypeFloat
> > - * The control stores a 32-bit floating point value
>
> What did the poor Float type do to you ? :(
Oops. My sincere apologies to all the Floats in this world, I never
meant to wish for your disappearance, even if you can't express the
speed of light with enough accuracy
(https://git.linuxtv.org/libcamera.git/tree/test/controls/control_value.cpp#n228).
> > + * \var ControlTypeString
> > + * The control stores a string value as an array of char
> > */
> >
> > /**
> > @@ -164,7 +165,8 @@ ControlValue &ControlValue::operator=(const ControlValue &other)
> > * \brief Retrieve the number of elements stored in the ControlValue
> > *
> > * For instances storing an array, this function returns the number of elements
> > - * in the array. Otherwise, it returns 1.
> > + * in the array. For instances storing a string, it returns the length of the
> > + * string, not counting the terminating '\0'. Otherwise, it returns 1.
> > *
> > * \return The number of elements stored in the ControlValue
> > */
> > @@ -192,6 +194,11 @@ std::string ControlValue::toString() const
> > return "<ValueType Error>";
> >
> > const uint8_t *data = ControlValue::data().data();
> > +
> > + if (type_ == ControlTypeString)
> > + return std::string(reinterpret_cast<const char *>(data),
> > + numElements_);
> > +
> > std::string str(isArray_ ? "[ " : "");
> >
> > for (unsigned int i = 0; i < numElements_; ++i) {
> > @@ -222,6 +229,7 @@ std::string ControlValue::toString() const
> > break;
> > }
> > case ControlTypeNone:
> > + case ControlTypeString:
> > break;
> > }
> >
> > @@ -439,12 +447,22 @@ ControlInfo::ControlInfo(const ControlValue &min,
> > /**
> > * \fn ControlInfo::min()
> > * \brief Retrieve the minimum value of the control
> > + *
> > + * For string controls, this is the minimum length of the string, not counting
> > + * the terminating '\0'. For all other control types, this is the minimum value
> > + * of each element.
> > + *
> > * \return A ControlValue with the minimum value for the control
> > */
> >
> > /**
> > * \fn ControlInfo::max()
> > * \brief Retrieve the maximum value of the control
> > + *
> > + * For string controls, this is the maximum length of the string, not counting
> > + * the terminating '\0'. For all other control types, this is the maximum value
> > + * of each element.
> > + *
> > * \return A ControlValue with the maximum value for the control
> > */
> >
> > @@ -653,7 +671,16 @@ void ControlInfoMap::generateIdmap()
> > idmap_.clear();
> >
> > for (const auto &ctrl : *this) {
> > - if (ctrl.first->type() != ctrl.second.min().type()) {
> > + /*
> > + * For string controls, min and max define the valid
> > + * range for the string size, not for the individual
> > + * values.
> > + */
> > + ControlType rangeType = ctrl.first->type() == ControlTypeString
> > + ? ControlTypeInteger32 : ctrl.first->type();
> > + const ControlInfo &info = ctrl.second;
> > +
> > + if (info.min().type() != rangeType) {
> > LOG(Controls, Error)
> > << "Control " << utils::hex(ctrl.first->id())
> > << " type and info type mismatch";
> > diff --git a/src/libcamera/gen-controls.py b/src/libcamera/gen-controls.py
> > index ff8bda6b16c1..87c3d52ada6d 100755
> > --- a/src/libcamera/gen-controls.py
> > +++ b/src/libcamera/gen-controls.py
> > @@ -42,10 +42,11 @@ ${description}
> > name, ctrl = ctrl.popitem()
> > id_name = snake_case(name).upper()
> >
> > - if ctrl.get('size'):
> > - ctrl_type = 'Span<const %s>' % ctrl['type']
> > - else:
> > - ctrl_type = ctrl['type']
> > + ctrl_type = ctrl['type']
> > + if ctrl_type == 'string':
> > + ctrl_type = 'std::string'
> > + elif ctrl.get('size'):
> > + ctrl_type = 'Span<const %s>' % ctrl_type
> >
> > info = {
> > 'name': name,
> > @@ -97,10 +98,11 @@ def generate_h(controls):
> >
> > ids.append('\t' + id_name + ' = ' + str(id_value) + ',')
> >
> > - if ctrl.get('size'):
> > - ctrl_type = 'Span<const %s>' % ctrl['type']
> > - else:
> > - ctrl_type = ctrl['type']
> > + ctrl_type = ctrl['type']
> > + if ctrl_type == 'string':
> > + ctrl_type = 'std::string'
> > + elif ctrl.get('size'):
> > + ctrl_type = 'Span<const %s>' % ctrl_type
> >
> > info = {
> > 'name': name,
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list