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

[Kieran Bingham]

Quoting Milan Zamazal (2024-10-18 15:02:54)
> Hi Kieran,
> 
> thank you for review.
> 
> Kieran Bingham <kieran.bingham at ideasonboard.com> writes:
> 
> > 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;
> 
> OK.
> 
> >> +       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 ?
> 
> A forgotten statement for checking the value, should be removed.

Ack, so with that removed, CI is clear - so we can merge the next
version!

https://gitlab.freedesktop.org/camera/libcamera/-/pipelines/1292807

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