[libcamera-devel] [PATCH 02/12] libcamera: controls: Make ControlValue get/set accessors template methods
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun Sep 29 13:32:30 CEST 2019
Hi Jacopo,
On Sun, Sep 29, 2019 at 10:58:04AM +0200, Jacopo Mondi wrote:
> On Sat, Sep 28, 2019 at 06:22:28PM +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>
> > ---
> > 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;
>
> This was probably here already, but with the option of getting
> integer64_ from controls set as int, what would we get ?
> The union occupies 8 bytes, we set 4 with integer32_ = value, then get
> back 8 with get<int64_t>(). Is this ok?
Probably not. I think this needs to be addressed, and we need to decide
how to do so. One option would be to disallow reading a 32-bit control
as a 64-bit value. I'm not opposed to that, but the consequences need to
be considered. It used to cause issues, one of them being, if I remember
connectly, that V4L2 controls min and max values were always stored as
64-bit, leading to a .set(min) on a ControlValue storing a 64-bit
integer, even for 32-bit controls. This one should be fixed now that we
have a more explicit API, and it may be time to disallow unsafe integer
conversions.
What's the general opinion, should we give this a try ?
> Other than that, the burden of having get<type>() is repaid in full by
> how nice set() looks like.
>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>
> Ah, wait, can we mix the two ? getInt() and set(Control<int>, value) ?
> Is this desirable ?
ControlValue::get is typically called from ControlList::get(const
Control<T> &), so the template version of ControlValue::get is required
to make this possible. We could also have a getInt in addition to
get<int>, but I think that would be overkill, beside the fact that
having two functions that fulfil the same purpose isn't very nice.
> > }
> > -
> > -/**
> > - * \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
More information about the libcamera-devel
mailing list