[libcamera-devel] [RFC PATCH 2/2] WIP: ipa: ipu3: Add support for IPU3 AWB algorithm
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Feb 23 16:44:09 CET 2021
Hi Jean-Michel,
On Tue, Feb 23, 2021 at 03:27:56PM +0100, Jean-Michel Hautbois wrote:
> On 22/02/2021 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.
> >
> > 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.
>
> Thanks, I only have a few questions :-).
>
> >> 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)
> >> +{
> >> + 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];
> >> +
> >> + 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));
> >> + }
> >> + }
> >> + std::sort(brightnessVec_.begin(), brightnessVec_.end());
> >> +
> >> + /* \todo create a class to generate histograms ! */
> >> + std::unordered_map<uint32_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).
>
> OK, I will !
>
> >> + 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_;
> >> + }
> >> +}
> >> +
> >> +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.
> >
>
> Indeed... :-/
>
> >> + 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 ?
> >
>
> This is an error, I copy/pasted it to ipu3 and it should be called only
> when AE has converged. If there is a sufficient scene change, then AE
> will diverge and so it will be called again after the new convergence
> (does it make sense :-) ?).
Do you mean this should be
if (!AE has converged)
return;
? It sounds sensible. I'm sure we'll find out we'll need something more
complex, but for now I think it's good enough :-)
> >> +
> >> + 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.
>
> That is indeed a blocker here.
> I will rename the function, but I think we need to let AWB fill the
> parameters related to it and not directly filling those in
> IPAIPU3::fillParams().
>
> >> +{
> >> + 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 :-)
>
> I will remove them now, thanks !
> It will be used later as this is how the stats are organized internally.
>
> >> +
> >> +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 ? :-)
>
> Yes, which ones ?? ;-)
> Well, as in RPi, there will be threads later, if the algorithms are too
> complicated (they probably will be)...
>
> >> + 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 ? :-)
>
> Ahem ->[]
>
> >> const ControlInfoMap &sensorControls = sensor->controls();
> >> const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;
> >> int32_t minExposure = v4l2Exposure.min().get<int32_t>() * lineDuration;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list