[libcamera-devel] [PATCH 11/12] ipa: ipu3: agc: Rewrite and simplify the brightness loop
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Sep 29 14:55:56 CEST 2021
Hello,
On Tue, Sep 28, 2021 at 05:26:01PM +0100, Kieran Bingham wrote:
> On Thu, Sep 23, 2021 at 10:16:24AM +0200, Jean-Michel Hautbois wrote:
> > Now that we know how exactly the AWB statistics are formatted, use a
> > simplified loop in processBrightness() to parse the green values and get the
> > histogram.
> >
> > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> > ---
> > src/ipa/ipu3/algorithms/agc.cpp | 54 +++++++++++++--------------------
> > src/ipa/ipu3/algorithms/agc.h | 2 ++
> > 2 files changed, 23 insertions(+), 33 deletions(-)
> >
> > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> > index 5ff50f4a..ebbc5676 100644
> > --- a/src/ipa/ipu3/algorithms/agc.cpp
> > +++ b/src/ipa/ipu3/algorithms/agc.cpp
> > @@ -6,6 +6,7 @@
> > */
> >
> > #include "agc.h"
> > +#include "awb.h"
> >
> > #include <algorithm>
> > #include <chrono>
> > @@ -47,9 +48,6 @@ static constexpr uint32_t kMaxExposure = 1976;
> > static constexpr uint32_t knumHistogramBins = 256;
> > static constexpr double kEvGainTarget = 0.5;
> >
> > -/* A cell is 8 bytes and contains averages for RGB values and saturation ratio */
> > -static constexpr uint8_t kCellSize = 8;
> > -
> > Agc::Agc()
> > : frameCount_(0), lastFrame_(0), iqMean_(0.0), lineDuration_(0s),
> > maxExposureTime_(0s), prevExposure_(0s), prevExposureNoDg_(0s),
> > @@ -60,6 +58,10 @@ Agc::Agc()
> > int Agc::configure([[maybe_unused]] IPAContext &context,
>
> looks like context is no longer [[maybe_unused]].
>
> > const IPAConfigInfo &configInfo)
> > {
> > + const ipu3_uapi_grid_config &grid = context.configuration.grid.bdsGrid;
> > + /* The grid is aligned to the next multiple of 4 */
> > + stride_ = (grid.width + 3) / 4 * 4;
>
> This is familiar...
utils::alignUp() to the rescue again :-)
> How about putting a stride into context.configuration.grid.stride and
> calculating it once during calculateBdsGrid()?
>
> > +
> > lineDuration_ = configInfo.sensorInfo.lineLength * 1.0s
> > / configInfo.sensorInfo.pixelRate;
> > maxExposureTime_ = kMaxExposure * lineDuration_;
> > @@ -70,37 +72,23 @@ int Agc::configure([[maybe_unused]] IPAContext &context,
> > void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,
> > const ipu3_uapi_grid_config &grid)
> > {
> > - const struct ipu3_uapi_grid_config statsAeGrid = stats->stats_4a_config.awb_config.grid;
> > - Rectangle aeRegion = { statsAeGrid.x_start,
> > - statsAeGrid.y_start,
> > - static_cast<unsigned int>(statsAeGrid.x_end - statsAeGrid.x_start) + 1,
> > - static_cast<unsigned int>(statsAeGrid.y_end - statsAeGrid.y_start) + 1 };
> > - Point topleft = aeRegion.topLeft();
> > - int topleftX = topleft.x >> grid.block_width_log2;
> > - int topleftY = topleft.y >> grid.block_height_log2;
> > -
> > - /* Align to the grid cell width and height */
> > - uint32_t startX = topleftX << grid.block_width_log2;
> > - uint32_t startY = topleftY * grid.width << grid.block_width_log2;
> > - uint32_t endX = (startX + (aeRegion.size().width >> grid.block_width_log2)) << grid.block_width_log2;
> > - uint32_t i, j;
> > - uint32_t count = 0;
> > -
> > uint32_t hist[knumHistogramBins] = { 0 };
> > - for (j = topleftY;
> > - j < topleftY + (aeRegion.size().height >> grid.block_height_log2);
> > - j++) {
> > - for (i = startX + startY; i < endX + startY; i += kCellSize) {
> > - /*
> > - * The grid width (and maybe height) is not reliable.
> > - * We observed a bit shift which makes the value 160 to be 32 in the stats grid.
> > - * Use the one passed at init time.
> > - */
> > - if (stats->awb_raw_buffer.meta_data[i + 4 + j * grid.width] == 0) {
> > - uint8_t Gr = stats->awb_raw_buffer.meta_data[i + 0 + j * grid.width];
> > - uint8_t Gb = stats->awb_raw_buffer.meta_data[i + 3 + j * grid.width];
> > - hist[(Gr + Gb) / 2]++;
> > - count++;
> > +
> > + for (unsigned int cellY = 0; cellY < grid.height; cellY++) {
> > + for (unsigned int cellX = 0; cellX < stride_; cellX++) {
Same issue as in AWB, you'll use the padding cells in your calculations,
while they should be ignored.
> > + uint32_t cellPosition = cellY * stride_ + cellX
> > + * sizeof(Ipu3AwbCell);
> > +
> > + /* Cast the initial IPU3 structure to simplify the reading */
> > + const Ipu3AwbCell *cell =
> > + reinterpret_cast<const Ipu3AwbCell *>(
> > + &stats->awb_raw_buffer.meta_data[cellPosition]
> > + );
> > +
> > + if (cell->satRatio == 0) {
>
> Is the satRatio usage clear yet? Is it always 0? or do you see other
> values here?
>
> Should it be less than a saturation threshold? (perhaps not unless we
> have a way to configure that anyway)
>
> > + uint8_t gr = cell->greenRedAvg;
> > + uint8_t gb = cell->greenBlueAvg;
> > + hist[(gr + gb) / 2]++;
> > }
> > }
> > }
> > diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
> > index e36797d3..64b71c65 100644
> > --- a/src/ipa/ipu3/algorithms/agc.h
> > +++ b/src/ipa/ipu3/algorithms/agc.h
> > @@ -50,6 +50,8 @@ private:
> > Duration prevExposureNoDg_;
> > Duration currentExposure_;
> > Duration currentExposureNoDg_;
> > +
> > + uint32_t stride_;
> > };
> >
> > } /* namespace ipa::ipu3::algorithms */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list