[libcamera-devel] [PATCH v2 1/4] ipa: ipu3: Move the AWB stats structures
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Aug 31 00:59:38 CEST 2021
Hi Jean-Michel,
Thank you for the patch.
On Fri, Aug 27, 2021 at 10:02:24AM +0200, Jean-Michel Hautbois wrote:
> The structure Ipu3AwbCell describes the AWB stats layout on the kernel
> side. We will need it to be used by the AGC algorithm to be introduced
> later, so let's make it visible from ipa::ipu3::algorithms and not only
> for the AWB class.
>
> This structure should probably go into the intel-ipu3.h file, whichs
> means a kernel patch, let's keep it in mind for the moment.
>
> - IspStatsRegion is renamed Accumulator, and is storing the total number
> of pixels along with the counted pixels now.
>
> - The RGB structure is a more generic structure, it should be in a more
> generic header.
>
> - AwbStatus is used for the internal storage of the awb gains to update
> the context in prepare. Align the IPAFrameContext to have the same
> structure layout.
Why, what does having the same structure layout bring ? And why are
there two different structures if they're the same ? Please re-read my
review of v1.
This is all a big melting pot of random changes with no structure. The
easiest way to fix that is to split it in multiple patches:
- Move Ipu3AwbCell, RGB and IspStatsRegion from awb.cpp to awb.h
- Rename IspStatsRegion to Accumulator
- Replace Accumulator::uncounted with Accumulator::total
- Fix the documentation of Accumulator::uncounted and Accumulator::total
(see below)
Each commit message should explain why.
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> ---
> src/ipa/ipu3/algorithms/awb.cpp | 47 ++++++++++---------
> src/ipa/ipu3/algorithms/awb.h | 81 +++++++++++++++++----------------
> src/ipa/ipu3/ipa_context.h | 1 +
> src/ipa/ipu3/ipu3.cpp | 3 ++
> 4 files changed, 71 insertions(+), 61 deletions(-)
>
> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
> index e05647c9..136e79e0 100644
> --- a/src/ipa/ipu3/algorithms/awb.cpp
> +++ b/src/ipa/ipu3/algorithms/awb.cpp
> @@ -21,25 +21,27 @@ static constexpr uint32_t kMinZonesCounted = 16;
> static constexpr uint32_t kMinGreenLevelInZone = 32;
>
> /**
> - * \struct IspStatsRegion
> + * \struct Accumulator
> * \brief RGB statistics for a given region
> *
> - * The IspStatsRegion structure is intended to abstract the ISP specific
> - * statistics and use an agnostic algorithm to compute AWB.
> + * The Accumulator structure stores the sum of the pixel values in a region of
> + * the image, as well as the number of relevant pixels in this same region. A
> + * relevant pixel is an unsaturated pixel for this algorithm.
> + * \todo extend the notion of relevant to something else ?
Sounds confusing indeed.
> *
> - * \var IspStatsRegion::counted
> - * \brief Number of pixels used to calculate the sums
> + * \var Accumulator::total
> + * \brief Total number of pixels in the region
> *
> - * \var IspStatsRegion::uncounted
> - * \brief Remaining number of pixels in the region
> + * \var Accumulator::counted
> + * \brief Number of relevant pixels used to calculate the sums
This doesn't seem to be correct, don't those two fields store a number
of zones, not a number of pixels ?
> *
> - * \var IspStatsRegion::rSum
> + * \var Accumulator::rSum
> * \brief Sum of the red values in the region
> *
> - * \var IspStatsRegion::gSum
> + * \var Accumulator::gSum
> * \brief Sum of the green values in the region
> *
> - * \var IspStatsRegion::bSum
> + * \var Accumulator::bSum
> * \brief Sum of the blue values in the region
> */
>
> @@ -117,9 +119,9 @@ static const struct ipu3_uapi_ccm_mat_config imguCssCcmDefault = {
> Awb::Awb()
> : Algorithm()
> {
> - asyncResults_.blueGain = 1.0;
> - asyncResults_.greenGain = 1.0;
> - asyncResults_.redGain = 1.0;
> + asyncResults_.gains.blue = 1.0;
> + asyncResults_.gains.green = 1.0;
> + asyncResults_.gains.red = 1.0;
> asyncResults_.temperatureK = 4500;
>
> zones_.reserve(kAwbStatsSizeX * kAwbStatsSizeY);
> @@ -204,6 +206,7 @@ void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats,
> awbStats_[awbRegionPosition].rSum += currentCell->redAvg;
> awbStats_[awbRegionPosition].bSum += currentCell->blueAvg;
> }
> + awbStats_[awbRegionPosition].total++;
This can be moved outside of the loops, you can calculate the total
value instead of incrementing it for each pixel.
> }
> }
> }
> @@ -215,7 +218,7 @@ void Awb::clearAwbStats()
> awbStats_[i].rSum = 0;
> awbStats_[i].gSum = 0;
> awbStats_[i].counted = 0;
> - awbStats_[i].uncounted = 0;
> + awbStats_[i].total = 0;
> }
> }
>
> @@ -254,9 +257,9 @@ void Awb::awbGreyWorld()
>
> /* Color temperature is not relevant in Grey world but still useful to estimate it :-) */
> asyncResults_.temperatureK = estimateCCT(sumRed.R, sumRed.G, sumBlue.B);
> - asyncResults_.redGain = redGain;
> - asyncResults_.greenGain = 1.0;
> - asyncResults_.blueGain = blueGain;
> + asyncResults_.gains.red = redGain;
> + asyncResults_.gains.green = 1.0;
> + asyncResults_.gains.blue = blueGain;
> }
>
> void Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats,
> @@ -270,8 +273,8 @@ void Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats,
> LOG(IPU3Awb, Debug) << "Valid zones: " << zones_.size();
> if (zones_.size() > 10) {
> awbGreyWorld();
> - LOG(IPU3Awb, Debug) << "Gain found for red: " << asyncResults_.redGain
> - << " and for blue: " << asyncResults_.blueGain;
> + LOG(IPU3Awb, Debug) << "Gain found for red: " << asyncResults_.gains.red
> + << " and for blue: " << asyncResults_.gains.blue;
> }
> }
>
> @@ -284,9 +287,9 @@ void Awb::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
> * The results are cached, so if no results were calculated, we set the
> * cached values from asyncResults_ here.
> */
> - context.frameContext.awb.gains.blue = asyncResults_.blueGain;
> - context.frameContext.awb.gains.green = asyncResults_.greenGain;
> - context.frameContext.awb.gains.red = asyncResults_.redGain;
> + context.frameContext.awb.gains.blue = asyncResults_.gains.blue;
> + context.frameContext.awb.gains.green = asyncResults_.gains.green;
> + context.frameContext.awb.gains.red = asyncResults_.gains.red;
> }
>
> void Awb::prepare(IPAContext &context, ipu3_uapi_params *params)
> diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h
> index a16dd68d..f7e2f4cd 100644
> --- a/src/ipa/ipu3/algorithms/awb.h
> +++ b/src/ipa/ipu3/algorithms/awb.h
> @@ -23,6 +23,38 @@ namespace ipa::ipu3::algorithms {
> static constexpr uint32_t kAwbStatsSizeX = 16;
> static constexpr uint32_t kAwbStatsSizeY = 12;
>
> +/* \todo Move the cell layout into intel-ipu3.h kernel header */
> +struct Ipu3AwbCell {
> + unsigned char greenRedAvg;
> + unsigned char redAvg;
> + unsigned char blueAvg;
> + unsigned char greenBlueAvg;
> + unsigned char satRatio;
> + unsigned char padding[3];
> +} __attribute__((packed));
> +
> +/* \todo Make these structs available to all the ISPs ? */
> +struct RGB {
> + RGB(double _R = 0, double _G = 0, double _B = 0)
> + : R(_R), G(_G), B(_B)
> + {
> + }
> + double R, G, B;
> + RGB &operator+=(RGB const &other)
> + {
> + R += other.R, G += other.G, B += other.B;
> + return *this;
> + }
> +};
There's nothing AWB-specific in this structure, this is really not the
right place to store it. Given that it's not used in the rest of this
series, you can take the easy option and leave it where it is.
> +
This is missing a \todo comment to tell where this should be moved, but
the best would be to move it already.
> +struct Accumulator {
> + unsigned int total;
> + unsigned int counted;
> + unsigned long long rSum;
> + unsigned long long gSum;
> + unsigned long long bSum;
Another patch in this series should change the type of the last three
fields to uint64_t. If we follow the AwbStatus model, you could also
change it to
struct Accumulator {
unsigned int total;
unsigned int counted;
struct {
uint64_t red;
uint64_t green;
uint64_t blue;
} sum;
};
Up to you for this last change (which can be in the same patch as the
switch to uint64_t if you decide to change the structure layout).
> +};
> +
> class Awb : public Algorithm
> {
> public:
> @@ -32,44 +64,6 @@ public:
> void prepare(IPAContext &context, ipu3_uapi_params *params) override;
> void process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;
>
> - struct Ipu3AwbCell {
> - unsigned char greenRedAvg;
> - unsigned char redAvg;
> - unsigned char blueAvg;
> - unsigned char greenBlueAvg;
> - unsigned char satRatio;
> - unsigned char padding[3];
> - } __attribute__((packed));
> -
> - /* \todo Make these three structs available to all the ISPs ? */
> - struct RGB {
> - RGB(double _R = 0, double _G = 0, double _B = 0)
> - : R(_R), G(_G), B(_B)
> - {
> - }
> - double R, G, B;
> - RGB &operator+=(RGB const &other)
> - {
> - R += other.R, G += other.G, B += other.B;
> - return *this;
> - }
> - };
> -
> - struct IspStatsRegion {
> - unsigned int counted;
> - unsigned int uncounted;
> - unsigned long long rSum;
> - unsigned long long gSum;
> - unsigned long long bSum;
> - };
> -
> - struct AwbStatus {
> - double temperatureK;
> - double redGain;
> - double greenGain;
> - double blueGain;
> - };
> -
> private:
> void calculateWBGains(const ipu3_uapi_stats_3a *stats,
> const ipu3_uapi_grid_config &grid);
> @@ -80,8 +74,17 @@ private:
> void awbGreyWorld();
> uint32_t estimateCCT(double red, double green, double blue);
>
> + struct AwbStatus {
> + double temperatureK;
> + struct {
> + double red;
> + double green;
> + double blue;
> + } gains;
> + };
> +
> std::vector<RGB> zones_;
> - IspStatsRegion awbStats_[kAwbStatsSizeX * kAwbStatsSizeY];
> + Accumulator awbStats_[kAwbStatsSizeX * kAwbStatsSizeY];
> AwbStatus asyncResults_;
> };
>
> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> index 9d9444dc..3a292ad7 100644
> --- a/src/ipa/ipu3/ipa_context.h
> +++ b/src/ipa/ipu3/ipa_context.h
> @@ -30,6 +30,7 @@ struct IPAFrameContext {
> } agc;
>
> struct {
> + double temperatureK;
> struct {
> double red;
> double green;
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 0ed0a6f1..cbb3f440 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -112,6 +112,9 @@
> * \struct IPAFrameContext::awb
> * \brief Context for the Automatic White Balance algorithm
> *
> + * \var IPAFrameContext::awb::temperatureK
> + * \brief Estimated color temperature in Kelvin
> + *
> * \struct IPAFrameContext::awb::gains
> * \brief White balance gains
> *
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list