[libcamera-devel] [PATCH 1/4] ipa: ipu3: Return filtered value

[Kieran Bingham]

Quoting Jean-Michel Hautbois (2021-11-25 10:21:40)
> When the current exposure value is calculated, it is cached and used by
> filterExposure(). Use private filteredExposure_ and pass currentExposure
> as a member variable.
> 
> In order to limit the use of filteredExposure_, return the value from
> filterExposure().
> 
> While at it, remove a stale comment.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> ---
>  src/ipa/ipu3/algorithms/agc.cpp | 31 ++++++++++++++++---------------
>  src/ipa/ipu3/algorithms/agc.h   |  3 +--
>  2 files changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index 582f0ae1..2945a138 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -71,7 +71,7 @@ static constexpr double kRelativeLuminanceTarget = 0.16;
>  
>  Agc::Agc()
>         : frameCount_(0), lineDuration_(0s), minShutterSpeed_(0s),
> -         maxShutterSpeed_(0s), filteredExposure_(0s), currentExposure_(0s)
> +         maxShutterSpeed_(0s), filteredExposure_(0s)
>  {
>  }
>  
> @@ -143,7 +143,7 @@ double Agc::measureBrightness(const ipu3_uapi_stats_3a *stats,
>  /**
>   * \brief Apply a filter on the exposure value to limit the speed of changes
>   */
> -void Agc::filterExposure()
> +utils::Duration Agc::filterExposure(utils::Duration currentExposure)
>  {
>         double speed = 0.2;
>  
> @@ -152,23 +152,24 @@ void Agc::filterExposure()
>                 speed = 1.0;
>  
>         if (filteredExposure_ == 0s) {
> -               /* DG stands for digital gain.*/
> -               filteredExposure_ = currentExposure_;
> +               filteredExposure_ = currentExposure;
>         } else {
>                 /*
>                  * If we are close to the desired result, go faster to avoid making
>                  * multiple micro-adjustments.
>                  * \todo Make this customisable?
>                  */
> -               if (filteredExposure_ < 1.2 * currentExposure_ &&
> -                   filteredExposure_ > 0.8 * currentExposure_)
> +               if (filteredExposure_ < 1.2 * currentExposure &&
> +                   filteredExposure_ > 0.8 * currentExposure)
>                         speed = sqrt(speed);
>  
> -               filteredExposure_ = speed * currentExposure_ +
> +               filteredExposure_ = speed * currentExposure +
>                                 filteredExposure_ * (1.0 - speed);
>         }
>  
>         LOG(IPU3Agc, Debug) << "After filtering, total_exposure " << filteredExposure_;
> +
> +       return filteredExposure_;
>  }
>  
>  /**
> @@ -212,19 +213,19 @@ void Agc::computeExposure(IPAFrameContext &frameContext, double yGain,
>          * Calculate the current exposure value for the scene as the latest
>          * exposure value applied multiplied by the new estimated gain.
>          */
> -       currentExposure_ = effectiveExposureValue * evGain;
> +       utils::Duration currentExposure = effectiveExposureValue * evGain;
>  
>         /* Clamp the exposure value to the min and max authorized */
>         utils::Duration maxTotalExposure = maxShutterSpeed_ * maxAnalogueGain_;
> -       currentExposure_ = std::min(currentExposure_, maxTotalExposure);
> -       LOG(IPU3Agc, Debug) << "Target total exposure " << currentExposure_
> +       currentExposure = std::min(currentExposure, maxTotalExposure);
> +       LOG(IPU3Agc, Debug) << "Target total exposure " << currentExposure
>                             << ", maximum is " << maxTotalExposure;
>  
> -       /* \todo: estimate if we need to desaturate */
> -       filterExposure();
> -
> -       /* Divide the exposure value as new exposure and gain values */
> -       utils::Duration exposureValue = filteredExposure_;
> +       /*
> +        * Divide the exposure value as new exposure and gain values
> +        * \todo: estimate if we need to desaturate

While refactoring these, this might be a good time to look at those
comments. I'm not entirely sure what they mean?

"Divide the exposure value as new exposure and gain values"
Is that directly related to the filter? Or is that more about somethign
that will happen further down?

Filtering isn't dividing as a new exposure and gain ...?

And the:
 "todo: estimate if we need to desaturate"

Doesn't make sense in the context of the filter either?
The filter won't ever saturate will it? it simply slows the
response/change rate of the exposure value....


That said, refactoring those can be done as a separate patch.
/this/ patch is making the filter itself clearer, and it's only moving
existing comments.

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>


> +        */
> +       utils::Duration exposureValue = filterExposure(currentExposure);
>         utils::Duration shutterTime;
>  
>         /*
> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
> index 96ec7005..84bfe045 100644
> --- a/src/ipa/ipu3/algorithms/agc.h
> +++ b/src/ipa/ipu3/algorithms/agc.h
> @@ -33,7 +33,7 @@ public:
>  private:
>         double measureBrightness(const ipu3_uapi_stats_3a *stats,
>                                  const ipu3_uapi_grid_config &grid) const;
> -       void filterExposure();
> +       utils::Duration filterExposure(utils::Duration currentExposure);
>         void computeExposure(IPAFrameContext &frameContext, double yGain,
>                              double iqMeanGain);
>         double estimateLuminance(IPAFrameContext &frameContext,
> @@ -51,7 +51,6 @@ private:
>         double maxAnalogueGain_;
>  
>         utils::Duration filteredExposure_;
> -       utils::Duration currentExposure_;
>  
>         uint32_t stride_;
>  };
> -- 
> 2.32.0
>