[libcamera-devel] [PATCH v3 5/9] ipa: ipu3: Introduce a modular contrast algorithm
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Aug 19 11:53:00 CEST 2021
Hi Jean-Michel,
On Thu, Aug 19, 2021 at 10:11:11AM +0200, Jean-Michel Hautbois wrote:
> On 18/08/2021 23:41, Laurent Pinchart wrote:
> > On Wed, Aug 18, 2021 at 05:53:59PM +0200, 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
> >> 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 initialisation anymore.
> >>
> >> 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 is 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>
> >> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >> ---
> >> src/ipa/ipu3/algorithms/contrast.cpp | 53 ++++++++++++++++++++++++++++
> >> src/ipa/ipu3/algorithms/contrast.h | 32 +++++++++++++++++
> >> src/ipa/ipu3/algorithms/meson.build | 1 +
> >> src/ipa/ipu3/ipa_context.h | 8 +++++
> >> src/ipa/ipu3/ipu3.cpp | 15 +++++---
> >> 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 +-
> >> 9 files changed, 113 insertions(+), 53 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/contrast.cpp b/src/ipa/ipu3/algorithms/contrast.cpp
> >> new file mode 100644
> >> index 00000000..eb92d3c7
> >> --- /dev/null
> >> +++ b/src/ipa/ipu3/algorithms/contrast.cpp
> >> @@ -0,0 +1,53 @@
> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >> +/*
> >> + * Copyright (C) 2021, Google inc.
> >> + *
> >> + * contrast.cpp - IPU3 Contrast and Gamma control
> >> + */
> >> +
> >> +#include "contrast.h"
> >> +
> >> +#include <cmath>
> >> +
> >> +namespace libcamera {
> >> +
> >> +namespace ipa::ipu3::algorithms {
> >> +
> >> +
> >
> > One extra blank line.
> >
> >> +Contrast::Contrast()
> >> + : gamma_(1.0)
> >> +{
> >> +}
> >> +
> >> +void Contrast::prepare([[maybe_unused]] IPAContext &context, ipu3_uapi_params ¶ms)
> >> +{
> >> + /* Copy the calculated LUT into the parameters buffer */
> >
> > s/buffer/buffer./
> >
> >> + memcpy(params.acc_param.gamma.gc_lut.lut,
> >
> > Missing header.
> >
> >> + context.frameContext.contrast.gammaCorrection.lut,
> >> + IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES * sizeof(params.acc_param.gamma.gc_lut.lut[0]));
> >
> > Line wrap:
> >
> > IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES *
> > sizeof(params.acc_param.gamma.gc_lut.lut[0]));
> >
> >> +
> >> + /* Enable the custom gamma table */
> >
> > s/table/table./
> >
> > I won't repeat this comment everywhere, you can apply it through the
> > series.
> >
> >> + params.use.acc_gamma = 1;
> >> + params.acc_param.gamma.gc_ctrl.enable = 1;
> >> +}
> >> +
> >> +void Contrast::process([[maybe_unused]] IPAContext &context, [[maybe_unused]] const ipu3_uapi_stats_3a *stats)
> >
> > Line wrap.
> >
> > I'll stop repeating this comment too :-)
> >
> >> +{
> >> + /* Limit the gamma effect for now */
> >
> > What do you mean by "limit" ? I understand it as
> >
> > /*
> > * Hardcode gamma to 1.1 as a default for now.
> > *
> > * \todo Expose gamma control setting through the libcamera control API
> > */
> >
> >> + /* \todo expose gamma control setting through the libcamera control API */
> >> + 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 = static_cast<double>(i)
> >> + / (IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES - 1);
> >> + double gamma = std::pow(j, 1.0 / gamma_);
> >> +
> >> + /* The maximum value 255 is represented on 13 bits in the IPU3 */
> >
> > I think "The output value is expressed on 13 bits." would be clearer.
> >
> >> + context.frameContext.contrast.gammaCorrection.lut[i] = gamma * 8191;
> >> + }
> >
> > How about using std::size to avoid overflowing the array by mistake ?
> >
> > struct ipu3_uapi_gamma_corr_lut &lut =
> > context.frameContext.contrast.gammaCorrection;
> >
> > for (uint32_t i = 0; i < std::size(lut.lut); i++) {
> > double j = static_cast<double>(i) / (std::size(lut.lut) - 1);
> > double gamma = std::pow(j, 1.0 / gamma_);
> >
> > /* The output value is expressed on 13 bits. */
> > lut.lut[i] = gamma * 8191;
> > }
> >
> > Up to you.
> >
> >> +}
> >> +
> >> +} /* 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..6cd6c5db
> >> --- /dev/null
> >> +++ b/src/ipa/ipu3/algorithms/contrast.h
> >> @@ -0,0 +1,32 @@
> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >> +/*
> >> + * Copyright (C) 2021, Google inc.
> >> + *
> >> + * contrast.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
> >
> > I wonder of the algorithm shouldn't be called ToneMapping as it does
> > more than just contrast.
> >
> >> +{
> >> +public:
> >> + Contrast();
> >> + ~Contrast() = default;
> >
> > You can drop the destructor, it's not needed.
> >
> >> +
> >> + void prepare(IPAContext &context, ipu3_uapi_params ¶ms) override;
> >> + void process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;
> >
> > Missing blank line.
> >
> >> +private:
> >> + double gamma_;
> >> +};
> >> +
> >> +} /* 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 dc538b79..e3ff3b78 100644
> >> --- a/src/ipa/ipu3/algorithms/meson.build
> >> +++ b/src/ipa/ipu3/algorithms/meson.build
> >> @@ -2,4 +2,5 @@
> >>
> >> ipu3_ipa_algorithms = files([
> >> 'algorithm.cpp',
> >> + 'contrast.cpp',
> >> ])
> >> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> >> index 46291d9e..5964ba6d 100644
> >> --- a/src/ipa/ipu3/ipa_context.h
> >> +++ b/src/ipa/ipu3/ipa_context.h
> >> @@ -24,6 +24,14 @@ struct IPAConfiguration {
> >> };
> >>
> >> struct IPAFrameContext {
> >> + struct Agc {
> >> + uint32_t exposure;
> >> + double gain;
> >> + } agc;
> >
> > I think this belongs to patch 7/9, along with the changes to
> > ipu3_agc.cpp and ipu3_agc.h.
> >
> >> +
> >> + struct Contrast {
> >> + struct ipu3_uapi_gamma_corr_lut gammaCorrection;
> >> + } contrast;
> >
> > Missing documentation :-)
>
> Should I add the doc in tone_mapping.cpp (yes, I changed the name :-))
> or in ipu3.cpp as the rest of the structure ?
The structure is documented in ipu3.cpp, so that's where the field
should be documented. If desired we could move the Contrast structure
definition to tone_mapping.cpp, and the documentation of the structure
and its gammaCorrection field should then move to tone_mapping.cpp, but
the contract field of IPAFrameContext should go with the definition of
IPAFrameContext itself.
> >> };
> >>
> >> struct IPAContext {
> >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> >> index dc6f0735..ee0dd9fe 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 "ipu3_agc.h"
> >> #include "ipu3_awb.h"
> >> #include "libipa/camera_sensor_helper.h"
> >> @@ -218,6 +219,9 @@ int IPAIPU3::init(const IPASettings &settings,
> >>
> >> *ipaControls = ControlInfoMap(std::move(controls), controls::controls);
> >>
> >> + /* Construct our Algorithms */
> >> + algorithms_.emplace_back(new algorithms::Contrast());
> >
> > Or
> >
> > algorithms_.push_back(std::make_unique<algorithms::Contrast>());
> >
> > ? It won't make much practical difference, but may better show we're
> > dealing with unique pointers.
>
> OK for me !
>
> >> +
> >> return 0;
> >> }
> >>
> >> @@ -416,7 +420,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_;
> >>
> >> @@ -431,13 +435,16 @@ void IPAIPU3::parseStatistics(unsigned int frame,
> >> [[maybe_unused]] const ipu3_uapi_stats_3a *stats)
> >> {
> >> ControlList ctrls(controls::controls);
> >> + /* \todo These fields should not be written by the IPAIPU3 layer */
> >> + context_.frameContext.agc.gain = camHelper_->gain(gain_);
> >> + context_.frameContext.agc.exposure = exposure_;
> >>
> >> for (auto const &algo : algorithms_)
> >> algo->process(context_, stats);
> >>
> >> - double gain = camHelper_->gain(gain_);
> >> - agcAlgo_->process(stats, exposure_, gain);
> >> - gain_ = camHelper_->gainCode(gain);
> >> + agcAlgo_->process(context_, stats);
> >> + exposure_ = context_.frameContext.agc.exposure;
> >> + gain_ = camHelper_->gainCode(context_.frameContext.agc.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..f8290abd 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);
> >> 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 4bb321b3..c2d9e0c1 100644
> >> --- a/src/ipa/ipu3/ipu3_awb.cpp
> >> +++ b/src/ipa/ipu3/ipu3_awb.cpp
> >> @@ -133,31 +133,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()
> >> {
> >> @@ -197,10 +172,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);
> >> }
> >>
> >> @@ -350,7 +321,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.
> >> @@ -362,18 +333,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