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

14775 diff mbox series

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@ideasonboard.com>
---
 src/ipa/ipu3/algorithms/agc.cpp | 31 ++++++++++++++++---------------
 src/ipa/ipu3/algorithms/agc.h   |  3 +--
 2 files changed, 17 insertions(+), 17 deletions(-)

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@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@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
>

Hello,

On Thu, Nov 25, 2021 at 11:50:10AM +0000, Kieran Bingham wrote:
> 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.

s/member variable/parameter/

> > 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@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

Missing \param and \return. Kieran has proposed a good documentation for
this function in the RkISP1 IPA series.

> >   */
> > -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....

The first comment is about the process that happens after filtering. I'd
write

	utils::Duration exposureValue = effectiveExposureValue * evGain;

	/* Clamp the exposure value to the min and max authorized */
	utils::Duration maxTotalExposure = maxShutterSpeed_ * maxAnalogueGain_;
	exposureValue = std::min(exposureValue, maxTotalExposure);
	LOG(IPU3Agc, Debug) << "Target total exposure " << exposureValue
			    << ", maximum is " << maxTotalExposure;

	/*
	 * Filter the exposure.
	 * \todo: estimate if we need to desaturate
	 */
	exposureValue = filterExposure(exposureValue);

	/*
	 * Divide the exposure value as new exposure and gain values.
	 *
	 * Push the shutter time up to the maximum first, and only then
	 * increase the gain.
	 */
	utils::Duration shutterTime =
		std::clamp<utils::Duration>(exposureValue / minAnalogueGain_,
					    minShutterSpeed_, maxShutterSpeed_);

(this removes the name "currentExposure" which wasn't actually
"current".

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> 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@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_;
> >  };