[libcamera-devel] [RFC PATCH 3/5] ipa: ipu3: Introduce a modular contrast algorithm
Umang Jain
umang.jain at ideasonboard.com
Mon Aug 9 12:10:04 CEST 2021
Hi JM and Kieran,
On 8/9/21 3:22 PM, Kieran Bingham wrote:
> On 09/08/2021 10:20, Jean-Michel Hautbois wrote:
>> Implement a new modular framework for algorithms with a common context
>> structure that is passed to each algorithm through a common API.
> This doesn't implement the framework. (This text is likely leftover from
> a previous split).
>
> This patch:
>
> """
> Introduce a new algorithm to manage the contrast handling of the IPU3.
>
> """
>
>> The initial algorithm is chosen to configure the gamma contrast curve
>> which replicates the implementation from AWB for simplicity. As it is
> You don't replicate it anymore, you move it.
Indeed. I think the commit message can be a more descriptive, as things
happening here are:
- Remove existing gamma constrast handling from IPU3Awb
- Introduce Constrast class which handles gamma constrast handling
- Params are now set directly by Contrast::Initialise() through IPAContext
I wonder if it can be split up in two patches?
>
>> initialised with a default gamma value of 1.0, there is no need to use
>> the default table at init time anymore.>
>> 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/contrast.cpp | 52 ++++++++++++++++++++++++++++
>> src/ipa/ipu3/algorithms/contrast.h | 32 +++++++++++++++++
>> src/ipa/ipu3/algorithms/meson.build | 1 +
>> src/ipa/ipu3/ipu3.cpp | 6 +++-
>> src/ipa/ipu3/ipu3_agc.cpp | 5 +--
>> src/ipa/ipu3/ipu3_agc.h | 3 --
>> src/ipa/ipu3/ipu3_awb.cpp | 41 ++--------------------
>> src/ipa/ipu3/ipu3_awb.h | 2 +-
>> 8 files changed, 94 insertions(+), 48 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..832dbf35
>> --- /dev/null
>> +++ b/src/ipa/ipu3/algorithms/contrast.cpp
>> @@ -0,0 +1,52 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2021, Ideas On Board
>> + *
>> + * constrast.cpp - IPU3 Contrast and Gamma control
>> + */
>> +
>> +#include "contrast.h"
>> +
>> +#include <cmath>
>> +
>> +#include <libcamera/base/log.h>
>> +
>> +namespace libcamera {
>> +
>> +namespace ipa::ipu3::algorithms {
>> +
>> +LOG_DEFINE_CATEGORY(IPU3Contrast)
>> +
>> +Contrast::Contrast()
>> + : gamma_(1.0)
>> +{
>> + LOG(IPU3Contrast, Info) << "Instantiate Gamma";
>> +}
>> +
>> +int Contrast::initialise(IPAContext &context)
>> +{
>> + ipu3_uapi_params ¶ms = context.params;
>> +
>> + params.use.acc_gamma = 1;
>> + params.acc_param.gamma.gc_ctrl.enable = 1;
>> +
>> + /* Limit the gamma effect for now */
>> + gamma_ = 1.1;
> The commit message stated we initialise to 1.0 (which we do on
> construction) but here we actually initialise to 1.1...
>
>
>
>> +
>> + /* Plot the gamma curve into the look up table */
>> + for (uint32_t i = 0; i < 256; 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 */
>> + params.acc_param.gamma.gc_lut.lut[i] = gamma * 8191;
>> + }
>> +
>> + LOG(IPU3Contrast, Info) << "Processed Gamma Curve";
> I think we can drop Info line now or move it to Debug if you think it's
> helpful to keep it... But if we keep it as Debug, we should print
> something more useful, like what gamma value we actually set.
>
> I think it's probably not helpful though.
+1
>
>
>> +
>> + return 0;
>> +}
>> +
>> +} /* 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..958e8bb8
>> --- /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
>> + *
>> + * 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;
>> +
>> + int initialise(IPAContext &context) override;
There is no need for configure() and process() here? Is creating the
table is all what needs to be done in Initialise()?
>> +
>> +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 67148333..f71d1e61 100644
>> --- a/src/ipa/ipu3/algorithms/meson.build
>> +++ b/src/ipa/ipu3/algorithms/meson.build
>> @@ -1,4 +1,5 @@
>> # SPDX-License-Identifier: CC0-1.0
>>
>> ipu3_ipa_algorithms = files([
>> + 'contrast.cpp',
>> ])
>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
>> index 55c4da72..7035802f 100644
>> --- a/src/ipa/ipu3/ipu3.cpp
>> +++ b/src/ipa/ipu3/ipu3.cpp
>> @@ -23,6 +23,7 @@
>> #include "libcamera/internal/framebuffer.h"
>>
>> #include "algorithms/algorithm.h"
>> +#include "algorithms/contrast.h"
>> #include "ipa_context.h"
>>
>> #include "ipu3_agc.h"
>> @@ -103,6 +104,9 @@ int IPAIPU3::init(const IPASettings &settings)
>> return -ENODEV;
>> }
>>
>> + /* Construct our Algorithms */
>> + algorithms_.emplace_back(new algorithms::Contrast());
>> +
>> /* Reset all the hardware settings */
>> context_.params = {};
>>
>> @@ -290,7 +294,7 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,
>> void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
>> {
>> if (agcAlgo_->updateControls())
>> - awbAlgo_->updateWbParameters(context_.params, agcAlgo_->gamma());
>> + awbAlgo_->updateWbParameters(context_.params);
>>
>> *params = context_.params;
>>
>> diff --git a/src/ipa/ipu3/ipu3_agc.cpp b/src/ipa/ipu3/ipu3_agc.cpp
>> index 6253ab94..733814dd 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);
>> }
>> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h
>> index e3e1d28b..3b7f781e 100644
>> --- a/src/ipa/ipu3/ipu3_agc.h
>> +++ b/src/ipa/ipu3/ipu3_agc.h
>> @@ -37,8 +37,6 @@ public:
>> void process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, double &gain);
>> 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);
>> @@ -54,7 +52,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 9b409c8f..043c3838 100644
>> --- a/src/ipa/ipu3/ipu3_awb.cpp
>> +++ b/src/ipa/ipu3/ipu3_awb.cpp
>> @@ -134,31 +134,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()
>> {
>> @@ -198,10 +173,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);
>> }
>>
>> @@ -351,7 +322,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.
>> @@ -363,18 +334,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 284e3844..8b05ac03 100644
>> --- a/src/ipa/ipu3/ipu3_awb.h
>> +++ b/src/ipa/ipu3/ipu3_awb.h
>> @@ -32,7 +32,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;
>>
More information about the libcamera-devel
mailing list