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

[Milan Zamazal]

Kieran Bingham <kieran.bingham at ideasonboard.com> writes:

> 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...

Added in v3 (the only change there).

> 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
>> >>
>>