| Message ID | 20240322131451.3092931-9-dan.scally@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
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 >
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 */
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 */
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(+)