[libcamera-devel] [PATCH 03/12] libcamera: controls: Use explicit 32-bit integer types
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun Sep 29 13:39:54 CEST 2019
Hi Jacopo,
On Sun, Sep 29, 2019 at 11:08:48AM +0200, Jacopo Mondi wrote:
> 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 ?
It's still legal, and will still compile, the same way that get<long>
will match get<int64_t> on 64-bit platforms. However, get<long long>
won't, as long long and long are not the same type, even if they end up
having the same size and behaving exactly the same. On 32-bit platforms,
it's the other way around, get<long long> will match get<int64_t>, but
get<long> won't match get<int32_t>.
The reason that pushed me to go for int32_t is to push authors to
consider the data type when writing code, instead of blindly relying on
"an integer being good enough". Down the line I think it would be useful
to replace V4L2_CID_* with Control instances to get the same type safety
for V4L2 controls that we have for libcamera controls, but that will
require manually defining Control instances for all the V4L2 controls,
which isn't very nice. I still need to think about it.
> Anyway
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>
> > 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
More information about the libcamera-devel
mailing list