[v3,12/14] libcamera: ipa: simple: Use symbolic constants for adjust defaults

25777 diff mbox series

The adjust algorithm already uses a symbolic constant for gamma.  Let's
introduce similar constants for contrast and saturation to prevent
copying the numeric defaults to multiple places.

Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
---
 src/ipa/simple/algorithms/adjust.cpp | 11 +++++++----
 src/ipa/simple/algorithms/adjust.h   |  2 ++
 2 files changed, 9 insertions(+), 4 deletions(-)

2026. 01. 14. 12:30 keltezéssel, Milan Zamazal írta:
> The adjust algorithm already uses a symbolic constant for gamma.  Let's
> introduce similar constants for contrast and saturation to prevent
> copying the numeric defaults to multiple places.
> 
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>   src/ipa/simple/algorithms/adjust.cpp | 11 +++++++----
>   src/ipa/simple/algorithms/adjust.h   |  2 ++
>   2 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/src/ipa/simple/algorithms/adjust.cpp b/src/ipa/simple/algorithms/adjust.cpp
> index 95032799f..60a191380 100644
> --- a/src/ipa/simple/algorithms/adjust.cpp
> +++ b/src/ipa/simple/algorithms/adjust.cpp
> @@ -23,10 +23,13 @@ LOG_DEFINE_CATEGORY(IPASoftAdjust)
>   
>   int Adjust::init(IPAContext &context, [[maybe_unused]] const YamlObject &tuningData)
>   {
> -	context.ctrlMap[&controls::Gamma] = ControlInfo(0.1f, 10.0f, kDefaultGamma);
> -	context.ctrlMap[&controls::Contrast] = ControlInfo(0.0f, 2.0f, 1.0f);
> +	context.ctrlMap[&controls::Gamma] =
> +		ControlInfo(0.1f, 10.0f, kDefaultGamma);
> +	context.ctrlMap[&controls::Contrast] =
> +		ControlInfo(0.0f, 2.0f, kDefaultContrast);
>   	if (context.ccmEnabled)
> -		context.ctrlMap[&controls::Saturation] = ControlInfo(0.0f, 2.0f, 1.0f);
> +		context.ctrlMap[&controls::Saturation] =
> +			ControlInfo(0.0f, 2.0f, kDefaultSaturation);
>   	return 0;
>   }
>   
> @@ -118,7 +121,7 @@ void Adjust::process([[maybe_unused]] IPAContext &context,
>   		metadata.set(controls::Contrast, contrast.value());
>   
>   	const auto &saturation = frameContext.saturation;
> -	metadata.set(controls::Saturation, saturation.value_or(1.0));
> +	metadata.set(controls::Saturation, saturation.value_or(kDefaultSaturation));
>   }
>   
>   REGISTER_IPA_ALGORITHM(Adjust, "Adjust")
> diff --git a/src/ipa/simple/algorithms/adjust.h b/src/ipa/simple/algorithms/adjust.h
> index 190d2079f..11d8297ca 100644
> --- a/src/ipa/simple/algorithms/adjust.h
> +++ b/src/ipa/simple/algorithms/adjust.h
> @@ -20,6 +20,8 @@ namespace libcamera {
>   namespace ipa::soft::algorithms {
>   
>   const float kDefaultGamma = 2.2f;
> +const float kDefaultContrast = 1.0f;
> +const float kDefaultSaturation = 1.0f;

If these are not used outside of adjust.cpp, then I would consider
potentially moving them there. Either way, it looks ok to me.

Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>


>   
>   class Adjust : public Algorithm
>   {

Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes:

> 2026. 01. 14. 12:30 keltezéssel, Milan Zamazal írta:
>> The adjust algorithm already uses a symbolic constant for gamma.  Let's
>> introduce similar constants for contrast and saturation to prevent
>> copying the numeric defaults to multiple places.
>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>> ---
>>   src/ipa/simple/algorithms/adjust.cpp | 11 +++++++----
>>   src/ipa/simple/algorithms/adjust.h   |  2 ++
>>   2 files changed, 9 insertions(+), 4 deletions(-)
>> diff --git a/src/ipa/simple/algorithms/adjust.cpp b/src/ipa/simple/algorithms/adjust.cpp
>> index 95032799f..60a191380 100644
>> --- a/src/ipa/simple/algorithms/adjust.cpp
>> +++ b/src/ipa/simple/algorithms/adjust.cpp
>> @@ -23,10 +23,13 @@ LOG_DEFINE_CATEGORY(IPASoftAdjust)
>>     int Adjust::init(IPAContext &context, [[maybe_unused]] const YamlObject &tuningData)
>>   {
>> -	context.ctrlMap[&controls::Gamma] = ControlInfo(0.1f, 10.0f, kDefaultGamma);
>> -	context.ctrlMap[&controls::Contrast] = ControlInfo(0.0f, 2.0f, 1.0f);
>> +	context.ctrlMap[&controls::Gamma] =
>> +		ControlInfo(0.1f, 10.0f, kDefaultGamma);
>> +	context.ctrlMap[&controls::Contrast] =
>> +		ControlInfo(0.0f, 2.0f, kDefaultContrast);
>>   	if (context.ccmEnabled)
>> -		context.ctrlMap[&controls::Saturation] = ControlInfo(0.0f, 2.0f, 1.0f);
>> +		context.ctrlMap[&controls::Saturation] =
>> +			ControlInfo(0.0f, 2.0f, kDefaultSaturation);
>>   	return 0;
>>   }
>>   @@ -118,7 +121,7 @@ void Adjust::process([[maybe_unused]] IPAContext &context,
>>   		metadata.set(controls::Contrast, contrast.value());
>>     	const auto &saturation = frameContext.saturation;
>> -	metadata.set(controls::Saturation, saturation.value_or(1.0));
>> +	metadata.set(controls::Saturation, saturation.value_or(kDefaultSaturation));
>>   }
>>     REGISTER_IPA_ALGORITHM(Adjust, "Adjust")
>> diff --git a/src/ipa/simple/algorithms/adjust.h b/src/ipa/simple/algorithms/adjust.h
>> index 190d2079f..11d8297ca 100644
>> --- a/src/ipa/simple/algorithms/adjust.h
>> +++ b/src/ipa/simple/algorithms/adjust.h
>> @@ -20,6 +20,8 @@ namespace libcamera {
>>   namespace ipa::soft::algorithms {
>>     const float kDefaultGamma = 2.2f;
>> +const float kDefaultContrast = 1.0f;
>> +const float kDefaultSaturation = 1.0f;
>
> If these are not used outside of adjust.cpp, then I would consider
> potentially moving them there. 

OK, kDefaultContrast and kDefaultSaturation can be moved there.

And all of them can be constexpr.

> Either way, it looks ok to me.
>
> Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>
>
>>     class Adjust : public Algorithm
>>   {