[libcamera-devel] [PATCH v2 2/5] libcamera: controls: Use DataType and DataValue
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu Sep 26 17:29:32 CEST 2019
Hi Jacopo,
On 24/09/2019 18:14, Jacopo Mondi wrote:
> 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 <jacopo at jmondi.org>
> ---
> include/libcamera/controls.h | 77 ++-------
> src/libcamera/controls.cpp | 235 ++--------------------------
> 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, 25 insertions(+), 367 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 <unordered_map>
>
> #include <libcamera/control_ids.h>
> +#include <libcamera/data_value.h>
>
> 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);
I believe at least one of these was to allow the direct comparison
between a ControlInfo and a ControlId so that ControlLists can be
indexed by ControlInfo or ControlId with the operator[]s.
Clearly we did not add a test to support this yet, or the removal of
these should have broken something from within the control_list tests.
I'll let you and Laurent decide whether it should be kept.
I'm not particularly attached to this otherwise.
--
Kieran
> -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<ControlId, ControlInfo>;
>
> class ControlList
> {
> private:
> - using ControlListMap = std::unordered_map<const ControlInfo *, ControlValue>;
> + using ControlListMap = std::unordered_map<const ControlInfo *, DataValue>;
>
> 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..154d9512f660 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 "<None>";
> - case ControlValueBool:
> - return bool_ ? "True" : "False";
> - case ControlValueInteger:
> - return std::to_string(integer_);
> - case ControlValueInteger64:
> - return std::to_string(integer64_);
> - }
> -
> - return "<ValueType Error>";
> -}
> -
> /**
> * \enum ControlId
> * \brief Numerical control ID
> @@ -269,9 +112,9 @@ extern const std::unordered_map<ControlId, ControlIdentifier> 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()) {
> @@ -297,78 +140,22 @@ 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
> + * \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<ControlId, ControlIdentifier>" > 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 <iostream>
> -
> -#include <libcamera/controls.h>
> -
> -#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
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list