[libcamera-devel] [PATCH v2 1/4] ipa: ipu3: Move the AWB stats structures
Jean-Michel Hautbois
jeanmichel.hautbois at ideasonboard.com
Tue Aug 31 10:32:18 CEST 2021
Hi Laurent,
On 31/08/2021 00:59, Laurent Pinchart wrote:
> 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.
Oh, yeah, I removed it in another branch (-ETOOMUCHBRANCHES)...
Sorry for the noise here...
> 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
Those are already in awb.h, just getting out from
libcamera::ipa::ipu3::algorithms::awb to
libcamera::ipa::ipu3::algorithms directly.
> - Rename IspStatsRegion to Accumulator
One patch only for renaming the structure ? Sounds like a really short one ?
> - Replace Accumulator::uncounted with Accumulator::total
> - Fix the documentation of Accumulator::uncounted and Accumulator::total
> (see below)
Thanks (but isn't only one patch enough for everything related to
IspStatsRegion ?).
> 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.
>
Indeed :-).
>> }
>> }
>> }
>> @@ -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.
>
That's probably what I will do ;-).
>> +
>
> 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).
It could be in the same patch as uncounted->total probably.
>> +};
>> +
>> 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
>> *
>
More information about the libcamera-devel
mailing list