[libcamera-devel,3/5] ipa: ipu3: agc: Rename currentYGain
diff mbox series

Message ID 20211116162615.27777-4-laurent.pinchart@ideasonboard.com
State Superseded
Headers show
Series
  • ipa: ipu3: agc: Misc improvements
Related show

Commit Message

Laurent Pinchart Nov. 16, 2021, 4:26 p.m. UTC 
The "current" prefix in the currentYGain variable name is confusing:

- In Agc::estimateLuminance(), the variable contains the gain to be
  applied to the image, which is neither a "current" gain nor a "Y"
  gain. Rename it to "gain".

- In Agc::computeExposure(), the variable contains the gain computed by
  the relative luminance method, so rename it to "yGain".

While at it, rename variables to match the libcamera coding style.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/ipa/ipu3/algorithms/agc.cpp | 32 ++++++++++++++++----------------
 src/ipa/ipu3/algorithms/agc.h   |  4 ++--
 2 files changed, 18 insertions(+), 18 deletions(-)

Comments

Jean-Michel Hautbois Nov. 17, 2021, 10:21 a.m. UTC | #1 
Hi Laurent,

Thanks for the patch !

On 16/11/2021 17:26, Laurent Pinchart wrote:
> The "current" prefix in the currentYGain variable name is confusing:
> 
> - In Agc::estimateLuminance(), the variable contains the gain to be
>    applied to the image, which is neither a "current" gain nor a "Y"
>    gain. Rename it to "gain".

It is the gain to apply for the image to be at the desired relative 
luminance level.

> 
> - In Agc::computeExposure(), the variable contains the gain computed by
>    the relative luminance method, so rename it to "yGain".
> 
> While at it, rename variables to match the libcamera coding style.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>   src/ipa/ipu3/algorithms/agc.cpp | 32 ++++++++++++++++----------------
>   src/ipa/ipu3/algorithms/agc.h   |  4 ++--
>   2 files changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index d5840fb0aa97..6aab9fd5ebb5 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -173,9 +173,9 @@ void Agc::filterExposure()
>   /**
>    * \brief Estimate the new exposure and gain values
>    * \param[inout] frameContext The shared IPA frame Context
> - * \param[in] currentYGain The gain calculated on the current brightness level
> + * \param[in] yGain The gain calculated based on the relative luminance target
>    */
> -void Agc::computeExposure(IPAFrameContext &frameContext, double currentYGain)
> +void Agc::computeExposure(IPAFrameContext &frameContext, double yGain)
>   {
>   	/* Get the effective exposure and gain applied on the sensor. */
>   	uint32_t exposure = frameContext.sensor.exposure;
> @@ -189,8 +189,8 @@ void Agc::computeExposure(IPAFrameContext &frameContext, double currentYGain)
>   	 */
>   	double evGain = kEvGainTarget * knumHistogramBins / iqMean_;
>   
> -	if (evGain < currentYGain)
> -		evGain = currentYGain;
> +	if (evGain < yGain)
> +		evGain = yGain;
>   
>   	/* Consider within 1% of the target as correctly exposed */
>   	if (std::abs(evGain - 1.0) < 0.01)
> @@ -254,7 +254,7 @@ void Agc::computeExposure(IPAFrameContext &frameContext, double currentYGain)
>    * \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
> + * \param[in] gain The analogue gain to apply to the frame

I don't think so, it is a global gain to apply, not only the analogue 
gain. The goal is to get as closed as possible to a target. This gain 
will then be splitted into shutter speed and analogue gain.

>    * \return The relative luminance
>    *
>    * Luma is the weighted sum of gamma-compressed R′G′B′ components of a color
> @@ -268,7 +268,7 @@ void Agc::computeExposure(IPAFrameContext &frameContext, double currentYGain)
>   double Agc::estimateLuminance(IPAFrameContext &frameContext,
>   			      const ipu3_uapi_grid_config &grid,
>   			      const ipu3_uapi_stats_3a *stats,
> -			      double currentYGain)
> +			      double gain)
>   {
>   	double redSum = 0, greenSum = 0, blueSum = 0;
>   
> @@ -281,9 +281,9 @@ double Agc::estimateLuminance(IPAFrameContext &frameContext,
>   					&stats->awb_raw_buffer.meta_data[cellPosition]
>   				);
>   
> -			redSum += cell->R_avg * currentYGain;
> -			greenSum += (cell->Gr_avg + cell->Gb_avg) / 2 * currentYGain;
> -			blueSum += cell->B_avg * currentYGain;
> +			redSum += cell->R_avg * gain;
> +			greenSum += (cell->Gr_avg + cell->Gb_avg) / 2 * gain;
> +			blueSum += cell->B_avg * gain;
>   		}
>   	}
>   
> @@ -310,7 +310,7 @@ void Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
>   {
>   	measureBrightness(stats, context.configuration.grid.bdsGrid);
>   
> -	double currentYGain = 1.0;
> +	double yGain = 1.0;
>   	double yTarget = kRelativeLuminanceTarget;
>   
>   	/*
> @@ -320,18 +320,18 @@ void Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
>   	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, yTarget / (yValue + .001));
> +						  stats, yGain);
> +		double extraGain = std::min(10.0, yTarget / (yValue + .001));
>   
> -		currentYGain *= extra_gain;
> +		yGain *= extraGain;
>   		LOG(IPU3Agc, Debug) << "Y value: " << yValue
>   				    << ", Y target: " << yTarget
> -				    << ", gives gain " << currentYGain;
> -		if (extra_gain < 1.01)
> +				    << ", gives gain " << yGain;
> +		if (extraGain < 1.01)
>   			break;
>   	}
>   
> -	computeExposure(context.frameContext, currentYGain);
> +	computeExposure(context.frameContext, yGain);
>   	frameCount_++;
>   }
>   
> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
> index 943c354a820e..0c868d6737f1 100644
> --- a/src/ipa/ipu3/algorithms/agc.h
> +++ b/src/ipa/ipu3/algorithms/agc.h
> @@ -34,11 +34,11 @@ private:
>   	void measureBrightness(const ipu3_uapi_stats_3a *stats,
>   			       const ipu3_uapi_grid_config &grid);
>   	void filterExposure();
> -	void computeExposure(IPAFrameContext &frameContext, double currentYGain);
> +	void computeExposure(IPAFrameContext &frameContext, double yGain);
>   	double estimateLuminance(IPAFrameContext &frameContext,
>   				 const ipu3_uapi_grid_config &grid,
>   				 const ipu3_uapi_stats_3a *stats,
> -				 double currentYGain);
> +				 double gain);
>   
>   	uint64_t frameCount_;
>   
>
Laurent Pinchart Nov. 17, 2021, 10:34 a.m. UTC | #2 
Hi Jean-Michel,

On Wed, Nov 17, 2021 at 11:21:10AM +0100, Jean-Michel Hautbois wrote:
> On 16/11/2021 17:26, Laurent Pinchart wrote:
> > The "current" prefix in the currentYGain variable name is confusing:
> > 
> > - In Agc::estimateLuminance(), the variable contains the gain to be
> >    applied to the image, which is neither a "current" gain nor a "Y"
> >    gain. Rename it to "gain".
> 
> It is the gain to apply for the image to be at the desired relative 
> luminance level.

Not exactly. The estimateLuminance() function estimates the luminance of
the image that would have been produced if that extra gain had been
applied. The caller uses an iterative approach to try and find the gain
that will result in a given average relative luminance, but from the
point of view of the estimateLuminance() function, that's not relevant.

> > - In Agc::computeExposure(), the variable contains the gain computed by
> >    the relative luminance method, so rename it to "yGain".
> > 
> > While at it, rename variables to match the libcamera coding style.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >   src/ipa/ipu3/algorithms/agc.cpp | 32 ++++++++++++++++----------------
> >   src/ipa/ipu3/algorithms/agc.h   |  4 ++--
> >   2 files changed, 18 insertions(+), 18 deletions(-)
> > 
> > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> > index d5840fb0aa97..6aab9fd5ebb5 100644
> > --- a/src/ipa/ipu3/algorithms/agc.cpp
> > +++ b/src/ipa/ipu3/algorithms/agc.cpp
> > @@ -173,9 +173,9 @@ void Agc::filterExposure()
> >   /**
> >    * \brief Estimate the new exposure and gain values
> >    * \param[inout] frameContext The shared IPA frame Context
> > - * \param[in] currentYGain The gain calculated on the current brightness level
> > + * \param[in] yGain The gain calculated based on the relative luminance target
> >    */
> > -void Agc::computeExposure(IPAFrameContext &frameContext, double currentYGain)
> > +void Agc::computeExposure(IPAFrameContext &frameContext, double yGain)
> >   {
> >   	/* Get the effective exposure and gain applied on the sensor. */
> >   	uint32_t exposure = frameContext.sensor.exposure;
> > @@ -189,8 +189,8 @@ void Agc::computeExposure(IPAFrameContext &frameContext, double currentYGain)
> >   	 */
> >   	double evGain = kEvGainTarget * knumHistogramBins / iqMean_;
> >   
> > -	if (evGain < currentYGain)
> > -		evGain = currentYGain;
> > +	if (evGain < yGain)
> > +		evGain = yGain;
> >   
> >   	/* Consider within 1% of the target as correctly exposed */
> >   	if (std::abs(evGain - 1.0) < 0.01)
> > @@ -254,7 +254,7 @@ void Agc::computeExposure(IPAFrameContext &frameContext, double currentYGain)
> >    * \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
> > + * \param[in] gain The analogue gain to apply to the frame
> 
> I don't think so, it is a global gain to apply, not only the analogue 
> gain. The goal is to get as closed as possible to a target. This gain 
> will then be splitted into shutter speed and analogue gain.

Indeed. I'll replace that to "The gain to apply to the frame".

We probably need a patch on top to use different names for "global
gains" (before they're split in analogue gain and integration time, and
digital gain in the future), and analogue/digital gains.

> >    * \return The relative luminance
> >    *
> >    * Luma is the weighted sum of gamma-compressed R′G′B′ components of a color
> > @@ -268,7 +268,7 @@ void Agc::computeExposure(IPAFrameContext &frameContext, double currentYGain)
> >   double Agc::estimateLuminance(IPAFrameContext &frameContext,
> >   			      const ipu3_uapi_grid_config &grid,
> >   			      const ipu3_uapi_stats_3a *stats,
> > -			      double currentYGain)
> > +			      double gain)
> >   {
> >   	double redSum = 0, greenSum = 0, blueSum = 0;
> >   
> > @@ -281,9 +281,9 @@ double Agc::estimateLuminance(IPAFrameContext &frameContext,
> >   					&stats->awb_raw_buffer.meta_data[cellPosition]
> >   				);
> >   
> > -			redSum += cell->R_avg * currentYGain;
> > -			greenSum += (cell->Gr_avg + cell->Gb_avg) / 2 * currentYGain;
> > -			blueSum += cell->B_avg * currentYGain;
> > +			redSum += cell->R_avg * gain;
> > +			greenSum += (cell->Gr_avg + cell->Gb_avg) / 2 * gain;
> > +			blueSum += cell->B_avg * gain;
> >   		}
> >   	}
> >   
> > @@ -310,7 +310,7 @@ void Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
> >   {
> >   	measureBrightness(stats, context.configuration.grid.bdsGrid);
> >   
> > -	double currentYGain = 1.0;
> > +	double yGain = 1.0;
> >   	double yTarget = kRelativeLuminanceTarget;
> >   
> >   	/*
> > @@ -320,18 +320,18 @@ void Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
> >   	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, yTarget / (yValue + .001));
> > +						  stats, yGain);
> > +		double extraGain = std::min(10.0, yTarget / (yValue + .001));
> >   
> > -		currentYGain *= extra_gain;
> > +		yGain *= extraGain;
> >   		LOG(IPU3Agc, Debug) << "Y value: " << yValue
> >   				    << ", Y target: " << yTarget
> > -				    << ", gives gain " << currentYGain;
> > -		if (extra_gain < 1.01)
> > +				    << ", gives gain " << yGain;
> > +		if (extraGain < 1.01)
> >   			break;
> >   	}
> >   
> > -	computeExposure(context.frameContext, currentYGain);
> > +	computeExposure(context.frameContext, yGain);
> >   	frameCount_++;
> >   }
> >   
> > diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
> > index 943c354a820e..0c868d6737f1 100644
> > --- a/src/ipa/ipu3/algorithms/agc.h
> > +++ b/src/ipa/ipu3/algorithms/agc.h
> > @@ -34,11 +34,11 @@ private:
> >   	void measureBrightness(const ipu3_uapi_stats_3a *stats,
> >   			       const ipu3_uapi_grid_config &grid);
> >   	void filterExposure();
> > -	void computeExposure(IPAFrameContext &frameContext, double currentYGain);
> > +	void computeExposure(IPAFrameContext &frameContext, double yGain);
> >   	double estimateLuminance(IPAFrameContext &frameContext,
> >   				 const ipu3_uapi_grid_config &grid,
> >   				 const ipu3_uapi_stats_3a *stats,
> > -				 double currentYGain);
> > +				 double gain);
> >   
> >   	uint64_t frameCount_;
> >   
> >

Patch
diff mbox series 

diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
index d5840fb0aa97..6aab9fd5ebb5 100644
--- a/src/ipa/ipu3/algorithms/agc.cpp
+++ b/src/ipa/ipu3/algorithms/agc.cpp
@@ -173,9 +173,9 @@  void Agc::filterExposure()
 /**
  * \brief Estimate the new exposure and gain values
  * \param[inout] frameContext The shared IPA frame Context
- * \param[in] currentYGain The gain calculated on the current brightness level
+ * \param[in] yGain The gain calculated based on the relative luminance target
  */
-void Agc::computeExposure(IPAFrameContext &frameContext, double currentYGain)
+void Agc::computeExposure(IPAFrameContext &frameContext, double yGain)
 {
 	/* Get the effective exposure and gain applied on the sensor. */
 	uint32_t exposure = frameContext.sensor.exposure;
@@ -189,8 +189,8 @@  void Agc::computeExposure(IPAFrameContext &frameContext, double currentYGain)
 	 */
 	double evGain = kEvGainTarget * knumHistogramBins / iqMean_;
 
-	if (evGain < currentYGain)
-		evGain = currentYGain;
+	if (evGain < yGain)
+		evGain = yGain;
 
 	/* Consider within 1% of the target as correctly exposed */
 	if (std::abs(evGain - 1.0) < 0.01)
@@ -254,7 +254,7 @@  void Agc::computeExposure(IPAFrameContext &frameContext, double currentYGain)
  * \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
+ * \param[in] gain The analogue gain to apply to the frame
  * \return The relative luminance
  *
  * Luma is the weighted sum of gamma-compressed R′G′B′ components of a color
@@ -268,7 +268,7 @@  void Agc::computeExposure(IPAFrameContext &frameContext, double currentYGain)
 double Agc::estimateLuminance(IPAFrameContext &frameContext,
 			      const ipu3_uapi_grid_config &grid,
 			      const ipu3_uapi_stats_3a *stats,
-			      double currentYGain)
+			      double gain)
 {
 	double redSum = 0, greenSum = 0, blueSum = 0;
 
@@ -281,9 +281,9 @@  double Agc::estimateLuminance(IPAFrameContext &frameContext,
 					&stats->awb_raw_buffer.meta_data[cellPosition]
 				);
 
-			redSum += cell->R_avg * currentYGain;
-			greenSum += (cell->Gr_avg + cell->Gb_avg) / 2 * currentYGain;
-			blueSum += cell->B_avg * currentYGain;
+			redSum += cell->R_avg * gain;
+			greenSum += (cell->Gr_avg + cell->Gb_avg) / 2 * gain;
+			blueSum += cell->B_avg * gain;
 		}
 	}
 
@@ -310,7 +310,7 @@  void Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
 {
 	measureBrightness(stats, context.configuration.grid.bdsGrid);
 
-	double currentYGain = 1.0;
+	double yGain = 1.0;
 	double yTarget = kRelativeLuminanceTarget;
 
 	/*
@@ -320,18 +320,18 @@  void Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
 	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, yTarget / (yValue + .001));
+						  stats, yGain);
+		double extraGain = std::min(10.0, yTarget / (yValue + .001));
 
-		currentYGain *= extra_gain;
+		yGain *= extraGain;
 		LOG(IPU3Agc, Debug) << "Y value: " << yValue
 				    << ", Y target: " << yTarget
-				    << ", gives gain " << currentYGain;
-		if (extra_gain < 1.01)
+				    << ", gives gain " << yGain;
+		if (extraGain < 1.01)
 			break;
 	}
 
-	computeExposure(context.frameContext, currentYGain);
+	computeExposure(context.frameContext, yGain);
 	frameCount_++;
 }
 
diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
index 943c354a820e..0c868d6737f1 100644
--- a/src/ipa/ipu3/algorithms/agc.h
+++ b/src/ipa/ipu3/algorithms/agc.h
@@ -34,11 +34,11 @@  private:
 	void measureBrightness(const ipu3_uapi_stats_3a *stats,
 			       const ipu3_uapi_grid_config &grid);
 	void filterExposure();
-	void computeExposure(IPAFrameContext &frameContext, double currentYGain);
+	void computeExposure(IPAFrameContext &frameContext, double yGain);
 	double estimateLuminance(IPAFrameContext &frameContext,
 				 const ipu3_uapi_grid_config &grid,
 				 const ipu3_uapi_stats_3a *stats,
-				 double currentYGain);
+				 double gain);
 
 	uint64_t frameCount_;