[libcamera-devel] [PATCH 1/2] libcamera: controls: Return ControlValue reference from ControlList::set()

[Jacopo Mondi]

Hi Laurent,

On Sun, Apr 26, 2020 at 02:16:22AM +0300, Laurent Pinchart wrote:
> It is useful for the caller of ControlList::set() to get a reference to
> the ControlValue that stores the control in the control list, in order
> to then modify that value directly. This allows creating an entry in
> the ControlList and then reserving memory for the control when getting
> V4L2 controls from a device. This is already possible by calling the
> get() function right after set(), but results in two lookups. Extend the
> id-based set() function to return the reference to the inserted
> ControlValue to avoid the second lookup.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  include/libcamera/controls.h | 2 +-
>  src/libcamera/controls.cpp   | 7 +++++--
>  2 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index 80944efc133a..5a5cfc3e52ca 100644
> --- a/include/libcamera/controls.h
> +++ b/include/libcamera/controls.h
> @@ -394,7 +394,7 @@ public:
>  	}
>
>  	const ControlValue &get(unsigned int id) const;
> -	void set(unsigned int id, const ControlValue &value);
> +	ControlValue &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 08df7f29e938..12822e87a4d7 100644
> --- a/src/libcamera/controls.cpp
> +++ b/src/libcamera/controls.cpp
> @@ -930,14 +930,17 @@ const ControlValue &ControlList::get(unsigned int id) const
>   *
>   * The behaviour is undefined if the control \a id is not supported by the
>   * object that the list refers to.
> + *
> + * \return A reference to the ControlValue stored in the control list
>   */
> -void ControlList::set(unsigned int id, const ControlValue &value)
> +ControlValue &ControlList::set(unsigned int id, const ControlValue &value)
>  {
>  	ControlValue *val = find(id);
>  	if (!val)
> -		return;
> +		return *val;

Wouldn't you dereference a nullptr ? Should we create a
        static ControlValue empty{};

at function begin and return that one ?

Overall, I think I'm still missing the use case for this tbh, but
we're not losing anything, as the previous return type was void, so
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks
  j

>
>  	*val = value;
> +	return *val;
>  }
>
>  /**
> --
> Regards,
>
> Laurent Pinchart
>