[libcamera-devel] [PATCH] ipa: rpi: cac: Minor code improvements and tidying

[David Plowman]

We make a few small improvements to the code:

* The arrayToSet method is prevented from overwriting the end of the
  array if there are too many values in the input table. If you supply
  a table, it will force you to put the correct number of elements in
  it.

* The arrayToSet and setStrength member functions are turned into
  static functions. (There may be a different public setStrength
  member function in future.)

* When no tables at all are given, the configuration is flagged as
  being disabled, so that we can avoid copying tables full of zeroes
  around. As a consequence, the pipeline handler too will disable this
  hardware block rather than run it needlessly. (Note that the tuning
  tool will put in a completely empty "rpi.cac" block if no CAC tuning
  images are supplied, benefiting from this behaviour.)

* The initialise member function is removed as it does nothing.

Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
---
 src/ipa/rpi/controller/rpi/cac.cpp | 82 ++++++++++++++++++++----------
 src/ipa/rpi/controller/rpi/cac.h   |  5 +-
 2 files changed, 55 insertions(+), 32 deletions(-)

diff --git a/src/ipa/rpi/controller/rpi/cac.cpp b/src/ipa/rpi/controller/rpi/cac.cpp
index 7c123da1..f2c8d282 100644
--- a/src/ipa/rpi/controller/rpi/cac.cpp
+++ b/src/ipa/rpi/controller/rpi/cac.cpp
@@ -27,40 +27,23 @@ char const *Cac::name() const
 	return NAME;
 }
 
-int Cac::read(const libcamera::YamlObject &params)
-{
-	arrayToSet(params["lut_rx"], config_.lutRx);
-	arrayToSet(params["lut_ry"], config_.lutRy);
-	arrayToSet(params["lut_bx"], config_.lutBx);
-	arrayToSet(params["lut_by"], config_.lutBy);
-	cacStatus_.lutRx = config_.lutRx;
-	cacStatus_.lutRy = config_.lutRy;
-	cacStatus_.lutBx = config_.lutBx;
-	cacStatus_.lutBy = config_.lutBy;
-	double strength = params["strength"].get<double>(1);
-	setStrength(config_.lutRx, cacStatus_.lutRx, strength);
-	setStrength(config_.lutBx, cacStatus_.lutBx, strength);
-	setStrength(config_.lutRy, cacStatus_.lutRy, strength);
-	setStrength(config_.lutBy, cacStatus_.lutBy, strength);
-	return 0;
-}
-
-void Cac::initialise()
-{
-}
-
-void Cac::arrayToSet(const libcamera::YamlObject &params, std::vector<double> &inputArray)
+static bool arrayToSet(const libcamera::YamlObject &params, std::vector<double> &inputArray, const Size &size)
 {
 	int num = 0;
-	const Size &size = getHardwareConfig().cacRegions;
-	inputArray.resize((size.width + 1) * (size.height + 1));
+	int max_num = (size.width + 1) * (size.height + 1);
+	inputArray.resize(max_num);
+
 	for (const auto &p : params.asList()) {
+		if (num == max_num)
+			return false;
 		inputArray[num++] = p.get<double>(0);
 	}
+
+	return num == max_num;
 }
 
-void Cac::setStrength(std::vector<double> &inputArray, std::vector<double> &outputArray,
-		      double strengthFactor)
+static void setStrength(std::vector<double> &inputArray, std::vector<double> &outputArray,
+			double strengthFactor)
 {
 	int num = 0;
 	for (const auto &p : inputArray) {
@@ -68,9 +51,52 @@ void Cac::setStrength(std::vector<double> &inputArray, std::vector<double> &outp
 	}
 }
 
+int Cac::read(const libcamera::YamlObject &params)
+{
+	config_.enabled = params.contains("lut_rx") && params.contains("lut_ry") &&
+			  params.contains("lut_bx") && params.contains("lut_by");
+	if (!config_.enabled)
+		return 0;
+
+	const Size &size = getHardwareConfig().cacRegions;
+
+	if (!arrayToSet(params["lut_rx"], config_.lutRx, size)) {
+		LOG(RPiCac, Error) << "Bad CAC lut_rx table";
+		return -EINVAL;
+	}
+
+	if (!arrayToSet(params["lut_ry"], config_.lutRy, size)) {
+		LOG(RPiCac, Error) << "Bad CAC lut_ry table";
+		return -EINVAL;
+	}
+
+	if (!arrayToSet(params["lut_bx"], config_.lutBx, size)) {
+		LOG(RPiCac, Error) << "Bad CAC lut_bx table";
+		return -EINVAL;
+	}
+
+	if (!arrayToSet(params["lut_by"], config_.lutBy, size)) {
+		LOG(RPiCac, Error) << "Bad CAC lut_by table";
+		return -EINVAL;
+	}
+
+	double strength = params["strength"].get<double>(1);
+	cacStatus_.lutRx = config_.lutRx;
+	cacStatus_.lutRy = config_.lutRy;
+	cacStatus_.lutBx = config_.lutBx;
+	cacStatus_.lutBy = config_.lutBy;
+	setStrength(config_.lutRx, cacStatus_.lutRx, strength);
+	setStrength(config_.lutBx, cacStatus_.lutBx, strength);
+	setStrength(config_.lutRy, cacStatus_.lutRy, strength);
+	setStrength(config_.lutBy, cacStatus_.lutBy, strength);
+
+	return 0;
+}
+
 void Cac::prepare(Metadata *imageMetadata)
 {
-	imageMetadata->set("cac.status", cacStatus_);
+	if (config_.enabled)
+		imageMetadata->set("cac.status", cacStatus_);
 }
 
 // Register algorithm with the system.
diff --git a/src/ipa/rpi/controller/rpi/cac.h b/src/ipa/rpi/controller/rpi/cac.h
index 419180ab..a7b14c00 100644
--- a/src/ipa/rpi/controller/rpi/cac.h
+++ b/src/ipa/rpi/controller/rpi/cac.h
@@ -12,6 +12,7 @@
 namespace RPiController {
 
 struct CacConfig {
+	bool enabled;
 	std::vector<double> lutRx;
 	std::vector<double> lutRy;
 	std::vector<double> lutBx;
@@ -24,15 +25,11 @@ public:
 	Cac(Controller *controller = NULL);
 	char const *name() const override;
 	int read(const libcamera::YamlObject &params) override;
-	void initialise() override;
 	void prepare(Metadata *imageMetadata) override;
-	void setStrength(std::vector<double> &inputArray, std::vector<double> &outputArray,
-			 double strengthFactor);
 
 private:
 	CacConfig config_;
 	CacStatus cacStatus_;
-	void arrayToSet(const libcamera::YamlObject &params, std::vector<double> &inputArray);
 };
 
 } // namespace RPiController
-- 
2.39.2