[libcamera-devel] [RFC PATCH 2/2] WIP: ipa: ipu3: Add support for IPU3 AWB algorithm
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Feb 22 05:56:09 CET 2021
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.
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)
> +{
> + 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).
> + 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.
> + 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