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

[Stefan Klug]

On Tue, Jul 02, 2024 at 11:51:03AM +0100, Kieran Bingham wrote:
> 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?

Digging into the git history... these were taken from the imx219 tuning
file (and scaled to 12bits) as sensible defaults. We could either keep
them or error out. My strategy was to stay compatible with existing
tuning files. If we go the "Either correct data or fail" route, we
should remove the defaults.

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

No that wouldn't make much sense, but is the old behaviour :-). I can
have a look there.

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

As said in my answer to Jacopo, the warning was on purpose, but I'm fine
with making it a debug output (or info?)

Cheers,
Stefan

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