[libcamera-devel] [PATCH 2/3] ipa: rpi: agc: Filter exposures before dealing with digital gain

[Naushir Patuck]

Hi David,

Nice simplification!

On Fri, 28 Jul 2023 at 14:37, David Plowman via libcamera-devel
<libcamera-devel at lists.libcamera.org> wrote:
>
> We now time-filter the exposure before sorting out how much digital
> gain is required. This is actually a little more natural and
> simplifies the code.  It also prepares us for some future work where
> this arrangement will be helpful.
>
> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>

Reviewed-by: Naushir Patuck <naush at raspberrpyi.com>

> ---
>  src/ipa/rpi/controller/rpi/agc.cpp | 30 +++++-------------------------
>  src/ipa/rpi/controller/rpi/agc.h   |  2 +-
>  2 files changed, 6 insertions(+), 26 deletions(-)
>
> diff --git a/src/ipa/rpi/controller/rpi/agc.cpp b/src/ipa/rpi/controller/rpi/agc.cpp
> index e8526355..6087fc60 100644
> --- a/src/ipa/rpi/controller/rpi/agc.cpp
> +++ b/src/ipa/rpi/controller/rpi/agc.cpp
> @@ -469,14 +469,14 @@ void Agc::process(StatisticsPtr &stats, Metadata *imageMetadata)
>         computeGain(stats, imageMetadata, gain, targetY);
>         /* Now compute the target (final) exposure which we think we want. */
>         computeTargetExposure(gain);
> +       /* The results have to be filtered so as not to change too rapidly. */
> +       filterExposure();
>         /*
>          * Some of the exposure has to be applied as digital gain, so work out
>          * what that is. This function also tells us whether it's decided to
>          * "desaturate" the image more quickly.
>          */
>         bool desaturate = applyDigitalGain(gain, targetY);
> -       /* The results have to be filtered so as not to change too rapidly. */
> -       filterExposure(desaturate);
>         /*
>          * The last thing is to divide up the exposure value into a shutter time
>          * and analogue gain, according to the current exposure mode.
> @@ -757,12 +757,12 @@ bool Agc::applyDigitalGain(double gain, double targetY)
>         if (desaturate)
>                 dg /= config_.fastReduceThreshold;
>         LOG(RPiAgc, Debug) << "Digital gain " << dg << " desaturate? " << desaturate;
> -       target_.totalExposureNoDG = target_.totalExposure / dg;
> -       LOG(RPiAgc, Debug) << "Target totalExposureNoDG " << target_.totalExposureNoDG;
> +       filtered_.totalExposureNoDG = filtered_.totalExposure / dg;
> +       LOG(RPiAgc, Debug) << "Target totalExposureNoDG " << filtered_.totalExposureNoDG;
>         return desaturate;
>  }
>
> -void Agc::filterExposure(bool desaturate)
> +void Agc::filterExposure()
>  {
>         double speed = config_.speed;
>         /*
> @@ -774,7 +774,6 @@ void Agc::filterExposure(bool desaturate)
>                 speed = 1.0;
>         if (!filtered_.totalExposure) {
>                 filtered_.totalExposure = target_.totalExposure;
> -               filtered_.totalExposureNoDG = target_.totalExposureNoDG;
>         } else {
>                 /*
>                  * If close to the result go faster, to save making so many
> @@ -785,26 +784,7 @@ void Agc::filterExposure(bool desaturate)
>                         speed = sqrt(speed);
>                 filtered_.totalExposure = speed * target_.totalExposure +
>                                           filtered_.totalExposure * (1.0 - speed);
> -               /*
> -                * When desaturing, take a big jump down in totalExposureNoDG,
> -                * which we'll hide with digital gain.
> -                */
> -               if (desaturate)
> -                       filtered_.totalExposureNoDG =
> -                               target_.totalExposureNoDG;
> -               else
> -                       filtered_.totalExposureNoDG =
> -                               speed * target_.totalExposureNoDG +
> -                               filtered_.totalExposureNoDG * (1.0 - speed);
>         }
> -       /*
> -        * We can't let the totalExposureNoDG exposure deviate too far below the
> -        * total exposure, as there might not be enough digital gain available
> -        * in the ISP to hide it (which will cause nasty oscillation).
> -        */
> -       if (filtered_.totalExposureNoDG <
> -           filtered_.totalExposure * config_.fastReduceThreshold)
> -               filtered_.totalExposureNoDG = filtered_.totalExposure * config_.fastReduceThreshold;
>         LOG(RPiAgc, Debug) << "After filtering, totalExposure " << filtered_.totalExposure
>                            << " no dg " << filtered_.totalExposureNoDG;
>  }
> diff --git a/src/ipa/rpi/controller/rpi/agc.h b/src/ipa/rpi/controller/rpi/agc.h
> index 939f9729..b7122de3 100644
> --- a/src/ipa/rpi/controller/rpi/agc.h
> +++ b/src/ipa/rpi/controller/rpi/agc.h
> @@ -93,8 +93,8 @@ private:
>         void computeGain(StatisticsPtr &statistics, Metadata *imageMetadata,
>                          double &gain, double &targetY);
>         void computeTargetExposure(double gain);
> +       void filterExposure();
>         bool applyDigitalGain(double gain, double targetY);
> -       void filterExposure(bool desaturate);
>         void divideUpExposure();
>         void writeAndFinish(Metadata *imageMetadata, bool desaturate);
>         libcamera::utils::Duration limitShutter(libcamera::utils::Duration shutter);
> --
> 2.30.2
>