[libcamera-devel] [PATCH v7 2/5] ipa: ipu3: Rename IspStatsRegion to Accumulator
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Sep 13 18:49:20 CEST 2021
On Mon, Sep 13, 2021 at 06:45:21PM +0200, Jean-Michel Hautbois wrote:
> Hi Laurent,
>
> On 13/09/2021 17:56, Laurent Pinchart wrote:
> > Hi Jean-Michel,
> >
> > Thank you for the patch.
> >
> > On Mon, Sep 13, 2021 at 03:28:00PM +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>
> >> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >> ---
> >> src/ipa/ipu3/algorithms/awb.cpp | 80 +++++++++++++++++++++++++--------
> >> src/ipa/ipu3/algorithms/awb.h | 5 +--
> >> 2 files changed, 64 insertions(+), 21 deletions(-)
> >>
> >> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
> >> index e05647c9..d993e21e 100644
> >> --- a/src/ipa/ipu3/algorithms/awb.cpp
> >> +++ b/src/ipa/ipu3/algorithms/awb.cpp
> >> @@ -21,26 +21,71 @@ 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
> >
> > Quoting your reply from v6:
> >
> > "[This] 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)."
> >
> > From a statistics point of view, the zones match the output of the BDS.
> > The image is then slightly cropped to achieve the required resolution. I
> > think that's out of scope here. I would thus drop the last point. The
> > image and text below need to be adapted to replace 1280 with 1296.
> >
> >> *
> >> - * \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) │ | │ │ │
> >> + * │ │ | │ │ │
> >> + * ├── ───┼─ ──────┼────┤ │
> >> + * │ │ │ | │ │ │
> >> + * │ │ │ │ │ │ | │ │ │
> >> + * └────┴────┴────┴────┴────┴─ ──────┴────┘ /
> >> *
> >> - * \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
>
> Replacing 1280 by 1296 except this sentence, right :-) ?
I'd write
"For example, a frame of 1296x720 is divided into 81x45 cells of [16x16] pixels."
We shouldn't mention the BDS here, this belongs to the documentation of
the ImgU, not the Accumulator structure.
> >> + * 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.
> >> *
> >> - * \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.
> >> + * \todo move this description and structure into a common header
> >> + *
> >> + * Cells which are saturated beyond the threshold defined in
> >> + * ipu3_uapi_awb_config_s are not included in the average.
> >> + *
> >> + * \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 unsaturated cell in the zone
> >> + *
> >> + * \var Accumulator::gSum
> >> + * \brief Sum of the average green values of each unsaturated cell in the zone
> >> + *
> >> + * \var Accumulator::bSum
> >> + * \brief Sum of the average blue values of each unsaturated cell in the zone
> >> */
> >>
> >> /**
> >> @@ -157,7 +202,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 +219,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 +260,6 @@ void Awb::clearAwbStats()
> >> awbStats_[i].rSum = 0;
> >> awbStats_[i].gSum = 0;
> >> awbStats_[i].counted = 0;
> >> - awbStats_[i].uncounted = 0;
> >> }
> >> }
> >>
> >> @@ -304,7 +348,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_;
> >> };
> >>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list