[08/10] ipa: rkisp1: Add parseStatistics() to Agc
diff mbox series

Message ID 20240322131451.3092931-9-dan.scally@ideasonboard.com
State Superseded
Headers show
Series
  • Centralise Agc into libipa
Related show

Commit Message

Dan Scally March 22, 2024, 1:14 p.m. UTC
In preparation for switching the RkISP1 Agc to a derivation of
MeanLuminanceAgc, add a function that parses the statistics and
stores the values for easy retrieval later.

Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
---
 src/ipa/rkisp1/algorithms/agc.cpp | 10 ++++++++++
 src/ipa/rkisp1/algorithms/agc.h   |  7 +++++++
 2 files changed, 17 insertions(+)

Comments

Stefan Klug March 27, 2024, 3:54 p.m. UTC | #1
Hi Daniel,

thanks for the patch.

On Fri, Mar 22, 2024 at 01:14:49PM +0000, Daniel Scally wrote:
> In preparation for switching the RkISP1 Agc to a derivation of
> MeanLuminanceAgc, add a function that parses the statistics and
> stores the values for easy retrieval later.
> 
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
>  src/ipa/rkisp1/algorithms/agc.cpp | 10 ++++++++++
>  src/ipa/rkisp1/algorithms/agc.h   |  7 +++++++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index 47a6f7b2..d380194d 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -373,6 +373,16 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
>  	metadata.set(controls::FrameDuration, frameDuration.get<std::micro>());
>  }
>  
> +void Agc::parseStatistics(const rkisp1_stat_buffer *stats,
> +			  IPAContext &context)
> +{
> +	const rkisp1_cif_isp_stat *params = &stats->params;
> +
> +	expMeans_ = { params->ae.exp_mean, context.hw->numAeCells };
> +	hist_ = Histogram(Span<const uint32_t>(params->hist.hist_bins,
> +					       context.hw->numHistogramBins));

As noted in irc, hist_ is only used inside process(). It could be
returned from the function. The problem is, that you still need the
expMeans_ member, as you can't forward that info into the
estimateLuminance function. It feels strange, to store references to
stats->params->ae inside expMeans_ which might be invalid after leaving
process(). Maybe return both (hist and expMeans) from this function and
only temporarily assign them to a member inside process()?

Cheers,
Stefan

> +}
> +
>  /**
>   * \brief Process RkISP1 statistics, and run AGC operations
>   * \param[in] context The shared IPA context
> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h
> index fb82a33f..b0de4898 100644
> --- a/src/ipa/rkisp1/algorithms/agc.h
> +++ b/src/ipa/rkisp1/algorithms/agc.h
> @@ -14,6 +14,8 @@
>  
>  #include <libcamera/geometry.h>
>  
> +#include "libipa/histogram.h"
> +
>  #include "algorithm.h"
>  
>  namespace libcamera {
> @@ -47,10 +49,15 @@ private:
>  	double measureBrightness(Span<const uint32_t> hist) const;
>  	void fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
>  			  ControlList &metadata);
> +	void parseStatistics(const rkisp1_stat_buffer *stats,
> +			     IPAContext &context);
>  
>  	uint64_t frameCount_;
>  
>  	utils::Duration filteredExposure_;
> +
> +	Histogram hist_;
> +	Span<const uint8_t> expMeans_;
>  };
>  
>  } /* namespace ipa::rkisp1::algorithms */
> -- 
> 2.34.1
>
Laurent Pinchart April 6, 2024, 1:41 a.m. UTC | #2
Hi Dan,

Thank you for the patch.

On Fri, Mar 22, 2024 at 01:14:49PM +0000, Daniel Scally wrote:
> In preparation for switching the RkISP1 Agc to a derivation of
> MeanLuminanceAgc, add a function that parses the statistics and
> stores the values for easy retrieval later.
> 
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>

As commented on 05/10, squash this with 09/10 as it's more difficult to
review in isolation.

> ---
>  src/ipa/rkisp1/algorithms/agc.cpp | 10 ++++++++++
>  src/ipa/rkisp1/algorithms/agc.h   |  7 +++++++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index 47a6f7b2..d380194d 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -373,6 +373,16 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
>  	metadata.set(controls::FrameDuration, frameDuration.get<std::micro>());
>  }
>  
> +void Agc::parseStatistics(const rkisp1_stat_buffer *stats,
> +			  IPAContext &context)
> +{
> +	const rkisp1_cif_isp_stat *params = &stats->params;
> +
> +	expMeans_ = { params->ae.exp_mean, context.hw->numAeCells };
> +	hist_ = Histogram(Span<const uint32_t>(params->hist.hist_bins,
> +					       context.hw->numHistogramBins));
> +}
> +
>  /**
>   * \brief Process RkISP1 statistics, and run AGC operations
>   * \param[in] context The shared IPA context
> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h
> index fb82a33f..b0de4898 100644
> --- a/src/ipa/rkisp1/algorithms/agc.h
> +++ b/src/ipa/rkisp1/algorithms/agc.h
> @@ -14,6 +14,8 @@
>  
>  #include <libcamera/geometry.h>
>  
> +#include "libipa/histogram.h"
> +
>  #include "algorithm.h"
>  
>  namespace libcamera {
> @@ -47,10 +49,15 @@ private:
>  	double measureBrightness(Span<const uint32_t> hist) const;
>  	void fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
>  			  ControlList &metadata);
> +	void parseStatistics(const rkisp1_stat_buffer *stats,
> +			     IPAContext &context);
>  
>  	uint64_t frameCount_;
>  
>  	utils::Duration filteredExposure_;
> +
> +	Histogram hist_;
> +	Span<const uint8_t> expMeans_;
>  };
>  
>  } /* namespace ipa::rkisp1::algorithms */

Patch
diff mbox series

diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
index 47a6f7b2..d380194d 100644
--- a/src/ipa/rkisp1/algorithms/agc.cpp
+++ b/src/ipa/rkisp1/algorithms/agc.cpp
@@ -373,6 +373,16 @@  void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
 	metadata.set(controls::FrameDuration, frameDuration.get<std::micro>());
 }
 
+void Agc::parseStatistics(const rkisp1_stat_buffer *stats,
+			  IPAContext &context)
+{
+	const rkisp1_cif_isp_stat *params = &stats->params;
+
+	expMeans_ = { params->ae.exp_mean, context.hw->numAeCells };
+	hist_ = Histogram(Span<const uint32_t>(params->hist.hist_bins,
+					       context.hw->numHistogramBins));
+}
+
 /**
  * \brief Process RkISP1 statistics, and run AGC operations
  * \param[in] context The shared IPA context
diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h
index fb82a33f..b0de4898 100644
--- a/src/ipa/rkisp1/algorithms/agc.h
+++ b/src/ipa/rkisp1/algorithms/agc.h
@@ -14,6 +14,8 @@ 
 
 #include <libcamera/geometry.h>
 
+#include "libipa/histogram.h"
+
 #include "algorithm.h"
 
 namespace libcamera {
@@ -47,10 +49,15 @@  private:
 	double measureBrightness(Span<const uint32_t> hist) const;
 	void fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
 			  ControlList &metadata);
+	void parseStatistics(const rkisp1_stat_buffer *stats,
+			     IPAContext &context);
 
 	uint64_t frameCount_;
 
 	utils::Duration filteredExposure_;
+
+	Histogram hist_;
+	Span<const uint8_t> expMeans_;
 };
 
 } /* namespace ipa::rkisp1::algorithms */