[PATCH v4 16/18] libcamera: software_isp: Use DelayedControls

[Milan Zamazal]

Use the standard libcamera mechanism to report the "current" controls
rather than delaying updates by counting from the last update.

MY SPECULATION -- valid or not?:
A problem is that with software ISP we cannot be sure about the sensor
delay.  The original implementation simply skips exposure updates for 2
frames, which should be enough in most cases.  After this change, we
assume the delay being exactly 2 frames, which may or may not be
correct and may work with outdated values if the delay is shorter.

This patch also prepares moving exposure+gain to an algorithm module
where the original delay mechanism would be a (possibly unnecessary)
complication.

Resolves software ISP TODO #11 + #12.

Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
---
 src/ipa/simple/soft_simple.cpp           | 16 +------------
 src/libcamera/pipeline/simple/simple.cpp | 18 ++++++++++++---
 src/libcamera/software_isp/TODO          | 29 ------------------------
 3 files changed, 16 insertions(+), 47 deletions(-)

diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
index 8f01bf7d..0e47b9c5 100644
--- a/src/ipa/simple/soft_simple.cpp
+++ b/src/ipa/simple/soft_simple.cpp
@@ -59,8 +59,7 @@ class IPASoftSimple : public ipa::soft::IPASoftInterface, public Module
 public:
 	IPASoftSimple()
 		: params_(nullptr), stats_(nullptr),
-		  context_({ {}, {}, { kMaxFrameContexts } }),
-		  ignoreUpdates_(0)
+		  context_({ {}, {}, { kMaxFrameContexts } })
 	{
 	}
 
@@ -98,7 +97,6 @@ private:
 	int32_t exposure_;
 	double againMin_, againMax_, againMinStep_;
 	double again_;
-	unsigned int ignoreUpdates_;
 };
 
 IPASoftSimple::~IPASoftSimple()
@@ -298,16 +296,6 @@ void IPASoftSimple::processStats(const uint32_t frame,
 
 	/* \todo Switch to the libipa/algorithm.h API someday. */
 
-	/*
-	 * AE / AGC, use 2 frames delay to make sure that the exposure and
-	 * the gain set have applied to the camera sensor.
-	 * \todo This could be handled better with DelayedControls.
-	 */
-	if (ignoreUpdates_ > 0) {
-		--ignoreUpdates_;
-		return;
-	}
-
 	/*
 	 * Calculate Mean Sample Value (MSV) according to formula from:
 	 * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf
@@ -356,8 +344,6 @@ void IPASoftSimple::processStats(const uint32_t frame,
 	ctrls.set(V4L2_CID_ANALOGUE_GAIN,
 		  static_cast<int32_t>(camHelper_ ? camHelper_->gainCode(again_) : again_));
 
-	ignoreUpdates_ = 2;
-
 	setSensorControls.emit(ctrls);
 
 	LOG(IPASoft, Debug) << "exposureMSV " << exposureMSV
diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
index ed50ab52..9a778994 100644
--- a/src/libcamera/pipeline/simple/simple.cpp
+++ b/src/libcamera/pipeline/simple/simple.cpp
@@ -32,6 +32,7 @@
 #include "libcamera/internal/camera.h"
 #include "libcamera/internal/camera_sensor.h"
 #include "libcamera/internal/converter.h"
+#include "libcamera/internal/delayed_controls.h"
 #include "libcamera/internal/device_enumerator.h"
 #include "libcamera/internal/media_device.h"
 #include "libcamera/internal/pipeline_handler.h"
@@ -278,6 +279,8 @@ public:
 	std::vector<Configuration> configs_;
 	std::map<PixelFormat, std::vector<const Configuration *>> formats_;
 
+	std::unique_ptr<DelayedControls> delayedCtrls_;
+
 	std::vector<std::unique_ptr<FrameBuffer>> conversionBuffers_;
 	std::queue<std::map<const Stream *, FrameBuffer *>> conversionQueue_;
 	bool useConversion_;
@@ -900,14 +903,13 @@ void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer)
 
 void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)
 {
-	/* \todo Use the DelayedControls class */
 	swIsp_->processStats(frame, bufferId,
-			     sensor_->getControls({ V4L2_CID_ANALOGUE_GAIN,
-						    V4L2_CID_EXPOSURE }));
+			     delayedCtrls_->get(frame));
 }
 
 void SimpleCameraData::setSensorControls(const ControlList &sensorControls)
 {
+	delayedCtrls_->push(sensorControls);
 	ControlList ctrls(sensorControls);
 	sensor_->setControls(&ctrls);
 }
@@ -1288,6 +1290,16 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
 	if (outputCfgs.empty())
 		return 0;
 
+	std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {
+		{ V4L2_CID_ANALOGUE_GAIN, { 2, false } },
+		{ V4L2_CID_EXPOSURE, { 2, false } },
+	};
+	data->delayedCtrls_ =
+		std::make_unique<DelayedControls>(data->sensor_->device(),
+						  params);
+	data->video_->frameStart.connect(data->delayedCtrls_.get(),
+					 &DelayedControls::applyControls);
+
 	StreamConfiguration inputCfg;
 	inputCfg.pixelFormat = pipeConfig->captureFormat;
 	inputCfg.size = pipeConfig->captureSize;
diff --git a/src/libcamera/software_isp/TODO b/src/libcamera/software_isp/TODO
index 9978afc0..8eeede46 100644
--- a/src/libcamera/software_isp/TODO
+++ b/src/libcamera/software_isp/TODO
@@ -209,35 +209,6 @@ At some point, yes.
 
 ---
 
-11. Improve handling the sensor controls which take effect with a delay
-
-> void IPASoftSimple::processStats(const ControlList &sensorControls)
-> {
->       ...
->	/*
->	 * AE / AGC, use 2 frames delay to make sure that the exposure and
->	 * the gain set have applied to the camera sensor.
->	 */
->	if (ignore_updates_ > 0) {
->		--ignore_updates_;
->		return;
->	}
-
-This could be handled better with DelayedControls.
-
----
-
-12. Use DelayedControls class in ispStatsReady()
-
-> void SimpleCameraData::ispStatsReady()
-> {
-> 	swIsp_->processStats(sensor_->getControls({ V4L2_CID_ANALOGUE_GAIN,
-> 						    V4L2_CID_EXPOSURE }));
-
-You should use the DelayedControls class.
-
----
-
 13. Improve black level and colour gains application
 
 I think the black level should eventually be moved before debayering, and
-- 
2.44.1