[libcamera-devel] [libcamera-devel 5/5] ipa: rkisp1: add support of Black Level Correction default values

[Laurent Pinchart]

Hi Florian,

On Wed, Jun 01, 2022 at 10:19:44AM +0200, Florian Sylvestre wrote:
> >> +     const YamlObject &blcObject = (*params)["BlackLevelCorrection"];
> >> +
> >> +     if (!blcObject.isDictionary())
> >> +             return -EINVAL;
> >> +
> >> +     blackLevelRed_ = blcObject["R"].get<int32_t>(256);
> > 
> > Can the value be negative ? If not, let go to uint32_t. Actually, you'll
> > have uint16_t support when rebasing on top of the YAML parser
> > improvements, so you can use that.
> 
> The values can be negatives.
> FYI: Extracted from .h comments:
>  * struct rkisp1_cif_isp_bls_fixed_val - BLS fixed subtraction values
>  *
>  * The values will be subtracted from the sensor
>  * values. Therefore a negative value means addition instead of subtraction!

I don't expect negative values to be very useful, but that's fine.

> I have seen the [RFC] on modifications for Yaml Parser to support int16,
> but since it's 'only' at [RFC] state, should we not provide this version and
> update it when the support of int16 will be available?

You can rebase on top of the RFC version (v2 available from
https://gitlab.com/ideasonboard/nxp/libcamera/-/commits/pinchartl/yaml/)
as it should get merged very soon.

> Le jeu. 26 mai 2022 à 13:04, Laurent Pinchart a écrit :
> > On Mon, May 23, 2022 at 11:24:35AM +0200, Florian Sylvestre wrote:
> > > Get default values for Black Level Correction algorithm form Yaml
> > > configuration file.
> >
> > s/form Yaml configuration/from YAML tuning/
> >
> > > Signed-off-by: Florian Sylvestre <fsylvestre at baylibre.com>
> > > ---
> > >  src/ipa/rkisp1/algorithms/blc.cpp | 46 ++++++++++++++++++++++++++++---
> > >  src/ipa/rkisp1/algorithms/blc.h   |  9 ++++++
> > >  2 files changed, 51 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/src/ipa/rkisp1/algorithms/blc.cpp b/src/ipa/rkisp1/algorithms/blc.cpp
> > > index 0c5948ff..177bc22e 100644
> > > --- a/src/ipa/rkisp1/algorithms/blc.cpp
> > > +++ b/src/ipa/rkisp1/algorithms/blc.cpp
> > > @@ -7,6 +7,10 @@
> > >
> > >  #include "blc.h"
> > >
> > > +#include <libcamera/base/log.h>
> > > +
> > > +#include <libcamera/internal/yaml_parser.h>
> >
> > For internal headers we use "" instead of <>.
> >
> > > +
> > >  /**
> > >   * \file blc.h
> > >   */
> > > @@ -29,6 +33,40 @@ namespace ipa::rkisp1::algorithms {
> > >   * isn't currently supported.
> > >   */
> > >
> > > +LOG_DEFINE_CATEGORY(RkISP1Blc)
> > > +
> > > +/**
> > > + * \copydoc libcamera::ipa::Algorithm::init
> > > + */
> > > +int BlackLevelCorrection::init(IPAContext &context,
> > > +                            const YamlObject *params)
> > > +{
> > > +     if (context.frameContext.frameCount > 0)
> > > +             return -EBUSY;
> > > +
> > > +     /* Parse property "BlackLevelCorrection" */
> > > +     if (!params->contains("BlackLevelCorrection"))
> > > +             return -EINVAL;
> > > +
> > > +     const YamlObject &blcObject = (*params)["BlackLevelCorrection"];
> > > +
> > > +     if (!blcObject.isDictionary())
> > > +             return -EINVAL;
> > > +
> > > +     blackLevelRed_ = blcObject["R"].get<int32_t>(256);
> >
> > Can the value be negative ? If not, let go to uint32_t. Actually, you'll
> > have uint16_t support when rebasing on top of the YAML parser
> > improvements, so you can use that.
> >
> > > +     blackLevelGreenR_ = blcObject["Gr"].get<int32_t>(256);
> > > +     blackLevelGreenB_ = blcObject["Gb"].get<int32_t>(256);
> > > +     blackLevelBlue_ = blcObject["B"].get<int32_t>(256);
> > > +
> > > +     LOG(RkISP1Blc, Debug)
> > > +             << " Read black levels red " << blackLevelRed_
> > > +             << " green (red) " << blackLevelGreenR_
> > > +             << " green (blue) " << blackLevelGreenB_
> > > +             << " blue " << blackLevelBlue_;
> >
> >                 << " Read black levels red " << blackLevelRed_
> >                 << ", green (red) " << blackLevelGreenR_
> >                 << ", green (blue) " << blackLevelGreenB_
> >                 << ", blue " << blackLevelBlue_;
> >
> > > +
> > > +     return 0;
> > > +}
> > > +
> > >  /**
> > >   * \copydoc libcamera::ipa::Algorithm::prepare
> > >   */
> > > @@ -42,10 +80,10 @@ void BlackLevelCorrection::prepare(IPAContext &context,
> > >        * \todo Use a configuration file for it ?
> > >        */
> > >       params->others.bls_config.enable_auto = 0;
> > > -     params->others.bls_config.fixed_val.r = 256;
> > > -     params->others.bls_config.fixed_val.gr = 256;
> > > -     params->others.bls_config.fixed_val.gb = 256;
> > > -     params->others.bls_config.fixed_val.b = 256;
> > > +     params->others.bls_config.fixed_val.r = blackLevelRed_;
> > > +     params->others.bls_config.fixed_val.gr = blackLevelGreenR_;
> > > +     params->others.bls_config.fixed_val.gb = blackLevelGreenB_;
> > > +     params->others.bls_config.fixed_val.b = blackLevelBlue_;
> > >
> > >       params->module_en_update |= RKISP1_CIF_ISP_MODULE_BLS;
> > >       params->module_ens |= RKISP1_CIF_ISP_MODULE_BLS;
> > > diff --git a/src/ipa/rkisp1/algorithms/blc.h b/src/ipa/rkisp1/algorithms/blc.h
> > > index 69874d8f..2f6e2d82 100644
> > > --- a/src/ipa/rkisp1/algorithms/blc.h
> > > +++ b/src/ipa/rkisp1/algorithms/blc.h
> > > @@ -13,6 +13,8 @@
> > >
> > >  namespace libcamera {
> > >
> > > +class YamlObject;
> >
> > You can drop this as it's alread forward-declared by algorithm.h;
> >
> > > +
> > >  struct IPACameraSensorInfo;
> > >
> > >  namespace ipa::rkisp1::algorithms {
> > > @@ -23,7 +25,14 @@ public:
> > >       BlackLevelCorrection() = default;
> > >       ~BlackLevelCorrection() = default;
> > >
> > > +     int init(IPAContext &context, const YamlObject *params) override;
> > >       void prepare(IPAContext &context, rkisp1_params_cfg *params) override;
> > > +
> > > +private:
> > > +     int16_t blackLevelRed_;
> > > +     int16_t blackLevelGreenR_;
> > > +     int16_t blackLevelGreenB_;
> > > +     int16_t blackLevelBlue_;
> >
> > If the values can't be negative let's use uint16_t here.
> >
> > >  };
> > >
> > >  } /* namespace ipa::rkisp1::algorithms */

-- 
Regards,

Laurent Pinchart