[libcamera-devel] [PATCH 2/5] libcamera: controls: Use DataType and DataValue
Jacopo Mondi
jacopo at jmondi.org
Mon Sep 23 13:06:02 CEST 2019
Hi Kieran,
On Fri, Sep 20, 2019 at 12:01:31PM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 18/09/2019 11:34, 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.
>
> I think my only worry in this patch is the removal of the operator==
> overrides.
They're not used anywhere in the code
>
> Everything else looks good ...
>
>
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> > 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 <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);
> > -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);
> > -}
> > -
>
> Do we need /make use of these operator overrides, and are they now
> provided by the DataInfo ?
>
> I'm sure I recall them being used for matching somewhere... perhaps
> that's handled differently now.
>
>
> > 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..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 "<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()) {
> > @@ -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
>
> Is it inaccurate to leave this as "Retrieve the control data type"?
> Isn't that what is returned?
Yes, I chose 'designated' to convey that a control has a type assigned
'by design'. It's redundant, I'll remove.
>
> > */
> >
> > /**
> > * \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;
> > -}
> > -
>
> I'm quite a bit scared seeing those operator== removals in this patch.
> Have you run all the tests on this patch?
> Do they still function (i.e. maintain bisect-ability) at this point?
of course
>
> I wonder if we even had anything testing this specific part in fact.
Probably not, but I don't see them being used anywhere in the code
base
>
>
>
> > /**
> > * \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
> > --
> > 2.23.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
> >
>
> --
> Regards
> --
> Kieran
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190923/7412c4eb/attachment-0001.sig>
More information about the libcamera-devel
mailing list