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

Message ID 20211119210239.18540-4-laurent.pinchart@ideasonboard.com
State Accepted
Commit 6e02f674578e3c40ab3f140b25422ed195e4fc28
Headers show
Series
  • ipa: ipu3: agc: Misc improvements
Related show

Commit Message

Laurent Pinchart Nov. 19, 2021, 9:02 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. 22, 2021, 6:42 a.m. UTC | #1 
Hi Laurent,

On 19/11/2021 22:02, 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".
> 
> - 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>
Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@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 9cd2ded501ed..2d196fd63c7e 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 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_;
>   
>
Kieran Bingham Nov. 22, 2021, 2:48 p.m. UTC | #2 
Quoting Laurent Pinchart (2021-11-19 21:02:37)
> 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.

clang-tidy seems to have extra methods to be able to validate variable
name 'style's. Including the ability to ensure things like a postfix on
private member variables.

clang-tidy requires going through the compiler though, so needs a build
- and might not be so easy to integrate to checkstyle. (It's also a lot
slower, so more appropriate for some integration time, or server side
checks).

anyway,

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

> 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 9cd2ded501ed..2d196fd63c7e 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 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_;
>  
> -- 
> Regards,
> 
> Laurent Pinchart
>

Patch
diff mbox series 

diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
index 9cd2ded501ed..2d196fd63c7e 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 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_;