From patchwork Wed Sep 18 10:31:29 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jacopo Mondi X-Patchwork-Id: 1979 Return-Path: Received: from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net [217.70.183.201]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 08CC460BB0; Wed, 18 Sep 2019 12:30:01 +0200 (CEST) X-Originating-IP: 2.224.242.101 Received: from uno.lan (2-224-242-101.ip172.fastwebnet.it [2.224.242.101]) (Authenticated sender: jacopo@jmondi.org) by relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 6F6E41BF205; Wed, 18 Sep 2019 10:30:00 +0000 (UTC) From: Jacopo Mondi To: libcamera-devel@lists.libcamera.org, To:libcamera-devel@lists.libcamera.org Date: Wed, 18 Sep 2019 12:31:29 +0200 Message-Id: <20190918103133.14296-2-jacopo@jmondi.org> X-Mailer: git-send-email 2.23.0 In-Reply-To: <20190918103133.14296-1-jacopo@jmondi.org> References: <20190918103133.14296-1-jacopo@jmondi.org> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH 1/5] libcamera: Create DataValue and DataInfo X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 18 Sep 2019 10:30:01 -0000 Add the data_value.[h|c] files that provide an abstracted polymorphic data value type and a generic data info type that represent static information associated with a data value. DataValue is a slightly re-worked copy of the ControlValue type defined in controls.h, provided in a generic header to be used a foundation for both Controls and V4L2Controls classes that will be re-worked in the next patches in the series. Add a test for data value which is an adapted copy of the control value test, that will be removed in the next patch. Signed-off-by: Jacopo Mondi --- include/libcamera/data_value.h | 84 +++++++++ include/libcamera/meson.build | 1 + src/libcamera/data_value.cpp | 317 +++++++++++++++++++++++++++++++++ src/libcamera/meson.build | 1 + test/data_value/data_value.cpp | 59 ++++++ test/data_value/meson.build | 12 ++ test/meson.build | 1 + 7 files changed, 475 insertions(+) create mode 100644 include/libcamera/data_value.h create mode 100644 src/libcamera/data_value.cpp create mode 100644 test/data_value/data_value.cpp create mode 100644 test/data_value/meson.build diff --git a/include/libcamera/data_value.h b/include/libcamera/data_value.h new file mode 100644 index 000000000000..0fcd9bd04a65 --- /dev/null +++ b/include/libcamera/data_value.h @@ -0,0 +1,84 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2019, Google Inc. + * + * data_value.h - Polymorphic data value type + */ +#ifndef __LIBCAMERA_DATA_VALUE_H__ +#define __LIBCAMERA_DATA_VALUE_H__ + +#include +#include + +namespace libcamera { + +enum DataType { + DataTypeNone, + DataTypeBool, + DataTypeInteger, + DataTypeInteger64, + DataTypeNumber, +}; + +static constexpr size_t DataSize[DataTypeNumber] = { + [DataTypeNone] = 1, + [DataTypeBool] = 4, + [DataTypeInteger] = 4, + [DataTypeInteger64] = 8, +}; + +class DataValue +{ +public: + DataValue(); + DataValue(const DataValue &other); + DataValue(bool value); + DataValue(int value); + DataValue(int64_t value); + + DataValue &operator=(const DataValue &other); + + enum DataType type() const { return type_; } + size_t size() const { return size_; } + + void set(bool value); + void set(int value); + void set(int64_t value); + + bool getBool() const; + int getInt() const; + int64_t getInt64() const; + + std::string toString() const; + +private: + DataType type_; + size_t size_; + + union { + bool bool_; + int integer_; + int64_t integer64_; + }; +}; + +class DataInfo +{ +public: + DataInfo(const DataValue &min, const DataValue &max) + { + min_ = min; + max_ = max; + } + + const DataValue &min() const { return min_; } + const DataValue &max() const { return max_; } + +private: + DataValue max_; + DataValue min_; +}; + +} /* namespace libcamera */ + +#endif /* __LIBCAMERA_DATA_VALUE_H__ */ diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build index 868f1a6bf1ab..e3f3ad504446 100644 --- a/include/libcamera/meson.build +++ b/include/libcamera/meson.build @@ -5,6 +5,7 @@ libcamera_api = files([ 'camera_manager.h', 'control_ids.h', 'controls.h', + 'data_value.h', 'event_dispatcher.h', 'event_notifier.h', 'geometry.h', diff --git a/src/libcamera/data_value.cpp b/src/libcamera/data_value.cpp new file mode 100644 index 000000000000..f07b5fb75a8a --- /dev/null +++ b/src/libcamera/data_value.cpp @@ -0,0 +1,317 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2019, Google Inc. + * + * data_value.cpp - Polymorphic data value type + */ + +#include + +#include +#include +#include + +#include "utils.h" +#include "log.h" + +/** + * \file data_value.h + * \brief Polymorphic data type + */ + +namespace libcamera { + +/** + * \enum DataType + * \brief Define the supported data types + * \var DataTypeNone + * Identifies an unset value + * \var DataTypeBool + * Identifies DataType storing a boolean value + * \var DataTypeInteger + * Identifies DataType storing an integer value + * \var DataTypeInteger64 + * Identifies DataType storing a 64-bit integer value + */ + +/** + * \var DataSize + * \brief Associate to each data type the memory size in bytes + */ + +#if 0 +/** + * \class Data + * \brief Base class for data value and data info instances + * + * DataValue and DataInfo share basic information like size and type. + * This class provides common fields and operations for them. + */ + +/** + * \brief Construct a Data of \a type + * \param[in] type The Data type defined by DataType + */ +Data::Data(DataType type) + : type_(type) +{ + /* + * \todo The size of compound data types depend on the number of + * elements too. + */ + size_ = DataSize[type_]; +} + +/** + * \var Data::type_ + * \brief The data type + */ + +/** + * \var Data::size_ + * \brief The data size + */ + +#endif + +/** + * \class DataValue + * \brief Polymorphic data value + * + * DataValue holds values of different types, defined by DataType providing + * a unified interface to access and modify the data content. + * + * DataValue instances are used by classes that need to represent and store + * numerical values of different types. + * + * \todo Add support for compound data types, such as arrays. + */ + +/** + * \brief Construct an empty DataValue + */ +DataValue::DataValue() + : type_(DataTypeNone), size_(DataSize[type_]) +{ +} + +/** + * \brief Construct a DataValue copying data from \a other + * \param[in] other The DataValue to copy data from + * + * DataValue copy constructor. + */ +DataValue::DataValue(const DataValue &other) + : type_(other.type()), size_(DataSize[type_]) +{ + switch (type_) { + case DataTypeBool: + bool_ = other.getBool(); + break; + case DataTypeInteger: + integer_ = other.getInt(); + break; + case DataTypeInteger64: + integer64_ = other.getInt64(); + break; + default: + bool_ = integer_ = integer64_ = 0; + break; + } +} + +/** + * \brief Construct a boolean DataValue + * \param[in] value Boolean value to store + */ +DataValue::DataValue(bool value) + : type_(DataTypeBool), size_(DataSize[type_]), bool_(value) +{ +} + +/** + * \brief Construct an integer DataValue + * \param[in] value Integer value to store + */ +DataValue::DataValue(int value) + : type_(DataTypeInteger), size_(DataSize[type_]), integer_(value) +{ +} + +/** + * \brief Construct a 64 bit integer DataValue + * \param[in] value 64-bits integer value to store + */ +DataValue::DataValue(int64_t value) + : type_(DataTypeInteger64), size_(DataSize[type_]), integer64_(value) +{ +} + +/** + * \brief Assign the DataValue type and value using the ones from \a other + * \param[in] other The DataValue to copy type and value from + * + * DataValue assignement operator. + * + * \return The DataValue with fields updated using the ones from \a other + */ +DataValue &DataValue::operator=(const DataValue &other) +{ + DataType newType = other.type(); + type_ = newType; + size_ = DataSize[type_]; + + switch (newType) { + case DataTypeBool: + bool_ = other.getBool(); + break; + case DataTypeInteger: + integer_ = other.getInt(); + break; + case DataTypeInteger64: + integer64_ = other.getInt64(); + break; + default: + bool_ = integer_ = integer64_ = 0; + break; + } + + return *this; +} + +/** + * \fn DataValue::type() + * \brief Retrieve the data type of the data + * \return The type of the data + */ + +/** + * \fn DataValue::size() + * \brief Retrieve the size in bytes of the data + * \return The size in bytes of the data + */ + +/** + * \brief Set the value with a boolean + * \param[in] value Boolean value to store + */ +void DataValue::set(bool value) +{ + type_ = DataTypeBool; + size_ = DataSize[type_]; + bool_ = value; +} + +/** + * \brief Set the value with an integer + * \param[in] value Integer value to store + */ +void DataValue::set(int value) +{ + type_ = DataTypeInteger; + size_ = DataSize[type_]; + integer_ = value; +} + +/** + * \brief Set the value with a 64 bit integer + * \param[in] value 64 bit integer value to store + */ +void DataValue::set(int64_t value) +{ + type_ = DataTypeInteger64; + size_ = DataSize[type_]; + integer64_ = value; +} + +/** + * \brief Get the boolean value + * + * The value type must be Boolean. + * + * \return The boolean value + */ +bool DataValue::getBool() const +{ + ASSERT(type_ == DataTypeBool); + + return bool_; +} + +/** + * \brief Get the integer value + * + * The value type must be Integer or Integer64. + * + * \return The integer value + */ +int DataValue::getInt() const +{ + ASSERT(type_ == DataTypeInteger || type_ == DataTypeInteger64); + + return integer_; +} + +/** + * \brief Get the 64-bit integer value + * + * The value type must be Integer or Integer64. + * + * \return The 64-bit integer value + */ +int64_t DataValue::getInt64() const +{ + ASSERT(type_ == DataTypeInteger || type_ == DataTypeInteger64); + + return integer64_; +} + +/** + * \brief Assemble and return a string describing the value + * \return A string describing the DataValue + */ +std::string DataValue::toString() const +{ + switch (type_) { + case DataTypeBool: + return bool_ ? "True" : "False"; + case DataTypeInteger: + return std::to_string(integer_); + case DataTypeInteger64: + return std::to_string(integer64_); + default: + return ""; + } +} + +/** + * \class DataInfo + * \brief Validation informations associated with a data value + * + * The DataInfo class represents static information associated with data + * types that represent plymorhpic values abstracted by the DataValue class. + * + * DataInfo stores static informations such as the value minimum and maximum + * values and for compound values the maximum and minimum number of elements. + */ + +/** + * \fn DataInfo::DataInfo + * \brief Construct a data info with \a min and \a max values + * \param[in] min The minimum allowed value + * \param[in] max The maximum allowed value + */ + +/** + * \fn DataInfo::min() + * \brief Retrieve the DataInfo minimum allowed value + * \return A DataValue representing the minimum allowed value + */ + +/** + * \fn DataInfo::max() + * \brief Retrieve the DataInfo maximum allowed value + * \return A DataValue representing the maximum allowed value + */ + +} /* namespace libcamera */ diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build index 755149c55c7b..c3100a1709e0 100644 --- a/src/libcamera/meson.build +++ b/src/libcamera/meson.build @@ -5,6 +5,7 @@ libcamera_sources = files([ 'camera_manager.cpp', 'camera_sensor.cpp', 'controls.cpp', + 'data_value.cpp', 'device_enumerator.cpp', 'device_enumerator_sysfs.cpp', 'event_dispatcher.cpp', diff --git a/test/data_value/data_value.cpp b/test/data_value/data_value.cpp new file mode 100644 index 000000000000..64061000c1e4 --- /dev/null +++ b/test/data_value/data_value.cpp @@ -0,0 +1,59 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * Copyright (C) 2019, Google Inc. + * + * data_value.cpp - DataValue tests + */ + +#include + +#include + +#include "test.h" + +using namespace std; +using namespace libcamera; + +class DataValueTest : public Test +{ +protected: + int run() + { + DataValue integer(1234); + DataValue boolean(true); + + /* Just a string conversion output test... no validation */ + cout << "Int: " << integer.toString() + << " Bool: " << boolean.toString() + << endl; + + if (integer.getInt() != 1234) { + cerr << "Failed to get Integer" << endl; + return TestFail; + } + + if (boolean.getBool() != true) { + cerr << "Failed to get Boolean" << endl; + return TestFail; + } + + /* Test an uninitialised value, and updating it. */ + + DataValue value; + value.set(true); + if (value.getBool() != true) { + cerr << "Failed to get Booleans" << endl; + return TestFail; + } + + value.set(10); + if (value.getInt() != 10) { + cerr << "Failed to get Integer" << endl; + return TestFail; + } + + return TestPass; + } +}; + +TEST_REGISTER(DataValueTest) diff --git a/test/data_value/meson.build b/test/data_value/meson.build new file mode 100644 index 000000000000..3858e6085a1f --- /dev/null +++ b/test/data_value/meson.build @@ -0,0 +1,12 @@ +data_value_tests = [ + [ 'data_value', 'data_value.cpp' ], +] + +foreach t : data_value_tests + exe = executable(t[0], t[1], + dependencies : libcamera_dep, + link_with : test_libraries, + include_directories : test_includes_internal) + test(t[0], exe, suite : 'data_value', is_parallel : false) +endforeach + diff --git a/test/meson.build b/test/meson.build index 84722cceb35d..5d414b22dc0c 100644 --- a/test/meson.build +++ b/test/meson.build @@ -2,6 +2,7 @@ subdir('libtest') subdir('camera') subdir('controls') +subdir('data_value') subdir('ipa') subdir('ipc') subdir('log') From patchwork Wed Sep 18 10:31:30 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jacopo Mondi X-Patchwork-Id: 1980 Return-Path: Received: from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net [217.70.183.201]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id DA21E60DED; Wed, 18 Sep 2019 12:30:01 +0200 (CEST) X-Originating-IP: 2.224.242.101 Received: from uno.lan (2-224-242-101.ip172.fastwebnet.it [2.224.242.101]) (Authenticated sender: jacopo@jmondi.org) by relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 474DA1BF208; Wed, 18 Sep 2019 10:30:01 +0000 (UTC) From: Jacopo Mondi To: libcamera-devel@lists.libcamera.org, To:libcamera-devel@lists.libcamera.org Date: Wed, 18 Sep 2019 12:31:30 +0200 Message-Id: <20190918103133.14296-3-jacopo@jmondi.org> X-Mailer: git-send-email 2.23.0 In-Reply-To: <20190918103133.14296-1-jacopo@jmondi.org> References: <20190918103133.14296-1-jacopo@jmondi.org> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH 2/5] libcamera: controls: Use DataType and DataValue X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 18 Sep 2019 10:30:02 -0000 Replace the use of ControlValue with DataValue and make ControlInfo a DataInfo derived class to maximize the code reuse with V4L2Control which will be modified in the next patch. Remove the now unused control_value test, replaced in the previous patch by data_value test. Signed-off-by: Jacopo Mondi --- include/libcamera/controls.h | 77 ++------- src/libcamera/controls.cpp | 237 ++-------------------------- src/libcamera/gen-controls.awk | 2 +- src/libcamera/pipeline/uvcvideo.cpp | 2 +- src/libcamera/pipeline/vimc.cpp | 2 +- test/controls/control_info.cpp | 4 +- test/controls/control_value.cpp | 69 -------- test/controls/meson.build | 1 - 8 files changed, 26 insertions(+), 368 deletions(-) delete mode 100644 test/controls/control_value.cpp diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h index fbb3a62274c6..e46cd8a78679 100644 --- a/include/libcamera/controls.h +++ b/include/libcamera/controls.h @@ -13,98 +13,39 @@ #include #include +#include namespace libcamera { class Camera; -enum ControlValueType { - ControlValueNone, - ControlValueBool, - ControlValueInteger, - ControlValueInteger64, -}; - -class ControlValue -{ -public: - ControlValue(); - ControlValue(bool value); - ControlValue(int value); - ControlValue(int64_t value); - - ControlValueType type() const { return type_; }; - bool isNone() const { return type_ == ControlValueNone; }; - - void set(bool value); - void set(int value); - void set(int64_t value); - - bool getBool() const; - int getInt() const; - int64_t getInt64() const; - - std::string toString() const; - -private: - ControlValueType type_; - - union { - bool bool_; - int integer_; - int64_t integer64_; - }; -}; - struct ControlIdentifier { ControlId id; const char *name; - ControlValueType type; + DataType type; }; -class ControlInfo +class ControlInfo : public DataInfo { public: - explicit ControlInfo(ControlId id, const ControlValue &min = 0, - const ControlValue &max = 0); - + explicit ControlInfo(ControlId id, const DataValue &min = 0, + const DataValue &max = 0); ControlId id() const { return ident_->id; } const char *name() const { return ident_->name; } - ControlValueType type() const { return ident_->type; } - - const ControlValue &min() const { return min_; } - const ControlValue &max() const { return max_; } + DataType type() const { return ident_->type; } std::string toString() const; private: const struct ControlIdentifier *ident_; - ControlValue min_; - ControlValue max_; }; -bool operator==(const ControlInfo &lhs, const ControlInfo &rhs); -bool operator==(const ControlId &lhs, const ControlInfo &rhs); -bool operator==(const ControlInfo &lhs, const ControlId &rhs); -static inline bool operator!=(const ControlInfo &lhs, const ControlInfo &rhs) -{ - return !(lhs == rhs); -} -static inline bool operator!=(const ControlId &lhs, const ControlInfo &rhs) -{ - return !(lhs == rhs); -} -static inline bool operator!=(const ControlInfo &lhs, const ControlId &rhs) -{ - return !(lhs == rhs); -} - using ControlInfoMap = std::unordered_map; class ControlList { private: - using ControlListMap = std::unordered_map; + using ControlListMap = std::unordered_map; public: ControlList(Camera *camera); @@ -123,8 +64,8 @@ public: std::size_t size() const { return controls_.size(); } void clear() { controls_.clear(); } - ControlValue &operator[](ControlId id); - ControlValue &operator[](const ControlInfo *info) { return controls_[info]; } + DataValue &operator[](ControlId id); + DataValue &operator[](const ControlInfo *info) { return controls_[info]; } void update(const ControlList &list); diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp index 727fdbd9450d..2d8adde5688e 100644 --- a/src/libcamera/controls.cpp +++ b/src/libcamera/controls.cpp @@ -24,163 +24,6 @@ namespace libcamera { LOG_DEFINE_CATEGORY(Controls) -/** - * \enum ControlValueType - * \brief Define the data type of value represented by a ControlValue - * \var ControlValueNone - * Identifies an unset control value - * \var ControlValueBool - * Identifies controls storing a boolean value - * \var ControlValueInteger - * Identifies controls storing an integer value - * \var ControlValueInteger64 - * Identifies controls storing a 64-bit integer value - */ - -/** - * \class ControlValue - * \brief Abstract type representing the value of a control - */ - -/** - * \brief Construct an empty ControlValue. - */ -ControlValue::ControlValue() - : type_(ControlValueNone) -{ -} - -/** - * \brief Construct a Boolean ControlValue - * \param[in] value Boolean value to store - */ -ControlValue::ControlValue(bool value) - : type_(ControlValueBool), bool_(value) -{ -} - -/** - * \brief Construct an integer ControlValue - * \param[in] value Integer value to store - */ -ControlValue::ControlValue(int value) - : type_(ControlValueInteger), integer_(value) -{ -} - -/** - * \brief Construct a 64 bit integer ControlValue - * \param[in] value Integer value to store - */ -ControlValue::ControlValue(int64_t value) - : type_(ControlValueInteger64), integer64_(value) -{ -} - -/** - * \fn ControlValue::type() - * \brief Retrieve the data type of the value - * \return The value data type - */ - -/** - * \fn ControlValue::isNone() - * \brief Determine if the value is not initialised - * \return True if the value type is ControlValueNone, false otherwise - */ - -/** - * \brief Set the value with a boolean - * \param[in] value Boolean value to store - */ -void ControlValue::set(bool value) -{ - type_ = ControlValueBool; - bool_ = value; -} - -/** - * \brief Set the value with an integer - * \param[in] value Integer value to store - */ -void ControlValue::set(int value) -{ - type_ = ControlValueInteger; - integer_ = value; -} - -/** - * \brief Set the value with a 64 bit integer - * \param[in] value 64 bit integer value to store - */ -void ControlValue::set(int64_t value) -{ - type_ = ControlValueInteger64; - integer64_ = value; -} - -/** - * \brief Get the boolean value - * - * The value type must be Boolean. - * - * \return The boolean value - */ -bool ControlValue::getBool() const -{ - ASSERT(type_ == ControlValueBool); - - return bool_; -} - -/** - * \brief Get the integer value - * - * The value type must be Integer or Integer64. - * - * \return The integer value - */ -int ControlValue::getInt() const -{ - ASSERT(type_ == ControlValueInteger || type_ == ControlValueInteger64); - - return integer_; -} - -/** - * \brief Get the 64-bit integer value - * - * The value type must be Integer or Integer64. - * - * \return The 64-bit integer value - */ -int64_t ControlValue::getInt64() const -{ - ASSERT(type_ == ControlValueInteger || type_ == ControlValueInteger64); - - return integer64_; -} - -/** - * \brief Assemble and return a string describing the value - * \return A string describing the ControlValue - */ -std::string ControlValue::toString() const -{ - switch (type_) { - case ControlValueNone: - return ""; - case ControlValueBool: - return bool_ ? "True" : "False"; - case ControlValueInteger: - return std::to_string(integer_); - case ControlValueInteger64: - return std::to_string(integer64_); - } - - return ""; -} - /** * \enum ControlId * \brief Numerical control ID @@ -269,9 +112,9 @@ extern const std::unordered_map controlTypes; * \param[in] min The control minimum value * \param[in] max The control maximum value */ -ControlInfo::ControlInfo(ControlId id, const ControlValue &min, - const ControlValue &max) - : min_(min), max_(max) +ControlInfo::ControlInfo(ControlId id, const DataValue &min, + const DataValue &max) + : DataInfo(min, max) { auto iter = controlTypes.find(id); if (iter == controlTypes.end()) { @@ -296,79 +139,23 @@ ControlInfo::ControlInfo(ControlId id, const ControlValue &min, /** * \fn ControlInfo::type() - * \brief Retrieve the control data type - * \return The control data type - */ - -/** - * \fn ControlInfo::min() - * \brief Retrieve the minimum value of the control - * \return A ControlValue with the minimum value for the control - */ - -/** - * \fn ControlInfo::max() - * \brief Retrieve the maximum value of the control - * \return A ControlValue with the maximum value for the control + * \brief Retrieve the control designated type + * \return The control type */ /** * \brief Provide a string representation of the ControlInfo + * \return The control name */ std::string ControlInfo::toString() const { std::stringstream ss; - ss << name() << "[" << min_.toString() << ".." << max_.toString() << "]"; + ss << name() << "[" << min().toString() << ".." << max().toString() << "]"; return ss.str(); } -/** - * \brief Compare control information for equality - * \param[in] lhs Left-hand side control information - * \param[in] rhs Right-hand side control information - * - * Control information is compared based on the ID only, as a camera may not - * have two separate controls with the same ID. - * - * \return True if \a lhs and \a rhs are equal, false otherwise - */ -bool operator==(const ControlInfo &lhs, const ControlInfo &rhs) -{ - return lhs.id() == rhs.id(); -} - -/** - * \brief Compare control ID and information for equality - * \param[in] lhs Left-hand side control identifier - * \param[in] rhs Right-hand side control information - * - * Control information is compared based on the ID only, as a camera may not - * have two separate controls with the same ID. - * - * \return True if \a lhs and \a rhs are equal, false otherwise - */ -bool operator==(const ControlId &lhs, const ControlInfo &rhs) -{ - return lhs == rhs.id(); -} - -/** - * \brief Compare control information and ID for equality - * \param[in] lhs Left-hand side control information - * \param[in] rhs Right-hand side control identifier - * - * Control information is compared based on the ID only, as a camera may not - * have two separate controls with the same ID. - * - * \return True if \a lhs and \a rhs are equal, false otherwise - */ -bool operator==(const ControlInfo &lhs, const ControlId &rhs) -{ - return lhs.id() == rhs; -} - /** * \typedef ControlInfoMap * \brief A map of ControlId to ControlInfo @@ -378,7 +165,7 @@ bool operator==(const ControlInfo &lhs, const ControlId &rhs) * \class ControlList * \brief Associate a list of ControlId with their values for a camera * - * A ControlList wraps a map of ControlId to ControlValue and provide + * A ControlList wraps a map of ControlId to DataValue and provide * additional validation against the control information exposed by a Camera. * * A list is only valid for as long as the camera it refers to is valid. After @@ -474,7 +261,7 @@ bool ControlList::contains(const ControlInfo *info) const /** * \fn ControlList::size() * \brief Retrieve the number of controls in the list - * \return The number of Control entries stored in the list + * \return The number of DataValue entries stored in the list */ /** @@ -494,7 +281,7 @@ bool ControlList::contains(const ControlInfo *info) const * * \return A reference to the value of the control identified by \a id */ -ControlValue &ControlList::operator[](ControlId id) +DataValue &ControlList::operator[](ControlId id) { const ControlInfoMap &controls = camera_->controls(); const auto iter = controls.find(id); @@ -503,7 +290,7 @@ ControlValue &ControlList::operator[](ControlId id) << "Camera " << camera_->name() << " does not support control " << id; - static ControlValue empty; + static DataValue empty; return empty; } @@ -543,7 +330,7 @@ void ControlList::update(const ControlList &other) for (auto it : other) { const ControlInfo *info = it.first; - const ControlValue &value = it.second; + const DataValue &value = it.second; controls_[info] = value; } diff --git a/src/libcamera/gen-controls.awk b/src/libcamera/gen-controls.awk index f3d068123012..abeb0218546b 100755 --- a/src/libcamera/gen-controls.awk +++ b/src/libcamera/gen-controls.awk @@ -92,7 +92,7 @@ function GenerateTable(file) { print "extern const std::unordered_map" > file print "controlTypes {" > file for (i=1; i <= id; ++i) { - printf "\t{ %s, { %s, \"%s\", ControlValue%s } },\n", names[i], names[i], names[i], types[i] > file + printf "\t{ %s, { %s, \"%s\", DataType%s } },\n", names[i], names[i], names[i], types[i] > file } print "};" > file ExitNameSpace(file) diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp index 8965210550d2..dd253f94ca3d 100644 --- a/src/libcamera/pipeline/uvcvideo.cpp +++ b/src/libcamera/pipeline/uvcvideo.cpp @@ -231,7 +231,7 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request) for (auto it : request->controls()) { const ControlInfo *ci = it.first; - ControlValue &value = it.second; + DataValue &value = it.second; switch (ci->id()) { case Brightness: diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp index f26a91f86ec1..3df239bdb277 100644 --- a/src/libcamera/pipeline/vimc.cpp +++ b/src/libcamera/pipeline/vimc.cpp @@ -284,7 +284,7 @@ int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request) for (auto it : request->controls()) { const ControlInfo *ci = it.first; - ControlValue &value = it.second; + DataValue &value = it.second; switch (ci->id()) { case Brightness: diff --git a/test/controls/control_info.cpp b/test/controls/control_info.cpp index aa3a65b1e5ef..c3f9b85580a7 100644 --- a/test/controls/control_info.cpp +++ b/test/controls/control_info.cpp @@ -26,7 +26,7 @@ protected: ControlInfo info(Brightness); if (info.id() != Brightness || - info.type() != ControlValueInteger || + info.type() != DataTypeInteger || info.name() != std::string("Brightness")) { cout << "Invalid control identification for Brightness" << endl; return TestFail; @@ -44,7 +44,7 @@ protected: info = ControlInfo(Contrast, 10, 200); if (info.id() != Contrast || - info.type() != ControlValueInteger || + info.type() != DataTypeInteger || info.name() != std::string("Contrast")) { cout << "Invalid control identification for Contrast" << endl; return TestFail; diff --git a/test/controls/control_value.cpp b/test/controls/control_value.cpp deleted file mode 100644 index 778efe5c115f..000000000000 --- a/test/controls/control_value.cpp +++ /dev/null @@ -1,69 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0-or-later */ -/* - * Copyright (C) 2019, Google Inc. - * - * control_value.cpp - ControlValue tests - */ - -#include - -#include - -#include "test.h" - -using namespace std; -using namespace libcamera; - -class ControlValueTest : public Test -{ -protected: - int run() - { - ControlValue integer(1234); - ControlValue boolean(true); - - /* Just a string conversion output test... no validation */ - cout << "Int: " << integer.toString() - << " Bool: " << boolean.toString() - << endl; - - if (integer.getInt() != 1234) { - cerr << "Failed to get Integer" << endl; - return TestFail; - } - - if (boolean.getBool() != true) { - cerr << "Failed to get Boolean" << endl; - return TestFail; - } - - /* Test an uninitialised value, and updating it. */ - - ControlValue value; - if (!value.isNone()) { - cerr << "Empty value is non-null" << endl; - return TestFail; - } - - value.set(true); - if (value.isNone()) { - cerr << "Failed to set an empty object" << endl; - return TestFail; - } - - if (value.getBool() != true) { - cerr << "Failed to get Booleans" << endl; - return TestFail; - } - - value.set(10); - if (value.getInt() != 10) { - cerr << "Failed to get Integer" << endl; - return TestFail; - } - - return TestPass; - } -}; - -TEST_REGISTER(ControlValueTest) diff --git a/test/controls/meson.build b/test/controls/meson.build index f4fc7b947dd9..9070be85718b 100644 --- a/test/controls/meson.build +++ b/test/controls/meson.build @@ -1,7 +1,6 @@ control_tests = [ [ 'control_info', 'control_info.cpp' ], [ 'control_list', 'control_list.cpp' ], - [ 'control_value', 'control_value.cpp' ], ] foreach t : control_tests From patchwork Wed Sep 18 10:31:31 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jacopo Mondi X-Patchwork-Id: 1981 Return-Path: Received: from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net [217.70.183.201]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id DA4E860BB0; Wed, 18 Sep 2019 12:30:02 +0200 (CEST) X-Originating-IP: 2.224.242.101 Received: from uno.lan (2-224-242-101.ip172.fastwebnet.it [2.224.242.101]) (Authenticated sender: jacopo@jmondi.org) by relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 207131BF20A; Wed, 18 Sep 2019 10:30:01 +0000 (UTC) From: Jacopo Mondi To: libcamera-devel@lists.libcamera.org, To:libcamera-devel@lists.libcamera.org Date: Wed, 18 Sep 2019 12:31:31 +0200 Message-Id: <20190918103133.14296-4-jacopo@jmondi.org> X-Mailer: git-send-email 2.23.0 In-Reply-To: <20190918103133.14296-1-jacopo@jmondi.org> References: <20190918103133.14296-1-jacopo@jmondi.org> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH 3/5] libcamera: v4l2_controls: Use DataValue and DataInfo X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 18 Sep 2019 10:30:03 -0000 Use DataValue and DataInfo in the V4L2 control handling classes to maximize code reuse with the libcamera controls classes. Signed-off-by: Jacopo Mondi --- src/libcamera/include/v4l2_controls.h | 22 ++++-------- src/libcamera/include/v4l2_device.h | 1 - src/libcamera/v4l2_controls.cpp | 49 +++++++-------------------- src/libcamera/v4l2_device.cpp | 25 ++++++-------- 4 files changed, 30 insertions(+), 67 deletions(-) diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h index 10b726504951..27b855f3407f 100644 --- a/src/libcamera/include/v4l2_controls.h +++ b/src/libcamera/include/v4l2_controls.h @@ -16,29 +16,18 @@ #include #include +#include + namespace libcamera { -class V4L2ControlInfo +class V4L2ControlInfo : public DataInfo { public: V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl); - unsigned int id() const { return id_; } - unsigned int type() const { return type_; } - size_t size() const { return size_; } - const std::string &name() const { return name_; } - - int64_t min() const { return min_; } - int64_t max() const { return max_; } private: unsigned int id_; - unsigned int type_; - size_t size_; - std::string name_; - - int64_t min_; - int64_t max_; }; using V4L2ControlInfoMap = std::map; @@ -49,14 +38,15 @@ public: V4L2Control(unsigned int id, int value = 0) : id_(id), value_(value) {} - int64_t value() const { return value_; } + DataType type() const { return value_.type(); } + int64_t value() const { return value_.getInt64(); } void setValue(int64_t value) { value_ = value; } unsigned int id() const { return id_; } private: unsigned int id_; - int64_t value_; + DataValue value_; }; class V4L2ControlList diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h index 75a52c33d821..3144adc26e36 100644 --- a/src/libcamera/include/v4l2_device.h +++ b/src/libcamera/include/v4l2_device.h @@ -44,7 +44,6 @@ protected: private: void listControls(); void updateControls(V4L2ControlList *ctrls, - const V4L2ControlInfo **controlInfo, const struct v4l2_ext_control *v4l2Ctrls, unsigned int count); diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp index 84258d9954d0..9bc4929cbd76 100644 --- a/src/libcamera/v4l2_controls.cpp +++ b/src/libcamera/v4l2_controls.cpp @@ -69,13 +69,10 @@ namespace libcamera { * \param ctrl The struct v4l2_query_ext_ctrl as returned by the kernel */ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl) + : DataInfo(DataValue(static_cast(ctrl.minimum)), + DataValue(static_cast(ctrl.maximum))), + id_(ctrl.id) { - id_ = ctrl.id; - type_ = ctrl.type; - name_ = static_cast(ctrl.name); - size_ = ctrl.elem_size * ctrl.elems; - min_ = ctrl.minimum; - max_ = ctrl.maximum; } /** @@ -84,36 +81,6 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl) * \return The V4L2 control ID */ -/** - * \fn V4L2ControlInfo::type() - * \brief Retrieve the control type as defined by V4L2_CTRL_TYPE_* - * \return The V4L2 control type - */ - -/** - * \fn V4L2ControlInfo::size() - * \brief Retrieve the control value data size (in bytes) - * \return The V4L2 control value data size - */ - -/** - * \fn V4L2ControlInfo::name() - * \brief Retrieve the control user readable name - * \return The V4L2 control user readable name - */ - -/** - * \fn V4L2ControlInfo::min() - * \brief Retrieve the control minimum value - * \return The V4L2 control minimum value - */ - -/** - * \fn V4L2ControlInfo::max() - * \brief Retrieve the control maximum value - * \return The V4L2 control maximum value - */ - /** * \typedef V4L2ControlInfoMap * \brief A map of control ID to V4L2ControlInfo @@ -134,6 +101,10 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl) * to be directly used but are instead intended to be grouped in * V4L2ControlList instances, which are then passed as parameters to * V4L2Device::setControls() and V4L2Device::getControls() operations. + * + * \todo Currently all V4L2Controls are integers. For the sake of keeping the + * implementation as simpler as possible treat all values as int64. The value() + * and setValue() operations use that single data type for now. */ /** @@ -143,6 +114,12 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl) * \param value The control value */ +/** + * \fn V4L2Control::type() + * \brief Retrieve the type of the control + * \return The control type + */ + /** * \fn V4L2Control::value() * \brief Retrieve the value of the control diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp index 349bf2d29704..2b7e3b1993ca 100644 --- a/src/libcamera/v4l2_device.cpp +++ b/src/libcamera/v4l2_device.cpp @@ -210,7 +210,7 @@ int V4L2Device::getControls(V4L2ControlList *ctrls) ret = errorIdx; } - updateControls(ctrls, controlInfo, v4l2Ctrls, count); + updateControls(ctrls, v4l2Ctrls, count); return ret; } @@ -262,8 +262,8 @@ int V4L2Device::setControls(V4L2ControlList *ctrls) v4l2Ctrls[i].id = info->id(); /* Set the v4l2_ext_control value for the write operation. */ - switch (info->type()) { - case V4L2_CTRL_TYPE_INTEGER64: + switch (ctrl->type()) { + case DataTypeInteger64: v4l2Ctrls[i].value64 = ctrl->value(); break; default: @@ -299,7 +299,7 @@ int V4L2Device::setControls(V4L2ControlList *ctrls) ret = errorIdx; } - updateControls(ctrls, controlInfo, v4l2Ctrls, count); + updateControls(ctrls, v4l2Ctrls, count); return ret; } @@ -352,8 +352,7 @@ void V4L2Device::listControls() ctrl.flags & V4L2_CTRL_FLAG_DISABLED) continue; - V4L2ControlInfo info(ctrl); - switch (info.type()) { + switch (ctrl.type) { case V4L2_CTRL_TYPE_INTEGER: case V4L2_CTRL_TYPE_BOOLEAN: case V4L2_CTRL_TYPE_MENU: @@ -364,11 +363,12 @@ void V4L2Device::listControls() break; /* \todo Support compound controls. */ default: - LOG(V4L2, Debug) << "Control type '" << info.type() + LOG(V4L2, Debug) << "Control type '" << ctrl.type << "' not supported"; continue; } + V4L2ControlInfo info(ctrl); controls_.emplace(ctrl.id, info); } } @@ -382,25 +382,22 @@ void V4L2Device::listControls() * \param[in] count The number of controls to update */ void V4L2Device::updateControls(V4L2ControlList *ctrls, - const V4L2ControlInfo **controlInfo, const struct v4l2_ext_control *v4l2Ctrls, unsigned int count) { for (unsigned int i = 0; i < count; ++i) { - const struct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i]; - const V4L2ControlInfo *info = controlInfo[i]; V4L2Control *ctrl = ctrls->getByIndex(i); - switch (info->type()) { - case V4L2_CTRL_TYPE_INTEGER64: - ctrl->setValue(v4l2Ctrl->value64); + switch (ctrl->type()) { + case DataTypeInteger64: + ctrl->setValue(v4l2Ctrls[i].value64); break; default: /* * \todo To be changed when support for string and * compound controls will be added. */ - ctrl->setValue(v4l2Ctrl->value); + ctrl->setValue(v4l2Ctrls[i].value); break; } } From patchwork Wed Sep 18 10:31:32 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jacopo Mondi X-Patchwork-Id: 1982 Return-Path: Received: from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net [217.70.183.201]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id CC7E460C1A; Wed, 18 Sep 2019 12:30:03 +0200 (CEST) X-Originating-IP: 2.224.242.101 Received: from uno.lan (2-224-242-101.ip172.fastwebnet.it [2.224.242.101]) (Authenticated sender: jacopo@jmondi.org) by relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 4DF691BF208; Wed, 18 Sep 2019 10:30:02 +0000 (UTC) From: Jacopo Mondi To: libcamera-devel@lists.libcamera.org, To:libcamera-devel@lists.libcamera.org Date: Wed, 18 Sep 2019 12:31:32 +0200 Message-Id: <20190918103133.14296-5-jacopo@jmondi.org> X-Mailer: git-send-email 2.23.0 In-Reply-To: <20190918103133.14296-1-jacopo@jmondi.org> References: <20190918103133.14296-1-jacopo@jmondi.org> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH 4/5] libcamera: controls: De-couple Controls from Camera X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 18 Sep 2019 10:30:04 -0000 ControlList requires a Camera class instance at construction time, preventing it from being re-constructed from serialized binary data. De-couple ControlList from Camera by internally storing a reference to the Camera's ControlInfoList. Signed-off-by: Jacopo Mondi --- include/libcamera/controls.h | 4 +-- src/libcamera/controls.cpp | 54 +++++++++++++++++----------------- src/libcamera/request.cpp | 4 +-- test/controls/control_list.cpp | 4 +-- 4 files changed, 33 insertions(+), 33 deletions(-) diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h index e46cd8a78679..d3065c0f3f16 100644 --- a/include/libcamera/controls.h +++ b/include/libcamera/controls.h @@ -48,7 +48,7 @@ private: using ControlListMap = std::unordered_map; public: - ControlList(Camera *camera); + ControlList(const ControlInfoMap &infoMap); using iterator = ControlListMap::iterator; using const_iterator = ControlListMap::const_iterator; @@ -70,7 +70,7 @@ public: void update(const ControlList &list); private: - Camera *camera_; + const ControlInfoMap &infoMap_; ControlListMap controls_; }; diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp index 2d8adde5688e..184d544c05bc 100644 --- a/src/libcamera/controls.cpp +++ b/src/libcamera/controls.cpp @@ -175,10 +175,10 @@ std::string ControlInfo::toString() const /** * \brief Construct a ControlList with a reference to the Camera it applies on - * \param[in] camera The camera + * \param[in] infoMap The ControlInfoMap of the camera the control list refers to */ -ControlList::ControlList(Camera *camera) - : camera_(camera) +ControlList::ControlList(const ControlInfoMap &infoMap) + : infoMap_(infoMap) { } @@ -229,17 +229,14 @@ ControlList::ControlList(Camera *camera) */ bool ControlList::contains(ControlId id) const { - const ControlInfoMap &controls = camera_->controls(); - const auto iter = controls.find(id); - if (iter == controls.end()) { - LOG(Controls, Error) - << "Camera " << camera_->name() - << " does not support control " << id; + const auto info = infoMap_.find(id); + if (info == infoMap_.end()) { + LOG(Controls, Error) << "Control " << id << " not supported"; return false; } - return controls_.find(&iter->second) != controls_.end(); + return controls_.find(&info->second) != controls_.end(); } /** @@ -283,18 +280,15 @@ bool ControlList::contains(const ControlInfo *info) const */ DataValue &ControlList::operator[](ControlId id) { - const ControlInfoMap &controls = camera_->controls(); - const auto iter = controls.find(id); - if (iter == controls.end()) { - LOG(Controls, Error) - << "Camera " << camera_->name() - << " does not support control " << id; + const auto info = infoMap_.find(id); + if (info == infoMap_.end()) { + LOG(Controls, Error) << "Control " << id << " not supported"; static DataValue empty; return empty; } - return controls_[&iter->second]; + return controls_[&info->second]; } /** @@ -322,18 +316,24 @@ DataValue &ControlList::operator[](ControlId id) */ void ControlList::update(const ControlList &other) { - if (other.camera_ != camera_) { - LOG(Controls, Error) - << "Can't update ControlList from a different camera"; - return; + /* + * Make sure if all controls in other's list are supported by this + * Camera. This is guaranteed to be true if the two lists refer to + * the same Camera. + */ + for (auto &control : other) { + ControlId id = control.first->id(); + + if (infoMap_.find(id) == infoMap_.end()) { + LOG(Controls, Error) + << "Failed to update control list with control: " + << id; + return; + } } - for (auto it : other) { - const ControlInfo *info = it.first; - const DataValue &value = it.second; - - controls_[info] = value; - } + for (auto &control : other) + controls_[control.first] = control.second; } } /* namespace libcamera */ diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp index ee2158fc7a9c..2b3e1870094e 100644 --- a/src/libcamera/request.cpp +++ b/src/libcamera/request.cpp @@ -55,8 +55,8 @@ LOG_DEFINE_CATEGORY(Request) * */ Request::Request(Camera *camera, uint64_t cookie) - : camera_(camera), controls_(camera), cookie_(cookie), - status_(RequestPending), cancelled_(false) + : camera_(camera), controls_(camera->controls()), + cookie_(cookie), status_(RequestPending), cancelled_(false) { } diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp index f1d79ff8fcfd..3c6eb40c091b 100644 --- a/test/controls/control_list.cpp +++ b/test/controls/control_list.cpp @@ -39,7 +39,7 @@ protected: int run() { - ControlList list(camera_.get()); + ControlList list(camera_->controls()); /* Test that the list is initially empty. */ if (!list.empty()) { @@ -155,7 +155,7 @@ protected: * the old list. Verify that the new list is empty and that the * new list contains the expected items and values. */ - ControlList newList(camera_.get()); + ControlList newList(camera_->controls()); newList[Brightness] = 128; newList[Saturation] = 255; From patchwork Wed Sep 18 10:31:33 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jacopo Mondi X-Patchwork-Id: 1983 Return-Path: Received: from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net [217.70.183.201]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 730DC60DED; Wed, 18 Sep 2019 12:30:04 +0200 (CEST) X-Originating-IP: 2.224.242.101 Received: from uno.lan (2-224-242-101.ip172.fastwebnet.it [2.224.242.101]) (Authenticated sender: jacopo@jmondi.org) by relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 01DD61BF208; Wed, 18 Sep 2019 10:30:03 +0000 (UTC) From: Jacopo Mondi To: libcamera-devel@lists.libcamera.org, To:libcamera-devel@lists.libcamera.org Date: Wed, 18 Sep 2019 12:31:33 +0200 Message-Id: <20190918103133.14296-6-jacopo@jmondi.org> X-Mailer: git-send-email 2.23.0 In-Reply-To: <20190918103133.14296-1-jacopo@jmondi.org> References: <20190918103133.14296-1-jacopo@jmondi.org> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH 5/5] libcamera: controls: Control framework refresh X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 18 Sep 2019 10:30:04 -0000 Now that the control data value and info storage have been unified with V4L2 control a refresh of naming and minor bits had to be performed. The control framework implementation uses the word 'control' in types and definition to refer to different things, making it harder to follow. Perform a rename of elements defined by the control framework to enforce the following concepts: - control metadata: static control metadata information such as id, type and name, defined by libcamera - control information: constant information associated with a control used for validation of control values, provided by pipeline handlers and defined by the capabilities of the device the control applies to - control: a control identifier with an associated value, provided by applications when setting a control to a specific value Update the framework documentation trying to use those concepts consistently. Signed-off-by: Jacopo Mondi --- include/libcamera/controls.h | 19 ++-- src/libcamera/controls.cpp | 129 +++++++++++++++++----------- src/libcamera/gen-controls.awk | 2 +- src/libcamera/meson.build | 6 +- src/libcamera/pipeline/uvcvideo.cpp | 2 +- src/libcamera/pipeline/vimc.cpp | 2 +- test/controls/control_list.cpp | 2 +- 7 files changed, 97 insertions(+), 65 deletions(-) diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h index d3065c0f3f16..174bc92732aa 100644 --- a/include/libcamera/controls.h +++ b/include/libcamera/controls.h @@ -19,7 +19,7 @@ namespace libcamera { class Camera; -struct ControlIdentifier { +struct ControlMetadata { ControlId id; const char *name; DataType type; @@ -37,21 +37,22 @@ public: std::string toString() const; private: - const struct ControlIdentifier *ident_; + const struct ControlMetadata *ident_; }; using ControlInfoMap = std::unordered_map; +using Control = DataValue; class ControlList { private: - using ControlListMap = std::unordered_map; + using ControlMap = std::unordered_map; public: ControlList(const ControlInfoMap &infoMap); - using iterator = ControlListMap::iterator; - using const_iterator = ControlListMap::const_iterator; + using iterator = ControlMap::iterator; + using const_iterator = ControlMap::const_iterator; iterator begin() { return controls_.begin(); } iterator end() { return controls_.end(); } @@ -64,14 +65,14 @@ public: std::size_t size() const { return controls_.size(); } void clear() { controls_.clear(); } - DataValue &operator[](ControlId id); - DataValue &operator[](const ControlInfo *info) { return controls_[info]; } + Control &operator[](ControlId id); + Control &operator[](const ControlInfo *info) { return controls_[info]; } - void update(const ControlList &list); + bool merge(const ControlList &list); private: const ControlInfoMap &infoMap_; - ControlListMap controls_; + ControlMap controls_; }; } /* namespace libcamera */ diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp index 184d544c05bc..028ffc1e80b9 100644 --- a/src/libcamera/controls.cpp +++ b/src/libcamera/controls.cpp @@ -7,9 +7,6 @@ #include -#include -#include - #include #include "log.h" @@ -17,7 +14,7 @@ /** * \file controls.h - * \brief Describes control framework and controls supported by a camera + * \brief Control framework implementation */ namespace libcamera { @@ -72,38 +69,50 @@ LOG_DEFINE_CATEGORY(Controls) */ /** - * \struct ControlIdentifier - * \brief Describe a ControlId with control specific constant meta-data + * \struct ControlMetadata + * \brief Describe the control static meta-data + * + * Defines the Control static meta-data information, auto-generated from the + * ControlId documentation. + * + * The control information are defined in the control_id.h file, parsed by + * the gen-controls.awk script to produce a control_metadata.cpp file that + * provides a static list of control id with an associated type and name. The + * file is not for public use, and so no suitable header exists as + * its sole usage is for the ControlMetadata definition. * - * Defines a Control with a unique ID, a name, and a type. - * This structure is used as static part of the auto-generated control - * definitions, which are generated from the ControlId documentation. + * \todo Expand the control meta-data definition to support more complex + * control types and describe their characteristics (ie. Number of deta elements + * for compound control types, versioning etc). * - * \var ControlIdentifier::id + * \var ControlMetadata::id * The unique ID for a control - * \var ControlIdentifier::name + * \var ControlMetadata::name * The string representation of the control - * \var ControlIdentifier::type + * \var ControlMetadata::type * The ValueType required to represent the control value */ /* - * The controlTypes are automatically generated to produce a control_types.cpp - * output. This file is not for public use, and so no suitable header exists - * for this sole usage of the controlTypes reference. As such the extern is - * only defined here for use during the ControlInfo constructor and should not + * The ControlTypes map is defined by the generated control_metadata.cpp file + * and only used here during the ControlInfo construction and should not * be referenced directly elsewhere. */ -extern const std::unordered_map controlTypes; +extern const std::unordered_map controlTypes; /** * \class ControlInfo * \brief Describe the information and capabilities of a Control * - * The ControlInfo represents control specific meta-data which is constant on a - * per camera basis. ControlInfo classes are constructed by pipeline handlers - * to expose the controls they support and the metadata needed to utilise those - * controls. + * The ControlInfo class associates the control's constant metadata defined by + * libcamera and collected in the ControlMetadata class, with its static + * information, such the range of supported values and the control size, + * described by the DataInfo base class and defined by the capabilities of + * the Camera device that implements the control. + * + * ControlInfo instances are constructed by pipeline handlers and associated + * with the Camera devices they register to expose the list of controls they + * support and the static information required to use those controls. */ /** @@ -118,7 +127,7 @@ ControlInfo::ControlInfo(ControlId id, const DataValue &min, { auto iter = controlTypes.find(id); if (iter == controlTypes.end()) { - LOG(Controls, Fatal) << "Attempt to create invalid ControlInfo"; + LOG(Controls, Fatal) << "Control " << id << " not defined"; return; } @@ -127,19 +136,19 @@ ControlInfo::ControlInfo(ControlId id, const DataValue &min, /** * \fn ControlInfo::id() - * \brief Retrieve the control ID + * \brief Retrieve the control ID from the control constant metadata * \return The control ID */ /** * \fn ControlInfo::name() - * \brief Retrieve the control name string + * \brief Retrieve the control name string from the control constant metadata * \return The control name string */ /** * \fn ControlInfo::type() - * \brief Retrieve the control designated type + * \brief Retrieve the control designated type from the control constant metadata * \return The control type */ @@ -161,21 +170,37 @@ std::string ControlInfo::toString() const * \brief A map of ControlId to ControlInfo */ +/** + * \typedef Control + * \brief Use 'Control' in place of DataValue in the ControlList class + * + * \todo If more data and operations than the ones defined by DataValue + * will need to be implemented for controls, make this typedef a class that + * inherits from DataValue. + */ + /** * \class ControlList - * \brief Associate a list of ControlId with their values for a camera + * \brief List of controls values * - * A ControlList wraps a map of ControlId to DataValue and provide - * additional validation against the control information exposed by a Camera. + * A ControlList wraps a map of ControlId to Control instances and provide + * additional validation against the control information reported by a Camera. * - * A list is only valid for as long as the camera it refers to is valid. After - * that calling any method of the ControlList class other than its destructor - * will cause undefined behaviour. + * ControlList instances are initially created empty and should be populated by + * assigning a value to each control added to the list. The added control is + * validated to verify it is supported by the Camera the list refers to, and + * validate the provided value against the static information provided by the + * ControlInfo instance associated with the control. */ /** - * \brief Construct a ControlList with a reference to the Camera it applies on + * \brief Construct a ControlList with a map of control information * \param[in] infoMap The ControlInfoMap of the camera the control list refers to + * + * The provided \a infoMap collects the control id and the associated static + * information for each control that can be set on the list. \a infoMap is + * provided by the Camera the list of controls refers to and it is used to make + * sure only controls supported by the camera can be added to the list. */ ControlList::ControlList(const ControlInfoMap &infoMap) : infoMap_(infoMap) @@ -221,10 +246,6 @@ ControlList::ControlList(const ControlInfoMap &infoMap) /** * \brief Check if the list contains a control with the specified \a id * \param[in] id The control ID - * - * The behaviour is undefined if the control \a id is not supported by the - * camera that the ControlList refers to. - * * \return True if the list contains a matching control, false otherwise */ bool ControlList::contains(ControlId id) const @@ -258,7 +279,7 @@ bool ControlList::contains(const ControlInfo *info) const /** * \fn ControlList::size() * \brief Retrieve the number of controls in the list - * \return The number of DataValue entries stored in the list + * \return The number of Control entries stored in the list */ /** @@ -276,15 +297,16 @@ bool ControlList::contains(const ControlInfo *info) const * The behaviour is undefined if the control \a id is not supported by the * camera that the ControlList refers to. * - * \return A reference to the value of the control identified by \a id + * \return A reference to the value of the control identified by \a id if the + * control is supported, an empty control otherwise */ -DataValue &ControlList::operator[](ControlId id) +Control &ControlList::operator[](ControlId id) { const auto info = infoMap_.find(id); if (info == infoMap_.end()) { LOG(Controls, Error) << "Control " << id << " not supported"; - static DataValue empty; + static Control empty; return empty; } @@ -299,22 +321,29 @@ DataValue &ControlList::operator[](ControlId id) * This method returns a reference to the control identified by \a info, * inserting it in the list if the info is not already present. * - * \return A reference to the value of the control identified by \a info + * The behaviour is undefined if the control \a info is not supported by the + * camera that the ControlList refers to. + * + * \return A reference to the value of the control identified by \a info if the + * control is supported, an mpty control otherwise */ /** - * \brief Update the list with a union of itself and \a other - * \param other The other list + * \brief Merge the control in the list with controls from \a other + * \param other The control list to merge with * * Update the control list to include all values from the \a other list. - * Elements in the list whose control IDs are contained in \a other are updated + * Elements in the list whose control IDs are contained in \a other are merged * with the value from \a other. Elements in the \a other list that have no * corresponding element in the list are added to the list with their value. * - * The behaviour is undefined if the two lists refer to different Camera - * instances. + * All controls in \a other should be supported by the list, otherwise no + * control is updated. + * + * \return True if control list have been merged, false otherwise + * */ -void ControlList::update(const ControlList &other) +bool ControlList::merge(const ControlList &other) { /* * Make sure if all controls in other's list are supported by this @@ -326,14 +355,16 @@ void ControlList::update(const ControlList &other) if (infoMap_.find(id) == infoMap_.end()) { LOG(Controls, Error) - << "Failed to update control list with control: " + << "Failed to merge control list with control: " << id; - return; + return false; } } for (auto &control : other) controls_[control.first] = control.second; + + return true; } } /* namespace libcamera */ diff --git a/src/libcamera/gen-controls.awk b/src/libcamera/gen-controls.awk index abeb0218546b..b7f6532dfd43 100755 --- a/src/libcamera/gen-controls.awk +++ b/src/libcamera/gen-controls.awk @@ -89,7 +89,7 @@ function GenerateTable(file) { EnterNameSpace(file) - print "extern const std::unordered_map" > file + print "extern const std::unordered_map" > file print "controlTypes {" > file for (i=1; i <= id; ++i) { printf "\t{ %s, { %s, \"%s\", DataType%s } },\n", names[i], names[i], names[i], types[i] > file diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build index c3100a1709e0..c4fcd0569bd7 100644 --- a/src/libcamera/meson.build +++ b/src/libcamera/meson.build @@ -60,13 +60,13 @@ endif gen_controls = files('gen-controls.awk') -control_types_cpp = custom_target('control_types_cpp', +control_metadata_cpp = custom_target('control_metadata_cpp', input : files('controls.cpp'), - output : 'control_types.cpp', + output : 'control_metadata.cpp', depend_files : gen_controls, command : [gen_controls, '@INPUT@', '--code', '@OUTPUT@']) -libcamera_sources += control_types_cpp +libcamera_sources += control_metadata_cpp gen_version = join_paths(meson.source_root(), 'utils', 'gen-version.sh') diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp index dd253f94ca3d..d7f7fb0d6322 100644 --- a/src/libcamera/pipeline/uvcvideo.cpp +++ b/src/libcamera/pipeline/uvcvideo.cpp @@ -231,7 +231,7 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request) for (auto it : request->controls()) { const ControlInfo *ci = it.first; - DataValue &value = it.second; + Control &value = it.second; switch (ci->id()) { case Brightness: diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp index 3df239bdb277..1d36ec54bc3b 100644 --- a/src/libcamera/pipeline/vimc.cpp +++ b/src/libcamera/pipeline/vimc.cpp @@ -284,7 +284,7 @@ int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request) for (auto it : request->controls()) { const ControlInfo *ci = it.first; - DataValue &value = it.second; + Control &value = it.second; switch (ci->id()) { case Brightness: diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp index 3c6eb40c091b..2a90cffe5106 100644 --- a/test/controls/control_list.cpp +++ b/test/controls/control_list.cpp @@ -159,7 +159,7 @@ protected: newList[Brightness] = 128; newList[Saturation] = 255; - newList.update(list); + newList.merge(list); list.clear();