[PATCH v2 1/1] libcamera: software_isp: Actually apply black level from tuning data

[Milan Zamazal]

Robert Mader <robert.mader at collabora.com> writes:

> Tested-by: Robert Mader <robert.mader at collabora.com>

Thanks for testing!

> On 03.12.24 10:38, Milan Zamazal wrote:
>> The black level obtained from the tuning file in software ISP is
>> retrieved in init (because this is the standard algorithm method with
>> access to tuning data) and stored into context.  But the context gets
>> reset in configure and the black level is lost and never applied.
>>
>> Let's store the black level from the tuning file into an algorithm
>> instance variable and put it into the context only later in configure.
>> This is similar to what rkisp1 IPA does with the values obtained from
>> the tuning file.
>>
>> Fixes: 41e3d61c745153779ed5a38634d7266bffd60d61 ("libcamera: software_isp: Clear IPA context on configure and stop")
>> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
>> ---
>>   src/ipa/simple/algorithms/blc.cpp | 7 +++++--
>>   src/ipa/simple/algorithms/blc.h   | 4 ++++
>>   src/ipa/simple/soft_simple.cpp    | 3 +--
>>   3 files changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
>> index b4e32fe1..1d7d370b 100644
>> --- a/src/ipa/simple/algorithms/blc.cpp
>> +++ b/src/ipa/simple/algorithms/blc.cpp
>> @@ -21,7 +21,8 @@ BlackLevel::BlackLevel()
>>   {
>>   }
>>   -int BlackLevel::init(IPAContext &context, const YamlObject &tuningData)
>> +int BlackLevel::init([[maybe_unused]] IPAContext &context,
>> +		     const YamlObject &tuningData)
>>   {
>>   	auto blackLevel = tuningData["blackLevel"].get<int16_t>();
>>   	if (blackLevel.has_value()) {
>> @@ -29,7 +30,7 @@ int BlackLevel::init(IPAContext &context, const YamlObject &tuningData)
>>   		 * Convert 16 bit values from the tuning file to 8 bit black
>>   		 * level for the SoftISP.
>>   		 */
>> -		context.configuration.black.level = blackLevel.value() >> 8;
>> +		definedLevel_ = blackLevel.value() >> 8;
>>   	}
>>   	return 0;
>>   }
>> @@ -37,6 +38,8 @@ int BlackLevel::init(IPAContext &context, const YamlObject &tuningData)
>>   int BlackLevel::configure(IPAContext &context,
>>   			  [[maybe_unused]] const IPAConfigInfo &configInfo)
>>   {
>> +	if (definedLevel_.has_value())
>> +		context.configuration.black.level = definedLevel_;
>>   	context.activeState.blc.level =
>>   		context.configuration.black.level.value_or(255);
>>   	return 0;
>> diff --git a/src/ipa/simple/algorithms/blc.h b/src/ipa/simple/algorithms/blc.h
>> index 67c688ae..52d59cab 100644
>> --- a/src/ipa/simple/algorithms/blc.h
>> +++ b/src/ipa/simple/algorithms/blc.h
>> @@ -7,6 +7,9 @@
>>     #pragma once
>>   +#include <optional>
>> +#include <stdint.h>
>> +
>>   #include "algorithm.h"
>>     namespace libcamera {
>> @@ -29,6 +32,7 @@ public:
>>   private:
>>   	int32_t exposure_;
>>   	double gain_;
>> +	std::optional<uint8_t> definedLevel_;
>>   };
>>     } /* namespace ipa::soft::algorithms */
>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
>> index ba3d5265..e1b6d3af 100644
>> --- a/src/ipa/simple/soft_simple.cpp
>> +++ b/src/ipa/simple/soft_simple.cpp
>> @@ -206,8 +206,7 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo)
>>   			(context_.configuration.agc.againMax -
>>   			 context_.configuration.agc.againMin) /
>>   			100.0;
>> -		if (!context_.configuration.black.level.has_value() &&
>> -		    camHelper_->blackLevel().has_value()) {
>> +		if (camHelper_->blackLevel().has_value()) {
>>   			/*
>>   			 * The black level from camHelper_ is a 16 bit value, software ISP
>>   			 * works with 8 bit pixel values, both regardless of the actual