[v3,1/5] ipa: rkisp1: agc: Wrap variable length C arrays in spans
diff mbox series

Message ID 20240218164908.15921-2-laurent.pinchart@ideasonboard.com
State Accepted
Commit 971c4904ff91e3525fb807567fdb4408b15df00e
Headers show
Series
  • ipa: rkisp1: Support the i.MX8MP ISP
Related show

Commit Message

Laurent Pinchart Feb. 18, 2024, 4:49 p.m. UTC 
The RkISP1 statistics structure contains multiple arrays whose length
varies depending on the hardware revision. Accessing those arrays is
error-prone, wrap them in spans at the top level to reduce risks of
out-of-bound accesses.

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

Comments

Paul Elder Feb. 20, 2024, 9:50 a.m. UTC | #1 
On Sun, Feb 18, 2024 at 06:49:04PM +0200, Laurent Pinchart wrote:
> The RkISP1 statistics structure contains multiple arrays whose length
> varies depending on the hardware revision. Accessing those arrays is
> error-prone, wrap them in spans at the top level to reduce risks of
> out-of-bound accesses.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
>  src/ipa/rkisp1/algorithms/agc.cpp | 27 +++++++++++++--------------
>  src/ipa/rkisp1/algorithms/agc.h   |  5 +++--
>  2 files changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index e5aeb3426eff..f83a9ba1686a 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -186,8 +186,8 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,
>  	/* Produce the luminance histogram. */
>  	params->meas.hst_config.mode = RKISP1_CIF_ISP_HISTOGRAM_MODE_Y_HISTOGRAM;
>  	/* Set an average weighted histogram. */
> -	for (unsigned int histBin = 0; histBin < numHistBins_; histBin++)
> -		params->meas.hst_config.hist_weight[histBin] = 1;
> +	Span<uint8_t> weights{ params->meas.hst_config.hist_weight, numHistBins_ };
> +	std::fill(weights.begin(), weights.end(), 1);
>  	/* Step size can't be less than 3. */
>  	params->meas.hst_config.histogram_predivider = 4;
>  
> @@ -318,7 +318,7 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,
>  
>  /**
>   * \brief Estimate the relative luminance of the frame with a given gain
> - * \param[in] ae The RkISP1 statistics and ISP results
> + * \param[in] expMeans The mean luminance values, from the RkISP1 statistics
>   * \param[in] gain The gain to apply to the frame
>   *
>   * This function estimates the average relative luminance of the frame that
> @@ -342,28 +342,27 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,
>   *
>   * \return The relative luminance
>   */
> -double Agc::estimateLuminance(const rkisp1_cif_isp_ae_stat *ae,
> -			      double gain)
> +double Agc::estimateLuminance(Span<const uint8_t> expMeans, double gain)
>  {
>  	double ySum = 0.0;
>  
>  	/* Sum the averages, saturated to 255. */
> -	for (unsigned int aeCell = 0; aeCell < numCells_; aeCell++)
> -		ySum += std::min(ae->exp_mean[aeCell] * gain, 255.0);
> +	for (uint8_t expMean : expMeans)
> +		ySum += std::min(expMean * gain, 255.0);
>  
>  	/* \todo Weight with the AWB gains */
>  
> -	return ySum / numCells_ / 255;
> +	return ySum / expMeans.size() / 255;
>  }
>  
>  /**
>   * \brief Estimate the mean value of the top 2% of the histogram
> - * \param[in] hist The histogram statistics computed by the ImgU
> + * \param[in] hist The histogram statistics computed by the RkISP1
>   * \return The mean value of the top 2% of the histogram
>   */
> -double Agc::measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const
> +double Agc::measureBrightness(Span<const uint32_t> hist) const
>  {
> -	Histogram histogram{ Span<const uint32_t>(hist->hist_bins, numHistBins_) };
> +	Histogram histogram{ hist };
>  	/* Estimate the quantile mean of the top 2% of the histogram. */
>  	return histogram.interQuantileMean(0.98, 1.0);
>  }
> @@ -415,11 +414,11 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>  	const rkisp1_cif_isp_stat *params = &stats->params;
>  	ASSERT(stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP);
>  
> -	const rkisp1_cif_isp_ae_stat *ae = &params->ae;
> -	const rkisp1_cif_isp_hist_stat *hist = &params->hist;
> +	Span<const uint8_t> ae{ params->ae.exp_mean, numCells_ };
> +	Span<const uint32_t> hist{ params->hist.hist_bins, numHistBins_ };
>  
>  	double iqMean = measureBrightness(hist);
> -	double iqMeanGain = kEvGainTarget * numHistBins_ / iqMean;
> +	double iqMeanGain = kEvGainTarget * hist.size() / iqMean;
>  
>  	/*
>  	 * Estimate the gain needed to achieve a relative luminance target. To
> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h
> index 8a22263741b6..ce8594f393ab 100644
> --- a/src/ipa/rkisp1/algorithms/agc.h
> +++ b/src/ipa/rkisp1/algorithms/agc.h
> @@ -9,6 +9,7 @@
>  
>  #include <linux/rkisp1-config.h>
>  
> +#include <libcamera/base/span.h>
>  #include <libcamera/base/utils.h>
>  
>  #include <libcamera/geometry.h>
> @@ -42,8 +43,8 @@ private:
>  	void computeExposure(IPAContext &Context, IPAFrameContext &frameContext,
>  			     double yGain, double iqMeanGain);
>  	utils::Duration filterExposure(utils::Duration exposureValue);
> -	double estimateLuminance(const rkisp1_cif_isp_ae_stat *ae, double gain);
> -	double measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const;
> +	double estimateLuminance(Span<const uint8_t> expMeans, double gain);
> +	double measureBrightness(Span<const uint32_t> hist) const;
>  	void fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
>  			  ControlList &metadata);
>  
> -- 
> Regards,
> 
> Laurent Pinchart
>
Stefan Klug Feb. 23, 2024, 12:53 p.m. UTC | #2 
Am 18.02.24 um 17:49 schrieb Laurent Pinchart:
> The RkISP1 statistics structure contains multiple arrays whose length
> varies depending on the hardware revision. Accessing those arrays is
> error-prone, wrap them in spans at the top level to reduce risks of
> out-of-bound accesses.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>

> ---
>   src/ipa/rkisp1/algorithms/agc.cpp | 27 +++++++++++++--------------
>   src/ipa/rkisp1/algorithms/agc.h   |  5 +++--
>   2 files changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index e5aeb3426eff..f83a9ba1686a 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -186,8 +186,8 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,
>   	/* Produce the luminance histogram. */
>   	params->meas.hst_config.mode = RKISP1_CIF_ISP_HISTOGRAM_MODE_Y_HISTOGRAM;
>   	/* Set an average weighted histogram. */
> -	for (unsigned int histBin = 0; histBin < numHistBins_; histBin++)
> -		params->meas.hst_config.hist_weight[histBin] = 1;
> +	Span<uint8_t> weights{ params->meas.hst_config.hist_weight, numHistBins_ };
> +	std::fill(weights.begin(), weights.end(), 1);
>   	/* Step size can't be less than 3. */
>   	params->meas.hst_config.histogram_predivider = 4;
>   
> @@ -318,7 +318,7 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,
>   
>   /**
>    * \brief Estimate the relative luminance of the frame with a given gain
> - * \param[in] ae The RkISP1 statistics and ISP results
> + * \param[in] expMeans The mean luminance values, from the RkISP1 statistics
>    * \param[in] gain The gain to apply to the frame
>    *
>    * This function estimates the average relative luminance of the frame that
> @@ -342,28 +342,27 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,
>    *
>    * \return The relative luminance
>    */
> -double Agc::estimateLuminance(const rkisp1_cif_isp_ae_stat *ae,
> -			      double gain)
> +double Agc::estimateLuminance(Span<const uint8_t> expMeans, double gain)
>   {
>   	double ySum = 0.0;
>   
>   	/* Sum the averages, saturated to 255. */
> -	for (unsigned int aeCell = 0; aeCell < numCells_; aeCell++)
> -		ySum += std::min(ae->exp_mean[aeCell] * gain, 255.0);
> +	for (uint8_t expMean : expMeans)
> +		ySum += std::min(expMean * gain, 255.0);
>   
>   	/* \todo Weight with the AWB gains */
>   
> -	return ySum / numCells_ / 255;
> +	return ySum / expMeans.size() / 255;
>   }
>   
>   /**
>    * \brief Estimate the mean value of the top 2% of the histogram
> - * \param[in] hist The histogram statistics computed by the ImgU
> + * \param[in] hist The histogram statistics computed by the RkISP1
>    * \return The mean value of the top 2% of the histogram
>    */
> -double Agc::measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const
> +double Agc::measureBrightness(Span<const uint32_t> hist) const
>   {
> -	Histogram histogram{ Span<const uint32_t>(hist->hist_bins, numHistBins_) };
> +	Histogram histogram{ hist };
>   	/* Estimate the quantile mean of the top 2% of the histogram. */
>   	return histogram.interQuantileMean(0.98, 1.0);
>   }
> @@ -415,11 +414,11 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>   	const rkisp1_cif_isp_stat *params = &stats->params;
>   	ASSERT(stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP);
>   
> -	const rkisp1_cif_isp_ae_stat *ae = &params->ae;
> -	const rkisp1_cif_isp_hist_stat *hist = &params->hist;
> +	Span<const uint8_t> ae{ params->ae.exp_mean, numCells_ };
> +	Span<const uint32_t> hist{ params->hist.hist_bins, numHistBins_ };
>   
>   	double iqMean = measureBrightness(hist);
> -	double iqMeanGain = kEvGainTarget * numHistBins_ / iqMean;
> +	double iqMeanGain = kEvGainTarget * hist.size() / iqMean;
>   
>   	/*
>   	 * Estimate the gain needed to achieve a relative luminance target. To
> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h
> index 8a22263741b6..ce8594f393ab 100644
> --- a/src/ipa/rkisp1/algorithms/agc.h
> +++ b/src/ipa/rkisp1/algorithms/agc.h
> @@ -9,6 +9,7 @@
>   
>   #include <linux/rkisp1-config.h>
>   
> +#include <libcamera/base/span.h>
>   #include <libcamera/base/utils.h>
>   
>   #include <libcamera/geometry.h>
> @@ -42,8 +43,8 @@ private:
>   	void computeExposure(IPAContext &Context, IPAFrameContext &frameContext,
>   			     double yGain, double iqMeanGain);
>   	utils::Duration filterExposure(utils::Duration exposureValue);
> -	double estimateLuminance(const rkisp1_cif_isp_ae_stat *ae, double gain);
> -	double measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const;
> +	double estimateLuminance(Span<const uint8_t> expMeans, double gain);
> +	double measureBrightness(Span<const uint32_t> hist) const;
>   	void fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
>   			  ControlList &metadata);
>

Patch
diff mbox series 

diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
index e5aeb3426eff..f83a9ba1686a 100644
--- a/src/ipa/rkisp1/algorithms/agc.cpp
+++ b/src/ipa/rkisp1/algorithms/agc.cpp
@@ -186,8 +186,8 @@  void Agc::prepare(IPAContext &context, const uint32_t frame,
 	/* Produce the luminance histogram. */
 	params->meas.hst_config.mode = RKISP1_CIF_ISP_HISTOGRAM_MODE_Y_HISTOGRAM;
 	/* Set an average weighted histogram. */
-	for (unsigned int histBin = 0; histBin < numHistBins_; histBin++)
-		params->meas.hst_config.hist_weight[histBin] = 1;
+	Span<uint8_t> weights{ params->meas.hst_config.hist_weight, numHistBins_ };
+	std::fill(weights.begin(), weights.end(), 1);
 	/* Step size can't be less than 3. */
 	params->meas.hst_config.histogram_predivider = 4;
 
@@ -318,7 +318,7 @@  void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,
 
 /**
  * \brief Estimate the relative luminance of the frame with a given gain
- * \param[in] ae The RkISP1 statistics and ISP results
+ * \param[in] expMeans The mean luminance values, from the RkISP1 statistics
  * \param[in] gain The gain to apply to the frame
  *
  * This function estimates the average relative luminance of the frame that
@@ -342,28 +342,27 @@  void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,
  *
  * \return The relative luminance
  */
-double Agc::estimateLuminance(const rkisp1_cif_isp_ae_stat *ae,
-			      double gain)
+double Agc::estimateLuminance(Span<const uint8_t> expMeans, double gain)
 {
 	double ySum = 0.0;
 
 	/* Sum the averages, saturated to 255. */
-	for (unsigned int aeCell = 0; aeCell < numCells_; aeCell++)
-		ySum += std::min(ae->exp_mean[aeCell] * gain, 255.0);
+	for (uint8_t expMean : expMeans)
+		ySum += std::min(expMean * gain, 255.0);
 
 	/* \todo Weight with the AWB gains */
 
-	return ySum / numCells_ / 255;
+	return ySum / expMeans.size() / 255;
 }
 
 /**
  * \brief Estimate the mean value of the top 2% of the histogram
- * \param[in] hist The histogram statistics computed by the ImgU
+ * \param[in] hist The histogram statistics computed by the RkISP1
  * \return The mean value of the top 2% of the histogram
  */
-double Agc::measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const
+double Agc::measureBrightness(Span<const uint32_t> hist) const
 {
-	Histogram histogram{ Span<const uint32_t>(hist->hist_bins, numHistBins_) };
+	Histogram histogram{ hist };
 	/* Estimate the quantile mean of the top 2% of the histogram. */
 	return histogram.interQuantileMean(0.98, 1.0);
 }
@@ -415,11 +414,11 @@  void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
 	const rkisp1_cif_isp_stat *params = &stats->params;
 	ASSERT(stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP);
 
-	const rkisp1_cif_isp_ae_stat *ae = &params->ae;
-	const rkisp1_cif_isp_hist_stat *hist = &params->hist;
+	Span<const uint8_t> ae{ params->ae.exp_mean, numCells_ };
+	Span<const uint32_t> hist{ params->hist.hist_bins, numHistBins_ };
 
 	double iqMean = measureBrightness(hist);
-	double iqMeanGain = kEvGainTarget * numHistBins_ / iqMean;
+	double iqMeanGain = kEvGainTarget * hist.size() / iqMean;
 
 	/*
 	 * Estimate the gain needed to achieve a relative luminance target. To
diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h
index 8a22263741b6..ce8594f393ab 100644
--- a/src/ipa/rkisp1/algorithms/agc.h
+++ b/src/ipa/rkisp1/algorithms/agc.h
@@ -9,6 +9,7 @@ 
 
 #include <linux/rkisp1-config.h>
 
+#include <libcamera/base/span.h>
 #include <libcamera/base/utils.h>
 
 #include <libcamera/geometry.h>
@@ -42,8 +43,8 @@  private:
 	void computeExposure(IPAContext &Context, IPAFrameContext &frameContext,
 			     double yGain, double iqMeanGain);
 	utils::Duration filterExposure(utils::Duration exposureValue);
-	double estimateLuminance(const rkisp1_cif_isp_ae_stat *ae, double gain);
-	double measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const;
+	double estimateLuminance(Span<const uint8_t> expMeans, double gain);
+	double measureBrightness(Span<const uint32_t> hist) const;
 	void fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
 			  ControlList &metadata);