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

21321 diff mbox series

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@redhat.com>
Reviewed-by: Kieran Bingham <kieran.bingham@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(-)

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@redhat.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Reviewed-by: Umang Jain <umang.jain@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