Message ID | 20190929190254.18920-3-laurent.pinchart@ideasonboard.com |
---|---|
State | Accepted |
Headers | show |
Series |
|
Related | show |
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@ideasonboard.com> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Reviewed-by: Niklas Söderlund <niklas.soderlund@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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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; }