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

[Dan Scally]

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".

> +
> +	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? 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'