[PATCH v5 2/2] libcamera: software_isp: Black level from tuning file

[Kieran Bingham]

Quoting Milan Zamazal (2024-10-18 10:26:28)
> This patch allows obtaining a black level from a tuning file in addition
> to the camera sensor helper.  If both of them define a black level, the
> one from the tuning file takes precedence.
> 
> The use cases are:
> 
> - A user wants to use a different black level, for whatever reason.
> 
> - There is a sensor without known gains but with a known black level.
>   Because a camera sensor helper cannot be defined without specifying
>   gains, the only way to specify the black level is using the tuning
>   file.  Software ISP uses its fallback gain handling in such a case.
> 
> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
> ---
>  src/ipa/simple/algorithms/blc.cpp | 9 +++++++++
>  src/ipa/simple/algorithms/blc.h   | 1 +
>  src/ipa/simple/soft_simple.cpp    | 3 ++-
>  3 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
> index a7af2e12c..8516c4335 100644
> --- a/src/ipa/simple/algorithms/blc.cpp
> +++ b/src/ipa/simple/algorithms/blc.cpp
> @@ -21,11 +21,20 @@ BlackLevel::BlackLevel()
>  {
>  }
>  
> +int BlackLevel::init(IPAContext &context, const YamlObject &tuningData)
> +{
> +       auto blackLevel = tuningData["blackLevel"].get<int16_t>();
> +       if (blackLevel.has_value())
> +               context.configuration.black.level = blackLevel.value() / 256;

It's not essential - but for converting between bit depths - I would
think using bit shifting would be clearer (keep this however you prefer
though).

	/*
	 * Convert 16 bit values from the tuning file to 8 bit black
	 * level for the SoftISP.
	 */
	.... = blackLevel.value() >> 8;

might be more clear than
	.... = blackLevel.value() / 256;

> +       return 0;
> +}
> +
>  int BlackLevel::configure(IPAContext &context,
>                           [[maybe_unused]] const IPAConfigInfo &configInfo)
>  {
>         context.activeState.blc.level =
>                 context.configuration.black.level.value_or(255);
> +       LOG(IPASoftBL, Info) << "pdm: blll: " << context.activeState.blc.level;

What's pdm: blll? Should this be removed? or changed to be Debug level ?


For the update itself with those handled, I think having the Tuning File
able to specify the black level is helpful:

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

>         return 0;
>  }
>  
> diff --git a/src/ipa/simple/algorithms/blc.h b/src/ipa/simple/algorithms/blc.h
> index 828ad8b18..2cf2a8774 100644
> --- a/src/ipa/simple/algorithms/blc.h
> +++ b/src/ipa/simple/algorithms/blc.h
> @@ -19,6 +19,7 @@ public:
>         BlackLevel();
>         ~BlackLevel() = default;
>  
> +       int init(IPAContext &context, const YamlObject &tuningData) override;
>         int configure(IPAContext &context, const IPAConfigInfo &configInfo) override;
>         void process(IPAContext &context, const uint32_t frame,
>                      IPAFrameContext &frameContext,
> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
> index e96e65bd1..03525b2f2 100644
> --- a/src/ipa/simple/soft_simple.cpp
> +++ b/src/ipa/simple/soft_simple.cpp
> @@ -201,7 +201,8 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo)
>                         (context_.configuration.agc.againMax -
>                          context_.configuration.agc.againMin) /
>                         100.0;
> -               if (camHelper_->blackLevel().has_value()) {
> +               if (!context_.configuration.black.level.has_value() &&
> +                   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
> -- 
> 2.44.1
>