[libcamera-devel] [PATCH v2 05/10] ipa: ipu3: Introduce a modular contrast algorithm
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Aug 16 00:53:55 CEST 2021
Hi Jean-Michel,
On Fri, Aug 13, 2021 at 10:31:17AM +0100, Kieran Bingham wrote:
> On 12/08/2021 17:52, Jean-Michel Hautbois wrote:
> > Introduce a new algorithm to manage the contrast handling of the IPU3.
> >
> > The initial algorithm is chosen to configure the gamma contrast curve
>
> I don't think this is the 'initial algorithm' any more ...
It could in v3 unless there's a disagreement regarding my comments on
03/10 though.
> > which moves the implementation out of AWB for simplicity. As it is
> > initialised with a default gamma value of 1.1, there is no need to use
> > the default table at init time anymore.
>
> s/at init time/at initialisation/
>
> > This demonstrates the way to use process() call when the EventStatReady
> > comes in. The function calculates the LUT in the context of a frame, and
> > when prepare() is called, the parameters are filled with the updated
> > values.
> >
> > AGC needs to be modified to take the new process interface into account.
> >
> > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> > ---
> > src/ipa/ipu3/algorithms/algorithm.h | 2 ++
> > src/ipa/ipu3/algorithms/contrast.cpp | 51 ++++++++++++++++++++++++++++
> > src/ipa/ipu3/algorithms/contrast.h | 33 ++++++++++++++++++
> > src/ipa/ipu3/algorithms/meson.build | 3 +-
> > src/ipa/ipu3/ipa_context.h | 8 +++++
> > src/ipa/ipu3/ipu3.cpp | 12 +++++--
> > src/ipa/ipu3/ipu3_agc.cpp | 9 +++--
> > src/ipa/ipu3/ipu3_agc.h | 5 +--
> > src/ipa/ipu3/ipu3_awb.cpp | 41 ++--------------------
> > src/ipa/ipu3/ipu3_awb.h | 2 +-
> > 10 files changed, 114 insertions(+), 52 deletions(-)
> > create mode 100644 src/ipa/ipu3/algorithms/contrast.cpp
> > create mode 100644 src/ipa/ipu3/algorithms/contrast.h
> >
> > diff --git a/src/ipa/ipu3/algorithms/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h
> > index b571f55a..13e5f2d4 100644
> > --- a/src/ipa/ipu3/algorithms/algorithm.h
> > +++ b/src/ipa/ipu3/algorithms/algorithm.h
> > @@ -28,6 +28,8 @@ public:
> > }
> >
> > virtual void prepare([[maybe_unused]] IPAContext &context, [[maybe_unused]] ipu3_uapi_params ¶ms) {}
> > +
> > + virtual void process([[maybe_unused]] IPAContext &context, [[maybe_unused]] const ipu3_uapi_stats_3a *stats) {}
Line wrap here too.
> > };
> >
> > } /* namespace ipa::ipu3 */
> > diff --git a/src/ipa/ipu3/algorithms/contrast.cpp b/src/ipa/ipu3/algorithms/contrast.cpp
> > new file mode 100644
> > index 00000000..bc00490f
> > --- /dev/null
> > +++ b/src/ipa/ipu3/algorithms/contrast.cpp
> > @@ -0,0 +1,51 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2021, Ideas On Board
>
> Keep this consistent with contrast.h please.
>
> > + *
> > + * constrast.cpp - IPU3 Contrast and Gamma control
>
> s/constrast/contrast/
>
> > + */
> > +
> > +#include "contrast.h"
> > +
> > +#include <cmath>
> > +
> > +#include <libcamera/base/log.h>
> > +
> > +namespace libcamera {
> > +
> > +namespace ipa::ipu3::algorithms {
> > +
> > +LOG_DEFINE_CATEGORY(IPU3Contrast)
This isn't used, so you could drop it.
> > +
> > +Contrast::Contrast()
> > + : gamma_(1.0)
> > +{
> > +}
> > +
> > +void Contrast::prepare([[maybe_unused]] IPAContext &context, ipu3_uapi_params ¶ms)
> > +{
> > + params.use.acc_gamma = 1;
> > + params.acc_param.gamma.gc_ctrl.enable = 1;
>
> The ordering is almost irrelevant to the code, but for some reason, I
> think you should set the enable and use flags /after/ filling in the data.
>
> Technically it doesn't matter, but conceptually, they should only be
> enabled, after they are filled in correctly.
I wonder if Kieran has his kernel developer hat on, thinking this will
be written to registers :-)
> > +
> > + for (uint32_t i = 0; i < IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES; i++)
> > + params.acc_param.gamma.gc_lut.lut[i] = context.frameContext.contrast.lut[i];
>
> We have two tables of a fixed number of entries.
> I wonder if a memcpy() would be more efficient than a loop, but perhaps
> it doesn't make a difference ?
It shouldn't be less efficient at least.
> Would the compiler do the right thing if you said:
>
> params.acc_param.gamma.gc_lut.lut = context.frameContext.contrast.lut;
You can assign structs, but not arrays.
> In fact, I see that
> params.acc_param.gamma.gc_lut.lut stores unsigned shorts,
> while
> context.frameContext.contrast.lut stores uint16_t
>
> Maybe we should store a
> struct ipu3_uapi_gamma_corr_lut
>
> in context.frameContext.contrast instead, Yes they're all the same size
> types - but we have access to the original type of the array, so why not
> use it to keep them identical..
>
> > +}
> > +
> > +void Contrast::process([[maybe_unused]] IPAContext &context, [[maybe_unused]] const ipu3_uapi_stats_3a *stats)
> > +{
> > + /* Limit the gamma effect for now */
>
> Can this be explained any more? Why do we set this here and not
> initialise to 1.1 in the initialisation list?
>
> Perhaps a
> \todo expose gamma control setting through the libcamera control API
>
> should be added here too to make it clear that this is hardcoded, when
> it should somehow be a parameter or control into the IPA or such.
>
> > + gamma_ = 1.1;
> > +
> > + /* Plot the gamma curve into the look up table */
> > + for (uint32_t i = 0; i < IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES; i++) {
> > + double j = i / 255.0;
> > + double gamma = std::pow(j, 1.0 / gamma_);
> > +
> > + /* The maximum value 255 is represented on 13 bits in the IPU3 */
> > + context.frameContext.contrast.lut[i] = gamma * 8191;
>
> 8191 still looks like a magic number, even with the comment.
So does 255.0, which I assume is IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES - 1.
Maybe
double j = static_cast<double>(i)
/ (IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES - 1);
> I wonder if we should define this somewhere, perhaps as
>
> /* The maximum value 255 is represented on 13 bits in the IPU3 */
I'm not sure to understand the 255 here. The gamma table contains values
in the [0.0, 1.0] range and maps them to [0, 8191]. 8191 thus
corresponds to 1.0, there's no 255 anywhere there.
(By the way, I only assume that 8191 is 1.0, I wonder if it could be
8192 that is mapped to 1.0, in which case the highest possible value
would be 0.9998779296875)
> #define LUT_INTEGER_SCALE(g) (g * ((1 << 13) - 1))
>
> context.frameContext.contrast.lut[i] = LUT_INTEGER_SCALE(gamma);
>
> LUT_INTEGER_SCALE may be a completely inappropriate name ... I'm guessing.
Do we need to round to the closest integer instead of the lower one, or
does this make little difference ?
> Also - even having said that - this patch is moving existing code - so
> this shouldn't be changed here - but it might be a distinct patch of
> it's own to come later.
A fixed point helper template class, with template parameters for the
number of integer bits and the number of fractional bits:
template<unsigned int, unsigned int>
class FixedPoint
{
...
};
with conversion functions, in a fashion similar to
std::chrono::duration. Something for later.
> > + }
> > +}
> > +
> > +} /* namespace ipa::ipu3::algorithms */
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/ipa/ipu3/algorithms/contrast.h b/src/ipa/ipu3/algorithms/contrast.h
> > new file mode 100644
> > index 00000000..50cb626f
> > --- /dev/null
> > +++ b/src/ipa/ipu3/algorithms/contrast.h
> > @@ -0,0 +1,33 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2021, Google
> > + *
> > + * constrast.h - IPU3 Contrast and Gamma control
> > + */
> > +#ifndef __LIBCAMERA_IPU3_ALGORITHMS_CONTRAST_H__
> > +#define __LIBCAMERA_IPU3_ALGORITHMS_CONTRAST_H__
> > +
> > +#include "algorithm.h"
> > +
> > +namespace libcamera {
> > +
> > +namespace ipa::ipu3::algorithms {
> > +
> > +class Contrast : public Algorithm
> > +{
> > +public:
> > + Contrast();
> > + ~Contrast() = default;
I don't think this is needed, and if it is, let's not make it an inline
function, you can define it in contrast.cpp as
Contrast::~Contrast() = default;
> > +
> > + void prepare(IPAContext &context, ipu3_uapi_params ¶ms) override;
> > + void process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;
> > +
> > +private:
> > + double gamma_;
I wonder if the gamma value should be stored in the context structure. A
bit more design documentation could answer this question :-)
Could you also please explain (just as a reply to this e-mail, not in
comments) why the gamma value was provided by the AGC algorithm, and the
gamma LUT filled by the AWB algorithm ? How does gamma relate, in
genetal, to AGC and/or AWB ?
Also, this doesn't handle contrast at the moment. Do you plan to expand
the algorithm ? I also wonder if it shouldn't be called tonemapping
instead of contrast.
> > +};
> > +
> > +} /* namespace ipa::ipu3::algorithms */
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_IPU3_ALGORITHMS_CONTRAST_H__ */
> > diff --git a/src/ipa/ipu3/algorithms/meson.build b/src/ipa/ipu3/algorithms/meson.build
> > index 3fb3ce56..69a59096 100644
> > --- a/src/ipa/ipu3/algorithms/meson.build
> > +++ b/src/ipa/ipu3/algorithms/meson.build
> > @@ -1,5 +1,6 @@
> > # SPDX-License-Identifier: CC0-1.0
> >
> > ipu3_ipa_algorithms = files([
> > - 'grid.cpp',
>
> Please fix this alignment in the patch that adds it.
>
> > + 'contrast.cpp',
> > + 'grid.cpp',
> > ])
> > diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> > index 90b2f2c2..e995c663 100644
> > --- a/src/ipa/ipu3/ipa_context.h
> > +++ b/src/ipa/ipu3/ipa_context.h
> > @@ -33,6 +33,14 @@ struct IPAConfiguration {
> > * lifetime, though for now a single instance is used.
> > */
> > struct IPAFrameContext {
> > + struct Agc {
> > + uint32_t exposure;
> > + double gain;
> > + } agc;
> > +
> > + struct Contrast {
> > + uint16_t lut[IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES];
>
> As highilghted earlier, I think this should be:
> struct ipu3_uapi_gamma_corr_lut lut
>
> > + } contrast;
> > };
> >
> > /* Global context of the IPA */
> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> > index 394a7a45..e09adfc3 100644
> > --- a/src/ipa/ipu3/ipu3.cpp
> > +++ b/src/ipa/ipu3/ipu3.cpp
> > @@ -30,6 +30,7 @@
> > #include "libcamera/internal/mapped_framebuffer.h"
> >
> > #include "algorithms/algorithm.h"
> > +#include "algorithms/contrast.h"
> > #include "algorithms/grid.h"
> > #include "ipu3_agc.h"
> > #include "ipu3_awb.h"
> > @@ -167,6 +168,7 @@ int IPAIPU3::init(const IPASettings &settings,
> > *ipaControls = ControlInfoMap(std::move(controls), controls::controls);
> > /* Construct our Algorithms */
> > algorithms_.emplace_back(new algorithms::Grid());
> > + algorithms_.emplace_back(new algorithms::Contrast());
> >
> > return 0;
> > }
> > @@ -309,7 +311,7 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
> > algo->prepare(context_, params_);
> >
> > if (agcAlgo_->updateControls())
> > - awbAlgo_->updateWbParameters(params_, agcAlgo_->gamma());
> > + awbAlgo_->updateWbParameters(params_);
> >
> > *params = params_;
> >
> > @@ -324,9 +326,15 @@ void IPAIPU3::parseStatistics(unsigned int frame,
> > [[maybe_unused]] const ipu3_uapi_stats_3a *stats)
> > {
> > ControlList ctrls(controls::controls);
>
> New line here to separate local variable declarations from code segments.
>
> > + for (auto const &algo : algorithms_)
> > + algo->process(context_, stats);
> >
> > double gain = camHelper_->gain(gain_);
> > - agcAlgo_->process(stats, exposure_, gain);
> > + context_.frameContext.agc.exposure = exposure_;
> > + context_.frameContext.agc.gain = gain;
>
> the IPU3 layer sould not be filling in agc specific structures. That's a
> layering violation to me.
>
> The algorithms (in this instance, the agc?) own those structures, so
> they should be manipulating them.
I think I agree, but how would you pass the exposure and gain parameters
to the algorithm in that case ?
> It's also showing where the algorithms need to know historical data.
> In this instance, presumably they need to know what gain or expsosure
> was set at the time that this buffer ran.
>
> This shows where we will need to keep historical versions of the
> FrameContext, and perhaps track them along with the frame counter.
>
> For now, while we get things moving to be more modular, I think we could
> keep this as it is - but it should be commented with a todo
>
> > + agcAlgo_->process(context_, stats);
> > + exposure_ = context_.frameContext.agc.exposure;
> > + gain = context_.frameContext.agc.gain;
> > gain_ = camHelper_->gainCode(gain);
> >
> > awbAlgo_->calculateWBGains(stats);
> > diff --git a/src/ipa/ipu3/ipu3_agc.cpp b/src/ipa/ipu3/ipu3_agc.cpp
> > index 408eb849..c6782ff4 100644
> > --- a/src/ipa/ipu3/ipu3_agc.cpp
> > +++ b/src/ipa/ipu3/ipu3_agc.cpp
> > @@ -52,7 +52,7 @@ static constexpr uint8_t kCellSize = 8;
> >
> > IPU3Agc::IPU3Agc()
> > : frameCount_(0), lastFrame_(0), converged_(false),
> > - updateControls_(false), iqMean_(0.0), gamma_(1.0),
> > + updateControls_(false), iqMean_(0.0),
> > lineDuration_(0s), maxExposureTime_(0s),
> > prevExposure_(0s), prevExposureNoDg_(0s),
> > currentExposure_(0s), currentExposureNoDg_(0s)
> > @@ -104,9 +104,6 @@ void IPU3Agc::processBrightness(const ipu3_uapi_stats_3a *stats)
> > }
> > }
> >
> > - /* Limit the gamma effect for now */
> > - gamma_ = 1.1;
> > -
> > /* Estimate the quantile mean of the top 2% of the histogram */
> > iqMean_ = Histogram(Span<uint32_t>(hist)).interQuantileMean(0.98, 1.0);
> > }
> > @@ -193,8 +190,10 @@ void IPU3Agc::lockExposureGain(uint32_t &exposure, double &gain)
> > lastFrame_ = frameCount_;
> > }
> >
> > -void IPU3Agc::process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, double &gain)
> > +void IPU3Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
> > {
> > + uint32_t &exposure = context.frameContext.agc.exposure;
> > + double &gain = context.frameContext.agc.gain;
> > processBrightness(stats);
> > lockExposureGain(exposure, gain);
> > frameCount_++;
> > diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h
> > index f00b98d6..8b32e52f 100644
> > --- a/src/ipa/ipu3/ipu3_agc.h
> > +++ b/src/ipa/ipu3/ipu3_agc.h
> > @@ -30,11 +30,9 @@ public:
> > ~IPU3Agc() = default;
> >
> > void initialise(struct ipu3_uapi_grid_config &bdsGrid, const IPACameraSensorInfo &sensorInfo);
> > - void process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, double &gain);
> > + void process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;
> > bool converged() { return converged_; }
> > bool updateControls() { return updateControls_; }
> > - /* \todo Use a metadata exchange between IPAs */
> > - double gamma() { return gamma_; }
> >
> > private:
> > void processBrightness(const ipu3_uapi_stats_3a *stats);
> > @@ -50,7 +48,6 @@ private:
> > bool updateControls_;
> >
> > double iqMean_;
> > - double gamma_;
> >
> > Duration lineDuration_;
> > Duration maxExposureTime_;
> > diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp
> > index 911760b3..0f3bcdc9 100644
> > --- a/src/ipa/ipu3/ipu3_awb.cpp
> > +++ b/src/ipa/ipu3/ipu3_awb.cpp
> > @@ -114,31 +114,6 @@ static const struct ipu3_uapi_ccm_mat_config imguCssCcmDefault = {
> > 0, 0, 8191, 0
> > };
> >
> > -/* Default settings for Gamma correction */
> > -const struct ipu3_uapi_gamma_corr_lut imguCssGammaLut = { {
> > - 63, 79, 95, 111, 127, 143, 159, 175, 191, 207, 223, 239, 255, 271, 287,
> > - 303, 319, 335, 351, 367, 383, 399, 415, 431, 447, 463, 479, 495, 511,
> > - 527, 543, 559, 575, 591, 607, 623, 639, 655, 671, 687, 703, 719, 735,
> > - 751, 767, 783, 799, 815, 831, 847, 863, 879, 895, 911, 927, 943, 959,
> > - 975, 991, 1007, 1023, 1039, 1055, 1071, 1087, 1103, 1119, 1135, 1151,
> > - 1167, 1183, 1199, 1215, 1231, 1247, 1263, 1279, 1295, 1311, 1327, 1343,
> > - 1359, 1375, 1391, 1407, 1423, 1439, 1455, 1471, 1487, 1503, 1519, 1535,
> > - 1551, 1567, 1583, 1599, 1615, 1631, 1647, 1663, 1679, 1695, 1711, 1727,
> > - 1743, 1759, 1775, 1791, 1807, 1823, 1839, 1855, 1871, 1887, 1903, 1919,
> > - 1935, 1951, 1967, 1983, 1999, 2015, 2031, 2047, 2063, 2079, 2095, 2111,
> > - 2143, 2175, 2207, 2239, 2271, 2303, 2335, 2367, 2399, 2431, 2463, 2495,
> > - 2527, 2559, 2591, 2623, 2655, 2687, 2719, 2751, 2783, 2815, 2847, 2879,
> > - 2911, 2943, 2975, 3007, 3039, 3071, 3103, 3135, 3167, 3199, 3231, 3263,
> > - 3295, 3327, 3359, 3391, 3423, 3455, 3487, 3519, 3551, 3583, 3615, 3647,
> > - 3679, 3711, 3743, 3775, 3807, 3839, 3871, 3903, 3935, 3967, 3999, 4031,
> > - 4063, 4095, 4127, 4159, 4223, 4287, 4351, 4415, 4479, 4543, 4607, 4671,
> > - 4735, 4799, 4863, 4927, 4991, 5055, 5119, 5183, 5247, 5311, 5375, 5439,
> > - 5503, 5567, 5631, 5695, 5759, 5823, 5887, 5951, 6015, 6079, 6143, 6207,
> > - 6271, 6335, 6399, 6463, 6527, 6591, 6655, 6719, 6783, 6847, 6911, 6975,
> > - 7039, 7103, 7167, 7231, 7295, 7359, 7423, 7487, 7551, 7615, 7679, 7743,
> > - 7807, 7871, 7935, 7999, 8063, 8127, 8191
> > -} };
> > -
> > IPU3Awb::IPU3Awb()
> > : Algorithm()
> > {
> > @@ -179,10 +154,6 @@ void IPU3Awb::initialise(ipu3_uapi_params ¶ms, const Size &bdsOutputSize, st
> > params.use.acc_ccm = 1;
> > params.acc_param.ccm = imguCssCcmDefault;
> >
> > - params.use.acc_gamma = 1;
> > - params.acc_param.gamma.gc_lut = imguCssGammaLut;
> > - params.acc_param.gamma.gc_ctrl.enable = 1;
> > -
> > zones_.reserve(kAwbStatsSizeX * kAwbStatsSizeY);
> > }
> >
> > @@ -332,7 +303,7 @@ void IPU3Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats)
> > }
> > }
> >
> > -void IPU3Awb::updateWbParameters(ipu3_uapi_params ¶ms, double agcGamma)
> > +void IPU3Awb::updateWbParameters(ipu3_uapi_params ¶ms)
> > {
> > /*
> > * Green gains should not be touched and considered 1.
> > @@ -344,18 +315,10 @@ void IPU3Awb::updateWbParameters(ipu3_uapi_params ¶ms, double agcGamma)
> > params.acc_param.bnr.wb_gains.b = 4096 * asyncResults_.blueGain;
> > params.acc_param.bnr.wb_gains.gb = 16;
> >
> > - LOG(IPU3Awb, Debug) << "Color temperature estimated: " << asyncResults_.temperatureK
> > - << " and gamma calculated: " << agcGamma;
> > + LOG(IPU3Awb, Debug) << "Color temperature estimated: " << asyncResults_.temperatureK;
> >
> > /* The CCM matrix may change when color temperature will be used */
> > params.acc_param.ccm = imguCssCcmDefault;
> > -
> > - for (uint32_t i = 0; i < 256; i++) {
> > - double j = i / 255.0;
> > - double gamma = std::pow(j, 1.0 / agcGamma);
> > - /* The maximum value 255 is represented on 13 bits in the IPU3 */
> > - params.acc_param.gamma.gc_lut.lut[i] = gamma * 8191;
> > - }
> > }
> >
> > } /* namespace ipa::ipu3 */
> > diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h
> > index ea2d4320..eeb2920b 100644
> > --- a/src/ipa/ipu3/ipu3_awb.h
> > +++ b/src/ipa/ipu3/ipu3_awb.h
> > @@ -31,7 +31,7 @@ public:
> >
> > void initialise(ipu3_uapi_params ¶ms, const Size &bdsOutputSize, struct ipu3_uapi_grid_config &bdsGrid);
> > void calculateWBGains(const ipu3_uapi_stats_3a *stats);
> > - void updateWbParameters(ipu3_uapi_params ¶ms, double agcGamma);
> > + void updateWbParameters(ipu3_uapi_params ¶ms);
> >
> > struct Ipu3AwbCell {
> > unsigned char greenRedAvg;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list