[PATCH v7 15/18] libcamera: software_isp: Use floating point for color parameters

[Umang Jain]

Hi Milan,

Thank you for the patch.

On 21/09/24 12:01 am, Milan Zamazal wrote:
> It's more natural to represent color gains 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.
>
> Black level still uses pixel (integer) values, for consistency with
> other libcamera parts.
>
> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>

double Sign-off-by


Otherwise,

Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>
> ---
>   src/ipa/simple/algorithms/awb.cpp |  9 ++++-----
>   src/ipa/simple/algorithms/lut.cpp | 13 ++++++++-----
>   src/ipa/simple/ipa_context.h      |  6 +++---
>   3 files changed, 15 insertions(+), 13 deletions(-)
>
> diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp
> index 3d76cc2f..195de41d 100644
> --- a/src/ipa/simple/algorithms/awb.cpp
> +++ b/src/ipa/simple/algorithms/awb.cpp
> @@ -24,7 +24,7 @@ int Awb::configure(IPAContext &context,
>   		   [[maybe_unused]] const IPAConfigInfo &configInfo)
>   {
>   	auto &gains = context.activeState.gains;
> -	gains.red = gains.green = gains.blue = 256;
> +	gains.red = gains.green = gains.blue = 1.0;
>   
>   	return 0;
>   }
> @@ -53,12 +53,11 @@ void Awb::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(IPASoftAwb, Debug) << "gain R/B " << gains.red << "/" << gains.blue;
>   }
> diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp
> index 44e13864..794d3644 100644
> --- a/src/ipa/simple/algorithms/lut.cpp
> +++ b/src/ipa/simple/algorithms/lut.cpp
> @@ -59,15 +59,18 @@ void Lut::prepare(IPAContext &context,
>   	auto &gammaTable = context.activeState.gamma.gammaTable;
>   	const unsigned int gammaTableSize = gammaTable.size();
>   	for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {
> -		const unsigned int div = static_cast<double>(DebayerParams::kRGBLookupSize) *
> -					 256 / gammaTableSize;
> +		const double div = static_cast<double>(DebayerParams::kRGBLookupSize) /
> +				   gammaTableSize;
>   		/* Apply gamma after gain! */
>   		unsigned int idx;
> -		idx = std::min({ i * gains.red / div, gammaTableSize - 1 });
> +		idx = std::min({ static_cast<unsigned int>(i * gains.red / div),
> +				 gammaTableSize - 1 });
>   		params->red[i] = gammaTable[idx];
> -		idx = std::min({ i * gains.green / div, gammaTableSize - 1 });
> +		idx = std::min({ static_cast<unsigned int>(i * gains.green / div),
> +				 gammaTableSize - 1 });
>   		params->green[i] = gammaTable[idx];
> -		idx = std::min({ i * gains.blue / div, gammaTableSize - 1 });
> +		idx = std::min({ static_cast<unsigned int>(i * gains.blue / div),
> +				 gammaTableSize - 1 });
>   		params->blue[i] = gammaTable[idx];
>   	}
>   }
> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
> index 737bbbc3..cf1ef3fd 100644
> --- a/src/ipa/simple/ipa_context.h
> +++ b/src/ipa/simple/ipa_context.h
> @@ -26,9 +26,9 @@ struct IPAActiveState {
>   	} blc;
>   
>   	struct {
> -		unsigned int red;
> -		unsigned int green;
> -		unsigned int blue;
> +		double red;
> +		double green;
> +		double blue;
>   	} gains;
>   
>   	static constexpr unsigned int kGammaLookupSize = 1024;