[PATCH v5 02/10] libcamera: software_isp: Use RGB type to represent gains

[Milan Zamazal]

Hi Laurent,

thank you for review.

Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:

> Hi Milan,
>
> Thank you for the patch.
>
> On Thu, Jan 30, 2025 at 07:14:39PM +0100, Milan Zamazal wrote:
>> Rather than using a custom struct to represent RGB values, let's use the
>> corresponding type.
>> 
>> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
>> ---
>>  src/ipa/simple/algorithms/awb.cpp | 14 ++++++++------
>>  src/ipa/simple/algorithms/lut.cpp |  6 +++---
>>  src/ipa/simple/ipa_context.h      |  7 ++-----
>>  3 files changed, 13 insertions(+), 14 deletions(-)
>> 
>> diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp
>> index 1efc7090..9c85b3f8 100644
>> --- a/src/ipa/simple/algorithms/awb.cpp
>> +++ b/src/ipa/simple/algorithms/awb.cpp
>> @@ -25,7 +25,7 @@ int Awb::configure(IPAContext &context,
>>  		   [[maybe_unused]] const IPAConfigInfo &configInfo)
>>  {
>>  	auto &gains = context.activeState.awb.gains;
>> -	gains.red = gains.green = gains.blue = 1.0;
>> +	gains = { { 1.0, 1.0, 1.0 } };
>>  
>>  	return 0;
>>  }
>> @@ -56,15 +56,17 @@ void Awb::process(IPAContext &context,
>>  	 * Clamp max gain at 4.0, this also avoids 0 division.
>>  	 */
>>  	auto &gains = context.activeState.awb.gains;
>> -	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 */
>> +	gains = { {
>> +		sumR <= sumG / 4 ? 4.0 : static_cast<double>(sumG) / sumR,
>> +		1.0,
>> +		sumB <= sumG / 4 ? 4.0 : static_cast<double>(sumG) / sumB,
>> +	} };
>>  
>> -	RGB<double> rgbGains{ { 1 / gains.red, 1 / gains.green, 1 / gains.blue } };
>> +	RGB<double> rgbGains{ { 1 / gains.r(), 1 / gains.g(), 1 / gains.b() } };
>>  	context.activeState.awb.temperatureK = estimateCCT(rgbGains);
>
> I've sent "[PATCH] ipa: libipa: vector: Add element-wise division
> operator" (https://patchwork.libcamera.org/patch/22721/). If it gets
> merged first, I'll replace this with 
>
> 	context.activeState.awb.temperatureK = estimateCCT(1.0 / gains);
>
> when merging your series. Otherwise I'll update the patch with this
> change.

OK.

>>  	LOG(IPASoftAwb, Debug)
>> -		<< "gain R/B: " << gains.red << "/" << gains.blue
>> +		<< "gain R/B: " << gains.r() << "/" << gains.b()
>>  		<< "; temperature: " << context.activeState.awb.temperatureK;
>
> You can simplify this to
>
>  	LOG(IPASoftAwb, Debug)
> 		<< "gains: " << gains << "; temperature: "
> 		<< context.activeState.awb.temperatureK;
>

OK.

>>  }
>>  
>> diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp
>> index d75ff710..bd37a955 100644
>> --- a/src/ipa/simple/algorithms/lut.cpp
>> +++ b/src/ipa/simple/algorithms/lut.cpp
>> @@ -104,13 +104,13 @@ void Lut::prepare(IPAContext &context,
>>  				   gammaTableSize;
>>  		/* Apply gamma after gain! */
>>  		unsigned int idx;
>> -		idx = std::min({ static_cast<unsigned int>(i * gains.red / div),
>> +		idx = std::min({ static_cast<unsigned int>(i * gains.r() / div),
>>  				 gammaTableSize - 1 });
>>  		params->red[i] = gammaTable[idx];
>> -		idx = std::min({ static_cast<unsigned int>(i * gains.green / div),
>> +		idx = std::min({ static_cast<unsigned int>(i * gains.g() / div),
>>  				 gammaTableSize - 1 });
>>  		params->green[i] = gammaTable[idx];
>> -		idx = std::min({ static_cast<unsigned int>(i * gains.blue / div),
>> +		idx = std::min({ static_cast<unsigned int>(i * gains.b() / div),
>>  				 gammaTableSize - 1 });
>>  		params->blue[i] = gammaTable[idx];
>
> This could be written
>
> 		const RGB<double> lutGains = (gains * i / div).min(gammaTableSize - 1);
> 		params->red[i] = gammaTable[static_cast<unsigned int>(lutGains.r())];
> 		params->green[i] = gammaTable[static_cast<unsigned int>(lutGains.g())];
> 		params->blue[i] = gammaTable[static_cast<unsigned int>(lutGains.b())];

Indeed, nicer.

> I wonder, should we add a Vector::cast<T> operator ? Then we could write
>
> 		const RGB<unsigned int> lutGains =
> 			(gains * i / div).cast<unsigned int>().min(gammaTableSize - 1);
> 		params->red[i] = gammaTable[lutGains.r())];
> 		params->green[i] = gammaTable[lutGains.g()];
> 		params->blue[i] = gammaTable[lutGains.b())];

Maybe yes if it was a repeating pattern but not worth at the moment.

> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
>>  	}
>> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
>> index 607af45a..df0552db 100644
>> --- a/src/ipa/simple/ipa_context.h
>> +++ b/src/ipa/simple/ipa_context.h
>> @@ -14,6 +14,7 @@
>>  #include <libcamera/controls.h>
>>  
>>  #include <libipa/fc_queue.h>
>> +#include <libipa/vector.h>
>>  
>>  namespace libcamera {
>>  
>> @@ -36,11 +37,7 @@ struct IPAActiveState {
>>  	} blc;
>>  
>>  	struct {
>> -		struct {
>> -			double red;
>> -			double green;
>> -			double blue;
>> -		} gains;
>> +		RGB<double> gains;
>>  		unsigned int temperatureK;
>>  	} awb;
>>