[libcamera-devel] [PATCH] libcamera: controls: Use template in the id-based interface

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, Dec 30, 2019 at 04:57:25PM +0100, Jacopo Mondi wrote:
> The ControlList Control<> and id based interfaces to set or get controls
> differ in the way they return or receive values. The Control<> based
> interface allows the usage of raw values, while the id-based interface
> always goes through ControlValue, resulting in one additional function
> call in the caller when accessing controls by numerical id.
> 
> Align the two implementations by providing access to raw values for the
> id-based interface.
> 
> -       controls.get(V4L2_CID_CONTRAST).get<int32_t>()
> +	controls.get<int32_t>(V4L2_CID_CONTRAST)

The commit message doesn't explain why this is desirable :-) This patch
may make sense as part of a bigger set of changes, but in isolation I
don't see anything wrong with accessing the ControlValue.

> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  include/libcamera/controls.h       | 23 ++++++++++++++++++-
>  src/libcamera/controls.cpp         | 37 +++++++++++++++++-------------
>  test/ipa/ipa_wrappers_test.cpp     |  6 ++---
>  test/v4l2_videodevice/controls.cpp | 12 +++++-----
>  4 files changed, 52 insertions(+), 26 deletions(-)
> 
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index b1b73367e874..0b55359890e6 100644
> --- a/include/libcamera/controls.h
> +++ b/include/libcamera/controls.h
> @@ -220,6 +220,18 @@ public:
>  		return val->get<T>();
>  	}
>  
> +	template<typename T>
> +	const T &get(unsigned int id) const
> +	{
> +		const ControlValue *val = find(id);
> +		if (!val) {
> +			static T t(0);
> +			return t;
> +		}
> +
> +		return val->get<T>();
> +	}
> +
>  	template<typename T>
>  	void set(const Control<T> &ctrl, const T &value)
>  	{
> @@ -230,7 +242,16 @@ public:
>  		val->set<T>(value);
>  	}
>  
> -	const ControlValue &get(unsigned int id) const;
> +	template<typename T>
> +	void set(unsigned int id, const T &value)
> +	{
> +		ControlValue *val = find(id);
> +		if (!val)
> +			return;
> +
> +		val->set<T>(value);
> +	}
> +
>  	void set(unsigned int id, const ControlValue &value);
>  
>  	const ControlInfoMap *infoMap() const { return infoMap_; }
> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> index 7d8a0e97ee3a..78729d55ee29 100644
> --- a/src/libcamera/controls.cpp
> +++ b/src/libcamera/controls.cpp
> @@ -726,6 +726,18 @@ bool ControlList::contains(unsigned int id) const
>   * \return The control value
>   */
>  
> +/**
> + * \fn template<typename T> const T &ControlList::get(unsigned int id) const
> + * \brief Get the value of control \a id
> + * \param[in] id The control numerical ID
> + *
> + * The behaviour is undefined if the control \a id is not present in the list.
> + * Use ControlList::contains() to test for the presence of a control in the
> + * list before retrieving its value.
> + *
> + * \return The control value
> + */
> +
>  /**
>   * \fn template<typename T> void ControlList::set(const Control<T> &ctrl, const T &value)
>   * \brief Set the control \a ctrl value to \a value
> @@ -741,25 +753,18 @@ bool ControlList::contains(unsigned int id) const
>   */
>  
>  /**
> - * \brief Get the value of control \a id
> - * \param[in] id The control numerical ID
> + * \fn template<typename T> void ControlList::set(unsigned int id, const T &value)
> + * \brief Set the value of control \a id to \a value
> + * \param[in] id The control ID
> + * \param[in] value The control value
>   *
> - * The behaviour is undefined if the control \a id is not present in the list.
> - * Use ControlList::contains() to test for the presence of a control in the
> - * list before retrieving its value.
> + * This method sets the value of a control in the control list. If the control
> + * is already present in the list, its value is updated, otherwise it is added
> + * to the list.
>   *
> - * \return The control value
> + * The behaviour is undefined if the control \a id is not supported by the
> + * object that the list refers to.
>   */
> -const ControlValue &ControlList::get(unsigned int id) const
> -{
> -	static ControlValue zero;
> -
> -	const ControlValue *val = find(id);
> -	if (!val)
> -		return zero;
> -
> -	return *val;
> -}
>  
>  /**
>   * \brief Set the value of control \a id to \a value
> diff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp
> index a1e34ad52317..11fea2ccc506 100644
> --- a/test/ipa/ipa_wrappers_test.cpp
> +++ b/test/ipa/ipa_wrappers_test.cpp
> @@ -181,9 +181,9 @@ public:
>  		}
>  
>  		const ControlList &controls = data.controls[0];
> -		if (controls.get(V4L2_CID_BRIGHTNESS).get<int32_t>() != 10 ||
> -		    controls.get(V4L2_CID_CONTRAST).get<int32_t>() != 20 ||
> -		    controls.get(V4L2_CID_SATURATION).get<int32_t>() != 30) {
> +		if (controls.get<int32_t>(V4L2_CID_BRIGHTNESS) != 10 ||
> +		    controls.get<int32_t>(V4L2_CID_CONTRAST) != 20 ||
> +		    controls.get<int32_t>(V4L2_CID_SATURATION) != 30) {
>  			cerr << "processEvent(): Invalid controls" << endl;
>  			return report(Op_processEvent, TestFail);
>  		}
> diff --git a/test/v4l2_videodevice/controls.cpp b/test/v4l2_videodevice/controls.cpp
> index 42c653d4435a..e6ab1b4c11bc 100644
> --- a/test/v4l2_videodevice/controls.cpp
> +++ b/test/v4l2_videodevice/controls.cpp
> @@ -57,9 +57,9 @@ protected:
>  			return TestFail;
>  		}
>  
> -		if (ctrls.get(V4L2_CID_BRIGHTNESS).get<int32_t>() == -1 ||
> -		    ctrls.get(V4L2_CID_CONTRAST).get<int32_t>() == -1 ||
> -		    ctrls.get(V4L2_CID_SATURATION).get<int32_t>() == -1) {
> +		if (ctrls.get<int32_t>(V4L2_CID_BRIGHTNESS) == -1 ||
> +		    ctrls.get<int32_t>(V4L2_CID_CONTRAST) == -1 ||
> +		    ctrls.get<int32_t>(V4L2_CID_SATURATION) == -1) {
>  			cerr << "Incorrect value for retrieved controls" << endl;
>  			return TestFail;
>  		}
> @@ -86,9 +86,9 @@ protected:
>  			return TestFail;
>  		}
>  
> -		if (ctrls.get(V4L2_CID_BRIGHTNESS) != brightness.min() ||
> -		    ctrls.get(V4L2_CID_CONTRAST) != contrast.max() ||
> -		    ctrls.get(V4L2_CID_SATURATION) != saturation.min().get<int32_t>() + 1) {
> +		if (ctrls.get<int32_t>(V4L2_CID_BRIGHTNESS) != brightness.min().get<int32_t>() ||
> +		    ctrls.get<int32_t>(V4L2_CID_CONTRAST) != contrast.max().get<int32_t>() ||
> +		    ctrls.get<int32_t>(V4L2_CID_SATURATION) != saturation.min().get<int32_t>() + 1) {
>  			cerr << "Controls not updated when set" << endl;
>  			return TestFail;
>  		}

-- 
Regards,

Laurent Pinchart