| Message ID | 20190929190254.18920-4-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, Thanks for your patch. On 2019-09-29 22:02:44 +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@ideasonboard.com> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > 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>()); > 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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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>()); 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; }