| Message ID | 20240322131451.3092931-6-dan.scally@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Daniel, Thanks for the patch. On Fri, Mar 22, 2024 at 01:14:46PM +0000, Daniel Scally wrote: > In preparation for switching to a derivation of MeanLuminanceAgc, add > a function to parse and store the statistics for easy retrieval in an > overriding estimateLuminance() function. > > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> > --- > src/ipa/ipu3/algorithms/agc.cpp | 33 +++++++++++++++++++++++++++++++++ > src/ipa/ipu3/algorithms/agc.h | 8 ++++++++ > 2 files changed, 41 insertions(+) > > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp > index 606a237a..ee9a42cf 100644 > --- a/src/ipa/ipu3/algorithms/agc.cpp > +++ b/src/ipa/ipu3/algorithms/agc.cpp > @@ -142,6 +142,39 @@ double Agc::measureBrightness(const ipu3_uapi_stats_3a *stats, > return Histogram(Span<uint32_t>(hist)).interQuantileMean(0.98, 1.0); > } > > +void Agc::parseStatistics(const ipu3_uapi_stats_3a *stats, > + const ipu3_uapi_grid_config &grid) > +{ > + uint32_t hist[knumHistogramBins] = { 0 }; > + > + reds_.clear(); > + greens_.clear(); > + blues_.clear(); > + > + for (unsigned int cellY = 0; cellY < grid.height; cellY++) { > + for (unsigned int cellX = 0; cellX < grid.width; cellX++) { > + uint32_t cellPosition = cellY * stride_ + cellX; > + > + const ipu3_uapi_awb_set_item *cell = > + reinterpret_cast<const ipu3_uapi_awb_set_item *>( > + &stats->awb_raw_buffer.meta_data[cellPosition]); > + > + reds_.push_back(cell->R_avg); > + greens_.push_back((cell->Gr_avg + cell->Gb_avg) / 2); > + blues_.push_back(cell->B_avg); > + > + /* > + * Store the average green value to estimate the > + * brightness. Even the overexposed pixels are > + * taken into account. > + */ > + hist[(cell->Gr_avg + cell->Gb_avg) / 2]++; Why are only the greens taken into account? Ahh its just a copy of existing code, that gets removed in patch 7/10. Still should that be changed to hist[r*0.299 + (gr + gb) / 2 * 0.587 + b * 0.114]++ ? Or am I missing something here? > + } > + } > + > + hist_ = Histogram(Span<uint32_t>(hist)); > +} > + > /** > * \brief Apply a filter on the exposure value to limit the speed of changes > * \param[in] exposureValue The target exposure from the AGC algorithm > diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h > index 9d6e3ff1..7ed0ef7a 100644 > --- a/src/ipa/ipu3/algorithms/agc.h > +++ b/src/ipa/ipu3/algorithms/agc.h > @@ -13,6 +13,8 @@ > > #include <libcamera/geometry.h> > > +#include "libipa/histogram.h" > + > #include "algorithm.h" > > namespace libcamera { > @@ -43,6 +45,8 @@ private: > const ipu3_uapi_grid_config &grid, > const ipu3_uapi_stats_3a *stats, > double gain); > + void parseStatistics(const ipu3_uapi_stats_3a *stats, > + const ipu3_uapi_grid_config &grid); > > uint64_t frameCount_; > > @@ -55,6 +59,10 @@ private: > utils::Duration filteredExposure_; > > uint32_t stride_; > + std::vector<uint8_t> reds_; > + std::vector<uint8_t> blues_; > + std::vector<uint8_t> greens_; > + Histogram hist_; > }; > > } /* namespace ipa::ipu3::algorithms */ > -- > 2.34.1 >
Hi Stefan On 26/03/2024 11:03, Stefan Klug wrote: > Hi Daniel, > > Thanks for the patch. > > On Fri, Mar 22, 2024 at 01:14:46PM +0000, Daniel Scally wrote: >> In preparation for switching to a derivation of MeanLuminanceAgc, add >> a function to parse and store the statistics for easy retrieval in an >> overriding estimateLuminance() function. >> >> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> >> --- >> src/ipa/ipu3/algorithms/agc.cpp | 33 +++++++++++++++++++++++++++++++++ >> src/ipa/ipu3/algorithms/agc.h | 8 ++++++++ >> 2 files changed, 41 insertions(+) >> >> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp >> index 606a237a..ee9a42cf 100644 >> --- a/src/ipa/ipu3/algorithms/agc.cpp >> +++ b/src/ipa/ipu3/algorithms/agc.cpp >> @@ -142,6 +142,39 @@ double Agc::measureBrightness(const ipu3_uapi_stats_3a *stats, >> return Histogram(Span<uint32_t>(hist)).interQuantileMean(0.98, 1.0); >> } >> >> +void Agc::parseStatistics(const ipu3_uapi_stats_3a *stats, >> + const ipu3_uapi_grid_config &grid) >> +{ >> + uint32_t hist[knumHistogramBins] = { 0 }; >> + >> + reds_.clear(); >> + greens_.clear(); >> + blues_.clear(); >> + >> + for (unsigned int cellY = 0; cellY < grid.height; cellY++) { >> + for (unsigned int cellX = 0; cellX < grid.width; cellX++) { >> + uint32_t cellPosition = cellY * stride_ + cellX; >> + >> + const ipu3_uapi_awb_set_item *cell = >> + reinterpret_cast<const ipu3_uapi_awb_set_item *>( >> + &stats->awb_raw_buffer.meta_data[cellPosition]); >> + >> + reds_.push_back(cell->R_avg); >> + greens_.push_back((cell->Gr_avg + cell->Gb_avg) / 2); >> + blues_.push_back(cell->B_avg); >> + >> + /* >> + * Store the average green value to estimate the >> + * brightness. Even the overexposed pixels are >> + * taken into account. >> + */ >> + hist[(cell->Gr_avg + cell->Gb_avg) / 2]++; > Why are only the greens taken into account? Ahh its just a copy of > existing code, that gets removed in patch 7/10. Still should that be > changed to hist[r*0.299 + (gr + gb) / 2 * 0.587 + b * 0.114]++ ? > > Or am I missing something here? No you're missing nothing; I think that that's the right thing to do but I wanted to keep this series as closely as possible to the existing behaviour to make it easier to integrate. I can add a patch on top to switch this though? > >> + } >> + } >> + >> + hist_ = Histogram(Span<uint32_t>(hist)); >> +} >> + >> /** >> * \brief Apply a filter on the exposure value to limit the speed of changes >> * \param[in] exposureValue The target exposure from the AGC algorithm >> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h >> index 9d6e3ff1..7ed0ef7a 100644 >> --- a/src/ipa/ipu3/algorithms/agc.h >> +++ b/src/ipa/ipu3/algorithms/agc.h >> @@ -13,6 +13,8 @@ >> >> #include <libcamera/geometry.h> >> >> +#include "libipa/histogram.h" >> + >> #include "algorithm.h" >> >> namespace libcamera { >> @@ -43,6 +45,8 @@ private: >> const ipu3_uapi_grid_config &grid, >> const ipu3_uapi_stats_3a *stats, >> double gain); >> + void parseStatistics(const ipu3_uapi_stats_3a *stats, >> + const ipu3_uapi_grid_config &grid); >> >> uint64_t frameCount_; >> >> @@ -55,6 +59,10 @@ private: >> utils::Duration filteredExposure_; >> >> uint32_t stride_; >> + std::vector<uint8_t> reds_; >> + std::vector<uint8_t> blues_; >> + std::vector<uint8_t> greens_; >> + Histogram hist_; >> }; >> >> } /* namespace ipa::ipu3::algorithms */ >> -- >> 2.34.1 >>
Hi Dan, Thank you for the patch. On Fri, Mar 22, 2024 at 01:14:46PM +0000, Daniel Scally wrote: > In preparation for switching to a derivation of MeanLuminanceAgc, add > a function to parse and store the statistics for easy retrieval in an > overriding estimateLuminance() function. This is hard to review separately from 06/10, I would squash the two patches together. > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> > --- > src/ipa/ipu3/algorithms/agc.cpp | 33 +++++++++++++++++++++++++++++++++ > src/ipa/ipu3/algorithms/agc.h | 8 ++++++++ > 2 files changed, 41 insertions(+) > > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp > index 606a237a..ee9a42cf 100644 > --- a/src/ipa/ipu3/algorithms/agc.cpp > +++ b/src/ipa/ipu3/algorithms/agc.cpp > @@ -142,6 +142,39 @@ double Agc::measureBrightness(const ipu3_uapi_stats_3a *stats, > return Histogram(Span<uint32_t>(hist)).interQuantileMean(0.98, 1.0); > } > > +void Agc::parseStatistics(const ipu3_uapi_stats_3a *stats, > + const ipu3_uapi_grid_config &grid) > +{ > + uint32_t hist[knumHistogramBins] = { 0 }; > + > + reds_.clear(); > + greens_.clear(); > + blues_.clear(); > + > + for (unsigned int cellY = 0; cellY < grid.height; cellY++) { > + for (unsigned int cellX = 0; cellX < grid.width; cellX++) { > + uint32_t cellPosition = cellY * stride_ + cellX; > + > + const ipu3_uapi_awb_set_item *cell = > + reinterpret_cast<const ipu3_uapi_awb_set_item *>( > + &stats->awb_raw_buffer.meta_data[cellPosition]); > + > + reds_.push_back(cell->R_avg); > + greens_.push_back((cell->Gr_avg + cell->Gb_avg) / 2); > + blues_.push_back(cell->B_avg); > + > + /* > + * Store the average green value to estimate the > + * brightness. Even the overexposed pixels are > + * taken into account. > + */ > + hist[(cell->Gr_avg + cell->Gb_avg) / 2]++; > + } > + } > + > + hist_ = Histogram(Span<uint32_t>(hist)); > +} > + > /** > * \brief Apply a filter on the exposure value to limit the speed of changes > * \param[in] exposureValue The target exposure from the AGC algorithm > diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h > index 9d6e3ff1..7ed0ef7a 100644 > --- a/src/ipa/ipu3/algorithms/agc.h > +++ b/src/ipa/ipu3/algorithms/agc.h > @@ -13,6 +13,8 @@ > > #include <libcamera/geometry.h> > > +#include "libipa/histogram.h" > + > #include "algorithm.h" > > namespace libcamera { > @@ -43,6 +45,8 @@ private: > const ipu3_uapi_grid_config &grid, > const ipu3_uapi_stats_3a *stats, > double gain); > + void parseStatistics(const ipu3_uapi_stats_3a *stats, > + const ipu3_uapi_grid_config &grid); > > uint64_t frameCount_; > > @@ -55,6 +59,10 @@ private: > utils::Duration filteredExposure_; > > uint32_t stride_; > + std::vector<uint8_t> reds_; > + std::vector<uint8_t> blues_; > + std::vector<uint8_t> greens_; > + Histogram hist_; > }; > > } /* namespace ipa::ipu3::algorithms */
diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp index 606a237a..ee9a42cf 100644 --- a/src/ipa/ipu3/algorithms/agc.cpp +++ b/src/ipa/ipu3/algorithms/agc.cpp @@ -142,6 +142,39 @@ double Agc::measureBrightness(const ipu3_uapi_stats_3a *stats, return Histogram(Span<uint32_t>(hist)).interQuantileMean(0.98, 1.0); } +void Agc::parseStatistics(const ipu3_uapi_stats_3a *stats, + const ipu3_uapi_grid_config &grid) +{ + uint32_t hist[knumHistogramBins] = { 0 }; + + reds_.clear(); + greens_.clear(); + blues_.clear(); + + for (unsigned int cellY = 0; cellY < grid.height; cellY++) { + for (unsigned int cellX = 0; cellX < grid.width; cellX++) { + uint32_t cellPosition = cellY * stride_ + cellX; + + const ipu3_uapi_awb_set_item *cell = + reinterpret_cast<const ipu3_uapi_awb_set_item *>( + &stats->awb_raw_buffer.meta_data[cellPosition]); + + reds_.push_back(cell->R_avg); + greens_.push_back((cell->Gr_avg + cell->Gb_avg) / 2); + blues_.push_back(cell->B_avg); + + /* + * Store the average green value to estimate the + * brightness. Even the overexposed pixels are + * taken into account. + */ + hist[(cell->Gr_avg + cell->Gb_avg) / 2]++; + } + } + + hist_ = Histogram(Span<uint32_t>(hist)); +} + /** * \brief Apply a filter on the exposure value to limit the speed of changes * \param[in] exposureValue The target exposure from the AGC algorithm diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h index 9d6e3ff1..7ed0ef7a 100644 --- a/src/ipa/ipu3/algorithms/agc.h +++ b/src/ipa/ipu3/algorithms/agc.h @@ -13,6 +13,8 @@ #include <libcamera/geometry.h> +#include "libipa/histogram.h" + #include "algorithm.h" namespace libcamera { @@ -43,6 +45,8 @@ private: const ipu3_uapi_grid_config &grid, const ipu3_uapi_stats_3a *stats, double gain); + void parseStatistics(const ipu3_uapi_stats_3a *stats, + const ipu3_uapi_grid_config &grid); uint64_t frameCount_; @@ -55,6 +59,10 @@ private: utils::Duration filteredExposure_; uint32_t stride_; + std::vector<uint8_t> reds_; + std::vector<uint8_t> blues_; + std::vector<uint8_t> greens_; + Histogram hist_; }; } /* namespace ipa::ipu3::algorithms */
In preparation for switching to a derivation of MeanLuminanceAgc, add a function to parse and store the statistics for easy retrieval in an overriding estimateLuminance() function. Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> --- src/ipa/ipu3/algorithms/agc.cpp | 33 +++++++++++++++++++++++++++++++++ src/ipa/ipu3/algorithms/agc.h | 8 ++++++++ 2 files changed, 41 insertions(+)