[PATCH 2/3] ipa: rkisp1: Add Lux algorithm module

[Laurent Pinchart]

On Fri, Apr 12, 2024 at 11:57:37AM +0100, Kieran Bingham wrote:
> Quoting Paul Elder (2024-04-12 10:16:59)
> > Add a lux algorithm module to rkisp1 IPA for estimating the lux level of
> > an image. This is reported in metadata, as well as saved in the frame
> > context so that other algorithms (mainly AGC) can use its value. It does
> > not set any controls.
> > 
> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > ---
> >  src/ipa/rkisp1/algorithms/lux.cpp     | 76 +++++++++++++++++++++++++++
> >  src/ipa/rkisp1/algorithms/lux.h       | 39 ++++++++++++++
> >  src/ipa/rkisp1/algorithms/meson.build |  1 +
> >  src/ipa/rkisp1/ipa_context.h          |  1 +
> >  4 files changed, 117 insertions(+)
> >  create mode 100644 src/ipa/rkisp1/algorithms/lux.cpp
> >  create mode 100644 src/ipa/rkisp1/algorithms/lux.h
> > 
> > diff --git a/src/ipa/rkisp1/algorithms/lux.cpp b/src/ipa/rkisp1/algorithms/lux.cpp
> > new file mode 100644
> > index 00000000..1816ddb2
> > --- /dev/null
> > +++ b/src/ipa/rkisp1/algorithms/lux.cpp
> > @@ -0,0 +1,76 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2024, Ideas On Board
> > + *
> > + * lux.cpp - RkISP1 Lux control
> > + */
> > +
> > +#include "lux.h"
> > +
> > +#include <algorithm>
> > +#include <cmath>
> > +
> > +#include <libcamera/base/log.h>
> > +
> > +#include <libcamera/control_ids.h>
> > +
> > +#include "libcamera/internal/yaml_parser.h"
> > +
> > +#include "libipa/histogram.h"
> > +#include "libipa/lux.h"
> > +
> > +/**
> > + * \file lux.h
> > + */
> > +
> > +namespace libcamera {
> > +
> > +namespace ipa::rkisp1::algorithms {
> > +
> > +/**
> > + * \class Lux
> > + * \brief RkISP1 Lux control
> > + *
> > + * The Lux algorithm is responsible for estimating the lux level of the image.
> > + * It doesn't take or generate any controls, but it provides a lux level for
> > + * other algorithms (such as AGC) to use.
> 
> I'm not yet convinced this is an 'algorithm', but could be put directly
> in agc. It might be interesting to explore why RPi choose to keep this
> separate though.

I've been wondering the same. I can be convinced to go either way.

> Does the lux value get used by any algorithm /other/ than AGC?
>
> > + */
> > +
> > +LOG_DEFINE_CATEGORY(RkISP1Lux)
> > +
> > +/**
> > + * \copydoc libcamera::ipa::Algorithm::init
> > + */
> > +int Lux::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)
> > +{
> > +       return lux_.readYaml(tuningData);
> > +}
> > +
> > +/**
> > + * \copydoc libcamera::ipa::Algorithm::process
> > + */
> > +void Lux::process(IPAContext &context,
> > +                 [[maybe_unused]] const uint32_t frame,
> > +                 IPAFrameContext &frameContext,
> > +                 const rkisp1_stat_buffer *stats,
> > +                 ControlList &metadata)
> > +{
> > +       utils::Duration exposureTime = context.configuration.sensor.lineDuration
> > +                                      * frameContext.sensor.exposure;
> > +       double gain = frameContext.sensor.gain;
> > +
> > +       const rkisp1_cif_isp_stat *params = &stats->params;
> > +       Histogram yHist = Histogram(Span<const uint32_t>(params->hist.hist_bins,
> > +                                                        context.hw->numHistogramBins), 4);
> 
> Are we calculating/processing the histogram multiple times now by
> keeping this out of the AGC module? Would that be optimised by havign
> this as a part of agc directly ?

Or by computing the cumulative histogram in the IPA module and storing
it in the active state. This departs a bit from the current practice of
operating on the hardware stats only, and would make the algorithms
dependent of the IPA module, but I think that's OK.

> > +
> > +       /* todo Update this when we support aperture */
> > +       double lux = lux_.process(gain, exposureTime, 1.0, yHist);
> > +       frameContext.agc.lux = lux;
> > +       metadata.set(controls::Lux, lux);
> > +}
> > +
> > +REGISTER_IPA_ALGORITHM(Lux, "Lux")
> > +
> > +} /* namespace ipa::rkisp1::algorithms */
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/ipa/rkisp1/algorithms/lux.h b/src/ipa/rkisp1/algorithms/lux.h
> > new file mode 100644
> > index 00000000..ea98c291
> > --- /dev/null
> > +++ b/src/ipa/rkisp1/algorithms/lux.h
> > @@ -0,0 +1,39 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2024, Ideas On Board
> > + *
> > + * lux.h - RkISP1 Lux control
> > + */
> > +
> > +#pragma once
> > +
> > +#include <sys/types.h>
> > +
> > +#include "libcamera/internal/yaml_parser.h"
> > +
> > +#include "libipa/lux.h"
> > +
> > +#include "algorithm.h"
> > +
> > +namespace libcamera {
> > +
> > +namespace ipa::rkisp1::algorithms {
> > +
> > +class Lux : public Algorithm
> > +{
> > +public:
> > +       Lux() = default;
> > +       ~Lux() = default;
> > +
> > +       int init(IPAContext &context, const YamlObject &tuningData) override;
> > +       void process(IPAContext &context, const uint32_t frame,
> > +                    IPAFrameContext &frameContext,
> > +                    const rkisp1_stat_buffer *stats,
> > +                    ControlList &metadata) override;
> > +
> > +private:
> > +       ipa::Lux lux_;
> > +};
> > +
> > +} /* namespace ipa::rkisp1::algorithms */
> > +} /* namespace libcamera */
> > diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build
> > index c9891e87..b0381d5f 100644
> > --- a/src/ipa/rkisp1/algorithms/meson.build
> > +++ b/src/ipa/rkisp1/algorithms/meson.build
> > @@ -11,4 +11,5 @@ rkisp1_ipa_algorithms = files([
> >      'filter.cpp',
> >      'gsl.cpp',
> >      'lsc.cpp',
> > +    'lux.cpp',
> >  ])
> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > index dc876da0..c39e0e9b 100644
> > --- a/src/ipa/rkisp1/ipa_context.h
> > +++ b/src/ipa/rkisp1/ipa_context.h
> > @@ -118,6 +118,7 @@ struct IPAFrameContext : public FrameContext {
> >                 int32_t exposureMode;
> >                 int32_t constraintMode;
> >                 utils::Duration maxShutterSpeed;
> > +               double lux;
> 
> Especially as we're putting the lux in the agc component here!
> 
> >         } agc;
> >  
> >         struct {

-- 
Regards,

Laurent Pinchart