[libcamera-devel] [PATCH v6 2/5] ipa: ipu3: Rename IspStatsRegion to Accumulator
Jean-Michel Hautbois
jeanmichel.hautbois at ideasonboard.com
Mon Sep 13 15:11:31 CEST 2021
Hi Laurent,
On 10/09/2021 17:18, Laurent Pinchart wrote:
> Hi Jean-Michel,
>
> Thank you for the patch.
>
> On Thu, Sep 09, 2021 at 03:54:46PM +0200, Jean-Michel Hautbois wrote:
>> The IspStatsRegion structure was introduced as an attempt to prepare for
>> a generic AWB algorithm structure. The structure name by itself is not
>> explicit and it is too optimistic to try and make a generic one for now.
>>
>> Its role is to accumulate the pixels in a given zone. Rename it to
>> accumulator, and remove the uncounted field at the same time. It is
>> always possible to know how many pixels are not relevant for the
>> algorithm by calculating total-counted. The uncounted field was only
>> declared and not used. Amend the documentation accordingly.
>>
>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
>> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> ---
>> src/ipa/ipu3/algorithms/awb.cpp | 79 +++++++++++++++++++++++++--------
>> src/ipa/ipu3/algorithms/awb.h | 5 +--
>> 2 files changed, 63 insertions(+), 21 deletions(-)
>>
>> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
>> index e05647c9..9b22c783 100644
>> --- a/src/ipa/ipu3/algorithms/awb.cpp
>> +++ b/src/ipa/ipu3/algorithms/awb.cpp
>> @@ -21,26 +21,70 @@ static constexpr uint32_t kMinZonesCounted = 16;
>> static constexpr uint32_t kMinGreenLevelInZone = 32;
>>
>> /**
>> - * \struct IspStatsRegion
>> - * \brief RGB statistics for a given region
>> + * \struct Accumulator
>> + * \brief RGB statistics for a given zone
>> *
>> - * The IspStatsRegion structure is intended to abstract the ISP specific
>> - * statistics and use an agnostic algorithm to compute AWB.
>> + * - Cells are defined in Pixels
>> + * - Zones are defined in Cells
>> + * - The total zones may overlap the image width and height to meet hardware
>> + * constraints
>
> What do you mean by "overlap the image width" here ?
>
It means that the zones will be adapted to the BDS output size which may
be greater than the rendered frame (asking for 1280x720 may lead to a
1296x720 BDS output and you will have 81 cells of 16 pixels width and
not 80)
>> *
>> - * \var IspStatsRegion::counted
>> - * \brief Number of pixels used to calculate the sums
>> + * 81 cells
>> + * /───────────── 1280 pixels ───────────\
>> + * 16 zones
>> + * 16
>> + * ┌────┬────┬────┬────┬────┬─ ──────┬────┐ \
>> + * │Cell│ │ │ │ │ | │ │ │
>> + *16 │ px │ │ │ │ │ | │ │ │
>> + * ├────┼────┼────┼────┼────┼─ ──────┼────┤ │
>> + * │ │ │ │ │ │ | │ │
>> + * │ │ │ │ │ │ | │ │ 7
>> + * │ ── │ ── │ ── │ ── │ ── │ ── ── ─┤ ── │ 1 2 4
>> + * │ │ │ │ │ │ | │ │ 2 0 5
>> *
>> - * \var IspStatsRegion::uncounted
>> - * \brief Remaining number of pixels in the region
>> + * │ │ │ │ │ │ | │ │ z p c
>> + * ├────┼────┼────┼────┼────┼─ ──────┼────┤ o i e
>> + * │ │ │ │ │ │ | │ │ n x l
>> + * │ │ | │ │ e e l
>> + * ├─── ───┼─ ──────┼────┤ s l s
>> + * │ │ | │ │ s
>> + * │ │ | │ │
>> + * ├─── Zone of Cells ───┼─ ──────┼────┤ │
>> + * │ (5 x 4) │ | │ │ │
>> + * │ │ | │ │ │
>> + * ├── ───┼─ ──────┼────┤ │
>> + * │ │ │ | │ │ │
>> + * │ │ │ │ │ │ | │ │ │
>> + * └────┴────┴────┴────┴────┴─ ──────┴────┘ /
>
> Nice diagram :-) I wonder how it will render with doxygen, but that's
> for later.
>
> Could you add one more space on the left side to have a space between
> "*" and "16" ?
>
>> *
>> - * \var IspStatsRegion::rSum
>> - * \brief Sum of the red values in the region
>> + * The algorithm works with a fixed number of zones \a kAwbStatsSizeX x
>> + * \a kAwbStatsSizeY. For example, a frame of 1280x720 is divided into 81x45
>> + * cells of [16x16] pixels with a BDS output size of 1296x720 to account for the
>> + * 16x16 pixel alignment restrictions. In the case of \a kAwbStatsSizeX=16 and
>> + * \a kAwbStatsSizeY=12 the zones are made of [5x4] cells. The cells are
>> + * left-aligned and calculated by IPAIPU3::calculateBdsGrid().
>> *
>> - * \var IspStatsRegion::gSum
>> - * \brief Sum of the green values in the region
>> + * Each statistics cell represents the average value of the pixels in that cell
>> + * split by colour components.
>
> This should probably be moved somewhere else, it explains how the
> Accumulator structure is used in the IPU3 IPA implementation, not what
> the Accumulator is. We can deal with that later (but not too late if
> possible, maybe at the time the structure moves out of awb.h as it
> shouldn't refer to kAwbStatsSizeX in that case).
>
I will add a \todo for that :-)
>> *
>> - * \var IspStatsRegion::bSum
>> - * \brief Sum of the blue values in the region
>> + * The Accumulator structure stores the sum of the average of each cell in a
>> + * zone of the image, as well as the number of cells which were unsaturated and
>> + * therefore included in the average.
>> + *
>> + * Cells which are saturated beyond the threshold defined in
>> + * ipu3_uapi_awb_config_s are not included in the average.
>
> This will need to be reworked too when we'll move the structure to a
> shared header, to avoid referring to ipu3_uapi_awb_config_s and just
> talk about thresholds in general (the reference to
> ipu3_uapi_awb_config_s should then be kept in awb.cpp though, as it's
> relevant for the IPU3).
dito
>
>> + *
>> + * \var Accumulator::counted
>> + * \brief Number of unsaturated cells used to calculate the sums
>> + *
>> + * \var Accumulator::rSum
>> + * \brief Sum of the average red values of each cell in the zone
>
> s/each cell/each unsaturated cell/ ? Same below.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
>> + *
>> + * \var Accumulator::gSum
>> + * \brief Sum of the average green values of each cell in the zone
>> + *
>> + * \var Accumulator::bSum
>> + * \brief Sum of the average blue values of each cell in the zone
>> */
>>
>> /**
>> @@ -157,7 +201,7 @@ uint32_t Awb::estimateCCT(double red, double green, double blue)
>> return 449 * n * n * n + 3525 * n * n + 6823.3 * n + 5520.33;
>> }
>>
>> -/* Generate an RGB vector with the average values for each region */
>> +/* Generate an RGB vector with the average values for each zone */
>> void Awb::generateZones(std::vector<RGB> &zones)
>> {
>> for (unsigned int i = 0; i < kAwbStatsSizeX * kAwbStatsSizeY; i++) {
>> @@ -174,7 +218,7 @@ void Awb::generateZones(std::vector<RGB> &zones)
>> }
>> }
>>
>> -/* Translate the IPU3 statistics into the default statistics region array */
>> +/* Translate the IPU3 statistics into the default statistics zone array */
>> void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats,
>> const ipu3_uapi_grid_config &grid)
>> {
>> @@ -215,7 +259,6 @@ void Awb::clearAwbStats()
>> awbStats_[i].rSum = 0;
>> awbStats_[i].gSum = 0;
>> awbStats_[i].counted = 0;
>> - awbStats_[i].uncounted = 0;
>> }
>> }
>>
>> @@ -304,7 +347,7 @@ void Awb::prepare(IPAContext &context, ipu3_uapi_params *params)
>>
>> /*
>> * Optical center is column start (respectively row start) of the
>> - * region of interest minus its X center (respectively Y center).
>> + * cell of interest minus its X center (respectively Y center).
>> *
>> * For the moment use BDS as a first approximation, but it should
>> * be calculated based on Shading (SHD) parameters.
>> diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h
>> index cc848060..ac8ccc84 100644
>> --- a/src/ipa/ipu3/algorithms/awb.h
>> +++ b/src/ipa/ipu3/algorithms/awb.h
>> @@ -33,9 +33,8 @@ struct Ipu3AwbCell {
>> unsigned char padding[3];
>> } __attribute__((packed));
>>
>> -struct IspStatsRegion {
>> +struct Accumulator {
>> unsigned int counted;
>> - unsigned int uncounted;
>> unsigned long long rSum;
>> unsigned long long gSum;
>> unsigned long long bSum;
>> @@ -82,7 +81,7 @@ private:
>> uint32_t estimateCCT(double red, double green, double blue);
>>
>> std::vector<RGB> zones_;
>> - IspStatsRegion awbStats_[kAwbStatsSizeX * kAwbStatsSizeY];
>> + Accumulator awbStats_[kAwbStatsSizeX * kAwbStatsSizeY];
>> AwbStatus asyncResults_;
>> };
>>
More information about the libcamera-devel
mailing list