[libcamera-devel] [PATCH 2/5] ipa: ipu3: agc: Standardize vocabulary on "relative luminance"

[Jean-Michel Hautbois]

Hi Laurent,

Thanks for the patch !

On 16/11/2021 17:26, Laurent Pinchart wrote:
> The AGC computes the average relative luminance of the frame and calls
> the value "normalized luma", "brightness" or "initialY". The latter is
> the most accurate term, as the relative luminance is abbreviated Y, but
> the "initial" prefix isn't accurate.
> 
> Standardize the vocabulary on "relative luminance" in code and comments,
> abbreviating it to Y when needed.
> 
> While at it, rename variables to match the libcamera coding style.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>   src/ipa/ipu3/algorithms/agc.cpp | 44 ++++++++++++++++-----------------
>   src/ipa/ipu3/algorithms/agc.h   |  8 +++---
>   2 files changed, 26 insertions(+), 26 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index 43a39ffd57d6..d5840fb0aa97 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -62,12 +62,12 @@ static constexpr double kEvGainTarget = 0.5;
>   static constexpr uint32_t kNumStartupFrames = 10;
>   
>   /*
> - * Normalized luma value target.
> + * Relative luminance target.
>    *
>    * It's a number that's chosen so that, when the camera points at a grey
>    * target, the resulting image brightness is considered right.
>    */
> -static constexpr double kNormalizedLumaTarget = 0.16;
> +static constexpr double kRelativeLuminanceTarget = 0.16;
>   
>   Agc::Agc()
>   	: frameCount_(0), iqMean_(0.0), lineDuration_(0s), minShutterSpeed_(0s),
> @@ -250,12 +250,12 @@ void Agc::computeExposure(IPAFrameContext &frameContext, double currentYGain)
>   }
>   
>   /**
> - * \brief Estimate the average brightness of the frame
> + * \brief Estimate the relative luminance of the frame with a given gain
>    * \param[in] frameContext The shared IPA frame context
>    * \param[in] grid The grid used to store the statistics in the IPU3
>    * \param[in] stats The IPU3 statistics and ISP results
>    * \param[in] currentYGain The gain calculated on the current brightness level
> - * \return The normalized luma
> + * \return The relative luminance
>    *
>    * Luma is the weighted sum of gamma-compressed R′G′B′ components of a color
>    * video. The luma values are normalized as 0.0 to 1.0, with 1.0 being a
> @@ -263,12 +263,12 @@ void Agc::computeExposure(IPAFrameContext &frameContext, double currentYGain)
>    * luma here.
>    *
>    * More detailed information can be found in:
> - * https://en.wikipedia.org/wiki/Luma_(video)
> + * https://en.wikipedia.org/wiki/Relative_luminance
>    */
> -double Agc::computeInitialY(IPAFrameContext &frameContext,
> -			    const ipu3_uapi_grid_config &grid,
> -			    const ipu3_uapi_stats_3a *stats,
> -			    double currentYGain)
> +double Agc::estimateLuminance(IPAFrameContext &frameContext,
> +			      const ipu3_uapi_grid_config &grid,
> +			      const ipu3_uapi_stats_3a *stats,
> +			      double currentYGain)

Shouldn't it be named estimateRelativeLuminance then ? Or is it too long 
maybe...

>   {
>   	double redSum = 0, greenSum = 0, blueSum = 0;
>   
> @@ -288,14 +288,14 @@ double Agc::computeInitialY(IPAFrameContext &frameContext,
>   	}
>   
>   	/*
> -	 * Estimate the sum of the brightness values, weighted with the gains
> -	 * applied on the channels in AWB as the Rec. 601 luma.
> +	 * Compute the relative luminance using the Rec. 601 formula, and
> +	 * normalize it.
>   	 */

Relative luminance is normalized, as it is between 0.0 and 1.0 :-).
Why did you remove the gain weights mention though ?

> -	double Y_sum = redSum * frameContext.awb.gains.red * .299 +
> -		       greenSum * frameContext.awb.gains.green * .587 +
> -		       blueSum * frameContext.awb.gains.blue * .114;
> +	double ySum = redSum * frameContext.awb.gains.red * .299 +
> +		      greenSum * frameContext.awb.gains.green * .587 +
> +		      blueSum * frameContext.awb.gains.blue * .114;
>   
> -	return Y_sum / (grid.height * grid.width) / 255;
> +	return ySum / (grid.height * grid.width) / 255;

How could we let that one... :-/

>   }
>   
>   /**
> @@ -311,22 +311,22 @@ void Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
>   	measureBrightness(stats, context.configuration.grid.bdsGrid);
>   
>   	double currentYGain = 1.0;
> -	double targetY = kNormalizedLumaTarget;
> +	double yTarget = kRelativeLuminanceTarget;
>   
>   	/*
>   	 * Do this calculation a few times as brightness increase can be
>   	 * non-linear when there are saturated regions.
>   	 */
> -	for (int i = 0; i < 8; i++) {
> -		double initialY = computeInitialY(context.frameContext,
> +	for (unsigned int i = 0; i < 8; i++) {
> +		double yValue = estimateLuminance(context.frameContext,
>   						  context.configuration.grid.bdsGrid,
>   						  stats, currentYGain);
> -		double extra_gain = std::min(10.0, targetY / (initialY + .001));
> +		double extra_gain = std::min(10.0, yTarget / (yValue + .001));
>   
>   		currentYGain *= extra_gain;
> -		LOG(IPU3Agc, Debug) << "Initial Y " << initialY
> -				    << " target " << targetY
> -				    << " gives gain " << currentYGain;
> +		LOG(IPU3Agc, Debug) << "Y value: " << yValue
> +				    << ", Y target: " << yTarget
> +				    << ", gives gain " << currentYGain;
>   		if (extra_gain < 1.01)
>   			break;
>   	}
> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
> index 31c5a6e519d4..943c354a820e 100644
> --- a/src/ipa/ipu3/algorithms/agc.h
> +++ b/src/ipa/ipu3/algorithms/agc.h
> @@ -35,10 +35,10 @@ private:
>   			       const ipu3_uapi_grid_config &grid);
>   	void filterExposure();
>   	void computeExposure(IPAFrameContext &frameContext, double currentYGain);
> -	double computeInitialY(IPAFrameContext &frameContext,
> -			       const ipu3_uapi_grid_config &grid,
> -			       const ipu3_uapi_stats_3a *stats,
> -			       double currentYGain);
> +	double estimateLuminance(IPAFrameContext &frameContext,
> +				 const ipu3_uapi_grid_config &grid,
> +				 const ipu3_uapi_stats_3a *stats,
> +				 double currentYGain);
>   
>   	uint64_t frameCount_;
>   
>