[libcamera-devel] [PATCH 03/12] libcamera: controls: Use explicit 32-bit integer types
Jacopo Mondi
jacopo at jmondi.org
Sun Sep 29 11:08:48 CEST 2019
Hi Laurent,
On Sat, Sep 28, 2019 at 06:22:29PM +0300, Laurent Pinchart wrote:
> Make the control API more explicit when dealing with integer controls by
> specifying the size. We already do so for 64-bit integers, using int64_t
> and ControlTypeInteger64, do the same for 32-bit integers.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> include/libcamera/controls.h | 6 ++---
> src/libcamera/controls.cpp | 36 ++++++++++++++---------------
> src/libcamera/pipeline/uvcvideo.cpp | 10 ++++----
> src/libcamera/pipeline/vimc.cpp | 6 ++---
> test/controls/control_info.cpp | 10 ++++----
> test/controls/control_list.cpp | 16 ++++++-------
> test/controls/control_value.cpp | 6 ++---
> 7 files changed, 46 insertions(+), 44 deletions(-)
>
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index 0886370f0901..a3089064c4b5 100644
> --- a/include/libcamera/controls.h
> +++ b/include/libcamera/controls.h
> @@ -21,7 +21,7 @@ class Camera;
> enum ControlType {
> ControlTypeNone,
> ControlTypeBool,
> - ControlTypeInteger,
> + ControlTypeInteger32,
> ControlTypeInteger64,
> };
>
> @@ -30,7 +30,7 @@ class ControlValue
> public:
> ControlValue();
> ControlValue(bool value);
> - ControlValue(int value);
> + ControlValue(int32_t value);
> ControlValue(int64_t value);
>
> ControlType type() const { return type_; };
> @@ -48,7 +48,7 @@ private:
>
> union {
> bool bool_;
> - int integer_;
> + int32_t integer32_;
> int64_t integer64_;
> };
> };
> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> index 88aab88db327..295ccd55a622 100644
> --- a/src/libcamera/controls.cpp
> +++ b/src/libcamera/controls.cpp
> @@ -31,8 +31,8 @@ LOG_DEFINE_CATEGORY(Controls)
> * Invalid type, for empty values
> * \var ControlTypeBool
> * The control stores a boolean value
> - * \var ControlTypeInteger
> - * The control stores an integer value
> + * \var ControlTypeInteger32
> + * The control stores a 32-bit integer value
> * \var ControlTypeInteger64
> * The control stores a 64-bit integer value
> */
> @@ -63,8 +63,8 @@ ControlValue::ControlValue(bool value)
> * \brief Construct an integer ControlValue
> * \param[in] value Integer value to store
> */
> -ControlValue::ControlValue(int value)
> - : type_(ControlTypeInteger), integer_(value)
> +ControlValue::ControlValue(int32_t value)
> + : type_(ControlTypeInteger32), integer32_(value)
> {
> }
>
> @@ -115,17 +115,17 @@ const bool &ControlValue::get<bool>() const
> }
>
> template<>
> -const int &ControlValue::get<int>() const
> +const int32_t &ControlValue::get<int32_t>() const
> {
> - ASSERT(type_ == ControlTypeInteger || type_ == ControlTypeInteger64);
> + ASSERT(type_ == ControlTypeInteger32 || type_ == ControlTypeInteger64);
>
> - return integer_;
> + return integer32_;
> }
>
> template<>
> const int64_t &ControlValue::get<int64_t>() const
> {
> - ASSERT(type_ == ControlTypeInteger || type_ == ControlTypeInteger64);
> + ASSERT(type_ == ControlTypeInteger32 || type_ == ControlTypeInteger64);
>
> return integer64_;
> }
> @@ -138,10 +138,10 @@ void ControlValue::set<bool>(const bool &value)
> }
>
> template<>
> -void ControlValue::set<int>(const int &value)
> +void ControlValue::set<int32_t>(const int32_t &value)
> {
> - type_ = ControlTypeInteger;
> - integer_ = value;
> + type_ = ControlTypeInteger32;
> + integer32_ = value;
> }
>
> template<>
> @@ -163,8 +163,8 @@ std::string ControlValue::toString() const
> return "<None>";
> case ControlTypeBool:
> return bool_ ? "True" : "False";
> - case ControlTypeInteger:
> - return std::to_string(integer_);
> + case ControlTypeInteger32:
> + return std::to_string(integer32_);
> case ControlTypeInteger64:
> return std::to_string(integer64_);
> }
> @@ -186,35 +186,35 @@ std::string ControlValue::toString() const
>
> /**
> * \var Brightness
> - * ControlType: Integer
> + * ControlType: Integer32
> *
> * Specify a fixed brightness parameter.
> */
>
> /**
> * \var Contrast
> - * ControlType: Integer
> + * ControlType: Integer32
> *
> * Specify a fixed contrast parameter.
> */
>
> /**
> * \var Saturation
> - * ControlType: Integer
> + * ControlType: Integer32
> *
> * Specify a fixed saturation parameter.
> */
>
> /**
> * \var ManualExposure
> - * ControlType: Integer
> + * ControlType: Integer32
> *
> * Specify a fixed exposure time in milli-seconds
> */
>
> /**
> * \var ManualGain
> - * ControlType: Integer
> + * ControlType: Integer32
> *
> * Specify a fixed gain parameter
> */
> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> index 81c548af2c64..0d56758e3e1d 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.get<int>());
> + controls.add(V4L2_CID_BRIGHTNESS, value.get<int32_t>());
Due to C++ internal type casting rules (I presume) the previous syntax
value.get<int>() is still legal. I assume the template argument
substitution process matches this with the <int32_t> specialization.
Shouldn't we keep using the shorter one ?
Anyway
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
Thanks
j
> break;
>
> case Contrast:
> - controls.add(V4L2_CID_CONTRAST, value.get<int>());
> + controls.add(V4L2_CID_CONTRAST, value.get<int32_t>());
> break;
>
> case Saturation:
> - controls.add(V4L2_CID_SATURATION, value.get<int>());
> + controls.add(V4L2_CID_SATURATION, value.get<int32_t>());
> break;
>
> case ManualExposure:
> controls.add(V4L2_CID_EXPOSURE_AUTO, 1);
> - controls.add(V4L2_CID_EXPOSURE_ABSOLUTE, value.get<int>());
> + controls.add(V4L2_CID_EXPOSURE_ABSOLUTE, value.get<int32_t>());
> break;
>
> case ManualGain:
> - controls.add(V4L2_CID_GAIN, value.get<int>());
> + controls.add(V4L2_CID_GAIN, value.get<int32_t>());
> break;
>
> default:
> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> index 3e34e7a0edbf..e549dc64b996 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.get<int>());
> + controls.add(V4L2_CID_BRIGHTNESS, value.get<int32_t>());
> break;
>
> case Contrast:
> - controls.add(V4L2_CID_CONTRAST, value.get<int>());
> + controls.add(V4L2_CID_CONTRAST, value.get<int32_t>());
> break;
>
> case Saturation:
> - controls.add(V4L2_CID_SATURATION, value.get<int>());
> + controls.add(V4L2_CID_SATURATION, value.get<int32_t>());
> break;
>
> default:
> diff --git a/test/controls/control_info.cpp b/test/controls/control_info.cpp
> index faefaaa444d9..2aba568a302e 100644
> --- a/test/controls/control_info.cpp
> +++ b/test/controls/control_info.cpp
> @@ -26,13 +26,14 @@ protected:
> ControlInfo info(Brightness);
>
> if (info.id() != Brightness ||
> - info.type() != ControlTypeInteger ||
> + info.type() != ControlTypeInteger32 ||
> info.name() != std::string("Brightness")) {
> cout << "Invalid control identification for Brightness" << endl;
> return TestFail;
> }
>
> - if (info.min().get<int>() != 0 || info.max().get<int>() != 0) {
> + if (info.min().get<int32_t>() != 0 ||
> + info.max().get<int32_t>() != 0) {
> cout << "Invalid control range for Brightness" << endl;
> return TestFail;
> }
> @@ -44,13 +45,14 @@ protected:
> info = ControlInfo(Contrast, 10, 200);
>
> if (info.id() != Contrast ||
> - info.type() != ControlTypeInteger ||
> + info.type() != ControlTypeInteger32 ||
> info.name() != std::string("Contrast")) {
> cout << "Invalid control identification for Contrast" << endl;
> return TestFail;
> }
>
> - if (info.min().get<int>() != 10 || info.max().get<int>() != 200) {
> + if (info.min().get<int32_t>() != 10 ||
> + info.max().get<int32_t>() != 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 0402e7c23dba..5215840b1c4e 100644
> --- a/test/controls/control_list.cpp
> +++ b/test/controls/control_list.cpp
> @@ -96,7 +96,7 @@ protected:
> return TestFail;
> }
>
> - if (list[Brightness].get<int>() != 255) {
> + if (list[Brightness].get<int32_t>() != 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].get<int>() != 64 ||
> - list[contrast].get<int>() != 128) {
> + if (list[brightness].get<int32_t>() != 64 ||
> + list[contrast].get<int32_t>() != 128) {
> cout << "Failed to retrieve control value" << endl;
> return TestFail;
> }
> @@ -134,8 +134,8 @@ protected:
> list[brightness] = 10;
> list[contrast] = 20;
>
> - if (list[brightness].get<int>() != 10 ||
> - list[contrast].get<int>() != 20) {
> + if (list[brightness].get<int32_t>() != 10 ||
> + list[contrast].get<int32_t>() != 20) {
> cout << "Failed to update control value" << endl;
> return TestFail;
> }
> @@ -185,9 +185,9 @@ protected:
> return TestFail;
> }
>
> - if (newList[Brightness].get<int>() != 10 ||
> - newList[Contrast].get<int>() != 20 ||
> - newList[Saturation].get<int>() != 255) {
> + if (newList[Brightness].get<int32_t>() != 10 ||
> + newList[Contrast].get<int32_t>() != 20 ||
> + newList[Saturation].get<int32_t>() != 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 397c43f799ad..a1ffa842f29e 100644
> --- a/test/controls/control_value.cpp
> +++ b/test/controls/control_value.cpp
> @@ -27,7 +27,7 @@ protected:
> << " Bool: " << boolean.toString()
> << endl;
>
> - if (integer.get<int>() != 1234) {
> + if (integer.get<int32_t>() != 1234) {
> cerr << "Failed to get Integer" << endl;
> return TestFail;
> }
> @@ -56,8 +56,8 @@ protected:
> return TestFail;
> }
>
> - value.set<int>(10);
> - if (value.get<int>() != 10) {
> + value.set<int32_t>(10);
> + if (value.get<int32_t>() != 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
-------------- 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/20190929/2f6762fb/attachment.sig>
More information about the libcamera-devel
mailing list