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

[Umang Jain]

Hi Milan,

On 21/09/24 12:01 am, Milan Zamazal wrote:
> Use the standard libcamera mechanism to report the "current" controls
> rather than delaying updates by counting from the last update.
>
> 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.
>
> According to Kieran, the wrong parts are also wrong on the
> IPU3/RKISP1/Mali pipelines and only RPi have this correct.  We need to
> fix this, by correctly specifying the gains in the libipa camera sensor
> helpers.  The sooner the better because this change could introduce a
> risk of increasing oscillations.
>
> 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>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

Reviewed-by: Umang Jain <umang.jain at ideasonboard.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 f8d923c5..60310a8e 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 61157fe6..1e13bd74 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_;
> @@ -896,14 +899,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);
>   }
> @@ -1284,6 +1286,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