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

[Kieran Bingham]

Quoting Milan Zamazal (2024-10-10 09:33:21)
> Kieran Bingham <kieran.bingham at ideasonboard.com> writes:
> 
> > 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 ...
> 
> Software ISP currently works with 8 bits everywhere.  The sensor pixels
> are cut to 8bit if needed before processing, see e.g. https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/software_isp/debayer_cpu.cpp#n168.
> 
> This might change with GPU processing but in such a case all the
> algorithms would have to be revised.
> 
> >> +       } 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?
> 
> The black level from CameraSensorHelper is a 16 bit value
> (https://libcamera.org/internal-api-html/classlibcamera_1_1ipa_1_1CameraSensorHelper.html#ad6d462fc5828e841563289eef22d3174)
> and software ISP works with 8 bit values regardless of the number of
> sensor pixel bits.  That means all the values are sensor pixel width
> independent.

It would be really helpful to add a comment here stating that I think...


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

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