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

[Robert Mader]

Quickly retested on top of latest master, including "libcamera: 
software_isp: Initialize exposure+gain before agc calculations"

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

On 08.11.24 10:43, 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 b4e32fe1c..1d7d370b5 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 2cf2a8774..453123d27 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:
>   	uint32_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 c8ad55a21..825c06757 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