[libcamera-devel,RFC,3/5] ipa: ipu3: Introduce a modular contrast algorithm
diff mbox series

Message ID 20210809092007.79067-4-jeanmichel.hautbois@ideasonboard.com
State Superseded
Headers show
Series
  • IPU3: Introduce modularity for algorithms
Related show

Commit Message

Jean-Michel Hautbois Aug. 9, 2021, 9:20 a.m. UTC 
Implement a new modular framework for algorithms with a common context
structure that is passed to each algorithm through a common API.

The initial algorithm is chosen to configure the gamma contrast curve
which replicates the implementation from AWB for simplicity. As it is
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@ideasonboard.com>
Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@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

Comments

Kieran Bingham Aug. 9, 2021, 9:52 a.m. UTC | #1 
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.

> 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@ideasonboard.com>
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@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 &params = 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.


> +
> +	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;
> +
> +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 &params, 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 &params, double agcGamma)
> +void IPU3Awb::updateWbParameters(ipu3_uapi_params &params)
>  {
>  	/*
>  	 * Green gains should not be touched and considered 1.
> @@ -363,18 +334,10 @@ void IPU3Awb::updateWbParameters(ipu3_uapi_params &params, 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 &params, const Size &bdsOutputSize, struct ipu3_uapi_grid_config &bdsGrid);
>  	void calculateWBGains(const ipu3_uapi_stats_3a *stats);
> -	void updateWbParameters(ipu3_uapi_params &params, double agcGamma);
> +	void updateWbParameters(ipu3_uapi_params &params);
>  
>  	struct Ipu3AwbCell {
>  		unsigned char greenRedAvg;
>
Umang Jain Aug. 9, 2021, 10:10 a.m. UTC | #2 
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@ideasonboard.com>
>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@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 &params = 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 &params, 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 &params, double agcGamma)
>> +void IPU3Awb::updateWbParameters(ipu3_uapi_params &params)
>>   {
>>   	/*
>>   	 * Green gains should not be touched and considered 1.
>> @@ -363,18 +334,10 @@ void IPU3Awb::updateWbParameters(ipu3_uapi_params &params, 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 &params, const Size &bdsOutputSize, struct ipu3_uapi_grid_config &bdsGrid);
>>   	void calculateWBGains(const ipu3_uapi_stats_3a *stats);
>> -	void updateWbParameters(ipu3_uapi_params &params, double agcGamma);
>> +	void updateWbParameters(ipu3_uapi_params &params);
>>   
>>   	struct Ipu3AwbCell {
>>   		unsigned char greenRedAvg;
>>
Jean-Michel Hautbois Aug. 10, 2021, 7:49 a.m. UTC | #3 
Hi Kieran,

On 09/08/2021 11:52, 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.
> 
>> 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@ideasonboard.com>
>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@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 &params = 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...
> 

Indeed ! I will change the commit message :-).

> 
>> +
>> +	/* 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.

It might not be helpful, indeed, and the one in the constructor could
also be removed ?

> 
>> +
>> +	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;
>> +
>> +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 &params, 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 &params, double agcGamma)
>> +void IPU3Awb::updateWbParameters(ipu3_uapi_params &params)
>>  {
>>  	/*
>>  	 * Green gains should not be touched and considered 1.
>> @@ -363,18 +334,10 @@ void IPU3Awb::updateWbParameters(ipu3_uapi_params &params, 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 &params, const Size &bdsOutputSize, struct ipu3_uapi_grid_config &bdsGrid);
>>  	void calculateWBGains(const ipu3_uapi_stats_3a *stats);
>> -	void updateWbParameters(ipu3_uapi_params &params, double agcGamma);
>> +	void updateWbParameters(ipu3_uapi_params &params);
>>  
>>  	struct Ipu3AwbCell {
>>  		unsigned char greenRedAvg;
>>

Patch
diff mbox series 

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 &params = context.params;
+
+	params.use.acc_gamma = 1;
+	params.acc_param.gamma.gc_ctrl.enable = 1;
+
+	/* Limit the gamma effect for now */
+	gamma_ = 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";
+
+	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;
+
+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 &params, 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 &params, double agcGamma)
+void IPU3Awb::updateWbParameters(ipu3_uapi_params &params)
 {
 	/*
 	 * Green gains should not be touched and considered 1.
@@ -363,18 +334,10 @@  void IPU3Awb::updateWbParameters(ipu3_uapi_params &params, 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 &params, const Size &bdsOutputSize, struct ipu3_uapi_grid_config &bdsGrid);
 	void calculateWBGains(const ipu3_uapi_stats_3a *stats);
-	void updateWbParameters(ipu3_uapi_params &params, double agcGamma);
+	void updateWbParameters(ipu3_uapi_params &params);
 
 	struct Ipu3AwbCell {
 		unsigned char greenRedAvg;