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

[David Plowman]

Hi Naush

Thanks for the patch.

On Mon, 5 Sept 2022 at 08:40, Naushir Patuck via libcamera-devel
<libcamera-devel at lists.libcamera.org> 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.
>
> 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>

Thanks!
David

> ---
>  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.
> --
> 2.25.1
>