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

[Stefan Klug]

Hi Jacopo,

On Tue, Jul 02, 2024 at 10:00:44AM +0200, Jacopo Mondi wrote:
> 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 ?

The strategy now is to move away from the having the back levels in the
config. After discussion with Laurent, libcamera should at least know
the pedestal values. When we add additional measurements they could be
added to the tuning file as offsets. So camHelper->blackLevels() is the
default, the tuning file is kept for backwards compatibility and to be
able to specify the values when libcamera doesn't have that data.

> 
> 
> > +			     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);
> > +
> > +		if (blackLevels)
> 
> Is this a Warning ? Same question above. I would Warn only if no BLC in
> sensor helpers AND in config file.

Yes, I would say so. Basically we are depricating the black levels from
the tuning file and moving them into sensor helpers. The first one
should be issued as it points to an incomplete implementation in
libcamera. The second one only shows up, if the fisrt one didn't show
up.

Best regards,
Stefan

> 
> > +			LOG(RkISP1Blc, Warning)
> > +				<< "Black levels overwritten by tuning file";
> > +	}
> >
> >  	tuningParameters_ = true;
> >
> > --
> > 2.43.0
> >