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

[Laurent Pinchart]

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

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

-- 
Regards,

Laurent Pinchart