[libcamera-devel,02/12] libcamera: controls: Make ControlValue get/set accessors template methods
diff mbox series

Message ID 20190928152238.23752-3-laurent.pinchart@ideasonboard.com
State Superseded
Headers show
Series
  • Improve the application-facing controls API
Related show

Commit Message

Laurent Pinchart Sept. 28, 2019, 3:22 p.m. UTC 
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>
---
 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(-)

Comments

Jacopo Mondi Sept. 29, 2019, 8:58 a.m. UTC | #1 
Hi Laurent,

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@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?

Other than that, the burden of having get<type>() is repaid in full by
how nice set() looks like.

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Ah, wait, can we mix the two ? getInt() and set(Control<int>, value) ?
Is this desirable ?

Thanks
   j


>  }
> -
> -/**
> - * \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
Laurent Pinchart Sept. 29, 2019, 11:32 a.m. UTC | #2 
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@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@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;
> >  		}

Patch
diff mbox series 

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;
 		}