[PATCH 1/1] libcamera: software_isp: Get black level from the camera helper

[Kieran Bingham]

Quoting Milan Zamazal (2024-08-15 09:37:16)
> The black level in software ISP is unconditionally guessed from the
> obtained frames.  CameraSensorHelper optionally provides the black level
> from camera specifications now.  Let's use the value if available.
> 
> The black level value initialization is moved from init to configure
> because this is where we get the other configuration (gains) from
> CameraSensorHelper in software ISP.
> 
> If the black level is not available from the given CameraSensorHelper
> instance, it's still determined on the fly.
> 
> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
> ---
>  src/ipa/simple/algorithms/blc.cpp | 10 +++++++---
>  src/ipa/simple/algorithms/blc.h   |  3 ++-
>  src/ipa/simple/ipa_context.h      |  3 ++-
>  src/ipa/simple/soft_simple.cpp    |  3 +++
>  4 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
> index d01c2c14..5dc924bb 100644
> --- a/src/ipa/simple/algorithms/blc.cpp
> +++ b/src/ipa/simple/algorithms/blc.cpp
> @@ -21,10 +21,11 @@ BlackLevel::BlackLevel()
>  {
>  }
>  
> -int BlackLevel::init(IPAContext &context,
> -                    [[maybe_unused]] const YamlObject &tuningData)
> +int BlackLevel::configure(typename Module::Context &context,
> +                         [[maybe_unused]] const typename Module::Config &configInfo)
>  {
> -       context.activeState.black.level = 1.0;
> +       context.activeState.black.level =
> +               context.configuration.black.level.value_or(1.0);
>         return 0;
>  }
>  
> @@ -34,6 +35,9 @@ void BlackLevel::process(IPAContext &context,
>                          const SwIspStats *stats,
>                          [[maybe_unused]] ControlList &metadata)
>  {
> +       if (context.configuration.black.level.has_value())
> +               return;
> +
>         if (frameContext.sensor.exposure == exposure_ &&
>             frameContext.sensor.gain == gain_) {
>                 return;
> diff --git a/src/ipa/simple/algorithms/blc.h b/src/ipa/simple/algorithms/blc.h
> index 9b9daab1..586a23d2 100644
> --- a/src/ipa/simple/algorithms/blc.h
> +++ b/src/ipa/simple/algorithms/blc.h
> @@ -24,7 +24,8 @@ public:
>         BlackLevel();
>         ~BlackLevel() = default;
>  
> -       int init(IPAContext &context, const YamlObject &tuningData)
> +       int configure(typename Module::Context &context,
> +                     const typename Module::Config &configInfo)
>                 override;
>         void process(IPAContext &context, const uint32_t frame,
>                      IPAFrameContext &frameContext,
> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
> index 0e2096f8..08f965f4 100644
> --- a/src/ipa/simple/ipa_context.h
> +++ b/src/ipa/simple/ipa_context.h
> @@ -8,6 +8,7 @@
>  #pragma once
>  
>  #include <array>
> +#include <optional>
>  
>  #include <libcamera/controls.h>
>  
> @@ -24,7 +25,7 @@ struct IPASessionConfiguration {
>                 double againMin, againMax, againMinStep;
>         } agc;
>         struct {
> -               double level;
> +               std::optional<double> level;
>         } black;
>  };
>  
> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
> index aef5f6e5..ac7a22b7 100644
> --- a/src/ipa/simple/soft_simple.cpp
> +++ b/src/ipa/simple/soft_simple.cpp
> @@ -201,6 +201,9 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo)
>                         (context_.configuration.agc.againMax -
>                          context_.configuration.agc.againMin) /
>                         100.0;
> +               if (camHelper_->blackLevel().has_value())
> +                       context_.configuration.black.level =
> +                               camHelper_->blackLevel().value() / 65536.0;

Shouldn't this scale the value depending on the cameras configured bit
depth or such for when you use it in your calculations on pixel values?

Why 65536? 

I've recently just pushed the IMX283 black level value as:

> +		/* From datasheet: 0x32 at 10bits. */
> +		blackLevel_ = 3200;


Do you treat black level as value between 0 and 1 ?

--
Kieran



>         } else {
>                 /*
>                  * The camera sensor gain (g) is usually not equal to the value written
> -- 
> 2.44.1
>