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

Mon Jun 24 11:09:06 CEST 2019

Hi Laurent,

On 22/06/2019 23:39, Laurent Pinchart wrote:
> 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.

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.

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

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

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

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

If the set() call is changed to setInt() would this make you more
comfortable?

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

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