[PATCH 1/3] ipa: libipa: Add Lux helper

[Laurent Pinchart]

On Tue, May 21, 2024 at 11:11:24AM +0100, Daniel Scally wrote:
> Hi Paul - thanks for the patches
> 
> On 12/04/2024 10:16, Paul Elder wrote:
> > Add a Lux helper to libipa that does the estimation of the lux level
> > given gain, exposure, aperture, and luminance histogram. The helper also
> > handles reading the reference values from the tuning file. These are
> > expected to be common operations of lux algorithm modules in IPAs, and
> > is modeled/copied from Raspberry Pi.
> >
> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > ---
> >   src/ipa/libipa/lux.cpp     | 119 +++++++++++++++++++++++++++++++++++++
> >   src/ipa/libipa/lux.h       |  45 ++++++++++++++
> >   src/ipa/libipa/meson.build |   2 +
> >   3 files changed, 166 insertions(+)
> >   create mode 100644 src/ipa/libipa/lux.cpp
> >   create mode 100644 src/ipa/libipa/lux.h
> >
> > diff --git a/src/ipa/libipa/lux.cpp b/src/ipa/libipa/lux.cpp
> > new file mode 100644
> > index 00000000..756cd3c4
> > --- /dev/null
> > +++ b/src/ipa/libipa/lux.cpp
> > @@ -0,0 +1,119 @@
> > +/* SPDX-License-Identifier: BSD-2-Clause */
> > +/*
> > + * Copyright (C) 2019, Raspberry Pi Ltd
> > + * Copyright (C) 2024, Paul Elder <paul.elder at ideasonboard.com>
> > + *
> > + * lux.h - Helper class that implements lux estimation
> > + */
> No filename please
> > +#include "lux.h"
> > +
> > +#include <algorithm>
> > +#include <chrono>
> > +
> > +#include <libcamera/base/log.h>
> > +
> > +#include "libcamera/internal/yaml_parser.h"
> > +
> > +#include "histogram.h"
> > +
> > +/**
> > + * \file lux.h
> > + * \brief Helper class that implements lux estimation
> > + *
> > + * As estimating the lux level of an image is expected to be a common
> > + * operation, it is implemented in a helper in libipa.
> > + */
> > +
> > +namespace libcamera {
> > +
> > +using namespace std::literals::chrono_literals;
> > +
> > +LOG_DEFINE_CATEGORY(Lux)
> > +
> > +namespace ipa {
> > +
> > +/**
> > + * \class Lux
> > + * \brief Class that implements lux estimation
> > + */
> > +
> > +/**
> > + * \var Lux::referenceExposureTime_
> > + * \brief The exposure time of the reference image, in microseconds.
> > + */
> > +
> > +/**
> > + * \var Lux::referenceGain_
> > + * \brief The analogue gain of the reference image.
> > + */
> 
> Analogue gain only? What if there was digital gain applied when the reference was collected?
> 
> > +
> > +/**
> > + * \var Lux::referenceAperture_
> > + * \brief The aperture of the reference image in units of 1/f.
> > + */
> > +
> > +/**
> > + * \var Lux::referenceY_
> > + * \brief The measured luminance of the reference image, out of 65536.
> 
> 65536? Or 65535?
> 
> > + */
> > +
> > +/**
> > + * \var Lux::referenceLux_
> > + * \brief The estimated lux level of the reference image.
> > + */
> > +
> > +int Lux::readYaml(const YamlObject &tuningData)
> 
> This is a public function, so I think it should have a doxygen comment?
> 
> > +{
> > +	auto value = tuningData["reference_exposure_time"].get<double>();
> > +	if (!value) {
> > +		LOG(Lux, Error) << "Missing tuning parameter: 'reference_exposure_time'";
> > +		return -EINVAL;
> > +	}
> > +	referenceExposureTime_ = *value * 1.0us;
> 
> Laurent suggested switching to integration time everywhere, as exposure time tends to get shortened 
> to "exposure".

I think we need a tree-wide patch for this. The naming in this patch can
be kept if it gets merged first.

> > +
> > +	value = tuningData["reference_gain"].get<double>();
> > +	if (!value) {
> > +		LOG(Lux, Error) << "Missing tuning parameter: 'reference_gain'";
> > +		return -EINVAL;
> > +	}
> > +	referenceGain_ = *value;
> > +
> > +	referenceAperture_ = tuningData["reference_aperture"].get<double>(1.0);
> > +
> > +	value = tuningData["reference_Y"].get<double>();
> > +	if (!value) {
> > +		LOG(Lux, Error) << "Missing tuning parameter: 'reference_Y'";
> > +		return -EINVAL;
> > +	}
> > +	referenceY_ = *value;
> > +
> > +	value = tuningData["reference_lux"].get<double>();
> > +	if (!value) {
> > +		LOG(Lux, Error) << "Missing tuning parameter: 'reference_lux'";
> > +		return -EINVAL;
> > +	}
> > +	referenceLux_ = *value;
> > +
> > +	return 0;
> > +}
> > +
> > +double Lux::process(double gain, utils::Duration exposureTime, double aperture,
> > +		    const Histogram &yHist) const
>
> We've no constraints on the name here, so I think "estimateLux" would be better. Also needs a 
> doxygen comment I think?
>
> > +{
> > +	double currentY = yHist.interQuantileMean(0, 1);
> > +	double gainRatio = referenceGain_ / gain;
> > +	double exposureTimeRatio = referenceExposureTime_ / exposureTime;
> > +	double apertureRatio = referenceAperture_ / aperture;
> > +	double yRatio = currentY * (65536 / yHist.bins()) / referenceY_;
> > +
> > +	double estimatedLux = exposureTimeRatio * gainRatio *
> > +			      apertureRatio * apertureRatio *
> > +			      yRatio * referenceLux_;
> 
> apertureRatio is in here twice; is that accidental?

The aperture is expressed as a number proportional to the diameter of
the entrance pupil, while the exposure if proportional to the area of
the pupil. This is worth a comment.

We don't support aperture control though, so maybe the simplest option
would be to drop it.

> Do you know the background to the formula? It's 
> a bit strange, in my view...our assumption from the mean luminance AGC is of a linear relationship 
> between "total exposure" (comprising integration time multiplied by gain) and the average of the 
> yHist, but that's not really reflected here.
> 
> > +
> > +	LOG(Lux, Debug) << "Estimated lux " << estimatedLux;
> > +	return estimatedLux;
> > +}
> > +
> > +} /* namespace ipa */
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/ipa/libipa/lux.h b/src/ipa/libipa/lux.h
> > new file mode 100644
> > index 00000000..6bc9cf9f
> > --- /dev/null
> > +++ b/src/ipa/libipa/lux.h
> > @@ -0,0 +1,45 @@
> > +/* SPDX-License-Identifier: BSD-2-Clause */
> > +/*
> > + * Copyright (C) 2019, Raspberry Pi Ltd
> > + * Copyright (C) 2024, Paul Elder <paul.elder at ideasonboard.com>
> > + *
> > + * lux.h - Helper class that implements lux estimation
> > + */
> 
> No file name please
> 
> > +
> > +#pragma once
> > +
> > +#include <algorithm>
> > +#include <tuple>
> > +#include <vector>
> > +
> > +#include <libcamera/base/utils.h>
> > +
> > +#include "libcamera/internal/yaml_parser.h"
> > +
> > +#include "histogram.h"
> > +
> > +namespace libcamera {
> > +
> > +namespace ipa {
> > +
> > +class Lux
> > +{
> > +public:
> > +	Lux() = default;
> > +	~Lux() = default;
> > +
> > +	int readYaml(const YamlObject &tuningData);
> > +	double process(double gain, utils::Duration exposureTime,
> > +		       double aperture, const Histogram &yHist) const;
> > +
> > +private:
> > +	utils::Duration referenceExposureTime_;
> > +	double referenceGain_;
> > +	double referenceAperture_;
> > +	double referenceY_;
> > +	double referenceLux_;
> > +};
> > +
> > +} /* namespace ipa */
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> > index 0796982e..b6d58900 100644
> > --- a/src/ipa/libipa/meson.build
> > +++ b/src/ipa/libipa/meson.build
> > @@ -8,6 +8,7 @@ libipa_headers = files([
> >       'exposure_mode_helper.h',
> >       'fc_queue.h',
> >       'histogram.h',
> > +    'lux.h',
> >       'matrix.h',
> >       'module.h',
> >       'pwl.h',
> > @@ -21,6 +22,7 @@ libipa_sources = files([
> >       'exposure_mode_helper.cpp',
> >       'fc_queue.cpp',
> >       'histogram.cpp',
> > +    'lux.cpp',
> >       'matrix.cpp',
> >       'module.cpp',
> >       'pwl.cpp'

-- 
Regards,

Laurent Pinchart