[libcamera-devel] [RFC PATCH v2 1/9] libcamera: Value: Provide abstract value class

[Laurent Pinchart]

Hi Kieran,

On Mon, Jun 24, 2019 at 04:39:41PM +0100, Kieran Bingham wrote:
> On 24/06/2019 13:09, Laurent Pinchart wrote:
> > 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).
> 
> Yes, it can be done later, I don't want to postpone Jacopo's series.
> 
> I'd still like to keep this called Value, do you want it all renamed to
> ControlValue ?

I have a slight preference for that, especially given that the
implementation is restricted to the types needed for controls, and that
it tries to optimise for control-related use cases.

> >>>> +};
> >>>> +
> >>>> +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.
> 
> Am I misunderstanding /your/ definition of ASSERT? :
> 
> >  * \def ASSERT(condition)
> >  * \brief Abort program execution if assertion fails
> >  *
> >  * If \a condition is false, ASSERT() logs an error message with the Fatal log
> >  * level and aborts program execution.
> >  *
> >  * If the macro NDEBUG is defined before including log.h, ASSERT() generates no
> >  * code.
> 
> ASSERT should never fire in production, because NDEBUG should be set for
> BuildType=Release builds.
> 
> >>>
> >>> 	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
> 
> Eurgh ... an 'ok' bool sounds icky... I'll look at the Qt code ...

Yes, I don't like it much :-(

> > 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 ? :-)
> 
> Crashing in debug builds only :D and returning an undefined value otherwise.
> 
> (it will be whatever the contents of the union is, with whatever
> implicit type conversion occurs, which 'may' be as correct as it can be).

Should we return a fixed value instead, to make the behaviour
reproducible ? Otherwise this may lead to bugs that are hard to
reproduce.

> > 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.
> 
> Aha, well I didn't need to paste that in above then :)
> 
> Too late ! I'll leave it there :P
> 
> >> If the set() call is changed to setInt() would this make you more
> >> comfortable?
> > 
> > A little bit :-)
> 
> Ok - I'll remove the implicit type sets.

If we go for returning a fixed default value when the type doesn't
match, and if we add conversion between compatible types, then
overloading the set() function is fine.

> >>>> +
> >>>> +	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.
> 
> I agree, I'd rather the compiler tell us if we have a missing enum type.
> 
> >>>> +}
> >>>> +
> >>>> +/**
> >>>> + * \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