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

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

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.

> +	ValueNull,

Let's call it ValueUnset or ValueNone. Null has a well established
meaning.

> +	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.

> +{
> +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() ?

> +
> +	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.

> +	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.

> +	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.

> +};
> +
> +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.

> +
> +#include "log.h" // For ASSERT()

You don't need to keep the comment.

> +
> +/**
> + * \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.

> + */
> +
> +/**
> + * \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


	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).

> +
> +	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 ?

> +}
> +
> +/**
> + * \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