[PATCH v2 2/2] ipa: rkisp1: Add Lux algorithm module
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Dec 18 04:20:34 CET 2024
On Tue, Dec 17, 2024 at 12:56:08PM +0900, Paul Elder wrote:
> On Mon, Dec 16, 2024 at 11:29:49AM +0000, Kieran Bingham wrote:
> > Quoting Kieran Bingham (2024-12-16 11:26:44)
> > > Quoting Paul Elder (2024-12-16 09:49:33)
> > > > 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>
> > > >
> > > > ---
> > > > Changes in v2:
> > > > - fix bitrot
> > > > - fixes corresponding to changes in the previous patch
> > > > ---
> > > > 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 000000000000..333167b15f64
> > > > --- /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>
Not needed.
> > > > +
> > > > +#include <libcamera/base/log.h>
> > > > +
> > > > +#include <libcamera/control_ids.h>
> > > > +
> > > > +#include "libcamera/internal/yaml_parser.h"
Not needed.
> > > > +
> > > > +#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.
> > > > + */
> > > > +
> > > > +LOG_DEFINE_CATEGORY(RkISP1Lux)
You don't use this.
> > > > +
> > > > +/**
> > > > + * \copydoc libcamera::ipa::Algorithm::init
> > > > + */
> > > > +int Lux::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)
> > > > +{
> > > > + lux_.setBinSize(65535);
A comment to explain where the magic value comes from would be useful.
> > > > + 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({ params->hist.hist_bins, context.hw->numHistogramBins },
> > > > + [](uint32_t x) { return x >> 4; });
It's a bit annoying that the histogram is calculated here, as well as in
Agc::process(). Would it make sense to compute it in the IPA module and
store it in the frame context ? This can be addressed later, but a todo
comment would then be good.
> > > > +
> > > > + double lux = lux_.estimateLux(exposureTime, gain, 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 000000000000..ea98c29120eb
> > > > --- /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"
Not needed.
> > > > +
> > > > +#include "libipa/lux.h"
> > > > +
> > > > +#include "algorithm.h"
> > > > +
> > > > +namespace libcamera {
> > > > +
> > > > +namespace ipa::rkisp1::algorithms {
> > > > +
> > > > +class Lux : public Algorithm
> > > > +{
> > > > +public:
> > > > + Lux() = default;
> > > > + ~Lux() = default;
Not needed.
> > > > +
> > > > + 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 1734a6675f78..c66b0b70b82f 100644
> > > > --- a/src/ipa/rkisp1/algorithms/meson.build
> > > > +++ b/src/ipa/rkisp1/algorithms/meson.build
> > > > @@ -12,4 +12,5 @@ rkisp1_ipa_algorithms = files([
> > > > 'goc.cpp',
> > > > 'gsl.cpp',
> > > > 'lsc.cpp',
> > > > + 'lux.cpp',
> > > > ])
> > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > > > index deb8c196f1b8..e7bdc51bfa4c 100644
> > > > --- a/src/ipa/rkisp1/ipa_context.h
> > > > +++ b/src/ipa/rkisp1/ipa_context.h
> > > > @@ -130,6 +130,7 @@ struct IPAFrameContext : public FrameContext {
> > > > controls::AeMeteringModeEnum meteringMode;
> > > > utils::Duration maxFrameDuration;
> > > > bool updateMetering;
> > > > + double lux;
> > >
> > > Is this really part of agc ? Is it used anywhere that it needs to be
> > > stored in the FrameContext? (Or will it be used somewhere?)
>
> Uh yeah it's not exclusively bound to agc. It was in v1 but now that we
> have another user (the reason why v2 is able to exist), it's not
> anymore.
>
> > >
> > > I'm curious on what it will be used for and whether we need to guarantee
> > > ordering that the lux runs before AGC if it's /really/ used by the AGC.
>
> Maybe and error message would do the trick :)
> Or a fallback to non-lux execution. I'm more inclined towards the
> latter, even if it does add more complexity.
>
> Although considering other algos already fail the entire capture from a
> malformed tuning file, maybe it's fine to bail out?
It's likely fine, but I think we should have a mechanism to declare
dependencies between algos, and detect when they're not honoured.
> > > Perhaps not storing it for now would be easier until it's needed, as
> > > then it would be clearer - but I don't object to storing it.
>
> We need to store it to use it :)
>
> > > Does libtuning already prepare and store the lux calibration reference
> > > points ?
>
> It does not.
>
> > Having read the cover letter - it sounds like we will need to make sure
> > this is run before both AWB and AGC.
>
> Yes we will.
>
> > I'm fine with keeping it in the agc structure though, as it probably
> > gets a bit lonely as framecontext.lux.lux ... ? And it really is closely
> > related to AGC control...
I wouldn't mind storing it in lux.lux to be honest, I think it would
make things clearer.
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >
> > > --
> > >
> > > > } agc;
> > > >
> > > > struct {
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list