[PATCH 3/5] ipa: rkisp1: blc: Query black levels from camera sensor helper

[Kieran Bingham]

Quoting Jacopo Mondi (2024-07-02 09:00:44)
> Hi Stefan
> 
> On Mon, Jul 01, 2024 at 04:38:26PM GMT, Stefan Klug wrote:
> > The black levels from the camera sensor helper are then used to do the
> > black level correction. Black levels can still be overwritten by the
> > tuning file.
> >
> > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > ---
> >  src/ipa/rkisp1/algorithms/blc.cpp | 28 ++++++++++++++++++++++++----
> >  1 file changed, 24 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/ipa/rkisp1/algorithms/blc.cpp b/src/ipa/rkisp1/algorithms/blc.cpp
> > index d2e743541c99..0c39c3b47da5 100644
> > --- a/src/ipa/rkisp1/algorithms/blc.cpp
> > +++ b/src/ipa/rkisp1/algorithms/blc.cpp
> > @@ -46,10 +46,30 @@ BlackLevelCorrection::BlackLevelCorrection()
> >  int BlackLevelCorrection::init([[maybe_unused]] IPAContext &context,
> >                              const YamlObject &tuningData)
> >  {
> > -     blackLevelRed_ = tuningData["R"].get<int16_t>(256);
> > -     blackLevelGreenR_ = tuningData["Gr"].get<int16_t>(256);
> > -     blackLevelGreenB_ = tuningData["Gb"].get<int16_t>(256);
> > -     blackLevelBlue_ = tuningData["B"].get<int16_t>(256);
> > +     auto blackLevels = context.camHelper->blackLevels();
> > +     if (blackLevels) {
> > +             Span<const int32_t, 4> levels = *blackLevels;
> > +             blackLevelRed_ = levels[0];
> > +             blackLevelGreenR_ = levels[1];
> > +             blackLevelGreenB_ = levels[2];
> > +             blackLevelBlue_ = levels[3];
> > +     } else
> > +             LOG(RkISP1Blc, Warning)
> > +                     << "No black levels provided by camera sensor helper";
> > +
> > +     if (!blackLevels || (tuningData.contains("R") &&
> 
> Is there any need to get context.camHelper->blackLevels() if the
> values are in the config file ?
> 
> 
> > +                          tuningData.contains("Gr") &&
> > +                          tuningData.contains("Gb") &&
> > +                          tuningData.contains("B"))) {
> > +             blackLevelRed_ = tuningData["R"].get<int16_t>(256);
> > +             blackLevelGreenR_ = tuningData["Gr"].get<int16_t>(256);
> > +             blackLevelGreenB_ = tuningData["Gb"].get<int16_t>(256);
> > +             blackLevelBlue_ = tuningData["B"].get<int16_t>(256);

Why 256 here? Does a value have to be specified?

Maybe we should move a parse black level helper out to some common yaml
helpers (Do we have anywhere for that yet, that can read defined
libcamera types from yaml?) ... and it should only return all values if
all of them are present and parsed correctly - otherwise R=3200, Gr=256,
Gb=256, B=256 probably wouldn't make sense?



> > +
> > +             if (blackLevels)
> 
> Is this a Warning ? Same question above. I would Warn only if no BLC in
> sensor helpers AND in config file.
> 
> > +                     LOG(RkISP1Blc, Warning)
> > +                             << "Black levels overwritten by tuning file";

Yes, I think the tuning file should take precedence, so I don't think
it's a warning. Perhaps a debug print to say which values get loaded and
where from ?


> > +     }
> >
> >       tuningParameters_ = true;
> >
> > --
> > 2.43.0
> >