[v3,20/23] libcamera: software_isp: Use floating point for color parameters
diff mbox series

Message ID 20240717085444.289997-21-mzamazal@redhat.com
State Superseded
Headers show
Series
  • Software ISP refactoring
Related show

Commit Message

Milan Zamazal July 17, 2024, 8:54 a.m. UTC
It's more natural to represent color gains and black level as floating
point numbers rather than using a particular pixel-related
representation.

double is used rather than float because it's a more common floating
point type in libcamera algorithms.  Otherwise there is no obvious
reason to select one over the other here.

The constructed color tables still use integer representation for
efficiency.

Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
---
 src/ipa/simple/algorithms/blc.cpp    |  8 ++++----
 src/ipa/simple/algorithms/colors.cpp | 26 ++++++++++++++------------
 src/ipa/simple/ipa_context.h         | 11 +++++------
 3 files changed, 23 insertions(+), 22 deletions(-)

Comments

Daniel Scally Aug. 13, 2024, 11:10 a.m. UTC | #1
Hi Milan

On 17/07/2024 09:54, Milan Zamazal wrote:
> It's more natural to represent color gains and black level as floating
> point numbers rather than using a particular pixel-related
> representation.
>
> double is used rather than float because it's a more common floating
> point type in libcamera algorithms.  Otherwise there is no obvious
> reason to select one over the other here.
>
> The constructed color tables still use integer representation for
> efficiency.
>
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>   src/ipa/simple/algorithms/blc.cpp    |  8 ++++----
>   src/ipa/simple/algorithms/colors.cpp | 26 ++++++++++++++------------
>   src/ipa/simple/ipa_context.h         | 11 +++++------
>   3 files changed, 23 insertions(+), 22 deletions(-)
>
> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
> index 3b73d830..ab7e7dd9 100644
> --- a/src/ipa/simple/algorithms/blc.cpp
> +++ b/src/ipa/simple/algorithms/blc.cpp
> @@ -24,7 +24,7 @@ BlackLevel::BlackLevel()
>   int BlackLevel::init(IPAContext &context,
>   		     [[maybe_unused]] const YamlObject &tuningData)
>   {
> -	context.activeState.black.level = 255;
> +	context.activeState.black.level = 1.0;


I'm not sure I agree with that one actually...I expect it to be represented as an absolute pixel 
intensity value. Is there any functional benefit to making it a floating point? Side note: I think 
I'd also expect the starting point to be 0 rather than 255 (or 1.0), though I don't suppose it 
matters too much.

>   	return 0;
>   }
>   
> @@ -44,16 +44,16 @@ void BlackLevel::process(IPAContext &context,
>   	const unsigned int total =
>   		std::accumulate(begin(histogram), end(histogram), 0);
>   	const unsigned int pixelThreshold = ignoredPercentage_ * total;
> -	const unsigned int histogramRatio = 256 / SwIspStats::kYHistogramSize;
>   	const unsigned int currentBlackIdx =
> -		context.activeState.black.level / histogramRatio;
> +		context.activeState.black.level * SwIspStats::kYHistogramSize;
>   
>   	for (unsigned int i = 0, seen = 0;
>   	     i < currentBlackIdx && i < SwIspStats::kYHistogramSize;
>   	     i++) {
>   		seen += histogram[i];
>   		if (seen >= pixelThreshold) {
> -			context.activeState.black.level = i * histogramRatio;
> +			context.activeState.black.level =
> +				static_cast<double>(i) / SwIspStats::kYHistogramSize;
>   			LOG(IPASoftBL, Debug)
>   				<< "Auto-set black level: "
>   				<< i << "/" << SwIspStats::kYHistogramSize
> diff --git a/src/ipa/simple/algorithms/colors.cpp b/src/ipa/simple/algorithms/colors.cpp
> index 60fdca15..77492d9e 100644
> --- a/src/ipa/simple/algorithms/colors.cpp
> +++ b/src/ipa/simple/algorithms/colors.cpp
> @@ -36,7 +36,7 @@ int Colors::init(IPAContext &context,
>   	updateGammaTable(context);
>   
>   	auto &gains = context.activeState.gains;
> -	gains.red = gains.green = gains.blue = 256;
> +	gains.red = gains.green = gains.blue = 1.0;
Gains being floats I agree with.
>   
>   	return 0;
>   }
> @@ -47,7 +47,7 @@ void Colors::updateGammaTable(IPAContext &context)
>   	auto &blackLevel = context.activeState.gamma.blackLevel;
>   	blackLevel = context.activeState.black.level;
>   	const unsigned int blackIndex =
> -		blackLevel * IPAActiveState::kGammaLookupSize / 256;
> +		context.activeState.black.level * IPAActiveState::kGammaLookupSize;
>   	std::fill(gammaTable.begin(), gammaTable.begin() + blackIndex, 0);
>   	const float divisor = kGammaLookupSize - blackIndex - 1.0;
>   	for (unsigned int i = blackIndex; i < kGammaLookupSize; i++)
> @@ -67,15 +67,18 @@ void Colors::prepare(IPAContext &context,
>   	auto &gains = context.activeState.gains;
>   	auto &gammaTable = context.activeState.gamma.gammaTable;
>   	for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {
> -		constexpr unsigned int div =
> -			static_cast<double>(DebayerParams::kRGBLookupSize) * 256 / kGammaLookupSize;
> +		constexpr double div =
> +			static_cast<double>(DebayerParams::kRGBLookupSize) / kGammaLookupSize;
>   		/* Apply gamma after gain! */
>   		unsigned int idx;
> -		idx = std::min({ i * gains.red / div, kGammaLookupSize - 1 });
> +		idx = std::min({ static_cast<unsigned int>(i * gains.red / div),
> +				 kGammaLookupSize - 1 });
>   		params->red[i] = gammaTable[idx];
> -		idx = std::min({ i * gains.green / div, kGammaLookupSize - 1 });
> +		idx = std::min({ static_cast<unsigned int>(i * gains.green / div),
> +				 kGammaLookupSize - 1 });
>   		params->green[i] = gammaTable[idx];
> -		idx = std::min({ i * gains.blue / div, kGammaLookupSize - 1 });
> +		idx = std::min({ static_cast<unsigned int>(i * gains.blue / div),
> +				 kGammaLookupSize - 1 });
>   		params->blue[i] = gammaTable[idx];
>   	}
>   }
> @@ -87,7 +90,7 @@ void Colors::process(IPAContext &context,
>   		     [[maybe_unused]] ControlList &metadata)
>   {
>   	const SwIspStats::Histogram &histogram = stats->yHistogram;
> -	const uint8_t blackLevel = context.activeState.black.level;
> +	const double blackLevel = context.activeState.black.level;
>   
>   	/*
>   	 * Black level must be subtracted to get the correct AWB ratios, they
> @@ -104,12 +107,11 @@ void Colors::process(IPAContext &context,
>   	/*
>   	 * Calculate red and blue gains for AWB.
>   	 * Clamp max gain at 4.0, this also avoids 0 division.
> -	 * Gain: 128 = 0.5, 256 = 1.0, 512 = 2.0, etc.
>   	 */
>   	auto &gains = context.activeState.gains;
> -	gains.red = sumR <= sumG / 4 ? 1024 : 256 * sumG / sumR;
> -	gains.blue = sumB <= sumG / 4 ? 1024 : 256 * sumG / sumB;
> -	/* Green gain is fixed to 256 */
> +	gains.red = sumR <= sumG / 4 ? 4.0 : static_cast<double>(sumG) / sumR;
> +	gains.blue = sumB <= sumG / 4 ? 4.0 : static_cast<double>(sumG) / sumB;
> +	/* Green gain is fixed to 1.0 */
>   
>   	LOG(IPASoftColors, Debug) << "gain R/B " << gains.red << "/" << gains.blue;
>   }
> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
> index 979ddb8f..552d6cde 100644
> --- a/src/ipa/simple/ipa_context.h
> +++ b/src/ipa/simple/ipa_context.h
> @@ -9,7 +9,6 @@
>   #pragma once
>   
>   #include <array>
> -#include <stdint.h>
>   
>   #include <libipa/fc_queue.h>
>   
> @@ -23,17 +22,17 @@ struct IPASessionConfiguration {
>   
>   struct IPAActiveState {
>   	struct {
> -		uint8_t level;
> +		double level;
>   	} black;
>   	struct {
> -		unsigned int red;
> -		unsigned int green;
> -		unsigned int blue;
> +		double red;
> +		double green;
> +		double blue;
>   	} gains;
>   	static constexpr unsigned int kGammaLookupSize = 1024;
>   	struct {
>   		std::array<double, kGammaLookupSize> gammaTable;
> -		uint8_t blackLevel;
> +		double blackLevel;
>   	} gamma;
>   };
>
Milan Zamazal Aug. 13, 2024, 11:32 a.m. UTC | #2
Hi Dan,

thank you for review.

Dan Scally <dan.scally@ideasonboard.com> writes:

> Hi Milan
>
> On 17/07/2024 09:54, Milan Zamazal wrote:
>> It's more natural to represent color gains and black level as floating
>> point numbers rather than using a particular pixel-related
>> representation.
>>
>> double is used rather than float because it's a more common floating
>> point type in libcamera algorithms.  Otherwise there is no obvious
>> reason to select one over the other here.
>>
>> The constructed color tables still use integer representation for
>> efficiency.
>>
>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>> ---
>>   src/ipa/simple/algorithms/blc.cpp    |  8 ++++----
>>   src/ipa/simple/algorithms/colors.cpp | 26 ++++++++++++++------------
>>   src/ipa/simple/ipa_context.h         | 11 +++++------
>>   3 files changed, 23 insertions(+), 22 deletions(-)
>>
>> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
>> index 3b73d830..ab7e7dd9 100644
>> --- a/src/ipa/simple/algorithms/blc.cpp
>> +++ b/src/ipa/simple/algorithms/blc.cpp
>> @@ -24,7 +24,7 @@ BlackLevel::BlackLevel()
>>   int BlackLevel::init(IPAContext &context,
>>   		     [[maybe_unused]] const YamlObject &tuningData)
>>   {
>> -	context.activeState.black.level = 255;
>> +	context.activeState.black.level = 1.0;
>
>
> I'm not sure I agree with that one actually...I expect it to be represented as an absolute pixel intensity value. Is there any functional
> benefit to making it a floating point? 

There is no functional benefit.  It's just to make it bit-width
independent but maybe it's better to avoid useless conversions.  Is this
what you mean or do you have other reasons to prefer the pixel value?

> Side note: I think I'd also expect the starting point to be 0 rather
> than 255 (or 1.0), though I don't suppose it matters too much.

It's the maximum so that the algorithm below automatically updates it;
note that it is also assumed and has been observed that the right (low)
black level may not be reached on the first image run.  I'll add this
explanation to the commit message.

>>   	return 0;
>>   }
>>   @@ -44,16 +44,16 @@ void BlackLevel::process(IPAContext &context,
>>   	const unsigned int total =
>>   		std::accumulate(begin(histogram), end(histogram), 0);
>>   	const unsigned int pixelThreshold = ignoredPercentage_ * total;
>> -	const unsigned int histogramRatio = 256 / SwIspStats::kYHistogramSize;
>>   	const unsigned int currentBlackIdx =
>> -		context.activeState.black.level / histogramRatio;
>> +		context.activeState.black.level * SwIspStats::kYHistogramSize;
>>     	for (unsigned int i = 0, seen = 0;
>>   	     i < currentBlackIdx && i < SwIspStats::kYHistogramSize;
>>   	     i++) {
>>   		seen += histogram[i];
>>   		if (seen >= pixelThreshold) {
>> -			context.activeState.black.level = i * histogramRatio;
>> +			context.activeState.black.level =
>> +				static_cast<double>(i) / SwIspStats::kYHistogramSize;
>>   			LOG(IPASoftBL, Debug)
>>   				<< "Auto-set black level: "
>>   				<< i << "/" << SwIspStats::kYHistogramSize
>> diff --git a/src/ipa/simple/algorithms/colors.cpp b/src/ipa/simple/algorithms/colors.cpp
>> index 60fdca15..77492d9e 100644
>> --- a/src/ipa/simple/algorithms/colors.cpp
>> +++ b/src/ipa/simple/algorithms/colors.cpp
>> @@ -36,7 +36,7 @@ int Colors::init(IPAContext &context,
>>   	updateGammaTable(context);
>>     	auto &gains = context.activeState.gains;
>> -	gains.red = gains.green = gains.blue = 256;
>> +	gains.red = gains.green = gains.blue = 1.0;
> Gains being floats I agree with.
>>     	return 0;
>>   }
>> @@ -47,7 +47,7 @@ void Colors::updateGammaTable(IPAContext &context)
>>   	auto &blackLevel = context.activeState.gamma.blackLevel;
>>   	blackLevel = context.activeState.black.level;
>>   	const unsigned int blackIndex =
>> -		blackLevel * IPAActiveState::kGammaLookupSize / 256;
>> +		context.activeState.black.level * IPAActiveState::kGammaLookupSize;
>>   	std::fill(gammaTable.begin(), gammaTable.begin() + blackIndex, 0);
>>   	const float divisor = kGammaLookupSize - blackIndex - 1.0;
>>   	for (unsigned int i = blackIndex; i < kGammaLookupSize; i++)
>> @@ -67,15 +67,18 @@ void Colors::prepare(IPAContext &context,
>>   	auto &gains = context.activeState.gains;
>>   	auto &gammaTable = context.activeState.gamma.gammaTable;
>>   	for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {
>> -		constexpr unsigned int div =
>> -			static_cast<double>(DebayerParams::kRGBLookupSize) * 256 / kGammaLookupSize;
>> +		constexpr double div =
>> +			static_cast<double>(DebayerParams::kRGBLookupSize) / kGammaLookupSize;
>>   		/* Apply gamma after gain! */
>>   		unsigned int idx;
>> -		idx = std::min({ i * gains.red / div, kGammaLookupSize - 1 });
>> +		idx = std::min({ static_cast<unsigned int>(i * gains.red / div),
>> +				 kGammaLookupSize - 1 });
>>   		params->red[i] = gammaTable[idx];
>> -		idx = std::min({ i * gains.green / div, kGammaLookupSize - 1 });
>> +		idx = std::min({ static_cast<unsigned int>(i * gains.green / div),
>> +				 kGammaLookupSize - 1 });
>>   		params->green[i] = gammaTable[idx];
>> -		idx = std::min({ i * gains.blue / div, kGammaLookupSize - 1 });
>> +		idx = std::min({ static_cast<unsigned int>(i * gains.blue / div),
>> +				 kGammaLookupSize - 1 });
>>   		params->blue[i] = gammaTable[idx];
>>   	}
>>   }
>> @@ -87,7 +90,7 @@ void Colors::process(IPAContext &context,
>>   		     [[maybe_unused]] ControlList &metadata)
>>   {
>>   	const SwIspStats::Histogram &histogram = stats->yHistogram;
>> -	const uint8_t blackLevel = context.activeState.black.level;
>> +	const double blackLevel = context.activeState.black.level;
>>     	/*
>>   	 * Black level must be subtracted to get the correct AWB ratios, they
>> @@ -104,12 +107,11 @@ void Colors::process(IPAContext &context,
>>   	/*
>>   	 * Calculate red and blue gains for AWB.
>>   	 * Clamp max gain at 4.0, this also avoids 0 division.
>> -	 * Gain: 128 = 0.5, 256 = 1.0, 512 = 2.0, etc.
>>   	 */
>>   	auto &gains = context.activeState.gains;
>> -	gains.red = sumR <= sumG / 4 ? 1024 : 256 * sumG / sumR;
>> -	gains.blue = sumB <= sumG / 4 ? 1024 : 256 * sumG / sumB;
>> -	/* Green gain is fixed to 256 */
>> +	gains.red = sumR <= sumG / 4 ? 4.0 : static_cast<double>(sumG) / sumR;
>> +	gains.blue = sumB <= sumG / 4 ? 4.0 : static_cast<double>(sumG) / sumB;
>> +	/* Green gain is fixed to 1.0 */
>>     	LOG(IPASoftColors, Debug) << "gain R/B " << gains.red << "/" << gains.blue;
>>   }
>> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
>> index 979ddb8f..552d6cde 100644
>> --- a/src/ipa/simple/ipa_context.h
>> +++ b/src/ipa/simple/ipa_context.h
>> @@ -9,7 +9,6 @@
>>   #pragma once
>>     #include <array>
>> -#include <stdint.h>
>>     #include <libipa/fc_queue.h>
>>   @@ -23,17 +22,17 @@ struct IPASessionConfiguration {
>>     struct IPAActiveState {
>>   	struct {
>> -		uint8_t level;
>> +		double level;
>>   	} black;
>>   	struct {
>> -		unsigned int red;
>> -		unsigned int green;
>> -		unsigned int blue;
>> +		double red;
>> +		double green;
>> +		double blue;
>>   	} gains;
>>   	static constexpr unsigned int kGammaLookupSize = 1024;
>>   	struct {
>>   		std::array<double, kGammaLookupSize> gammaTable;
>> -		uint8_t blackLevel;
>> +		double blackLevel;
>>   	} gamma;
>>   };
>>
Milan Zamazal Aug. 15, 2024, 8:41 a.m. UTC | #3
Milan Zamazal <mzamazal@redhat.com> writes:

> Hi Dan,
>
> thank you for review.
>
> Dan Scally <dan.scally@ideasonboard.com> writes:
>
>> Hi Milan
>>
>> On 17/07/2024 09:54, Milan Zamazal wrote:
>>> It's more natural to represent color gains and black level as floating
>>> point numbers rather than using a particular pixel-related
>>> representation.
>>>
>>> double is used rather than float because it's a more common floating
>>> point type in libcamera algorithms.  Otherwise there is no obvious
>>> reason to select one over the other here.
>>>
>>> The constructed color tables still use integer representation for
>>> efficiency.
>>>
>>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>>> ---
>>>   src/ipa/simple/algorithms/blc.cpp    |  8 ++++----
>>>   src/ipa/simple/algorithms/colors.cpp | 26 ++++++++++++++------------
>>>   src/ipa/simple/ipa_context.h         | 11 +++++------
>>>   3 files changed, 23 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
>>> index 3b73d830..ab7e7dd9 100644
>>> --- a/src/ipa/simple/algorithms/blc.cpp
>>> +++ b/src/ipa/simple/algorithms/blc.cpp
>>> @@ -24,7 +24,7 @@ BlackLevel::BlackLevel()
>>>   int BlackLevel::init(IPAContext &context,
>>>   		     [[maybe_unused]] const YamlObject &tuningData)
>>>   {
>>> -	context.activeState.black.level = 255;
>>> +	context.activeState.black.level = 1.0;
>>
>>
>> I'm not sure I agree with that one actually...I expect it to be represented as an absolute pixel intensity value. Is there any functional
>> benefit to making it a floating point? 
>
> There is no functional benefit.  It's just to make it bit-width
> independent but maybe it's better to avoid useless conversions.  Is this
> what you mean or do you have other reasons to prefer the pixel value?

And there is another option, to use the range that
CameraSensorHelper::blackLevel() uses, see "Get black level from the
camera helper" patch.

I don't have a strong preference for any of the options and I'm open to
arguments why some of them is better than the others.

>> Side note: I think I'd also expect the starting point to be 0 rather
>> than 255 (or 1.0), though I don't suppose it matters too much.
>
> It's the maximum so that the algorithm below automatically updates it;
> note that it is also assumed and has been observed that the right (low)
> black level may not be reached on the first image run.  I'll add this
> explanation to the commit message.
>
>>>   	return 0;
>>>   }
>>>   @@ -44,16 +44,16 @@ void BlackLevel::process(IPAContext &context,
>>>   	const unsigned int total =
>>>   		std::accumulate(begin(histogram), end(histogram), 0);
>>>   	const unsigned int pixelThreshold = ignoredPercentage_ * total;
>>> -	const unsigned int histogramRatio = 256 / SwIspStats::kYHistogramSize;
>>>   	const unsigned int currentBlackIdx =
>>> -		context.activeState.black.level / histogramRatio;
>>> +		context.activeState.black.level * SwIspStats::kYHistogramSize;
>>>     	for (unsigned int i = 0, seen = 0;
>>>   	     i < currentBlackIdx && i < SwIspStats::kYHistogramSize;
>>>   	     i++) {
>>>   		seen += histogram[i];
>>>   		if (seen >= pixelThreshold) {
>>> -			context.activeState.black.level = i * histogramRatio;
>>> +			context.activeState.black.level =
>>> +				static_cast<double>(i) / SwIspStats::kYHistogramSize;
>>>   			LOG(IPASoftBL, Debug)
>>>   				<< "Auto-set black level: "
>>>   				<< i << "/" << SwIspStats::kYHistogramSize
>>> diff --git a/src/ipa/simple/algorithms/colors.cpp b/src/ipa/simple/algorithms/colors.cpp
>>> index 60fdca15..77492d9e 100644
>>> --- a/src/ipa/simple/algorithms/colors.cpp
>>> +++ b/src/ipa/simple/algorithms/colors.cpp
>>> @@ -36,7 +36,7 @@ int Colors::init(IPAContext &context,
>>>   	updateGammaTable(context);
>>>     	auto &gains = context.activeState.gains;
>>> -	gains.red = gains.green = gains.blue = 256;
>>> +	gains.red = gains.green = gains.blue = 1.0;
>> Gains being floats I agree with.
>>>     	return 0;
>>>   }
>>> @@ -47,7 +47,7 @@ void Colors::updateGammaTable(IPAContext &context)
>>>   	auto &blackLevel = context.activeState.gamma.blackLevel;
>>>   	blackLevel = context.activeState.black.level;
>>>   	const unsigned int blackIndex =
>>> -		blackLevel * IPAActiveState::kGammaLookupSize / 256;
>>> +		context.activeState.black.level * IPAActiveState::kGammaLookupSize;
>>>   	std::fill(gammaTable.begin(), gammaTable.begin() + blackIndex, 0);
>>>   	const float divisor = kGammaLookupSize - blackIndex - 1.0;
>>>   	for (unsigned int i = blackIndex; i < kGammaLookupSize; i++)
>>> @@ -67,15 +67,18 @@ void Colors::prepare(IPAContext &context,
>>>   	auto &gains = context.activeState.gains;
>>>   	auto &gammaTable = context.activeState.gamma.gammaTable;
>>>   	for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {
>>> -		constexpr unsigned int div =
>>> -			static_cast<double>(DebayerParams::kRGBLookupSize) * 256 / kGammaLookupSize;
>>> +		constexpr double div =
>>> +			static_cast<double>(DebayerParams::kRGBLookupSize) / kGammaLookupSize;
>>>   		/* Apply gamma after gain! */
>>>   		unsigned int idx;
>>> -		idx = std::min({ i * gains.red / div, kGammaLookupSize - 1 });
>>> +		idx = std::min({ static_cast<unsigned int>(i * gains.red / div),
>>> +				 kGammaLookupSize - 1 });
>>>   		params->red[i] = gammaTable[idx];
>>> -		idx = std::min({ i * gains.green / div, kGammaLookupSize - 1 });
>>> +		idx = std::min({ static_cast<unsigned int>(i * gains.green / div),
>>> +				 kGammaLookupSize - 1 });
>>>   		params->green[i] = gammaTable[idx];
>>> -		idx = std::min({ i * gains.blue / div, kGammaLookupSize - 1 });
>>> +		idx = std::min({ static_cast<unsigned int>(i * gains.blue / div),
>>> +				 kGammaLookupSize - 1 });
>>>   		params->blue[i] = gammaTable[idx];
>>>   	}
>>>   }
>>> @@ -87,7 +90,7 @@ void Colors::process(IPAContext &context,
>>>   		     [[maybe_unused]] ControlList &metadata)
>>>   {
>>>   	const SwIspStats::Histogram &histogram = stats->yHistogram;
>>> -	const uint8_t blackLevel = context.activeState.black.level;
>>> +	const double blackLevel = context.activeState.black.level;
>>>     	/*
>>>   	 * Black level must be subtracted to get the correct AWB ratios, they
>>> @@ -104,12 +107,11 @@ void Colors::process(IPAContext &context,
>>>   	/*
>>>   	 * Calculate red and blue gains for AWB.
>>>   	 * Clamp max gain at 4.0, this also avoids 0 division.
>>> -	 * Gain: 128 = 0.5, 256 = 1.0, 512 = 2.0, etc.
>>>   	 */
>>>   	auto &gains = context.activeState.gains;
>>> -	gains.red = sumR <= sumG / 4 ? 1024 : 256 * sumG / sumR;
>>> -	gains.blue = sumB <= sumG / 4 ? 1024 : 256 * sumG / sumB;
>>> -	/* Green gain is fixed to 256 */
>>> +	gains.red = sumR <= sumG / 4 ? 4.0 : static_cast<double>(sumG) / sumR;
>>> +	gains.blue = sumB <= sumG / 4 ? 4.0 : static_cast<double>(sumG) / sumB;
>>> +	/* Green gain is fixed to 1.0 */
>>>     	LOG(IPASoftColors, Debug) << "gain R/B " << gains.red << "/" << gains.blue;
>>>   }
>>> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
>>> index 979ddb8f..552d6cde 100644
>>> --- a/src/ipa/simple/ipa_context.h
>>> +++ b/src/ipa/simple/ipa_context.h
>>> @@ -9,7 +9,6 @@
>>>   #pragma once
>>>     #include <array>
>>> -#include <stdint.h>
>>>     #include <libipa/fc_queue.h>
>>>   @@ -23,17 +22,17 @@ struct IPASessionConfiguration {
>>>     struct IPAActiveState {
>>>   	struct {
>>> -		uint8_t level;
>>> +		double level;
>>>   	} black;
>>>   	struct {
>>> -		unsigned int red;
>>> -		unsigned int green;
>>> -		unsigned int blue;
>>> +		double red;
>>> +		double green;
>>> +		double blue;
>>>   	} gains;
>>>   	static constexpr unsigned int kGammaLookupSize = 1024;
>>>   	struct {
>>>   		std::array<double, kGammaLookupSize> gammaTable;
>>> -		uint8_t blackLevel;
>>> +		double blackLevel;
>>>   	} gamma;
>>>   };
>>>

Patch
diff mbox series

diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
index 3b73d830..ab7e7dd9 100644
--- a/src/ipa/simple/algorithms/blc.cpp
+++ b/src/ipa/simple/algorithms/blc.cpp
@@ -24,7 +24,7 @@  BlackLevel::BlackLevel()
 int BlackLevel::init(IPAContext &context,
 		     [[maybe_unused]] const YamlObject &tuningData)
 {
-	context.activeState.black.level = 255;
+	context.activeState.black.level = 1.0;
 	return 0;
 }
 
@@ -44,16 +44,16 @@  void BlackLevel::process(IPAContext &context,
 	const unsigned int total =
 		std::accumulate(begin(histogram), end(histogram), 0);
 	const unsigned int pixelThreshold = ignoredPercentage_ * total;
-	const unsigned int histogramRatio = 256 / SwIspStats::kYHistogramSize;
 	const unsigned int currentBlackIdx =
-		context.activeState.black.level / histogramRatio;
+		context.activeState.black.level * SwIspStats::kYHistogramSize;
 
 	for (unsigned int i = 0, seen = 0;
 	     i < currentBlackIdx && i < SwIspStats::kYHistogramSize;
 	     i++) {
 		seen += histogram[i];
 		if (seen >= pixelThreshold) {
-			context.activeState.black.level = i * histogramRatio;
+			context.activeState.black.level =
+				static_cast<double>(i) / SwIspStats::kYHistogramSize;
 			LOG(IPASoftBL, Debug)
 				<< "Auto-set black level: "
 				<< i << "/" << SwIspStats::kYHistogramSize
diff --git a/src/ipa/simple/algorithms/colors.cpp b/src/ipa/simple/algorithms/colors.cpp
index 60fdca15..77492d9e 100644
--- a/src/ipa/simple/algorithms/colors.cpp
+++ b/src/ipa/simple/algorithms/colors.cpp
@@ -36,7 +36,7 @@  int Colors::init(IPAContext &context,
 	updateGammaTable(context);
 
 	auto &gains = context.activeState.gains;
-	gains.red = gains.green = gains.blue = 256;
+	gains.red = gains.green = gains.blue = 1.0;
 
 	return 0;
 }
@@ -47,7 +47,7 @@  void Colors::updateGammaTable(IPAContext &context)
 	auto &blackLevel = context.activeState.gamma.blackLevel;
 	blackLevel = context.activeState.black.level;
 	const unsigned int blackIndex =
-		blackLevel * IPAActiveState::kGammaLookupSize / 256;
+		context.activeState.black.level * IPAActiveState::kGammaLookupSize;
 	std::fill(gammaTable.begin(), gammaTable.begin() + blackIndex, 0);
 	const float divisor = kGammaLookupSize - blackIndex - 1.0;
 	for (unsigned int i = blackIndex; i < kGammaLookupSize; i++)
@@ -67,15 +67,18 @@  void Colors::prepare(IPAContext &context,
 	auto &gains = context.activeState.gains;
 	auto &gammaTable = context.activeState.gamma.gammaTable;
 	for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {
-		constexpr unsigned int div =
-			static_cast<double>(DebayerParams::kRGBLookupSize) * 256 / kGammaLookupSize;
+		constexpr double div =
+			static_cast<double>(DebayerParams::kRGBLookupSize) / kGammaLookupSize;
 		/* Apply gamma after gain! */
 		unsigned int idx;
-		idx = std::min({ i * gains.red / div, kGammaLookupSize - 1 });
+		idx = std::min({ static_cast<unsigned int>(i * gains.red / div),
+				 kGammaLookupSize - 1 });
 		params->red[i] = gammaTable[idx];
-		idx = std::min({ i * gains.green / div, kGammaLookupSize - 1 });
+		idx = std::min({ static_cast<unsigned int>(i * gains.green / div),
+				 kGammaLookupSize - 1 });
 		params->green[i] = gammaTable[idx];
-		idx = std::min({ i * gains.blue / div, kGammaLookupSize - 1 });
+		idx = std::min({ static_cast<unsigned int>(i * gains.blue / div),
+				 kGammaLookupSize - 1 });
 		params->blue[i] = gammaTable[idx];
 	}
 }
@@ -87,7 +90,7 @@  void Colors::process(IPAContext &context,
 		     [[maybe_unused]] ControlList &metadata)
 {
 	const SwIspStats::Histogram &histogram = stats->yHistogram;
-	const uint8_t blackLevel = context.activeState.black.level;
+	const double blackLevel = context.activeState.black.level;
 
 	/*
 	 * Black level must be subtracted to get the correct AWB ratios, they
@@ -104,12 +107,11 @@  void Colors::process(IPAContext &context,
 	/*
 	 * Calculate red and blue gains for AWB.
 	 * Clamp max gain at 4.0, this also avoids 0 division.
-	 * Gain: 128 = 0.5, 256 = 1.0, 512 = 2.0, etc.
 	 */
 	auto &gains = context.activeState.gains;
-	gains.red = sumR <= sumG / 4 ? 1024 : 256 * sumG / sumR;
-	gains.blue = sumB <= sumG / 4 ? 1024 : 256 * sumG / sumB;
-	/* Green gain is fixed to 256 */
+	gains.red = sumR <= sumG / 4 ? 4.0 : static_cast<double>(sumG) / sumR;
+	gains.blue = sumB <= sumG / 4 ? 4.0 : static_cast<double>(sumG) / sumB;
+	/* Green gain is fixed to 1.0 */
 
 	LOG(IPASoftColors, Debug) << "gain R/B " << gains.red << "/" << gains.blue;
 }
diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
index 979ddb8f..552d6cde 100644
--- a/src/ipa/simple/ipa_context.h
+++ b/src/ipa/simple/ipa_context.h
@@ -9,7 +9,6 @@ 
 #pragma once
 
 #include <array>
-#include <stdint.h>
 
 #include <libipa/fc_queue.h>
 
@@ -23,17 +22,17 @@  struct IPASessionConfiguration {
 
 struct IPAActiveState {
 	struct {
-		uint8_t level;
+		double level;
 	} black;
 	struct {
-		unsigned int red;
-		unsigned int green;
-		unsigned int blue;
+		double red;
+		double green;
+		double blue;
 	} gains;
 	static constexpr unsigned int kGammaLookupSize = 1024;
 	struct {
 		std::array<double, kGammaLookupSize> gammaTable;
-		uint8_t blackLevel;
+		double blackLevel;
 	} gamma;
 };