[PATCH v2 1/2] ipa: libipa: Add Lux helper

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Mon, Dec 16, 2024 at 06:49:32PM +0900, Paul Elder wrote:
> Add a Lux helper to libipa that does the estimation of the lux level
> given gain, exposure, 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>
> 
> ---
> Changes in v2:
> - improve documentation
> - add binSize member variable and corresponding setter
> - remove aperture
> - split gain into analogue and digital
> - change tuning file names into camel case
> ---
>  src/ipa/libipa/lux.cpp     | 183 +++++++++++++++++++++++++++++++++++++
>  src/ipa/libipa/lux.h       |  48 ++++++++++
>  src/ipa/libipa/meson.build |   2 +
>  3 files changed, 233 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 000000000000..55b771705ab7
> --- /dev/null
> +++ b/src/ipa/libipa/lux.cpp
> @@ -0,0 +1,183 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +/*
> + * Copyright (C) 2019, Raspberry Pi Ltd
> + * Copyright (C) 2024, Paul Elder <paul.elder at ideasonboard.com>
> + *
> + * Helper class that implements lux estimation
> + */
> +#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. It can be used to adjust
> + * the target Y value in AGC or for Bayesian AWB estimation.

That sounds more like a commit message. For the documentation, you can
write

 * Estimating the lux level of an image is a common operation that can for
 * instance be used to adjust the target Y value in AGC or for Bayesian AWB
 * estimation.

> + */
> +
> +namespace libcamera {
> +
> +using namespace std::literals::chrono_literals;
> +
> +LOG_DEFINE_CATEGORY(Lux)
> +
> +namespace ipa {
> +
> +/**
> + * \class Lux
> + * \brief Class that implements lux estimation
> + *
> + * IPAs that wish to use lux esimation should create a Lux algorithm module
> + * that lightly wraps this module by providing the platform-specific luminance
> + * histogram. The Lux entry in the tuning file must then precede the algorithms
> + * that depend on the estimated lux value.
> + */
> +
> +/**
> + * \var Lux::binSize_
> + * \brief The maximum count of each bin
> + *
> + * This must be populated via setBinSize() before estimateLux() is used.
> + */
> +
> +/**
> + * \var Lux::referenceExposureTime_
> + * \brief The exposure time of the reference image, in microseconds.

s/\.$//g

Same below.

> + */
> +
> +/**
> + * \var Lux::referenceAnalogueGain_
> + * \brief The analogue gain of the reference image.
> + */
> +
> +/**
> + * \var Lux::referenceDigitalGain_
> + * \brief The analogue gain of the reference image.
> + */
> +
> +/**
> + * \var Lux::referenceY_
> + * \brief The measured luminance of the reference image, out of the bin size.
> + *
> + * \sa binSize_
> + */
> +
> +/**
> + * \var Lux::referenceLux_
> + * \brief The estimated lux level of the reference image.
> + */
> +
> +/**
> +  * \brief Configure the bin size of the Lux helper module
> +  * \param[in] binSize The maximum count of each bin
> +  *
> +  * This must be configured befure estimateLux() is called, as it is
> +  * platform-specific.
> +  */
> +void Lux::setBinSize(unsigned int binSize)

In patch 2/2 this function is called at init() time with a compile-time
constant. Do you expect use cases where the value wouldn't be known at
compile time ? If not you could pass it to the constructor instead.

If the number of bins could vary, I'd rename this function to init(), to
more clearly indicate it is meant as a one-time initialization.

> +{
> +	binSize_ = binSize;
> +}
> +
> +/**
> + * \brief Parse tuning data
> + * \param[in] tuningData The YamlObject representing the tuning data
> + *
> + * This function parses yaml tuning data for the common Lux module. It requires
> + * reference exposure time, analogue gain, digital gain, and lux values.
> + *
> + * \code{.unparsed}
> + * algorithms:
> + *   - Lux:
> + *       referenceExposureTime: 10000
> + *       referenceAnalogueGain: 4.0
> + *       referenceDigitalGain: 1.0
> + *       referenceY: 12000

You're missing referenceLux.

> + * \endcode
> + *
> + * \return 0 on success or a negative error code
> + */
> +int Lux::readYaml(const YamlObject &tuningData)
> +{
> +	auto value = tuningData["referenceExposureTime"].get<double>();
> +	if (!value) {
> +		LOG(Lux, Error) << "Missing tuning parameter: 'referenceExposureTime'";

If you wrote

		LOG(Lux, Error) << "Missing tuning parameter: " << "referenceExposureTime";

I think the compiler could do string de-duplication. Same below.

> +		return -EINVAL;
> +	}
> +	referenceExposureTime_ = *value * 1.0us;
> +
> +	value = tuningData["referenceAnalogueGain"].get<double>();
> +	if (!value) {
> +		LOG(Lux, Error) << "Missing tuning parameter: 'referenceAnalogueGain'";
> +		return -EINVAL;
> +	}
> +	referenceAnalogueGain_ = *value;
> +
> +	value = tuningData["referenceDigitalGain"].get<double>();
> +	if (!value) {
> +		LOG(Lux, Error) << "Missing tuning parameter: 'referenceDigitalGain'";
> +		return -EINVAL;
> +	}
> +	referenceDigitalGain_ = *value;
> +
> +	value = tuningData["referenceY"].get<double>();
> +	if (!value) {
> +		LOG(Lux, Error) << "Missing tuning parameter: 'referenceY'";
> +		return -EINVAL;
> +	}
> +	referenceY_ = *value;
> +
> +	value = tuningData["referenceLux"].get<double>();
> +	if (!value) {
> +		LOG(Lux, Error) << "Missing tuning parameter: 'referenceLux'";
> +		return -EINVAL;
> +	}
> +	referenceLux_ = *value;
> +
> +	return 0;
> +}
> +
> +/**
> + * \brief Estimate lux given runtime values
> + * \param[in] exposureTime Exposure time applied to the frame
> + * \param[in] aGain Analogue gain applied to the frame
> + * \param[in] dGain Digital gain applied to the frame
> + * \param[in] yHist Histogram from the ISP statistics
> + *
> + * Estimate the lux given the exposure time, gain, and histogram.
> + *
> + * The bin size must be configured via setBinSize() before this function can be
> + * called.
> + *
> + * \return Estimated lux value
> + */
> +double Lux::estimateLux(utils::Duration exposureTime,
> +			double aGain, double dGain,
> +			const Histogram &yHist) const
> +{
> +	double currentY = yHist.interQuantileMean(0, 1);
> +	double exposureTimeRatio = referenceExposureTime_ / exposureTime;
> +	double aGainRatio = referenceAnalogueGain_ / aGain;
> +	double dGainRatio = referenceDigitalGain_ / dGain;
> +	double yRatio = currentY * (binSize_ / yHist.bins()) / referenceY_;
> +
> +	double estimatedLux = exposureTimeRatio * aGainRatio * dGainRatio *
> +			      yRatio * referenceLux_;
> +
> +	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 000000000000..3e6dcc5c3413
> --- /dev/null
> +++ b/src/ipa/libipa/lux.h
> @@ -0,0 +1,48 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +/*
> + * Copyright (C) 2019, Raspberry Pi Ltd
> + * Copyright (C) 2024, Paul Elder <paul.elder at ideasonboard.com>
> + *
> + * Helper class that implements lux estimation
> + */
> +
> +#pragma once
> +
> +#include <algorithm>
> +#include <tuple>
> +#include <vector>

None of these seem to be needed in the header file.

> +
> +#include <libcamera/base/utils.h>
> +
> +#include "libcamera/internal/yaml_parser.h"

This can be replaced by a forward declaration of the YamlObject class.

> +
> +#include "histogram.h"

Same here for the Histogram class.

> +
> +namespace libcamera {
> +
> +namespace ipa {
> +
> +class Lux
> +{
> +public:
> +	Lux() = default;
> +	~Lux() = default;

No need to declare the default constructor and destructor if they're
defaulted and the class has no other user-declared constructor.

> +
> +	void setBinSize(unsigned int binSize);
> +	int readYaml(const YamlObject &tuningData);

s/readYaml/parseTuningData/

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +	double estimateLux(utils::Duration exposureTime,
> +			   double aGain, double dGain,
> +			   const Histogram &yHist) const;
> +
> +private:
> +	unsigned int binSize_;
> +	utils::Duration referenceExposureTime_;
> +	double referenceAnalogueGain_;
> +	double referenceDigitalGain_;
> +	double referenceY_;
> +	double referenceLux_;
> +};
> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> index 93ae25da5e83..189ec0311bd3 100644
> --- a/src/ipa/libipa/meson.build
> +++ b/src/ipa/libipa/meson.build
> @@ -10,6 +10,7 @@ libipa_headers = files([
>      'histogram.h',
>      'interpolator.h',
>      'lsc_polynomial.h',
> +    'lux.h',
>      'module.h',
>      'pwl.h',
>      'vector.h',
> @@ -25,6 +26,7 @@ libipa_sources = files([
>      'histogram.cpp',
>      'interpolator.cpp',
>      'lsc_polynomial.cpp',
> +    'lux.cpp',
>      'module.cpp',
>      'pwl.cpp',
>      'vector.cpp',

-- 
Regards,

Laurent Pinchart