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

22341 diff mbox series

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@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

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@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@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@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@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',