[libcamera-devel] [PATCH v8 4/5] ipa: ipu3: Make the naming consistent
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Sep 15 00:21:08 CEST 2021
Hi Jean-Michel,
Thank you for the patch.
On Tue, Sep 14, 2021 at 06:37:08PM +0200, Jean-Michel Hautbois wrote:
> The variables mix the terms cell, region and zone. It can confuse the
> reader, and make the algorithm more difficult to follow. Rename the
> local variables consistent with their definitions:
> - Cells are defined in Pixels
> - Zones are defined in Cells
> There is no "region" as such, so replace it with the correct term.
This looks good for the AWB side, which should be in its own patch. The
AGC bug fix should be separate, and have a Fixes: line.
> While reading again the code for this renaming, the
> Agc::processBrightness() function was really too difficult to follow,
> and mixed pixels, cells, region of interest. The history is that AGC was
> meant to be done in one particular region of the input frame (the
> center, or anything else) but there is no need for that, and there is no
> easy way to define this region of interest right now. It will be better
> to introduce metering instead.
>
> While we are renaming and making things more consistent, let's improve
> Agc at the same time:
> - Remove the aeRegion reference, as start_x and y_start are always 0
> - Use only the grid passed to the function, and not the one in the stats
> - rename i and j accordingly, to follow the Awb way
> - remove the unused variables (count, kCellSize, etc.)
The most important part of this patch is that it fixes an actual bug in
the AGC code. What the bug is is supposed to be explained in the commit
message.
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> ---
> src/ipa/ipu3/algorithms/agc.cpp | 44 +++++++++------------------------
> src/ipa/ipu3/algorithms/awb.cpp | 26 +++++++++----------
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index 5ff50f4a..52e8015e 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),
> @@ -70,37 +68,19 @@ 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;
Why was there a copy ? :-(
> - 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];
> +
> + for (unsigned int cellY = 0; cellY < grid.height; cellY++) {
> + for (unsigned int cellX = 0; cellX < grid.width; cellX++) {
> + uint32_t cellPosition = (cellY * grid.width + cellX)
> + * sizeof(Ipu3AwbCell);
> +
> + /* Cast the initial IPU3 structure to simplify the reading */
> + Ipu3AwbCell *currentCell = reinterpret_cast<Ipu3AwbCell *>(const_cast<uint8_t *>(&stats->awb_raw_buffer.meta_data[cellPosition]));
Yuck. Why do you need a const_cast ? Is there anything wrong with
const Ipu3AwbCell *cell =
reinterpret_cast<const Ipu3AwbCell *>(
&stats->awb_raw_buffer.meta_data[cellPosition]
);
?
I know the awb code does the same below, which looks equally bad to me.
I'd go one step further.
uint32_t hist[knumHistogramBins] = { 0 };
for (unsigned int i = 0; i < grid.width * grid.height; ++i) {
const Ipu3AwbCell *cell =
reinterpret_cast<const Ipu3AwbCell *>(
&stats->awb_raw_buffer.meta_data[i * sizeof(Ipu3AwbCell)]
);
if (cell->satRatio == 0) {
uint8_t gr = cell->greenRedAvg;
uint8_t gb = cell->greenBlueAvg;
hist[(gr + gb) / 2]++;
}
}
Here's a possible commit message:
ipa: ipu3: agc: Fix invalid access to statistics cells
The code in Agc::processBrightness() that iterates over the statistics
cells is difficult to follow as it mixes pixels, cells, region of
interest. This has resulted in the index in the stats cells data being
calculated incorrectly based on pixel coordinates instead of cell
coordinates. The statistics are thus not read correctly.
We could clean up the code by using more descriptive variable names and
fix the issue, but this was designed with the assumption that the
algorithm would only operate on a particular region of the input frame,
and that assumption doesn't hold anymore. Let's simplify the code
instead, looping over all cells, and fixing the bug.
> + if (currentCell->satRatio == 0) {
> + uint8_t Gr = currentCell->greenRedAvg;
> + uint8_t Gb = currentCell->greenBlueAvg;
> hist[(Gr + Gb) / 2]++;
> - count++;
> }
> }
> }
> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
> index d78ae4f4..b96a6ebf 100644
> --- a/src/ipa/ipu3/algorithms/awb.cpp
> +++ b/src/ipa/ipu3/algorithms/awb.cpp
> @@ -220,31 +220,31 @@ void Awb::generateZones(std::vector<RGB> &zones)
> void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats,
> const ipu3_uapi_grid_config &grid)
> {
> - uint32_t regionWidth = round(grid.width / static_cast<double>(kAwbStatsSizeX));
> - uint32_t regionHeight = round(grid.height / static_cast<double>(kAwbStatsSizeY));
> + uint32_t cellsPerZoneX = round(grid.width / static_cast<double>(kAwbStatsSizeX));
> + uint32_t cellsPerZoneY = round(grid.height / static_cast<double>(kAwbStatsSizeY));
>
> /*
> * Generate a (kAwbStatsSizeX x kAwbStatsSizeY) array from the IPU3 grid which is
> * (grid.width x grid.height).
> */
> - for (unsigned int j = 0; j < kAwbStatsSizeY * regionHeight; j++) {
> - for (unsigned int i = 0; i < kAwbStatsSizeX * regionWidth; i++) {
> - uint32_t cellPosition = j * grid.width + i;
> - uint32_t cellX = (cellPosition / regionWidth) % kAwbStatsSizeX;
> - uint32_t cellY = ((cellPosition / grid.width) / regionHeight) % kAwbStatsSizeY;
> + for (unsigned int cellY = 0; cellY < kAwbStatsSizeY * cellsPerZoneY; cellY++) {
> + for (unsigned int cellX = 0; cellX < kAwbStatsSizeX * cellsPerZoneX; cellX++) {
> + uint32_t cellPosition = (cellY * grid.width + cellX)
> + * sizeof(Ipu3AwbCell);
> + uint32_t zoneX = cellX / cellsPerZoneX;
> + uint32_t zoneY = cellY / cellsPerZoneY;
>
> - uint32_t awbRegionPosition = cellY * kAwbStatsSizeX + cellX;
> - cellPosition *= 8;
> + uint32_t awbZonePosition = zoneY * kAwbStatsSizeX + zoneX;
>
> /* Cast the initial IPU3 structure to simplify the reading */
> Ipu3AwbCell *currentCell = reinterpret_cast<Ipu3AwbCell *>(const_cast<uint8_t *>(&stats->awb_raw_buffer.meta_data[cellPosition]));
> if (currentCell->satRatio == 0) {
> /* The cell is not saturated, use the current cell */
> - awbStats_[awbRegionPosition].counted++;
> + awbStats_[awbZonePosition].counted++;
> uint32_t greenValue = currentCell->greenRedAvg + currentCell->greenBlueAvg;
> - awbStats_[awbRegionPosition].sum.green += greenValue / 2;
> - awbStats_[awbRegionPosition].sum.red += currentCell->redAvg;
> - awbStats_[awbRegionPosition].sum.blue += currentCell->blueAvg;
> + awbStats_[awbZonePosition].sum.green += greenValue / 2;
> + awbStats_[awbZonePosition].sum.red += currentCell->redAvg;
> + awbStats_[awbZonePosition].sum.blue += currentCell->blueAvg;
> }
> }
> }
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list