[libcamera-devel] [PATCH v2 13/13] libcamera: controls: Use ControlValidator to validate ControlList

[Niklas Söderlund]

Hi Laurent,

Thanks for your patch.

On 2019-09-29 22:02:54 +0300, Laurent Pinchart wrote:
> Replace the manual validation of controls against a Camera with usage of
> the new ControlValidator interface.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

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

> ---
>  include/libcamera/controls.h   |  6 +++---
>  include/libcamera/request.h    |  7 ++++---
>  src/libcamera/controls.cpp     | 27 ++++++++++-----------------
>  src/libcamera/request.cpp      | 14 ++++++++++++--
>  test/controls/control_list.cpp | 15 ++++++++++++++-
>  5 files changed, 43 insertions(+), 26 deletions(-)
> 
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index d3eea643c0ec..90426753f40f 100644
> --- a/include/libcamera/controls.h
> +++ b/include/libcamera/controls.h
> @@ -13,7 +13,7 @@
>  
>  namespace libcamera {
>  
> -class Camera;
> +class ControlValidator;
>  
>  enum ControlType {
>  	ControlTypeNone,
> @@ -119,7 +119,7 @@ private:
>  	using ControlListMap = std::unordered_map<const ControlId *, ControlValue>;
>  
>  public:
> -	ControlList(Camera *camera);
> +	ControlList(ControlValidator *validator);
>  
>  	using iterator = ControlListMap::iterator;
>  	using const_iterator = ControlListMap::const_iterator;
> @@ -160,7 +160,7 @@ private:
>  	const ControlValue *find(const ControlId &id) const;
>  	ControlValue *find(const ControlId &id);
>  
> -	Camera *camera_;
> +	ControlValidator *validator_;
>  	ControlListMap controls_;
>  };
>  
> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> index 9edf1cedc054..e3db5243aaf3 100644
> --- a/include/libcamera/request.h
> +++ b/include/libcamera/request.h
> @@ -19,9 +19,9 @@ namespace libcamera {
>  
>  class Buffer;
>  class Camera;
> +class CameraControlValidator;
>  class Stream;
>  
> -
>  class Request
>  {
>  public:
> @@ -36,7 +36,7 @@ public:
>  	Request &operator=(const Request &) = delete;
>  	~Request();
>  
> -	ControlList &controls() { return controls_; }
> +	ControlList &controls() { return *controls_; }
>  	const std::map<Stream *, Buffer *> &buffers() const { return bufferMap_; }
>  	int addBuffer(std::unique_ptr<Buffer> buffer);
>  	Buffer *findBuffer(Stream *stream) const;
> @@ -56,7 +56,8 @@ private:
>  	bool completeBuffer(Buffer *buffer);
>  
>  	Camera *camera_;
> -	ControlList controls_;
> +	CameraControlValidator *validator_;
> +	ControlList *controls_;
>  	std::map<Stream *, Buffer *> bufferMap_;
>  	std::unordered_set<Buffer *> pending_;
>  
> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> index a7e9d069b31a..f3260edce0bc 100644
> --- a/src/libcamera/controls.cpp
> +++ b/src/libcamera/controls.cpp
> @@ -10,8 +10,7 @@
>  #include <sstream>
>  #include <string>
>  
> -#include <libcamera/camera.h>
> -
> +#include "control_validator.h"
>  #include "log.h"
>  #include "utils.h"
>  
> @@ -365,20 +364,16 @@ std::string ControlRange::toString() const
>   * \class ControlList
>   * \brief Associate a list of ControlId with their values for a camera
>   *
> - * A ControlList wraps a map of ControlId to ControlValue and provide
> - * additional validation against the control information exposed by a Camera.
> - *
> - * A list is only valid for as long as the camera it refers to is valid. After
> - * that calling any method of the ControlList class other than its destructor
> - * will cause undefined behaviour.
> + * A ControlList wraps a map of ControlId to ControlValue and optionally
> + * validates controls against a ControlValidator.
>   */
>  
>  /**
> - * \brief Construct a ControlList with a reference to the Camera it applies on
> - * \param[in] camera The camera
> + * \brief Construct a ControlList with an optional control validator
> + * \param[in] validator The validator (may be null)
>   */
> -ControlList::ControlList(Camera *camera)
> -	: camera_(camera)
> +ControlList::ControlList(ControlValidator *validator)
> +	: validator_(validator)
>  {
>  }
>  
> @@ -493,12 +488,10 @@ const ControlValue *ControlList::find(const ControlId &id) const
>  
>  ControlValue *ControlList::find(const ControlId &id)
>  {
> -	const ControlInfoMap &controls = camera_->controls();
> -	const auto iter = controls.find(&id);
> -	if (iter == controls.end()) {
> +	if (validator_ && !validator_->validate(id)) {
>  		LOG(Controls, Error)
> -			<< "Camera " << camera_->name()
> -			<< " does not support control " << id.name();
> +			<< "Control " << id.name()
> +			<< " is not valid for " << validator_->name();
>  		return nullptr;
>  	}
>  
> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index ee2158fc7a9c..19f6d0b9a0ae 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -13,6 +13,7 @@
>  #include <libcamera/camera.h>
>  #include <libcamera/stream.h>
>  
> +#include "camera_controls.h"
>  #include "log.h"
>  
>  /**
> @@ -55,9 +56,15 @@ LOG_DEFINE_CATEGORY(Request)
>   *
>   */
>  Request::Request(Camera *camera, uint64_t cookie)
> -	: camera_(camera), controls_(camera), cookie_(cookie),
> -	  status_(RequestPending), cancelled_(false)
> +	: camera_(camera), cookie_(cookie), status_(RequestPending),
> +	  cancelled_(false)
>  {
> +	/**
> +	 * \todo Should the Camera expose a validator instance, to avoid
> +	 * creating a new instance for each request?
> +	 */
> +	validator_ = new CameraControlValidator(camera);
> +	controls_ = new ControlList(validator_);
>  }
>  
>  Request::~Request()
> @@ -66,6 +73,9 @@ Request::~Request()
>  		Buffer *buffer = it.second;
>  		delete buffer;
>  	}
> +
> +	delete controls_;
> +	delete validator_;
>  }
>  
>  /**
> diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp
> index 8469ecf09439..1bcfecc467b5 100644
> --- a/test/controls/control_list.cpp
> +++ b/test/controls/control_list.cpp
> @@ -12,6 +12,7 @@
>  #include <libcamera/control_ids.h>
>  #include <libcamera/controls.h>
>  
> +#include "camera_controls.h"
>  #include "test.h"
>  
>  using namespace std;
> @@ -40,7 +41,8 @@ protected:
>  
>  	int run()
>  	{
> -		ControlList list(camera_.get());
> +		CameraControlValidator validator(camera_.get());
> +		ControlList list(&validator);
>  
>  		/* Test that the list is initially empty. */
>  		if (!list.empty()) {
> @@ -141,6 +143,17 @@ protected:
>  			return TestFail;
>  		}
>  
> +		/*
> +		 * Attempt to set an invalid control and verify that the
> +		 * operation failed.
> +		 */
> +		list.set(controls::AwbEnable, true);
> +
> +		if (list.contains(controls::AwbEnable)) {
> +			cout << "List shouldn't contain AwbEnable control" << endl;
> +			return TestFail;
> +		}
> +
>  		return TestPass;
>  	}
>  
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund