[libcamera-devel] [PATCH v2 02/13] libcamera: controls: Make ControlValue get/set accessors template methods
Niklas Söderlund
niklas.soderlund at ragnatech.se
Thu Oct 3 21:00:50 CEST 2019
Hi Laurent,
Thanks for your work.
On 2019-09-29 22:02:43 +0300, Laurent Pinchart wrote:
> The ControlValue get accessors are implemented with functions of
> different names, whlie the set accessors use polymorphism to support
> different control types. This isn't very consistent and intuitive. Make
> the API clearer by using template methods. This will also have the added
> advantage that support for the new types will only require adding
> template specialisations, without adding new methods.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> include/libcamera/controls.h | 11 ++-
> src/libcamera/controls.cpp | 101 +++++++++++++---------------
> src/libcamera/pipeline/uvcvideo.cpp | 10 +--
> src/libcamera/pipeline/vimc.cpp | 6 +-
> test/controls/control_info.cpp | 4 +-
> test/controls/control_list.cpp | 16 ++---
> test/controls/control_value.cpp | 12 ++--
> 7 files changed, 74 insertions(+), 86 deletions(-)
>
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index ffba880a66ff..0886370f0901 100644
> --- a/include/libcamera/controls.h
> +++ b/include/libcamera/controls.h
> @@ -36,13 +36,10 @@ public:
> ControlType type() const { return type_; };
> bool isNone() const { return type_ == ControlTypeNone; };
>
> - void set(bool value);
> - void set(int value);
> - void set(int64_t value);
> -
> - bool getBool() const;
> - int getInt() const;
> - int64_t getInt64() const;
> + template<typename T>
> + const T &get() const;
> + template<typename T>
> + void set(const T &value);
>
> std::string toString() const;
>
> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> index 9960a30dfa03..88aab88db327 100644
> --- a/src/libcamera/controls.cpp
> +++ b/src/libcamera/controls.cpp
> @@ -90,76 +90,67 @@ ControlValue::ControlValue(int64_t value)
> */
>
> /**
> - * \brief Set the value with a boolean
> - * \param[in] value Boolean value to store
> + * \fn template<typename T> const T &ControlValue::get() const
> + * \brief Get the control value
> + *
> + * The control value type shall match the type T, otherwise the behaviour is
> + * undefined.
> + *
> + * \return The control value
> */
> -void ControlValue::set(bool value)
> +
> +/**
> + * \fn template<typename T> void ControlValue::set(const T &value)
> + * \brief Set the control value to \a value
> + * \param[in] value The control value
> + */
> +
> +#ifndef __DOXYGEN__
> +template<>
> +const bool &ControlValue::get<bool>() const
> +{
> + ASSERT(type_ == ControlTypeBool);
> +
> + return bool_;
> +}
> +
> +template<>
> +const int &ControlValue::get<int>() const
> +{
> + ASSERT(type_ == ControlTypeInteger || type_ == ControlTypeInteger64);
> +
> + return integer_;
> +}
> +
> +template<>
> +const int64_t &ControlValue::get<int64_t>() const
> +{
> + ASSERT(type_ == ControlTypeInteger || type_ == ControlTypeInteger64);
> +
> + return integer64_;
> +}
> +
> +template<>
> +void ControlValue::set<bool>(const bool &value)
> {
> type_ = ControlTypeBool;
> bool_ = value;
> }
>
> -/**
> - * \brief Set the value with an integer
> - * \param[in] value Integer value to store
> - */
> -void ControlValue::set(int value)
> +template<>
> +void ControlValue::set<int>(const int &value)
> {
> type_ = ControlTypeInteger;
> 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)
> +template<>
> +void ControlValue::set<int64_t>(const int64_t &value)
> {
> type_ = ControlTypeInteger64;
> integer64_ = value;
> }
> -
> -/**
> - * \brief Get the boolean value
> - *
> - * The value type must be Boolean.
> - *
> - * \return The boolean value
> - */
> -bool ControlValue::getBool() const
> -{
> - ASSERT(type_ == ControlTypeBool);
> -
> - 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_ == ControlTypeInteger || type_ == ControlTypeInteger64);
> -
> - 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_ == ControlTypeInteger || type_ == ControlTypeInteger64);
> -
> - return integer64_;
> -}
> +#endif /* __DOXYGEN__ */
>
> /**
> * \brief Assemble and return a string describing the value
> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> index 8965210550d2..81c548af2c64 100644
> --- a/src/libcamera/pipeline/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo.cpp
> @@ -235,24 +235,24 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)
>
> switch (ci->id()) {
> case Brightness:
> - controls.add(V4L2_CID_BRIGHTNESS, value.getInt());
> + controls.add(V4L2_CID_BRIGHTNESS, value.get<int>());
> break;
>
> case Contrast:
> - controls.add(V4L2_CID_CONTRAST, value.getInt());
> + controls.add(V4L2_CID_CONTRAST, value.get<int>());
> break;
>
> case Saturation:
> - controls.add(V4L2_CID_SATURATION, value.getInt());
> + controls.add(V4L2_CID_SATURATION, value.get<int>());
> break;
>
> case ManualExposure:
> controls.add(V4L2_CID_EXPOSURE_AUTO, 1);
> - controls.add(V4L2_CID_EXPOSURE_ABSOLUTE, value.getInt());
> + controls.add(V4L2_CID_EXPOSURE_ABSOLUTE, value.get<int>());
> break;
>
> case ManualGain:
> - controls.add(V4L2_CID_GAIN, value.getInt());
> + controls.add(V4L2_CID_GAIN, value.get<int>());
> break;
>
> default:
> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> index f26a91f86ec1..3e34e7a0edbf 100644
> --- a/src/libcamera/pipeline/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc.cpp
> @@ -288,15 +288,15 @@ int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request)
>
> switch (ci->id()) {
> case Brightness:
> - controls.add(V4L2_CID_BRIGHTNESS, value.getInt());
> + controls.add(V4L2_CID_BRIGHTNESS, value.get<int>());
> break;
>
> case Contrast:
> - controls.add(V4L2_CID_CONTRAST, value.getInt());
> + controls.add(V4L2_CID_CONTRAST, value.get<int>());
> break;
>
> case Saturation:
> - controls.add(V4L2_CID_SATURATION, value.getInt());
> + controls.add(V4L2_CID_SATURATION, value.get<int>());
> break;
>
> default:
> diff --git a/test/controls/control_info.cpp b/test/controls/control_info.cpp
> index 8cda860b9fe9..faefaaa444d9 100644
> --- a/test/controls/control_info.cpp
> +++ b/test/controls/control_info.cpp
> @@ -32,7 +32,7 @@ protected:
> return TestFail;
> }
>
> - if (info.min().getInt() != 0 || info.max().getInt() != 0) {
> + if (info.min().get<int>() != 0 || info.max().get<int>() != 0) {
> cout << "Invalid control range for Brightness" << endl;
> return TestFail;
> }
> @@ -50,7 +50,7 @@ protected:
> return TestFail;
> }
>
> - if (info.min().getInt() != 10 || info.max().getInt() != 200) {
> + if (info.min().get<int>() != 10 || info.max().get<int>() != 200) {
> cout << "Invalid control range for Contrast" << endl;
> return TestFail;
> }
> diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp
> index f1d79ff8fcfd..0402e7c23dba 100644
> --- a/test/controls/control_list.cpp
> +++ b/test/controls/control_list.cpp
> @@ -96,7 +96,7 @@ protected:
> return TestFail;
> }
>
> - if (list[Brightness].getInt() != 255) {
> + if (list[Brightness].get<int>() != 255) {
> cout << "Incorrest Brightness control value" << endl;
> return TestFail;
> }
> @@ -125,8 +125,8 @@ protected:
> /*
> * Test control value retrieval and update through ControlInfo.
> */
> - if (list[brightness].getInt() != 64 ||
> - list[contrast].getInt() != 128) {
> + if (list[brightness].get<int>() != 64 ||
> + list[contrast].get<int>() != 128) {
> cout << "Failed to retrieve control value" << endl;
> return TestFail;
> }
> @@ -134,8 +134,8 @@ protected:
> list[brightness] = 10;
> list[contrast] = 20;
>
> - if (list[brightness].getInt() != 10 ||
> - list[contrast].getInt() != 20) {
> + if (list[brightness].get<int>() != 10 ||
> + list[contrast].get<int>() != 20) {
> cout << "Failed to update control value" << endl;
> return TestFail;
> }
> @@ -185,9 +185,9 @@ protected:
> return TestFail;
> }
>
> - if (newList[Brightness].getInt() != 10 ||
> - newList[Contrast].getInt() != 20 ||
> - newList[Saturation].getInt() != 255) {
> + if (newList[Brightness].get<int>() != 10 ||
> + newList[Contrast].get<int>() != 20 ||
> + newList[Saturation].get<int>() != 255) {
> cout << "New list contains incorrect values" << endl;
> return TestFail;
> }
> diff --git a/test/controls/control_value.cpp b/test/controls/control_value.cpp
> index 778efe5c115f..397c43f799ad 100644
> --- a/test/controls/control_value.cpp
> +++ b/test/controls/control_value.cpp
> @@ -27,12 +27,12 @@ protected:
> << " Bool: " << boolean.toString()
> << endl;
>
> - if (integer.getInt() != 1234) {
> + if (integer.get<int>() != 1234) {
> cerr << "Failed to get Integer" << endl;
> return TestFail;
> }
>
> - if (boolean.getBool() != true) {
> + if (boolean.get<bool>() != true) {
> cerr << "Failed to get Boolean" << endl;
> return TestFail;
> }
> @@ -45,19 +45,19 @@ protected:
> return TestFail;
> }
>
> - value.set(true);
> + value.set<bool>(true);
> if (value.isNone()) {
> cerr << "Failed to set an empty object" << endl;
> return TestFail;
> }
>
> - if (value.getBool() != true) {
> + if (value.get<bool>() != true) {
> cerr << "Failed to get Booleans" << endl;
> return TestFail;
> }
>
> - value.set(10);
> - if (value.getInt() != 10) {
> + value.set<int>(10);
> + if (value.get<int>() != 10) {
> cerr << "Failed to get Integer" << endl;
> return TestFail;
> }
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list