[libcamera-devel] [RFC PATCH 2/2] WIP: ipa: ipu3: Add support for IPU3 AWB algorithm
Jean-Michel Hautbois
jeanmichel.hautbois at ideasonboard.com
Mon Feb 22 21:32:02 CET 2021
Hi Dafna,
On 22/02/2021 19:50, Dafna Hirschfeld wrote:
> Hi,
>
> On 22.02.21 05:56, Laurent Pinchart wrote:
>> Hi Jean-Michel,
>>
>> Thank you for the patch.
>>
>> On Fri, Feb 19, 2021 at 06:22:24PM +0100, Jean-Michel Hautbois wrote:
>>> Inherit from the AWBAlgorithm and AGCAlgorithm classes to implement
>>> basic functions.
>>> While exposure is not locked, AWB is not calculated and corrected.
>>> Once AWB is done, a color temperature is estimated and default CCM
>>> matrices
>>> are used.
>>> Implement a basic "grey-world" AWB algorithm just for demonstration
>>> purpose.
>
> Are those algorithms specific to IPU3 ?
> If not then maybe it can used by rkisp1 as well?
> Maybe it worth to think of general API for basic 3a implementations that
> other IPAs can use?
Not at all ! And I will send a second patch version which will show how
to implement the image processing algorithm, and indeed it aims to be
platform agnostic (even if the parameters and statistics are always
specific ;-)) !
>>
>> Nice to see an initial implementation ! I'll provide a handful of review
>> comments below already. Some are related to coding style or similar
>> issues and should be addressed in v2. Others are related to the IPA
>> architecture, and could be fixed on top of this series to avoid delaying
>> it unnecessarily. They should in that case be captured in a \todo
>> comment.
>>
>> As for 1/2, I won't repeat the same comments all over the patch, so
>> please apply them wherever they're applicable.
>>
>>> Signed-off-by: Jean-Michel Hautbois
>>> <jeanmichel.hautbois at ideasonboard.com>
>>> ---
>>> src/ipa/ipu3/ipu3.cpp | 31 ++++-
>>> src/ipa/ipu3/ipu3_agc.cpp | 195 +++++++++++++++++++++++++++
>>> src/ipa/ipu3/ipu3_agc.h | 96 +++++++++++++
>>> src/ipa/ipu3/ipu3_awb.cpp | 182 +++++++++++++++++++++++++
>>> src/ipa/ipu3/ipu3_awb.h | 130 ++++++++++++++++++
>>> src/ipa/ipu3/meson.build | 8 +-
>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 1 +
>>> 7 files changed, 638 insertions(+), 5 deletions(-)
>>> create mode 100644 src/ipa/ipu3/ipu3_agc.cpp
>>> create mode 100644 src/ipa/ipu3/ipu3_agc.h
>>> create mode 100644 src/ipa/ipu3/ipu3_awb.cpp
>>> create mode 100644 src/ipa/ipu3/ipu3_awb.h
>>>
>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
>>> index b63e58be..c3859f39 100644
>>> --- a/src/ipa/ipu3/ipu3.cpp
>>> +++ b/src/ipa/ipu3/ipu3.cpp
>>> @@ -13,6 +13,7 @@
>>> #include <libcamera/buffer.h>
>>> #include <libcamera/control_ids.h>
>>> +#include <libcamera/ipa/ipa_controller.h>
>>> #include <libcamera/ipa/ipa_interface.h>
>>> #include <libcamera/ipa/ipa_module_info.h>
>>> #include <libcamera/ipa/ipu3_ipa_interface.h>
>>> @@ -21,6 +22,9 @@
>>> #include "libcamera/internal/buffer.h"
>>> #include "libcamera/internal/log.h"
>>> +#include "ipu3_agc.h"
>>> +#include "ipu3_awb.h"
>>> +
>>> namespace libcamera {
>>> LOG_DEFINE_CATEGORY(IPAIPU3)
>>> @@ -28,6 +32,9 @@ LOG_DEFINE_CATEGORY(IPAIPU3)
>>> class IPAIPU3 : public ipa::ipu3::IPAIPU3Interface
>>> {
>>> public:
>>> + IPAIPU3()
>>> + : controller_() {}
>>
>> : controller_()
>> {
>> }
>>
>> I may not repeat all the comments from 1/2, but they're generally
>> applicable to this patch as well.
>>
>>> +
>>> int init([[maybe_unused]] const IPASettings &settings) override
>>> {
>>> return 0;
>>> @@ -60,6 +67,11 @@ private:
>>> uint32_t gain_;
>>> uint32_t minGain_;
>>> uint32_t maxGain_;
>>> +
>>> + IPAController controller_;
>>> + IPU3Awb *awbAlgo_;
>>> + IPU3Agc *agcAlgo_;
>>> + ipu3_uapi_params params_;
>>> };
>>> void IPAIPU3::configure(const std::map<uint32_t, ControlInfoMap>
>>> &entityControls)
>>> @@ -83,11 +95,18 @@ void IPAIPU3::configure(const std::map<uint32_t,
>>> ControlInfoMap> &entityControls
>>> minExposure_ = std::max(itExp->second.min().get<int32_t>(), 1);
>>> maxExposure_ = itExp->second.max().get<int32_t>();
>>> - exposure_ = maxExposure_;
>>> + exposure_ = 123;
>>
>> A peculiar value :-)
>>
>>> minGain_ = std::max(itGain->second.min().get<int32_t>(), 1);
>>> maxGain_ = itGain->second.max().get<int32_t>();
>>> - gain_ = maxGain_;
>>> + gain_ = 1;
>>> +
>>> + awbAlgo_ = new IPU3Awb(&controller_);
>>> + awbAlgo_->Initialise(params_);
>>> + agcAlgo_ = new IPU3Agc(&controller_);
>>> +
>>> + /*\todo not used yet... */
>>> + controller_.Initialise();
>>> setControls(0);
>>> }
>>> @@ -162,10 +181,10 @@ void IPAIPU3::processControls([[maybe_unused]]
>>> unsigned int frame,
>>> void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
>>> {
>>> /* Prepare parameters buffer. */
>>> - memset(params, 0, sizeof(*params));
>>> + awbAlgo_->updateBNR(params_);
>>> + memcpy(params, ¶ms_, sizeof(*params));
>>
>> *params = params_;
>>
>>> /* \todo Fill in parameters buffer. */
>>> -
>>> ipa::ipu3::IPU3Action op;
>>> op.op = ipa::ipu3::ActionParamFilled;
>>> @@ -179,6 +198,10 @@ void IPAIPU3::parseStatistics(unsigned int frame,
>>> /* \todo React to statistics and update internal state
>>> machine. */
>>> /* \todo Add meta-data information to ctrls. */
>>> + agcAlgo_->Process(stats, exposure_, gain_);
>>> + if (agcAlgo_->Converged())
>>> + awbAlgo_->calculateWBGains(Rectangle(250, 160, 800, 400),
>>> stats);
>>> + setControls(frame);
>>> ipa::ipu3::IPU3Action op;
>>> op.op = ipa::ipu3::ActionMetadataReady;
>>> diff --git a/src/ipa/ipu3/ipu3_agc.cpp b/src/ipa/ipu3/ipu3_agc.cpp
>>> new file mode 100644
>>> index 00000000..db591e33
>>> --- /dev/null
>>> +++ b/src/ipa/ipu3/ipu3_agc.cpp
>>> @@ -0,0 +1,195 @@
>>> +/* SPDX-License-Identifier: BSD-2-Clause */
>>> +/*
>>> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited
>>> + *
>>> + * agc.cpp - AGC/AEC control algorithm
>>> + */
>>> +#include <iostream>
>>> +#include <numeric>
>>> +#include <unordered_map>
>>> +
>>> +#include "libcamera/internal/log.h"
>>> +
>>> +#include "ipu3_agc.h"
>>> +
>>> +using namespace libcamera;
>>> +
>>> +LOG_DEFINE_CATEGORY(IPU3Agc)
>>> +
>>> +#define NAME "ipu3.agc"
>>> +
>>> +IPU3Agc::IPU3Agc(IPAController *controller)
>>> + : AgcAlgorithm(controller), frameCount_(0),
>>> + ev_(1.0), flicker_period_(0.0),
>>> + max_shutter_(0), fixed_shutter_(0),
>>> + fixed_analogue_gain_(0.0), algoConverged_(false)
>>> +{
>>> +}
>>> +
>>> +char const *IPU3Agc::Name() const
>>> +{
>>> + return NAME;
>>> +}
>>> +
>>> +unsigned int IPU3Agc::GetConvergenceFrames() const
>>> +{
>>> + return config_.convergence_frames;
>>> +}
>>> +
>>> +void IPU3Agc::SetEv(double ev)
>>> +{
>>> + ev_ = ev;
>>> +}
>>> +
>>> +void IPU3Agc::SetFlickerPeriod(double flicker_period)
>>> +{
>>> + flicker_period_ = flicker_period;
>>> +}
>>> +
>>> +void IPU3Agc::SetMaxShutter(double max_shutter)
>>> +{
>>> + max_shutter_ = max_shutter;
>>> +}
>>> +
>>> +void IPU3Agc::SetFixedShutter(double fixed_shutter)
>>> +{
>>> + fixed_shutter_ = fixed_shutter;
>>> +}
>>> +
>>> +void IPU3Agc::SetFixedAnalogueGain(double fixed_analogue_gain)
>>> +{
>>> + fixed_analogue_gain_ = fixed_analogue_gain;
>>> +}
>>> +
>>> +void IPU3Agc::SetMeteringMode(std::string const &metering_mode_name)
>>> +{
>>> + metering_mode_name_ = metering_mode_name;
>>> +}
>>> +
>>> +void IPU3Agc::SetExposureMode(std::string const &exposure_mode_name)
>>> +{
>>> + exposure_mode_name_ = exposure_mode_name;
>>> +}
>>> +
>>> +void IPU3Agc::Prepare() {}
>>> +
>>> +void IPU3Agc::Process() {}
>>> +
>>> +/* \todo This function is taken from numerical recipes and
>>> calculates all moments
>>> + * It needs to be rewritten properly and maybe in a "math" class ? */
>>
>> /*
>> * \todo This function is taken from numerical recipes and calculates
>> all
>> * moments. It needs to be rewritten properly and maybe in a "math"
>> class ?
>> */
>>
>>> +void IPU3Agc::moments(std::unordered_map<uint32_t, uint32_t> &data,
>>> int n)
>
> 'data' is the hist, so why not call it 'hist' here?
Yes, why not :-) thanks !
>>> +{
>>> + int j;
>>> + double ep = 0.0, s, p;
>>> + double ave, adev, sdev;
>>> + double var, skew, curt;
>>> +
>>> + s = 0.0;
>>> + for (j = 1; j <= n; j++)
>>> + s += data[j];
>
> shouldn't that be:
> s += data[j] * j
>
> since data[j] is the number of pixels with value j
I don't think so, no, but I will have to verify...
>>> +
>>> + ave = s / n;
>>> + adev = var = skew = curt = 0.0;
>>> +
>>> + for (j = 1; j <= n; j++) {
>>> + adev += s = data[j] - (ave);
>>> + ep += s;
>>> + var += (p = s * s);
>>> + skew += (p *= s);
>>> + curt += (p *= s);
>>> + }
>>> +
>>> + adev /= n;
>>> + var = (var - ep * ep / n) / (n - 1);
>>> + sdev = std::sqrt(var);
>>> +
>>> + if (var) {
>>> + skew /= n * var * sdev;
>>> + curt = curt / (n * var * var) - 3.0;
>>> + }
>>> + skew_ = skew;
>>> +}
>>> +
>>> +void IPU3Agc::processBrightness(const ipu3_uapi_stats_3a *stats)
>>> +{
>>> + brightnessVec_.clear();
>>> +
>>> + /*\todo Replace constant values with real BDS configuration */
>>> + for (uint32_t j = 0; j < 45; j++) {
>>> + for (uint32_t i = 0; i < 160 * 45 * 8; i += 8) {
>>> + uint8_t Gr = stats->awb_raw_buffer.meta_data[i];
>>> + uint8_t R = stats->awb_raw_buffer.meta_data[i + 1];
>>> + uint8_t B = stats->awb_raw_buffer.meta_data[i + 2];
>>> + uint8_t Gb = stats->awb_raw_buffer.meta_data[i + 3];
>>> + brightnessVec_.push_back(static_cast<uint32_t>(0.299 * R
>>> + 0.587 * (Gr + Gb) / 2 + 0.114 * B));
>
> You iterate on j in the outer loop but don't use j anywhere for
> indexing, this is a bit suspicious
This is just for an easier splitting of rectangles (taken from the AWB
algorithm).
>>> + }
>>> + }
>>> + std::sort(brightnessVec_.begin(), brightnessVec_.end());
>
> I don't think the sort is is necessary
It is there from an old calculation of median value. Not needed indeed.
>>> +
>>> + /* \todo create a class to generate histograms ! */
>>> + std::unordered_map<uint32_t, uint32_t> hist;
>
> you want to have up to 256 indexes so you need to define with uint8_t
>
> std::unordered_map<uint8_t, uint32_t> hist;
ack
>
>>> + for (uint32_t const &val : brightnessVec_)
>>> + hist[val]++;
>>> + moments(hist, 256);
>>> +}
>>> +
>>> +void IPU3Agc::lockExposure(uint32_t &exposure, uint32_t &gain)
>>> +{
>>> + /* Algorithm initialization wait for first valid frames */
>>> + /* \todo - have a number of frames given by DelayedControls ?
>>> + * - implement a function for IIR */
>>> + if (frameCount_ == 4) {
>>> + prevExposure_ = exposure;
>>> +
>>> + prevSkew_ = skew_;
>>> + /* \tdo use configured values */
>>
>> s/tdo/todo/
>>
>>> + exposure = 800;
>>> + gain = 8;
>>> + currentExposure_ = exposure;
>>> + } else if (frameCount_ == 8) {
>>> + currentSkew_ = skew_;
>>> + exposure = ((currentExposure_ * prevSkew_) + (prevExposure_
>>> * currentSkew_)) / (prevSkew_ + currentSkew_);
>>
>> Line wrap. No need for the inner parentheses.
>>
>>> + nextExposure_ = exposure;
>>> + lastFrame_ = frameCount_;
>>> + } else if ((frameCount_ >= 12) && (frameCount_ - lastFrame_ >=
>>> 4)) {
>>
>> Could you define constants for magic values, to make the code more
>> readable ? They can be defined as static constexpr members of the
>> IPU3Agc class, and should be named with camelCase. While we don't do so
>> yet in many places, it's customary to start them with a 'k' prefix (e.g.
>> kInitialFrameSkipCount).
>>
>>> + currentSkew_ = skew_;
>>> + /* \todo properly calculate a gain */
>>> + if (frameCount_ == 12)
>>> + gain = ((8 * prevSkew_) + (1 * currentSkew_)) /
>>> (prevSkew_ + currentSkew_);
>>> +
>>> + if (currentSkew_ - prevSkew_ > 1) {
>>> + /* under exposed */
>>> + prevExposure_ = nextExposure_;
>>> + exposure = ((currentExposure_ * prevSkew_) +
>>> (prevExposure_ * currentSkew_)) / (prevSkew_ + currentSkew_);
>>> + nextExposure_ = exposure;
>>> + } else if (currentSkew_ - prevSkew_ < -1) {
>>> + /* over exposed */
>>> + currentExposure_ = nextExposure_;
>>> + exposure = ((currentExposure_ * prevSkew_) +
>>> (prevExposure_ * currentSkew_)) / (prevSkew_ + currentSkew_);
>>> + nextExposure_ = exposure;
>>> + } else {
>>> + /* we have converged */
>>> + algoConverged_ = true;
>>> + }
>>> + lastFrame_ = frameCount_;
>>> + prevSkew_ = currentSkew_;
>>> + }
>
> Could you explain the general idea behind this algorithm or give a link
> where it is explained?
Yes, I will, and this algorithm will evolve as I want to use Q3-Q1
inter-quartile measure to optimize auto exposure.
> Thanks,
> Dafna
>
>>> +}
>>> +
>>> +void IPU3Agc::Process(const ipu3_uapi_stats_3a *stats, uint32_t
>>> &exposure, uint32_t &gain)
>>> +{
>>> + processBrightness(stats);
>>> + if (!algoConverged_)
>>> + lockExposure(exposure, gain);
>>> + else {
>>> + /* Are we still well exposed ? */
>>> + if ((skew_ < 2) || (skew_ > 4))
>>> + algoConverged_ = false;
>>> + }
>>> + frameCount_++;
>>> +}
>>> +
>>> +bool IPU3Agc::Converged()
>>> +{
>>> + return algoConverged_;
>>> +}
>>> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h
>>> new file mode 100644
>>> index 00000000..9a69d628
>>> --- /dev/null
>>> +++ b/src/ipa/ipu3/ipu3_agc.h
>>> @@ -0,0 +1,96 @@
>>> +/* SPDX-License-Identifier: BSD-2-Clause */
>>> +/*
>>> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited
>>> + *
>>> + * ipu3_agc.h - AGC/AEC control algorithm
>>> + */
>>> +#ifndef __LIBCAMERA_IPU3_AGC_H__
>>> +#define __LIBCAMERA_IPU3_AGC_H__
>>> +
>>
>> #include <map>
>>
>>> +#include <mutex>
>>
>> This doesn't seem to be needed.
>>
>>> +#include <vector>
>>> +
>>> +#include <linux/intel-ipu3.h>
>>> +
>>> +#include <libcamera/geometry.h>
>>> +
>>> +#include <libcamera/ipa/agc_algorithm.h>
>>> +#include <libcamera/ipa/ipa_controller.h>
>>> +
>>> +#define AGC_STATS_SIZE 15
>>> +
>>> +namespace libcamera {
>>> +
>>> +struct AgcMeteringMode {
>>> + double weights[AGC_STATS_SIZE];
>>> + Rectangle metering_region[AGC_STATS_SIZE];
>>> +};
>>> +
>>> +struct AgcExposureMode {
>>> + std::vector<double> shutter;
>>> + std::vector<double> gain;
>>> +};
>>> +
>>> +struct AgcConfig {
>>> + std::map<std::string, AgcMeteringMode> metering_modes;
>>> + std::map<std::string, AgcExposureMode> exposure_modes;
>>> + double speed;
>>> + uint16_t startup_frames;
>>> + unsigned int convergence_frames;
>>> + std::string default_metering_mode;
>>> + std::string default_exposure_mode;
>>> + double default_exposure_time;
>>> + double default_analogue_gain;
>>> +};
>>> +
>>> +class IPU3Agc : public AgcAlgorithm
>>> +{
>>> +public:
>>> + IPU3Agc(IPAController *IPAcontroller);
>>
>> Blank line.
>>
>>> + char const *Name() const override;
>>> + unsigned int GetConvergenceFrames() const override;
>>> + void SetEv(double ev) override;
>>> + void SetFlickerPeriod(double flicker_period) override;
>>> + void SetMaxShutter(double max_shutter) override; // microseconds
>>> + void SetFixedShutter(double fixed_shutter) override; //
>>> microseconds
>>> + void SetFixedAnalogueGain(double fixed_analogue_gain) override;
>>> + void SetMeteringMode(std::string const &metering_mode_name)
>>> override;
>>> + void SetExposureMode(std::string const &exposure_mode_name)
>>> override;
>>> + void Prepare() override;
>>> + void Process() override;
>>> + void Process(const ipu3_uapi_stats_3a *stats, uint32_t
>>> &exposure, uint32_t &gain);
>>
>> The last two arguments should be replaced by a generic data container,
>> similar to the Metadata class in the RPi IPA (not something to fix now).
>>
>>> + bool Converged();
>>> +
>>> +private:
>>> + void moments(std::unordered_map<uint32_t, uint32_t> &data, int n);
>>> + void processBrightness(const ipu3_uapi_stats_3a *stats);
>>> + void lockExposure(uint32_t &exposure, uint32_t &gain);
>>> +
>>> + AgcConfig config_;
>>
>> This is never set, and only one member is read. I'd drop it for now.
>>
>>> + std::string metering_mode_name_;
>>> + std::string exposure_mode_name_;
>>> + uint64_t frameCount_;
>>> + uint64_t lastFrame_;
>>> +
>>> + /* Vector of calculated brightness for each cell */
>>> + std::vector<uint32_t> brightnessVec_;
>>
>> We avoid encoding the type in the variable name. Would this be better
>> named cellsBrightness_ ?
>>
>>> + double ev_;
>>
>> Never used. Same for other members below.
>>
>>> + double flicker_period_;
>>> + double max_shutter_;
>>> + double fixed_shutter_;
>>> + double fixed_analogue_gain_;
>>> +
>>> + /* Values for filtering */
>>> + uint32_t prevExposure_;
>>> + uint32_t currentExposure_;
>>> + uint32_t nextExposure_;
>>> +
>>> + double skew_;
>>> + double prevSkew_;
>>> + double currentSkew_;
>>> + bool algoConverged_;
>>
>> Maybe just converged_ ?
>>
>>> +};
>>> +
>>> +} /* namespace libcamera */
>>> +
>>> +#endif /* __LIBCAMERA_IPU3_AGC_H__ */
>>> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp
>>> new file mode 100644
>>> index 00000000..21286e4c
>>> --- /dev/null
>>> +++ b/src/ipa/ipu3/ipu3_awb.cpp
>>> @@ -0,0 +1,182 @@
>>> +/* SPDX-License-Identifier: BSD-2-Clause */
>>> +/*
>>> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited
>>> + *
>>> + * ipu3_awb.cpp - AWB control algorithm
>>> + */
>>> +#include <iostream>
>>
>> I don't think this is needed.
>>
>>> +#include <numeric>
>>> +#include <unordered_map>
>>> +
>>> +#include "libcamera/internal/log.h"
>>> +
>>> +#include "ipu3_awb.h"
>>> +
>>> +using namespace libcamera;
>>> +
>>> +LOG_DEFINE_CATEGORY(IPU3Awb)
>>> +
>>> +#define NAME "ipu3.awb"
>>> +
>>> +IPU3Awb::IPU3Awb(IPAController *controller)
>>> + : AwbAlgorithm(controller)
>>> +{
>>> +}
>>> +
>>> +IPU3Awb::~IPU3Awb()
>>> +{
>>> +}
>>> +
>>> +char const *IPU3Awb::Name() const
>>> +{
>>> + return NAME;
>>> +}
>>> +
>>> +void IPU3Awb::Initialise() {}
>>
>> void IPU3Awb::Initialise()
>> {
>> }
>>
>>> +
>>> +void IPU3Awb::Initialise(ipu3_uapi_params ¶ms)
>>> +{
>>> + memset(¶ms, 0, sizeof(params));
>>> + memset(wbGains_, 8192, sizeof(wbGains_));
>>
>> memset() initializes the memory byte by byte, this will not store 8192
>> in each entry of the array.
>>
>>
>>> + wbGains_[0] = 8192 * 0.8;
>>> + wbGains_[3] = 8192 * 0.8;
>>
>> I would group the initialization of member variables together, either
>> before, or after the initialization of params.
>>
>>> + params.use.acc_awb = 1;
>>> + /*\todo fill the grid calculated based on BDS configuration */
>>> + params.acc_param.awb.config = imgu_css_awb_defaults;
>>> +
>>> + params.use.acc_bnr = 1;
>>> + params.acc_param.bnr = imgu_css_bnr_defaults;
>>> +
>>> + params.use.acc_ccm = 1;
>>> + params.acc_param.ccm = imgu_css_ccm_3800k;
>>> +
>>> + params.use.acc_gamma = 1;
>>> + params.acc_param.gamma.gc_ctrl.enable = 1;
>>> +
>>> + uint32_t a = (32 * 245) / (245 - 9);
>>> +
>>> + for (uint32_t i = 0; i < 10; i++)
>>> + params.acc_param.gamma.gc_lut.lut[i] = 0;
>>> + for (uint32_t i = 10; i < 245; i++)
>>> + params.acc_param.gamma.gc_lut.lut[i] = a * i + (0 - a * 9);
>>> + for (uint32_t i = 245; i < 255; i++)
>>> + params.acc_param.gamma.gc_lut.lut[i] = 32 * 245;
>>> +
>>> + frame_count_ = 0;
>>> + algoConverged_ = false;
>>> +}
>>> +
>>> +unsigned int IPU3Awb::GetConvergenceFrames() const
>>> +{
>>> + // If colour gains have been explicitly set, there is no
>>> convergence
>>> + // to happen, so no need to drop any frames - return zero.
>>> + if (manual_r_ && manual_b_)
>>> + return 0;
>>> + else
>>> + return config_.convergence_frames;
>>> +}
>>> +
>>> +void IPU3Awb::SetMode(std::string const &mode_name)
>>> +{
>>> + mode_name_ = mode_name;
>>> +}
>>> +
>>> +void IPU3Awb::SetManualGains(double manual_r, double manual_b)
>>> +{
>>> + // If any of these are 0.0, we swich back to auto.
>>> + manual_r_ = manual_r;
>>> + manual_b_ = manual_b;
>>> +}
>>> +
>>> +uint32_t IPU3Awb::estimateCCT(uint8_t R, uint8_t G, uint8_t B)
>>> +{
>>> + double X = (-0.14282) * (R) + (1.54924) * (G) + (-0.95641) * (B);
>>> + double Y = (-0.32466) * (R) + (1.57837) * (G) + (-0.73191) * (B);
>>> + double Z = (-0.68202) * (R) + (0.77073) * (G) + (0.56332) * (B);
>>> +
>>> + double x = X / (X + Y + Z);
>>> + double y = Y / (X + Y + Z);
>>> +
>>> + double n = (x - 0.3320) / (0.1858 - y);
>>> + return static_cast<uint32_t>(449 * n * n * n + 3525 * n * n +
>>> 6823.3 * n + 5520.33);
>>> +}
>>> +
>>> +void IPU3Awb::calculateWBGains([[maybe_unused]] Rectangle roi, const
>>> ipu3_uapi_stats_3a *stats)
>>
>> Line wrap.
>>
>>> +{
>>> + if (algoConverged_)
>>> + return;
>>
>> So once it converges, you never update the gains anymore, even if the
>> scene changes ?
>>
>>> +
>>> + std::vector<uint32_t> R_v, Gr_v, Gb_v, B_v;
>>
>> camelCase. Feel free to also make the name slightly longer to make the
>> more explicit. redValues ?
>>
>>> + Point topleft = roi.topLeft();
>>> + uint32_t startY = (topleft.y / 16) * 160 * 8;
>>> + uint32_t startX = (topleft.x / 8) * 8;
>>> + uint32_t endX = (startX + (roi.size().width / 8)) * 8;
>>> +
>>> + for (uint32_t j = (topleft.y / 16); j < (topleft.y / 16) +
>>> (roi.size().height / 16); j++) {
>>> + for (uint32_t i = startX + startY; i < endX + startY; i += 8) {
>>> + Gr_v.push_back(stats->awb_raw_buffer.meta_data[i]);
>>> + R_v.push_back(stats->awb_raw_buffer.meta_data[i + 1]);
>>> + B_v.push_back(stats->awb_raw_buffer.meta_data[i + 2]);
>>> + Gb_v.push_back(stats->awb_raw_buffer.meta_data[i + 3]);
>>> + }
>>> + }
>>> +
>>> + std::sort(R_v.begin(), R_v.end());
>>> + std::sort(Gr_v.begin(), Gr_v.end());
>>> + std::sort(B_v.begin(), B_v.end());
>>> + std::sort(Gb_v.begin(), Gb_v.end());
>>> +
>>> + double Grmed = Gr_v[Gr_v.size() / 2];
>>> + double Rmed = R_v[R_v.size() / 2];
>>> + double Bmed = B_v[B_v.size() / 2];
>>> + double Gbmed = Gb_v[Gb_v.size() / 2];
>>> +
>>> + double Rgain = Grmed / Rmed;
>>> + double Bgain = Gbmed / Bmed;
>>> + LOG(IPU3Awb, Debug) << "max R, Gr, B, Gb: "
>>> + << R_v.back() << ","
>>> + << Gr_v.back() << ","
>>> + << B_v.back() << ","
>>> + << Gb_v.back();
>>> + tint_ = ((Rmed / Grmed) + (Bmed / Gbmed)) / 2;
>>> +
>>> + /* \todo Those are corrections when light is really low
>>> + * it should be taken into account by AGC somehow */
>>> + if ((Rgain >= 2) && (Bgain < 2)) {
>>> + wbGains_[0] = 4096 * tint_;
>>> + wbGains_[1] = 8192 * Rgain;
>>> + wbGains_[2] = 4096 * Bgain;
>>> + wbGains_[3] = 4096 * tint_;
>>> + } else if ((Bgain >= 2) && (Rgain < 2)) {
>>> + wbGains_[0] = 4096 * tint_;
>>> + wbGains_[1] = 4096 * Rgain;
>>> + wbGains_[2] = 8192 * Bgain;
>>> + wbGains_[3] = 4096 * tint_;
>>> + } else {
>>> + wbGains_[0] = 8192 * tint_;
>>> + wbGains_[1] = 8192 * Rgain;
>>> + wbGains_[2] = 8192 * Bgain;
>>> + wbGains_[3] = 8192 * tint_;
>>> + }
>>> +
>>> + frame_count_++;
>>> +
>>> + cct_ = estimateCCT(Rmed, (Grmed + Gbmed) / 2, Bmed);
>>> +
>>> + algoConverged_ = true;
>>> +}
>>> +
>>> +void IPU3Awb::updateBNR(ipu3_uapi_params ¶ms)
>>
>> updateBNR() isn't a great name for a function that updates the WB gains,
>> even if they're part of the Bayer noise reduction block of data.
>>
>> Should we push the ipu3_uapi_params structure down to individual
>> algorithms, or would it be better to decouple it, creating custom
>> structures to hold algorithm results, and fillin ipu3_uapi_params in
>> IPAIPU3::fillParams() instead ? On one hand moving ipu3_uapi_params out
>> will make the code more generic, and thus more reusable, but on the
>> other hand we would still depend on a custom stats format, so it may not
>> be worth it.
>>
>>> +{
>>> + if (!algoConverged_)
>>> + return;
>>> +
>>> + params.acc_param.bnr.wb_gains.gr = wbGains_[0];
>>> + params.acc_param.bnr.wb_gains.r = wbGains_[1];
>>> + params.acc_param.bnr.wb_gains.b = wbGains_[2];
>>> + params.acc_param.bnr.wb_gains.gb = wbGains_[3];
>>> + if (cct_ < 5500)
>>> + params.acc_param.ccm = imgu_css_ccm_3800k;
>>> + else
>>> + params.acc_param.ccm = imgu_css_ccm_6000k;
>>> +}
>>> diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h
>>> new file mode 100644
>>> index 00000000..06389020
>>> --- /dev/null
>>> +++ b/src/ipa/ipu3/ipu3_awb.h
>>> @@ -0,0 +1,130 @@
>>> +/* SPDX-License-Identifier: BSD-2-Clause */
>>> +/*
>>> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited
>>> + *
>>> + * awb.h - AWB control algorithm
>>> + */
>>> +#ifndef __LIBCAMERA_IPU3_AWB_H__
>>> +#define __LIBCAMERA_IPU3_AWB_H__
>>> +
>>> +#include <linux/intel-ipu3.h>
>>> +
>>> +#include <libcamera/geometry.h>
>>> +
>>> +#include <libcamera/ipa/awb_algorithm.h>
>>> +#include <libcamera/ipa/ipa_controller.h>
>>> +
>>> +namespace libcamera {
>>> +
>>> +const struct ipu3_uapi_bnr_static_config imgu_css_bnr_defaults = {
>>> + { 16, 16, 16, 16 }, /* wb_gains */
>>> + { 255, 255, 255, 255 }, /* wb_gains_thr */
>>> + { 0, 0, 8, 6, 0, 14 }, /* thr_coeffs */
>>> + { 0, 0, 0, 0 }, /* thr_ctrl_shd */
>>> + { -648, 0, -366, 0 }, /* opt_center */
>>> + { /* lut */
>>> + { 17, 23, 28, 32, 36, 39, 42, 45,
>>> + 48, 51, 53, 55, 58, 60, 62, 64,
>>> + 66, 68, 70, 72, 73, 75, 77, 78,
>>> + 80, 82, 83, 85, 86, 88, 89, 90 }
>>> + },
>>> + { 4, 0, 1, 8, 0, 8, 0, 8, 0 }, /* bp_ctrl */
>>> + { 8, 4, 4, 0, 8, 0, 1, 1, 1, 1, 0 }, /* dn_detect_ctrl */
>>> + 1296,
>>> + { 419904, 133956 },
>>> +};
>>> +
>>> +/* settings for Auto White Balance */
>>> +const struct ipu3_uapi_awb_config_s imgu_css_awb_defaults = {
>>> + 8191,
>>> + 8191,
>>> + 8191,
>>> + 8191 | /* rgbs_thr_gr/r/gb/b */
>>> + IPU3_UAPI_AWB_RGBS_THR_B_EN |
>>> IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT,
>>> + .grid = {
>>> + .width = 160,
>>> + .height = 45,
>>> + .block_width_log2 = 3,
>>> + .block_height_log2 = 4,
>>> + .x_start = 0,
>>> + .y_start = 0,
>>> + },
>>> +};
>>> +
>>> +const struct ipu3_uapi_ccm_mat_config imgu_css_ccm_6000k = {
>>> + 7239, -750, -37, 0,
>>> + -215, 9176, -200, 0,
>>> + -70, -589, 6810, 0
>>> +};
>>> +
>>> +const struct ipu3_uapi_ccm_mat_config imgu_css_ccm_4900k = {
>>> + 7811, -464, -466, 0,
>>> + -635, 8762, -533, 0,
>>> + -469, -154, 6583, 0
>>> +};
>>> +
>>> +const struct ipu3_uapi_ccm_mat_config imgu_css_ccm_3800k = {
>>> + 7379, -526, -296, 0,
>>> + -411, 7397, -415, 0,
>>> + -224, -564, 7244, 0
>>> +};
>>
>> All these should be moved to ipu3_awb.cpp, and be made static. Otherwise
>> they would get duplicated in every compilation unit in which this header
>> is included.
>>
>>> +
>>> +struct AwbConfig {
>>> + // Only repeat the AWB calculation every "this many" frames
>>> + uint16_t frame_period;
>>> + // number of initial frames for which speed taken as 1.0 (maximum)
>>> + uint16_t startup_frames;
>>> + unsigned int convergence_frames; // approx number of frames to
>>> converge
>>> + double speed; // IIR filter speed applied to algorithm results
>>> +};
>>> +
>>> +#if 0
>>> +typedef struct awb_public_set_item{
>>> + unsigned char Gr_avg;
>>> + unsigned char R_avg;
>>> + unsigned char B_avg;
>>> + unsigned char Gb_avg;
>>> + unsigned char sat_ratio;
>>> + unsigned char padding0; /**< Added the padding so that the
>>> public matches that private */
>>> + unsigned char padding1; /**< Added the padding so that the
>>> public matches that private */
>>> + unsigned char padding2; /**< Added the padding so that the
>>> public matches that private */
>>> +} awb_public_set_item_t;
>>> +#endif
>>
>> If the compiler doesn't need to see this, I think you can drop it
>> completely :-)
>>
>>> +
>>> +class IPU3Awb : public AwbAlgorithm
>>> +{
>>> +public:
>>> + IPU3Awb(IPAController *controller = NULL);
>>
>> This is C++, so nullptr :-) But there's no need for a default value, as
>> the controller parameter should either be mandatory, or removed
>> completely.
>>
>>> + ~IPU3Awb();
>>
>> Blank line.
>>
>>> + virtual char const *Name() const override;
>>
>> This is never used, you can drop it from the base class. When (and if)
>> we'll need this, I'd rather pass the name to the base class constructor,
>> store it there, and implement name() in the base class only, as a
>> non-virtual function.
>>
>>> + virtual void Initialise() override;
>>
>> No need to repeat the virtual keyword for overridden functions.
>>
>>> + void Initialise(ipu3_uapi_params ¶ms);
>>
>> This isn't very nice, as it's not aligned with the design of the
>> IPAAlgorithm class, but we can fix it later. I would move the function
>> down, after the mandatory functions, with calculateWBGains() and
>> updateBNR().
>>
>>> + unsigned int GetConvergenceFrames() const override;
>>> + void SetMode(std::string const &name) override;
>>> + void SetManualGains(double manual_r, double manual_b) override;
>>
>> A blank line here would be nice.
>>
>>> + void calculateWBGains(Rectangle roi,
>>
>> const Rectangle &roi
>>
>>> + const ipu3_uapi_stats_3a *stats);
>>> + void updateBNR(ipu3_uapi_params ¶ms);
>>> +
>>> +private:
>>> + uint32_t estimateCCT(uint8_t R, uint8_t G, uint8_t B);
>>
>> r, g and b.
>>
>>> +
>>> + /* configuration is read-only, and available to both threads */
>>
>> What threads ? :-)
>>
>>> + AwbConfig config_;
>>> + std::string mode_name_;
>>
>> Never used.
>>
>>> + /* manual r setting */
>>> + double manual_r_;
>>
>> manualRedGain_ ?
>>
>>> + /* manual b setting */
>>> + double manual_b_;
>>> + /* WB calculated gains */
>>> + uint16_t wbGains_[4];
>>> + double tint_;
>>> + uint32_t cct_;
>>> +
>>> + uint32_t frame_count_;
>>> +
>>> + bool algoConverged_;
>>> +};
>>> +} /* namespace libcamera */
>>> +
>>> +#endif /* __LIBCAMERA_IPU3_AWB_H__ */
>>> diff --git a/src/ipa/ipu3/meson.build b/src/ipa/ipu3/meson.build
>>> index a241f617..43ad0e0d 100644
>>> --- a/src/ipa/ipu3/meson.build
>>> +++ b/src/ipa/ipu3/meson.build
>>> @@ -2,8 +2,14 @@
>>> ipa_name = 'ipa_ipu3'
>>> +ipu3_ipa_sources = files([
>>> + 'ipu3.cpp',
>>> + 'ipu3_agc.cpp',
>>> + 'ipu3_awb.cpp',
>>> +])
>>> +
>>> mod = shared_module(ipa_name,
>>> - ['ipu3.cpp', libcamera_generated_ipa_headers],
>>> + [ipu3_ipa_sources,
>>> libcamera_generated_ipa_headers],
>>> name_prefix : '',
>>> include_directories : [ipa_includes,
>>> libipa_includes],
>>> dependencies : libcamera_dep,
>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp
>>> b/src/libcamera/pipeline/ipu3/ipu3.cpp
>>> index ff980b38..3809c943 100644
>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
>>> @@ -825,6 +825,7 @@ int
>>> PipelineHandlerIPU3::initControls(IPU3CameraData *data)
>>> */
>>> double lineDuration = sensorInfo.lineLength
>>> / (sensorInfo.pixelRate / 1e6);
>>> + LOG(IPU3, Error) << "line duration: " << lineDuration;
>>
>> Is this an error ? :-)
>>
>>> const ControlInfoMap &sensorControls = sensor->controls();
>>> const ControlInfo &v4l2Exposure =
>>> sensorControls.find(V4L2_CID_EXPOSURE)->second;
>>> int32_t minExposure = v4l2Exposure.min().get<int32_t>() *
>>> lineDuration;
>>
More information about the libcamera-devel
mailing list