[libcamera-devel,12/12] libcamera: controls: Use ControlValidator to validate ControlList

2054 diff mbox series

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 51abb4ea7f6f..f9124ca65e19 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)
 {
 }
 
@@ -492,12 +487,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;
 	}