[PATCH] pipeline: rkisp1: cproc: Fix default value handling

[Stefan Klug]

Hi Kieran,

thanks for the review and especially for the s/where/were/. I need to
put an autocorrect there :-)

The suggested change to kDefaultXXX makes it really easier to digest.

For reference I added the patch that will go in after gamma below.

Cheers,
Stefan

---

Subject: [PATCH v2] pipeline: rkisp1: cproc: Fix default value handling

Default control values were not applied to activeState. This had no negative
side effects in the first place, as the hardware defaults were used. The issue
became visible, when only one of the controls was set at runtime. In that case
the params for the other values were overwritten with 0 (reset value of
activeState) resulting in a black image.

While at it, only add the controls to the controls map if the algorithm is
contained in the tuning file.

Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
---
 src/ipa/rkisp1/algorithms/cproc.cpp | 58 +++++++++++++++++++++++++++--
 src/ipa/rkisp1/algorithms/cproc.h   |  3 ++
 src/ipa/rkisp1/rkisp1.cpp           |  3 --
 3 files changed, 58 insertions(+), 6 deletions(-)

diff --git a/src/ipa/rkisp1/algorithms/cproc.cpp b/src/ipa/rkisp1/algorithms/cproc.cpp
index 68bb8180..18c7b719 100644
--- a/src/ipa/rkisp1/algorithms/cproc.cpp
+++ b/src/ipa/rkisp1/algorithms/cproc.cpp
@@ -33,6 +33,55 @@ namespace ipa::rkisp1::algorithms {
 
 LOG_DEFINE_CATEGORY(RkISP1CProc)
 
+constexpr float kDefaultBrightness = 0.0f;
+constexpr float kDefaultContrast = 1.0f;
+constexpr float kDefaultSaturation = 1.0f;
+
+static int convertBrightness(const float v)
+{
+	return std::clamp<int>(std::lround(v * 128), -128, 127);
+}
+
+static int convertContrast(const float v)
+{
+	return std::clamp<int>(std::lround(v * 128), 0, 255);
+}
+
+static int convertSaturation(const float v)
+{
+	return std::clamp<int>(std::lround(v * 128), 0, 255);
+}
+
+/**
+ * \copydoc libcamera::ipa::Algorithm::init
+ */
+int ColorProcessing::init([[maybe_unused]] IPAContext &context,
+			  [[maybe_unused]] const YamlObject &tuningData)
+{
+	auto &cmap = context.ctrlMap;
+
+	cmap[&controls::Brightness] = ControlInfo(-1.0f, 0.993f, kDefaultBrightness);
+	cmap[&controls::Contrast] = ControlInfo(0.0f, 1.993f, kDefaultContrast);
+	cmap[&controls::Saturation] = ControlInfo(0.0f, 1.993f, kDefaultSaturation);
+
+	return 0;
+}
+
+/**
+ * \copydoc libcamera::ipa::Algorithm::configure
+ */
+int ColorProcessing::configure([[maybe_unused]] IPAContext &context,
+			       [[maybe_unused]] const IPACameraSensorInfo &configInfo)
+{
+	auto &cproc = context.activeState.cproc;
+
+	cproc.brightness = convertBrightness(kDefaultBrightness);
+	cproc.contrast = convertContrast(kDefaultContrast);
+	cproc.saturation = convertSaturation(kDefaultSaturation);
+
+	return 0;
+}
+
 /**
  * \copydoc libcamera::ipa::Algorithm::queueRequest
  */
@@ -44,9 +93,12 @@ void ColorProcessing::queueRequest(IPAContext &context,
 	auto &cproc = context.activeState.cproc;
 	bool update = false;
 
+	if (frame == 0)
+		update = true;
+
 	const auto &brightness = controls.get(controls::Brightness);
 	if (brightness) {
-		int value = std::clamp<int>(std::lround(*brightness * 128), -128, 127);
+		int value = convertBrightness(*brightness);
 		if (cproc.brightness != value) {
 			cproc.brightness = value;
 			update = true;
@@ -57,7 +109,7 @@ void ColorProcessing::queueRequest(IPAContext &context,
 
 	const auto &contrast = controls.get(controls::Contrast);
 	if (contrast) {
-		int value = std::clamp<int>(std::lround(*contrast * 128), 0, 255);
+		int value = convertContrast(*contrast);
 		if (cproc.contrast != value) {
 			cproc.contrast = value;
 			update = true;
@@ -68,7 +120,7 @@ void ColorProcessing::queueRequest(IPAContext &context,
 
 	const auto saturation = controls.get(controls::Saturation);
 	if (saturation) {
-		int value = std::clamp<int>(std::lround(*saturation * 128), 0, 255);
+		int value = convertSaturation(*saturation);
 		if (cproc.saturation != value) {
 			cproc.saturation = value;
 			update = true;
diff --git a/src/ipa/rkisp1/algorithms/cproc.h b/src/ipa/rkisp1/algorithms/cproc.h
index bafba5cc..e50e7200 100644
--- a/src/ipa/rkisp1/algorithms/cproc.h
+++ b/src/ipa/rkisp1/algorithms/cproc.h
@@ -21,6 +21,9 @@ public:
 	ColorProcessing() = default;
 	~ColorProcessing() = default;
 
+	int init(IPAContext &context, const YamlObject &tuningData) override;
+	int configure(IPAContext &context,
+		      const IPACameraSensorInfo &configInfo) override;
 	void queueRequest(IPAContext &context, const uint32_t frame,
 			  IPAFrameContext &frameContext,
 			  const ControlList &controls) override;
diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
index 17474408..62d56a3a 100644
--- a/src/ipa/rkisp1/rkisp1.cpp
+++ b/src/ipa/rkisp1/rkisp1.cpp
@@ -109,9 +109,6 @@ const ControlInfoMap::Map rkisp1Controls{
 	{ &controls::AeEnable, ControlInfo(false, true) },
 	{ &controls::AwbEnable, ControlInfo(false, true) },
 	{ &controls::ColourGains, ControlInfo(0.0f, 3.996f, 1.0f) },
-	{ &controls::Brightness, ControlInfo(-1.0f, 0.993f, 0.0f) },
-	{ &controls::Contrast, ControlInfo(0.0f, 1.993f, 1.0f) },
-	{ &controls::Saturation, ControlInfo(0.0f, 1.993f, 1.0f) },
 	{ &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) },
 	{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },
 };
-- 
2.43.0