[PATCH 4/5] ipa: rkisp1: blc: Report sensor black levels in metadata

[Paul Elder]

On Tue, Jul 02, 2024 at 05:39:56PM +0300, Laurent Pinchart wrote:
> On Tue, Jul 02, 2024 at 03:45:16PM +0200, Stefan Klug wrote:
> > On Tue, Jul 02, 2024 at 10:45:26AM +0200, Jacopo Mondi wrote:
> > > On Mon, Jul 01, 2024 at 04:38:27PM GMT, Stefan Klug wrote:
> > > > For the tuning process it is necessary to know the sensor black levels.
> > > > Add them to the metadata.
> > > >
> > > > Additionally enable raw support for this algorithm and add it to
> > > > uncalibrated.yaml, so that black levels get reported when capturing
> > > > tuning images.
> > > >
> > > > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > > > ---
> > > >  src/ipa/rkisp1/algorithms/blc.cpp     | 19 +++++++++++++++++++
> > > >  src/ipa/rkisp1/algorithms/blc.h       |  5 ++++-
> > > >  src/ipa/rkisp1/data/uncalibrated.yaml |  1 +
> > > >  3 files changed, 24 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/src/ipa/rkisp1/algorithms/blc.cpp b/src/ipa/rkisp1/algorithms/blc.cpp
> > > > index 0c39c3b47da5..a9f76b87d15a 100644
> > > > --- a/src/ipa/rkisp1/algorithms/blc.cpp
> > > > +++ b/src/ipa/rkisp1/algorithms/blc.cpp
> > > > @@ -9,6 +9,8 @@
> > > >
> > > >  #include <libcamera/base/log.h>
> > > >
> > > > +#include <libcamera/control_ids.h>
> > > > +
> > > >  #include "libcamera/internal/yaml_parser.h"
> > > >
> > > >  /**
> > > > @@ -38,6 +40,7 @@ LOG_DEFINE_CATEGORY(RkISP1Blc)
> > > >  BlackLevelCorrection::BlackLevelCorrection()
> > > >  	: tuningParameters_(false)
> > > >  {
> > > > +	supportsRaw_ = true;
> > > 
> > > Does it support raw for real ? Doesn't BLC require going through the
> > > ISP ?
> > 
> > Hm good question. What I wanted to achieve:
> > - black level data should be contained in the metadata so that we can
> >   capture raws with blacklevel

Ah I see so this is the one that's mainly causing the hack.

I was going to say yeah I didn't think that blc works without the ISP.

> > - It should be possible to overwrite the values with a tuning file, for
> >   cases, where you take the tuning files with a libcamera that doesn't
> >   know the correct black level for that sensor (it's arguable if that's
> >   an acceptable usecase, but I think it happens in practice)
> > 
> > So I could either move the whole configure logic into the ipa core, or
> > add raw support to the blc algorithm. To me it feels better to keep all
> > the black level related stuff in one place. But I'm ok with changing
> > that.
> 
> Supporting raw here sounds easier indeed, even if it may be cheating a
> bit. I'm fine with it, but a comment that explains what's going on would
> be useful. You probably want to update the prepare() function to return
> immediately when capturing raw, as programming the ISP makes little
> sense in that case.

+1


Paul

> 
> > > >  }
> > > >
> > > >  /**
> > > > @@ -107,6 +110,22 @@ void BlackLevelCorrection::prepare([[maybe_unused]] IPAContext &context,
> > > >  	params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_BLS;
> > > >  }
> > > >
> > > > +/**
> > > > + * \copydoc libcamera::ipa::Algorithm::process
> > > > + */
> > > > +void BlackLevelCorrection::process([[maybe_unused]] IPAContext &context,
> > > > +				   [[maybe_unused]] const uint32_t frame,
> > > > +				   [[maybe_unused]] IPAFrameContext &frameContext,
> > > > +				   [[maybe_unused]] const rkisp1_stat_buffer *stats,
> > > > +				   [[maybe_unused]] ControlList &metadata)
> > > > +{
> > > > +	metadata.set(controls::SensorBlackLevels,
> > > > +		     { static_cast<int32_t>(blackLevelRed_),
> > > > +		       static_cast<int32_t>(blackLevelGreenR_),
> > > > +		       static_cast<int32_t>(blackLevelGreenB_),
> > > > +		       static_cast<int32_t>(blackLevelBlue_) });
> > > 
> > > As black levels do not change you can store them in the context and
> > > populate the metadata in the main IPA module instead of making this
> > > algorithm support raw ?
> > 
> > Just to clarify: That also means the yaml reading code should be moved
> > there, right?
> 
> There are drawbacks to enabling raw support in this algorithms (it's a
> bit of a hack), and moving the code to the IPA module top-level code
> (it's a bit of a hack). I think this hack is cleaner.
> 
> > > > +}
> > > > +
> > > >  REGISTER_IPA_ALGORITHM(BlackLevelCorrection, "BlackLevelCorrection")
> > > >
> > > >  } /* namespace ipa::rkisp1::algorithms */
> > > > diff --git a/src/ipa/rkisp1/algorithms/blc.h b/src/ipa/rkisp1/algorithms/blc.h
> > > > index 460ebcc15739..4ecac233f88b 100644
> > > > --- a/src/ipa/rkisp1/algorithms/blc.h
> > > > +++ b/src/ipa/rkisp1/algorithms/blc.h
> > > > @@ -23,7 +23,10 @@ public:
> > > >  	void prepare(IPAContext &context, const uint32_t frame,
> > > >  		     IPAFrameContext &frameContext,
> > > >  		     rkisp1_params_cfg *params) override;
> > > > -
> > > > +	void process(IPAContext &context, const uint32_t frame,
> > > > +		     IPAFrameContext &frameContext,
> > > > +		     const rkisp1_stat_buffer *stats,
> > > > +		     ControlList &metadata) override;
> > > >  private:
> > > >  	bool tuningParameters_;
> > > >  	int16_t blackLevelRed_;
> > > > diff --git a/src/ipa/rkisp1/data/uncalibrated.yaml b/src/ipa/rkisp1/data/uncalibrated.yaml
> > > > index a7bbd8d84263..609012967e02 100644
> > > > --- a/src/ipa/rkisp1/data/uncalibrated.yaml
> > > > +++ b/src/ipa/rkisp1/data/uncalibrated.yaml
> > > > @@ -5,4 +5,5 @@ version: 1
> > > >  algorithms:
> > > >    - Agc:
> > > >    - Awb:
> > > > +  - BlackLevelCorrection:
> > > >  ...
> 
> -- 
> Regards,
> 
> Laurent Pinchart