[libcamera-devel] [PATCH v4 05/13] libcamera: controls: Extend ControlList to access controls by ID

[Niklas Söderlund]

Hi Laurent,

Thanks for your work.

On 2019-07-01 23:14:56 +0300, Laurent Pinchart wrote:
> The ControlList class implements a map from control specifier to control
> ID. To avoid constant lookups of ControlInfo when using the class in the
> libcamera core or in pipeline handlers, the map uses ControlInfo
> pointers instead of ControlId values. This is however not very
> convenient for applications or pipeline handlers, as they would be
> forced to first look up the ControlInfo pointers for the controls they
> want to access. Facilitate ease of use of ControlLists by implementing
> an internal lookup of the ControlInfo from the controls provided by the
> Camera.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> Changes since v3:
> 
> - Typo fixes
> ---
>  include/libcamera/controls.h |  2 ++
>  src/libcamera/controls.cpp   | 54 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 56 insertions(+)
> 
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index 18293c9462cf..fbb3a62274c6 100644
> --- a/include/libcamera/controls.h
> +++ b/include/libcamera/controls.h
> @@ -117,11 +117,13 @@ public:
>  	const_iterator begin() const { return controls_.begin(); }
>  	const_iterator end() const { return controls_.end(); }
>  
> +	bool contains(ControlId id) const;
>  	bool contains(const ControlInfo *info) const;
>  	bool empty() const { return controls_.empty(); }
>  	std::size_t size() const { return controls_.size(); }
>  	void clear() { controls_.clear(); }
>  
> +	ControlValue &operator[](ControlId id);
>  	ControlValue &operator[](const ControlInfo *info) { return controls_[info]; }
>  
>  	void update(const ControlList &list);
> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> index cd2cf337b379..42a2f8990bf6 100644
> --- a/src/libcamera/controls.cpp
> +++ b/src/libcamera/controls.cpp
> @@ -10,6 +10,8 @@
>  #include <sstream>
>  #include <string>
>  
> +#include <libcamera/camera.h>
> +
>  #include "log.h"
>  #include "utils.h"
>  
> @@ -387,6 +389,30 @@ ControlList::ControlList(Camera *camera)
>   * list
>   */
>  
> +/**
> + * \brief Check if the list contains a control with the specified \a id
> + * \param[in] id The control ID
> + *
> + * The behaviour is undefined if the control \a id is not supported by the
> + * camera that the ControlList refers to.

Is it undefined ? It looks like an Error is be printed and false is 
returned, right? Same comment bellow.

With or without this nit fixed,

Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>

> + *
> + * \return True if the list contains a matching control, false otherwise
> + */
> +bool ControlList::contains(ControlId id) const
> +{
> +	const ControlInfoMap &controls = camera_->controls();
> +	const auto iter = controls.find(id);
> +	if (iter == controls.end()) {
> +		LOG(Controls, Error)
> +			<< "Camera " << camera_->name()
> +			<< " does not support control " << id;
> +
> +		return false;
> +	}
> +
> +	return controls_.find(&iter->second) != controls_.end();
> +}
> +
>  /**
>   * \brief Check if the list contains a control with the specified \a info
>   * \param[in] info The control info
> @@ -414,6 +440,34 @@ bool ControlList::contains(const ControlInfo *info) const
>   * \brief Removes all controls from the list
>   */
>  
> +/**
> + * \brief Access or insert the control specified by \a id
> + * \param[in] id The control ID
> + *
> + * This method returns a reference to the control identified by \a id, inserting
> + * it in the list if the ID is not already present.
> + *
> + * The behaviour is undefined if the control \a id is not supported by the
> + * camera that the ControlList refers to.
> + *
> + * \return A reference to the value of the control identified by \a id
> + */
> +ControlValue &ControlList::operator[](ControlId id)
> +{
> +	const ControlInfoMap &controls = camera_->controls();
> +	const auto iter = controls.find(id);
> +	if (iter == controls.end()) {
> +		LOG(Controls, Error)
> +			<< "Camera " << camera_->name()
> +			<< " does not support control " << id;
> +
> +		static ControlValue empty;
> +		return empty;
> +	}
> +
> +	return controls_[&iter->second];
> +}
> +
>  /**
>   * \fn ControlList::operator[](const ControlInfo *info)
>   * \brief Access or insert the control specified by \a info
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund