[libcamera-devel] [RFC PATCH 2/2] WIP: ipa: ipu3: Add support for IPU3 AWB algorithm
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Feb 25 02:43:09 CET 2021
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 ?
> >>> + }
> >>> + }
> >>> + 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,
Laurent Pinchart
More information about the libcamera-devel
mailing list