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

[Kieran Bingham]

Quoting Milan Zamazal (2024-10-09 20:19:33)
> 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.
> 
> 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 | 6 +++++-
>  src/ipa/simple/ipa_context.h      | 4 ++++
>  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 b9f2aaa6d..a7af2e12c 100644
> --- a/src/ipa/simple/algorithms/blc.cpp
> +++ b/src/ipa/simple/algorithms/blc.cpp
> @@ -24,7 +24,8 @@ BlackLevel::BlackLevel()
>  int BlackLevel::configure(IPAContext &context,
>                           [[maybe_unused]] const IPAConfigInfo &configInfo)
>  {
> -       context.activeState.blc.level = 255;
> +       context.activeState.blc.level =
> +               context.configuration.black.level.value_or(255);
>         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/ipa_context.h b/src/ipa/simple/ipa_context.h
> index 3519f20f6..fd121eebe 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 <stdint.h>
>  
>  #include <libipa/fc_queue.h>
> @@ -22,6 +23,9 @@ struct IPASessionConfiguration {
>                 int32_t exposureMin, exposureMax;
>                 double againMin, againMax, againMinStep;
>         } agc;
> +       struct {
> +               std::optional<uint8_t> level;

Maybe I asked this before. Is 8 bits enough ? I guess it is if you're
dividing by 256 ... but I don't understand what happens on a 12 bit
sensor here ...

> +       } black;
>  };
>  
>  struct IPAActiveState {
> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
> index b28c7039f..ad6334ff6 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() / 256;

Is 256 going to work for all sensor modes here? What happens if a sensor
supports 8, 10, and 12 bits per pixel?

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