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

[Laurent Pinchart]

On Wed, Jul 03, 2024 at 01:30:58PM +0100, Kieran Bingham wrote:
> Quoting Stefan Klug (2024-07-03 13:22:49)
> > On Wed, Jul 03, 2024 at 01:15:55PM +0100, Kieran Bingham wrote:
> > > Quoting Stefan Klug (2024-07-03 11:39:51)
> > > > Add sensor black levels to the metadata of the rkisp1 pipeline.
> > > > 
> > > > Additionally enable raw support for this algorithm and add it to
> > > > uncalibrated.yaml, so that black levels get reported when capturing
> > > > tuning images. This is a bit of a hack, because no actual black level
> > > > correction is taking place in raw mode, but it is the easiest way to get
> > > > blacklevel reported for raw streams.
> > > > 
> > > > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > > > ---
> > > >  src/ipa/rkisp1/algorithms/blc.cpp     | 28 +++++++++++++++++++++++++++
> > > >  src/ipa/rkisp1/algorithms/blc.h       |  5 ++++-
> > > >  src/ipa/rkisp1/data/uncalibrated.yaml |  1 +
> > > >  3 files changed, 33 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/src/ipa/rkisp1/algorithms/blc.cpp b/src/ipa/rkisp1/algorithms/blc.cpp
> > > > index 87025e4f8c72..71c62b009707 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,13 @@ LOG_DEFINE_CATEGORY(RkISP1Blc)
> > > >  BlackLevelCorrection::BlackLevelCorrection()
> > > >         : tuningParameters_(false)
> > > >  {
> > > > +       /*
> > > > +        * This is a bit of a hack. In raw mode no black level correction
> > > > +        * happens. This flag is used to ensure the metadata gets populated with
> > > > +        * the black level which is needed to capture proper raw images for
> > > > +        * tuning.
> > > > +        */
> > > > +       supportsRaw_ = true;
> > > >  }
> > > >  
> > > >  /**
> > > > @@ -107,6 +116,9 @@ void BlackLevelCorrection::prepare([[maybe_unused]] IPAContext &context,
> > > >                                    [[maybe_unused]] IPAFrameContext &frameContext,
> > > >                                    rkisp1_params_cfg *params)
> > > >  {
> > > > +       if (context.configuration.raw)
> > > > +               return;
> > > > +
> > > >         if (frame > 0)
> > > >                 return;
> > > >  
> > > > @@ -125,6 +137,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)

Drop [[maybe_unused]] for metadata.

> > > > +{
> > > 
> > > Hrm. ... now ... should we report SensorBlackLevels at a bit-depth that
> > > matches the bit-depth of the image?
> > > 
> > > I.e ... do we need to scale these 16 bit numbers to 8,10,12 bit
> > > depending on the sensor configuration and output file bit-depth?
> > 
> > No, that is specified in the control description: ... These values are
> > returned as numbers out of a 16-bit pixel range (as if pixels ranged
> > from 0 to 65535)...
> > 
> > That's the reason why I believe it makes sense to unify that all over
> > libcamera. And I believe it is sensible as otherwise you would have to
> > explain that the sensor bitwidth and the output bitwidth can be
> > different and to wich one the black level relates...
> 
> Ack, in that case:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> I presume it would then be up to the application writing a dng for
> instance to know the correct way to write the black level. I "assume"
> any black level in a dng file is expected to match the sensor bit-depth,
> but I'm absolutely fine with the metadata being stipulated by us, as
> that's libcamera metadata and matching the value that would be set on a
> control certainly makes sense.

See dng_writer.cpp:

	/* Map the 16-bit value to the bits per sample range. */
	blackLevel[i] = level >> (16 - info->bitsPerSample);

:-)

> > > > +       metadata.set(controls::SensorBlackLevels,
> > > > +                    { static_cast<int32_t>(blackLevelRed_),
> > > > +                      static_cast<int32_t>(blackLevelGreenR_),
> > > > +                      static_cast<int32_t>(blackLevelGreenB_),
> > > > +                      static_cast<int32_t>(blackLevelBlue_) });
> > > > +}
> > > > +
> > > >  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