[libcamera-devel] [RFC PATCH 2/2] WIP: ipa: ipu3: Add support for IPU3 AWB algorithm
Dafna Hirschfeld
dafna.hirschfeld at collabora.com
Mon Feb 22 19:50:21 CET 2021
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?
>
> 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?
>> +{
>> + 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
>> +
>> + 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
>> + }
>> + }
>> + std::sort(brightnessVec_.begin(), brightnessVec_.end());
I don't think the sort is is necessary
>> +
>> + /* \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;
>> + 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?
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