[libcamera-devel] [RFC PATCH v2 1/9] libcamera: Value: Provide abstract value class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Jun 24 14:09:08 CEST 2019
Hi Kieran,
On Mon, Jun 24, 2019 at 10:09:06AM +0100, Kieran Bingham wrote:
> On 22/06/2019 23:39, Laurent Pinchart wrote:
> > On Fri, Jun 21, 2019 at 05:13:53PM +0100, Kieran Bingham wrote:
> >> To facilitate passing typed data generically, implement a class which stores
> >> the type and value as efficiently as possible.
> >>
> >> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >> ---
> >> include/libcamera/meson.build | 1 +
> >> include/libcamera/value.h | 63 ++++++++++
> >> src/libcamera/meson.build | 1 +
> >> src/libcamera/value.cpp | 226 ++++++++++++++++++++++++++++++++++
> >> 4 files changed, 291 insertions(+)
> >> create mode 100644 include/libcamera/value.h
> >> create mode 100644 src/libcamera/value.cpp
> >>
> >> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> >> index 201832105457..eb2211ae1fc3 100644
> >> --- a/include/libcamera/meson.build
> >> +++ b/include/libcamera/meson.build
> >> @@ -12,6 +12,7 @@ libcamera_api = files([
> >> 'signal.h',
> >> 'stream.h',
> >> 'timer.h',
> >> + 'value.h',
> >> 'version.h',
> >> ])
> >>
> >> diff --git a/include/libcamera/value.h b/include/libcamera/value.h
> >> new file mode 100644
> >> index 000000000000..00c5d53d5cc0
> >> --- /dev/null
> >> +++ b/include/libcamera/value.h
> >> @@ -0,0 +1,63 @@
> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >> +/*
> >> + * Copyright (C) 2019, Google Inc.
> >> + *
> >> + * value.h - Abstract value handling
> >> + */
> >> +
> >> +#include <map>
> >> +
> >> +#ifndef __LIBCAMERA_VALUE_H__
> >> +#define __LIBCAMERA_VALUE_H__
> >> +
> >> +namespace libcamera {
> >> +
> >> +enum ValueType : uint8_t {
> >
> > Do we need an explicit uint8_t here ? It won't save memory as the next
> > member of the Value class will be aligned to a 64-bits boundary.
>
> True, but ValueType might be used in other locations too? Although then
> they'd only be used for checking the type - so storage isn't really an
> issue there either.
>
> My actual reason for playing around with specifying the types on enums
> was to explore the effects on the ABI.
>
> The ValueType enum should certainly never exceed 255 entries, which is
> one benefit of being explicitly smaller - but I do think on this
> occasion it is completely redundant so I'll just remove it now.
>
> >> + ValueNull,
> >
> > Let's call it ValueUnset or ValueNone. Null has a well established
> > meaning.
>
> I started with ValueNone ... but moved to ValueNull ... but I'm happy to
> go back again :-)
>
> >> + ValueBool,
> >> + ValueInteger,
> >> + ValueInteger64,
> >> + ValueString,
> >> +};
> >> +
> >> +class Value
> >
> > I think I would call this ControlValue for now, and then expand it to a
> > more generic solution if needed, instead of trying to be too generic to
> > start with.
>
> And this whole class came from ControlValue
>
> >> +{
> >> +public:
> >> + Value();
> >> + Value(bool value);
> >> + Value(int value);
> >> + Value(const char *value);
> >> + Value(const std::string &value);
> >> +
> >> + ValueType type() const { return type_; };
> >> + bool isNull() const { return type_ == ValueNull; };
> >
> > isUnset() ?
>
> Well if it's not Null anymore, shouldn't it be isNone?
>
> Or do you prefer (Un)'set' to mirror the set() call.
I'm fine with either, let's just avoid "null" and make the type name and
function name consistent.
> >> +
> >> + void set(bool value);
> >> + void set(int value);
> >> + void set(int64_t value);
> >
> > This is asking for trouble, you will often end up with the wrong integer
> > type without even noticing, unless you can remember and apply flawlessly
> > the C++ integer types implicit conversions rules.
>
> Hrm... So types in the Function name?
There are multiple options. One of them is to treat all integer types as
an int64_t internally, and have getInt() and getInt64() operate properly
on them. Another option would be to use explicit function names. I like
the former in theory, but it requires conversions in the getters, so it
may be a little bit less efficient. Simplicity of use or efficiency in
this case ? :-)
> >> + void set(const char *value);
> >> + void set(const std::string &value);
> >> +
> >> + bool getBool() const;
> >> + int getInt() const;
> >> + int getInt64() const;
> >
> > This should return a 64-bit integer.
>
> Indeed.
>
> >> + std::string getString() const;
> >> +
> >> + std::string toString() const;
> >> +
> >> +private:
> >> + ValueType type_;
> >> +
> >> + union {
> >> + bool bool_;
> >> + int integer_;
> >> + int64_t integer64_;
> >> + };
> >> + std::string string_;
> >
> > As you've noticed, usage of an std::string makes it impossible to make
> > the string_ member part of the value union, which reduces memory and CPU
> > efficiency. Every integer value will need to construct an std::string.
> > For this reason I would store the data for complex types as a pointer
> > instead, and allocate it dynamically. It could be a pointer to a
> > std::string, or just a const char *. The latter would likely be more
> > efficient.
>
> Indeed. I think this is looking to get dropped rather than converted at
> the moment.
>
> I wanted to make the Value class a generic re-usable class that could be
> re-used in the existing Option parsers, and V4L2Controls, because we are
> already repeating the same patterns across the code base.
>
> But Jacopo doesn't want to make use of it there either ...
Let's model it to be efficient for its main use cases, and then we can
extend it to cover additional use cases if possible without an
efficiency impact. I quite like the idea of using it for both libcamera
and V4L2 controls though, but that could be done later (and isn't a
strong requirement).
> >> +};
> >> +
> >> +std::ostream &operator<<(std::ostream &stream, const Value &value);
> >> +
> >> +} /* namespace libcamera */
> >> +
> >> +#endif /* __LIBCAMERA_VALUE_H__ */
> >> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> >> index 68c7ce14b5b4..8e68373118df 100644
> >> --- a/src/libcamera/meson.build
> >> +++ b/src/libcamera/meson.build
> >> @@ -27,6 +27,7 @@ libcamera_sources = files([
> >> 'v4l2_device.cpp',
> >> 'v4l2_subdevice.cpp',
> >> 'v4l2_videodevice.cpp',
> >> + 'value.cpp',
> >> 'version.cpp',
> >> ])
> >>
> >> diff --git a/src/libcamera/value.cpp b/src/libcamera/value.cpp
> >> new file mode 100644
> >> index 000000000000..f713907b38dd
> >> --- /dev/null
> >> +++ b/src/libcamera/value.cpp
> >> @@ -0,0 +1,226 @@
> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >> +/*
> >> + * Copyright (C) 2019, Google Inc.
> >> + *
> >> + * value.cpp - Abstract value handling
> >> + */
> >> +
> >> +#include <limits.h>
> >> +#include <sstream>
> >> +#include <string>
> >> +
> >> +#include <libcamera/value.h>
> >
> > Please move this as the first include to ensure that it compiles without
> > depending on any implicitly included headers.
>
> Done.
>
> >> +
> >> +#include "log.h" // For ASSERT()
> >
> > You don't need to keep the comment.
>
> Removed.
>
> >> +
> >> +/**
> >> + * \file value.h
> >> + * \brief Handles variant value types as an abstract object
> >> + */
> >> +
> >> +namespace libcamera {
> >> +
> >> +/**
> >> + * \enum ValueType
> >> + * Determines the type of value represented by a \a Value
> >> + * \var ValueNull
> >> + * Identifies an unset value
> >> + * \var ValueBool
> >> + * Identifies controls storing a boolean value
> >> + * \var ValueInteger
> >> + * Identifies controls storing an integer value
> >> + * \var ValueString
> >> + * Identifies controls storing a string value
> >
> > You're missing the 64-bit integer.
>
> Added.
>
> >> + */
> >> +
> >> +/**
> >> + * \class Value
> >> + * \brief Abstract a value for a common data exchange
> >> + */
> >> +
> >> +/**
> >> + * \brief Construct an empty Value.
> >> + * The Value must be \a set() before being used.
> >> + */
> >> +Value::Value()
> >> + : type_(ValueNull)
> >> +{
> >> +}
> >> +
> >> +/**
> >> + * \brief Construct a Boolean Value
> >> + * \param[in] value Boolean value to store
> >> + */
> >> +Value::Value(bool value)
> >> + : type_(ValueBool), bool_(value)
> >> +{
> >> +}
> >> +
> >> +/**
> >> + * \brief Construct an integer Value
> >> + * \param[in] value Integer value to store
> >> + */
> >> +Value::Value(int value)
> >> + : type_(ValueInteger), integer_(value)
> >> +{
> >> +}
> >> +
> >> +/**
> >> + * \brief Construct a string Value
> >> + * \param[in] value String representation to store
> >> + */
> >> +Value::Value(const char *value)
> >> + : type_(ValueString), string_(value)
> >> +{
> >> +}
> >> +
> >> +/**
> >> + * \brief Construct a string Value
> >> + * \param[in] value String representation to store
> >> + */
> >> +Value::Value(const std::string &value)
> >> + : type_(ValueString), string_(value)
> >> +{
> >> +}
> >> +
> >> +/**
> >> + * \fn Value::type
> >> + * \brief Return the type of value represented by the object
> >> + */
> >> +
> >> +/**
> >> + * \fn Value::isNull
> >> + * \brief Determines if the Value is initialised
> >
> > s/Determines/Determine/
> >
> >> + * \return true if the value type is ValueNull, false otherwise
> >
> > s/true/True/
> >
> >> + */
> >> +
> >> +/**
> >> + * \brief Set the value with a boolean
> >> + * \param[in] value Boolean value to store
> >> + */
> >> +void Value::set(bool value)
> >> +{
> >> + type_ = ValueBool;
> >> + bool_ = value;
> >> +}
> >> +
> >> +/**
> >> + * \brief Set the value with an integer
> >> + * \param[in] value Integer value to store
> >> + */
> >> +void Value::set(int value)
> >> +{
> >> + type_ = ValueInteger;
> >> + integer_ = value;
> >> +}
> >> +
> >> +/**
> >> + * \brief Set the value with a 64 bit integer
> >> + * \param[in] value Integer value to store
> >> + */
> >> +void Value::set(int64_t value)
> >> +{
> >> + type_ = ValueInteger64;
> >> + integer64_ = value;
> >> +}
> >> +
> >> +/**
> >> + * \brief Set the value with a string representation
> >> + * \param[in] value String value to store
> >> + */
> >> +void Value::set(const char *value)
> >> +{
> >> + type_ = ValueString;
> >> + string_ = value;
> >> +}
> >> +
> >> +/**
> >> + * \brief Set the value with a string representation
> >> + * \param[in] value String value to store
> >> + */
> >> +void Value::set(const std::string &value)
> >> +{
> >> + type_ = ValueString;
> >> + string_ = value;
> >> +}
> >> +
> >> +/**
> >> + * \brief Get the boolean value.
> >> + *
> >> + * The Value type must be Boolean.
> >> + */
> >> +bool Value::getBool() const
> >> +{
> >> + ASSERT(type_ == ValueBool);
> >
> > That's quite dangerous... Isn't there a large risk that the value will
> > be set to a type and retrieved using a different but compatible type,
> > and that the issue would only be caught at runtime, possibly in uncommon
> > cases ? For instance consider the following code
>
> That's why I put the assert in - to catch those issues early?
>
> Warnings and Error prints will scroll off the logs...
With the condition being unlikely the ASSERT() may only trigger in
production, which wouldn't be very nice.
> >
> > Value v;
> >
> > if (likely(condition))
> > value.set(3);
> > else
> > value.set(0x100000000ULL);
> >
> > ...
> >
> > int i = v.getInt();
> >
> > This will cause a crash in the unlikely event that condition is false,
> > and could get unnoticed through testing. An explicit set API with type
> > names may help there. Given that the Value class will be used for
> > controls, I think we'll have to make sure we don't crash, invalid types
> > will need to be caught and reported. We could also decide than an
> > invalid type would result in the control taking a default value instead
> > of crashing (but then this should be logged).
>
> I'm not sure taking a default value is appropriate either. I don't think
> any 'default' would be correct.
>
> IMO - the types were important, and the asserts were to ensure strict
> typing when used.
Could we do it the Qt way, where the get functions return an invalid
value if the type isn't convertible ? For integers and booleans there's
no invalid values, so Qt adds an optional bool *ok argument. In practice
I expect the callers to rarely use it though, so I think we need to pick
between two error handling behaviours: returning a "default" value (0
for integers for instance) or crashing. What's best ? :-)
Now that I wrote this, I realised that ASSERT() will be compiled out in
release builds. Having it in debug builds is thus likely a good idea,
but we still need to decide on how to handle release builds.
> If the set() call is changed to setInt() would this make you more
> comfortable?
A little bit :-)
> >> +
> >> + return bool_;
> >> +}
> >> +
> >> +/**
> >> + * \brief Get the integer value.
> >> + *
> >> + * The Value type must be Integer.
> >> + */
> >> +int Value::getInt() const
> >> +{
> >> + ASSERT(type_ == ValueInteger);
> >> +
> >> + return integer_;
> >> +}
> >> +
> >> +/**
> >> + * \brief Get the 64bit integer value.
> >> + *
> >> + * The Value type must be Integer64.
> >> + */
> >> +int Value::getInt64() const
> >> +{
> >> + ASSERT(type_ == ValueInteger64);
> >> +
> >> + return integer64_;
> >> +}
> >> +
> >> +/**
> >> + * \brief Get the string value.
> >> + *
> >> + * The Value type must be String.
> >> + */
> >> +std::string Value::getString() const
> >> +{
> >> + ASSERT(type_ == ValueString);
> >> +
> >> + return string_;
> >> +}
> >> +
> >> +/**
> >> + * \brief Prepare a string representation of the Value
> >> + */
> >> +std::string Value::toString() const
> >> +{
> >> + switch (type_) {
> >> + case ValueNull:
> >> + return "<NULL>";
> >> + case ValueBool:
> >> + return bool_ ? "True" : "False";
> >> + case ValueInteger:
> >> + return std::to_string(integer_);
> >> + case ValueInteger64:
> >> + return std::to_string(integer64_);
> >> + case ValueString:
> >> + return string_;
> >> + }
> >> +
> >> + /* Unreachable */
> >> + return "<ValueType Error>";
> >
> > Do you need this ? Does the compiler complain when the switch-case
> > handles all the cases ?
>
> Yup.
>
> ../src/libcamera/value.cpp: In member function ‘std::__cxx11::string
> libcamera::Value::toString() const’:
> ../src/libcamera/value.cpp:212:1: error: control reaches end of non-void
> function [-Werror=return-type]
> }
> ^
> cc1plus: all warnings being treated as errors
>
> I can move it to a 'default:' statement which then removes the warning -
> but should still be redundant/ unreachable.
A default statement sounds nice, but I think that an error after the
switch() is better as in that case the compiler should warn you if you
add a new enum entry without adding a corresponding case.
> >> +}
> >> +
> >> +/**
> >> + * \brief Provide a string stream representation of the Value \a value to
> >> + * the \a stream.
> >
> > It's not a string stream, it's an output stream (and you should include
> > ostream instead of sstream).
> >
> >> + */
> >> +std::ostream &operator<<(std::ostream &stream, const Value &value)
> >> +{
> >> + return stream << value.toString();
> >> +}
> >> +
> >> +} /* namespace libcamera */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list