[libcamera-devel] [PATCH v3 7/7] ipa: raspberrypi: agc: Fix digital gain calculation for manual mode

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

On Mon, Sep 26, 2022 at 10:57:37AM +0100, Naushir Patuck via libcamera-devel wrote:
> The digital gain calculation uses a total exposure value computed with the
> current AGC state. However, this is wrong in the case of manual shutter/gain
> controls, as the total exposure value used must be the value computed when the
> AGC sent the manual shutter/gain controls to the pipeline handler to action.
> 
> To fix this, the IPA now adds the historical AgcStatus structure to the metadata
> (tagged with "agc.delayed_status"). This historical AgcStatus structure contains
> the total exposure value calculated when the AGC sent the manual shutter/gain
> controls to the pipeline handler.

Have you looked at the work we have recently merged that introduced a
frame context queue in libipa ? This has allowed us to solve a very
similar issue in the RkISP1 AWB, which needs to know the colour gains
that have been applied to the frame, and not just the latest gains (the
hardware computes the AWB statistics after applying the colour
gains...).

The issue was fixed in [1], with [2] introducing the frame context queue
in the AWB algorithm, and [3] providing documentation about how the
active state and frame context collaborate. The frame context queue
itself is very similar to the metadata array you introduce in patch 5/7
in this series.

While I have no doubt this series fixes your issue, I'm concerned about
how it relies on passing information through the DelayedControls class,
which affects all pipeline handlers. The more we diverge in the usage of
that class, the more difficult per-frame control will be difficult to
implement. If the differences related to this issue between the
Raspberry Pi on one side and RkISP1 and IPU3 on the other side were
mostly in the IPA module I wouldn't be so concerned, as the IPA modules
differ quite a lot already for historical reasons, but it expands beyond
the IPA side here.

Would you be able to have a look at the frame context queue ? Should we
look together at how we could align the platforms better ?

[1] https://git.libcamera.org/libcamera/libcamera.git/commit/?id=290ebeb59525eb7fdcc6f815d8dee6cbe3d8a314
[2] https://git.libcamera.org/libcamera/libcamera.git/commit/?id=128f22bce55ba2baea082010bceaffdba23f3f0d
[3] https://git.libcamera.org/libcamera/libcamera.git/commit/?id=a2f34f19578e8dae37cb7898710d15b83f176f94

> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> Tested-by: David Plowman <david.plowman at raspberrypi.com>
> ---
>  src/ipa/raspberrypi/controller/rpi/agc.cpp | 10 ++++++++--
>  src/ipa/raspberrypi/raspberrypi.cpp        | 11 +++++++++++
>  2 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> index bd54a639d637..b18bd7b5b19e 100644
> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> @@ -408,6 +408,12 @@ void Agc::switchMode(CameraMode const &cameraMode,
>  
>  void Agc::prepare(Metadata *imageMetadata)
>  {
> +	Duration totalExposureValue = status_.totalExposureValue;
> +	AgcStatus delayedStatus;
> +
> +	if (!imageMetadata->get("agc.delayed_status", delayedStatus))
> +		totalExposureValue = delayedStatus.totalExposureValue;
> +
>  	status_.digitalGain = 1.0;
>  	fetchAwbStatus(imageMetadata); /* always fetch it so that Process knows it's been done */
>  
> @@ -418,8 +424,8 @@ void Agc::prepare(Metadata *imageMetadata)
>  			Duration actualExposure = deviceStatus.shutterSpeed *
>  						  deviceStatus.analogueGain;
>  			if (actualExposure) {
> -				status_.digitalGain = status_.totalExposureValue / actualExposure;
> -				LOG(RPiAgc, Debug) << "Want total exposure " << status_.totalExposureValue;
> +				status_.digitalGain = totalExposureValue / actualExposure;
> +				LOG(RPiAgc, Debug) << "Want total exposure " << totalExposureValue;
>  				/*
>  				 * Never ask for a gain < 1.0, and also impose
>  				 * some upper limit. Make it customisable?
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 0e80ef9c7b95..bc438e99eff6 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -1025,6 +1025,17 @@ void IPARPi::prepareISP(const ISPConfig &data)
>  		embeddedBuffer = it->second.planes()[0];
>  	}
>  
> +	/*
> +	 * AGC wants to know the algorithm status from the time it actioned the
> +	 * sensor exposure/gain changes. So fetch it from the metadata list
> +	 * indexed by the IPA cookie returned, and put it in the current frame
> +	 * metadata.
> +	 */
> +	AgcStatus agcStatus;
> +	RPiController::Metadata &delayedMetadata = rpiMetadata_[data.ipaCookie];
> +	if (!delayedMetadata.get<AgcStatus>("agc.status", agcStatus))
> +		rpiMetadata.set("agc.delayed_status", agcStatus);
> +
>  	/*
>  	 * This may overwrite the DeviceStatus using values from the sensor
>  	 * metadata, and may also do additional custom processing.

-- 
Regards,

Laurent Pinchart