[libcamera-devel] [RFC PATCH 2/2] WIP: ipa: ipu3: Add support for IPU3 AWB algorithm
Jean-Michel Hautbois
jeanmichel.hautbois at gmail.com
Thu Feb 25 09:27:49 CET 2021
Hi Laurent,
On 25/02/2021 02:43, Laurent Pinchart wrote:
> Hi Jean-Michel,
>
> On Mon, Feb 22, 2021 at 09:32:02PM +0100, Jean-Michel Hautbois wrote:
>> On 22/02/2021 19:50, Dafna Hirschfeld wrote:
>>> On 22.02.21 05:56, Laurent Pinchart wrote:
>>>> 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...
>
> I would have agreed with Dafna here. If the code is correct, then maybe
> more explicit names for the variables, and/or comments, could help
> explaining what's going on.
>
>>>>> +
>>>>> + 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).
>
> Is the aim really to repeat the same inner loop 45 times ?
Oh, Dafna pointed it I misread, and now it is obvious...
No of course not !
It is now 45 times faster :-).
Thanks !
>>>>> + }
>>>>> + }
>>>>> + 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.
>>
>>>>> +}
>>>>> +
>>>>> +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;
>
--
Regards,
JM
More information about the libcamera-devel
mailing list